Re: [RFC PATCH v2 2/5] libfs: Check dentry before locking in simple_offset_empty()

[Chuck Lever <chuck.lever@oracle.com>]

On Wed, Nov 27, 2024 at 11:09:11AM +0800, yangerkun wrote:
> Thank you very much for your efforts on this issue!
> 
> 在 2024/11/26 23:54, cel@kernel.org 写道:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > Defensive change: Don't try to lock a dentry unless it is positive.
> > Trying to lock a negative entry will generate a refcount underflow.
> 
> Which member trigger this underflow?

dput() encounters a dentry refcount underflow because a negative
dentry's refcount is already zero.

But perhaps it would be more accurate to say this patch attempts to
avoid triggering the DEBUG_LOCKS_WARN_ON in hlock_class.


> > The underflow has been seen only while testing.
> > 
> > Fixes: ecba88a3b32d ("libfs: Add simple_offset_empty()")
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >   fs/libfs.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/libfs.c b/fs/libfs.c
> > index bf67954b525b..c88ed15437c7 100644
> > --- a/fs/libfs.c
> > +++ b/fs/libfs.c
> > @@ -347,13 +347,14 @@ int simple_offset_empty(struct dentry *dentry)
> >   	index = DIR_OFFSET_MIN;
> >   	octx = inode->i_op->get_offset_ctx(inode);
> >   	mt_for_each(&octx->mt, child, index, LONG_MAX) {
> > -		spin_lock(&child->d_lock);
> >   		if (simple_positive(child)) {
> > +			spin_lock(&child->d_lock);
> > +			if (simple_positive(child))
> > +				ret = 0;
> >   			spin_unlock(&child->d_lock);
> > -			ret = 0;
> > -			break;
> > +			if (!ret)
> > +				break;
> >   		}
> > -		spin_unlock(&child->d_lock);
> >   	}
> 
> Calltrace arrived here means this is a active dir(a dentry with positive
> inode), and nowdays only .rmdir / .rename2 for shmem can reach this point.
> Lock for this dir inode has already been held, maybe this can protect child
> been negative or active? So d_lock here is no need?

My assumption was that child->d_lock was necessary for an
authoritative determination of whether "child" is positive or
negative. If holding that lock isn't necessary, then I agree,
there's no need to take child->d_lock here at all... There's
clearly nothing else to protect in this code path.


> >   	return ret;

-- 
Chuck Lever