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

Hi Laurynas,

On Fri, 15 Mar 2013 13:14:20 -0000, Laurynas Biveinis wrote:
>> 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.
>

Right, but temporarily unlocking zip_dirty_flush_mutex only occurs after
locking another 2 global locks: page_hash_latch and the buffer pool
mutex. Which is still bad, because whether it is "not very long" or not
depends on many other factors.

>> 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.
>

I don't really see reasons for them to be different. The order relation
between LRU_list_mutex, flush_list_mutex and block_mutex priorities were
defined by our patches. And something tells me they have either been
defined arbitrarily, or to hide some implementation "drawback" :)

Seriously, what's the reason for them be different with respect to the
block mutex priority?

>> 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.

It doesn't apply to upstream merges. I'd first try to answer the "why is
it different?" question and then see how feasible it is to change that.

> - 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.
>
Yep, this doesn't look nice.