On Tue, 2024-02-27 at 06:51 -0800, Ben Greear wrote: > On 2/27/24 06:32, Johannes Berg wrote: > > On Tue, 2024-02-27 at 06:29 -0800, Ben Greear wrote: > > > > --- a/fs/debugfs/inode.c > > > > +++ b/fs/debugfs/inode.c > > > > @@ -751,13 +751,19 @@ static void __debugfs_file_removed(struct dentry *dentry) > > > > if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT) > > > > return; > > > > > > > > - /* if we hit zero, just wait for all to finish */ > > > > - if (!refcount_dec_and_test(&fsd->active_users)) { > > > > - wait_for_completion(&fsd->active_users_drained); > > > > - return; > > > > - } > > > > + /* > > > > + * Now that debugfs_file_get() no longer sees a valid entry, > > > > + * decrement the refcount to remove the initial reference. > > > > + */ > > > > + refcount_dec(&fsd->active_users); > > > > > > > > [ 94.576688] ------------[ cut here ]------------ > > > [ 94.576699] refcount_t: decrement hit 0; leaking memory. > > > > > > > Ah ... right, refcount_dec() doesn't like to hit 0, it's not meant for > > this path. > > > > I guess we can > > > > if (refcount_dec_and_test(...)) > > return; > > > > while (refcount_read(...)) { ... } > > > > johannes > > > > Like this? Yeah, looks about right. But not entirely sure I'm thinking this through correctly right now ... > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > index 034a617cb1a5..166053095610 100644 > --- a/fs/debugfs/inode.c > +++ b/fs/debugfs/inode.c > @@ -751,13 +751,20 @@ static void __debugfs_file_removed(struct dentry *dentry) > if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT) > return; > > - /* if we hit zero, just wait for all to finish */ > - if (!refcount_dec_and_test(&fsd->active_users)) { > - wait_for_completion(&fsd->active_users_drained); > + /* > + * Now that debugfs_file_get() no longer sees a valid entry, > + * decrement the refcount to remove the initial reference. > + */ > + if (!refcount_dec_and_test(&fsd->active_users)) > return; > - } > > - /* if we didn't hit zero, try to cancel any we can */ > + /* > + * As long as it's not zero, try to cancel any cancellations, > + * new incoming ones will wake up the completion as we might > + * have raced: debugfs_file_get() had already been done, but > + * debugfs_enter_cancellation() hadn't, by the time we got > + * to this point here. > + */ > while (refcount_read(&fsd->active_users)) { > struct debugfs_cancellation *c; > >