Messages in this thread | | | From | John Ogness <> | Subject | Re: dcache: remove trylock loops (was Re: [BUG] lock_parent() breakage when used from shrink_dentry_list()) | Date | Tue, 13 Mar 2018 21:46:48 +0100 |
| |
On 2018-03-12, Al Viro <viro@ZenIV.linux.org.uk> wrote: >> If someone else has grabbed a reference, it shouldn't be added to the >> lru list. Only decremented. >> >> if (entry->d_lockref.count == 1) > > Nah, better handle that in retain_dentry() itself. See updated > #work.dcache. > > + if (unlikely(dentry->d_lockref.count != 1)) { > + dentry->d_lockref.count--; > + } else if (likely(!retain_dentry(dentry))) { > + __dentry_kill(dentry); > + return parent; > + }
Although the updated version is correct (and saves on lines of code), I find putting the deref and lru_add code in the "true" case of retain_dentry() to be pretty tricky. IMHO the code is easier to understand if it looks like this:
if (unlikely(dentry->d_lockref.count != 1)) { dentry->d_lockref.count--; } else if (likely(!retain_dentry(dentry))) { __dentry_kill(dentry); return parent; } else { dentry->d_lockref.count--; dentry_lru_add(dentry); }
This is what your version is doing, but that final else is hiding in the retain_dentry() "true" case.
My suggestion is to revert 7479f57fecd2a4837b5c79ce1cf0dcf284db54be (and then fixup dput() to deref before calling dentry_lru_add()).
> FWIW, there's another trylock loop on dentries - one in > autofs get_next_positive_dentry(). Any plans re dealing > with that one?
I will need to dig into it a bit deeper (I am unfamiliar with autofs), but it looks like it is trying to do basically the same thing as the ascend loop in d_walk().
> I'd spent the last couple of weeks (when not being too sick > for any work) going through dcache.c and related code; hopefully > this time I will get the documentation into postable shape ;-/
Thank you for all your help in getting these changes cleaned and correctly implemented so quickly.
I've reviewed your latest trylock loop removal patches and found only 1 minor issue. I'll post that separately.
John Ogness
| |