From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Thu, 19 Apr 2018 09:43:29 +0100 Subject: [Cluster-devel] [GFS2 PATCH 2/2] GFS2: Take advantage of new EXSH glock mode for rgrps In-Reply-To: <1396928988.20968005.1524080380629.JavaMail.zimbra@redhat.com> References: <20180418165838.8342-1-rpeterso@redhat.com> <20180418165838.8342-3-rpeterso@redhat.com> <4b599258-ad4b-2187-cb77-f5268de164bf@redhat.com> <1396928988.20968005.1524080380629.JavaMail.zimbra@redhat.com> Message-ID: List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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.