[Cluster-devel] [GFS2 PATCH 2/2] GFS2: Take advantage of new EXSH glock mode for rgrps

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,


On 18/04/18 20:39, Bob Peterson wrote:
> ----- Original Message -----
>> Hi,
>>
>>
>> On 18/04/18 17:58, Bob Peterson wrote:
>>> This patch switches rgrp locking to EXSH mode so that it's only
>>> taken when the rgrp is added to an active transaction, or when
>>> blocks are being reserved. As soon as the transaction is ended,
>>> the rgrp exclusivity is released. This allows for rgrp sharing.
>> Adding the rwsem looks like a good plan. I'm not sure that I understand
>> why you need to lock the rgrp exclusively for the whole transaction and
>> have added the new list. Do we really need to save the pid, or is that
>> just there for debugging?
>>
>> Steve.
> Hi,
>
> The reason I chose to do it this way is because I wanted to guarantee
> any process twiddling any bit in the bitmap or any rgrp value would
> have the lock exclusively. Before anyone can twiddle a bit, they need
> to add the data to the transaction. Simple way to guarantee it.
>
> I could add the appropriate locking before every place it needs
> to twiddle the bits, but the patch will look a lot bigger.
> I thought the transaction is a convenient and centrally-located
> place for the locking, rather than scattering them about where
> future maintainers could introduce deadlocks and such.
>
> Regards,
>
> Bob Peterson
> Red Hat File Systems

You should be able to check that the rwsem is held in write mode at the 
point when the bits are updated. That is probably not the main issue 
though - the important thing is to make sure that we hold the lock 
exclusively between the search and update operations, or if not, that we 
use some other mechanism to ensure that no other local process can 
update those bits in the mean time. I wonder if we'll need to make the 
reservations locally compulsory (rather than hints) in order to be 
compatible with Andreas' iomap work? I've not had time to dive into the 
code and check that.

To get the maximum benefit we want to make the lock hold times as short 
as possible, and the more usual pattern of:

lock();
/* do something */
unlock();

is much easier to understand in the code too. We have the ability to 
look at each individual rgrp EX glock request separately and to upgrade 
each code path one at a time, so it might be better to break this into a 
series for each lock. It would make it easier to review, and also allow 
testing to show exactly which paths are the most performance critical too.

The numbers that you've shown are certainly very encouraging, but with 
some careful analysis, it should be possible to do even better still if 
we can reduce the local rwsem hold times even further,

Steve.