Messages in this thread |  | | From | NeilBrown <> | Date | Fri, 06 Apr 2018 11:33:58 +1000 | Subject | Re: [Cluster-devel] [PATCH v2 0/2] gfs2: Stop using rhashtable_walk_peek |
| |
On Wed, Apr 04 2018, Andreas Grünbacher wrote:
> Herbert Xu <herbert@gondor.apana.org.au> schrieb am Mi. 4. Apr. 2018 um > 17:51: > >> On Wed, Apr 04, 2018 at 11:46:28AM -0400, Bob Peterson wrote: >> > >> > The patches look good. The big question is whether to add them to this >> > merge window while it's still open. Opinions? >> >> We're still hashing out the rhashtable interface so I don't think now is >> the time to rush things. > > > Fair enough. No matter how rhashtable_walk_peek changes, we‘ll still need > these two patches to fix the glock dump though.
Those two patches look fine to me and don't depend on changes to rhashtable, so it is up to you when they go upstream.
However, I think the code can be substantially simplified, particularly once we make rhashtable a little cleverer. So this is what I'll probably be doing for a similar situation in lustre....
Having examined seqfile closely, it is apparent that if ->start never changes *ppos, and if ->next always increments it (except maybe on error) then
1/ ->next is only ever given a 'pos' that was returned by the previous call to ->start or ->next. So it should *always* return the next object, after the one previously returned by ->start or ->next. It never needs to 'seek'. The 'traverse()' function in seq_file.c does any seeking needed. ->next needs to increase 'pos' and when walking a dynamic list, it is easiest if it just increments it.
2/ ->start is only called with a pos of: 0 - in this case it should rewind to the start the last pos passed to ->start of ->next In this case it should return the same thing that was returned last time. If it no longer exists, then the following one should be returned. one more than the last pos passed to ->start or ->next In this case it should return the object after the last one returned.
The proposed enhancement to rhashtable_walk* is to add a rhashtable_walk_prev() which returns the previously returned object, if it is still in the table, or NULL. It also enhances rhashtable_walk_start() so that if the previously returned object is still in the table, it is preserved as the current cursor. This means that if you take some action to ensure that the previously returned object remains in the table until the next ->start, then you can reliably walk the table with no duplicates or omissions (unless a concurrent rehash causes duplicates) If you don't keep the object in the table and it gets removed, then the 'skip' counter is used to find your place, and you might get duplicates or omissions.
NeilBrown [unhandled content-type:application/pgp-signature] |  |