Code review comment for lp:~laurynas-biveinis/percona-server/bug1086680-5.1

> Though it is not clear if there
> are other conditions resulting in a dirty bpage read in
> buf_flush_batch(), e.g. not related to compressed page relocations, and
> if those will be solved by the patch as well.

IIRC I reviewed the code for other such issues.

> However, I don't like that we:
>
> 1) introduce another mutex. it's another potential source of regressions
> and another thing to care about on merges.

I don't like it neither, and I'd be glad to reimplement it some other way. But that was the least bad alternative I could come up with.

> 2) lock a global buffer pool mutex for the entire flushing loop in
> buf_flush_batch() when we in fact only want to protect individual pages
> on every loop iteration. Which means a buf_page_get_gen() call in
> another thread trying to relocate a completely unrelated (e.g. not even
> touched by the buf_flush_batch() loop) page will have to wait for
> buf_flush_batch() to complete flushing all the pages it wants to flush.

But in this patch the new mutex is temporarily released once the the block-to-be-flushed is fixed right before the flushing itself, where we don't have any block pointers acquired from the flush list. Thus at least the blocking is not very long. Thus what this mutex serializes are 1) single page relocations; 2) single page flushes.

> Are there any reasons to not follow the same logic in buf_flush_batch()
> wrt. flush_list_mutex as we do wrt. LRU_list_mutex? After all, the root
> cause of the problem is that we release flush_list_mutex too early and
> then read from the block while it's not protected either by
> flush_list_mutex or the block mutex. So let's just not release
> flush_list_mutex until we lock the block mutex a few lines down in the
> code, and that will also prevent relocations?

Unfortunately, the block and flush list mutex priority is wrong for this, obviously the easiest if it were possible, solution. Block mutexes have to locked before the flush list mutex. The LRU list mutex is different, it has to be locked before the block mutexes.

> flush_list_mutex can then
> be released temporarily while buf_flush_try_neighbors() is in
> progress. It will likely result in a much smaller & easier to maintain
> patch, and even increase locking granularity as opposed to a separate
> global mutex approach?

Yes, it would if not the mutex priority.

The alternatives I see:
- Rework buffer pool mutexes so that flush list mutex comes before the block mutex. Not something I'd want to maintain for the upstream merges and haven't looked into what kind of issues would such implementation raise.
- Overload (rename?) the LRU list mutex to take on the job of the new mutex too since it has the right priority for the job. This comes close to unsplitting the buffer pool mutex split, and has most of the disadvantages of the current solution.

Maybe I'm missing some other alternative or my reasoning above is wrong?

Thanks,