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@xxxxxxxxxx 写道:
> > From: Chuck Lever <chuck.lever@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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
