[lkp,nfsd] b44061d0b9: BUG: Dentry ffff880027d7c540{i=1846f,n=0a} still in use (1) [unmount of btrfs vda]
diff mbox

Message ID e6fb50d4-2a27-13cd-02c8-5b8439a16b3f@fb.com
State New
Headers show

Commit Message

Josef Bacik Aug. 10, 2016, 6:46 p.m. UTC
On 08/10/2016 02:25 PM, Linus Torvalds wrote:
> On Wed, Aug 10, 2016 at 11:22 AM, Josef Bacik <jbacik@fb.com> wrote:
>> On 08/10/2016 02:06 PM, Linus Torvalds wrote:
>>>
>>> More information in the original email on lkml.
>>
>> I'm not subscribed to lkml and for some reason I can't find the original
>> email in any of the lkml/linux-nfs archives.  Could you forward more of the
>> details?
>
> Done.
>

So my naive fix would be something like this


From: Josef Bacik <jbacik@fb.com>
Date: Wed, 10 Aug 2016 14:43:08 -0400
Subject: [PATCH] nfsd: fix dentry refcounting problem

b44061d0b9 introduced a dentry ref counting bug, previously we were grabbing one
ref to dchild in nfsd_create(), but with the creation of nfsd_create_locked() we
have a ref for dchild from the lookup in nfsd_create(), and then another ref in
nfsd_create_locked().  The ref from the lookup in nfsd_create() is never dropped
and results in dentries still in use at unmount.

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
  fs/nfsd/vfs.c | 9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Linus Torvalds Aug. 10, 2016, 6:56 p.m. UTC | #1
On Wed, Aug 10, 2016 at 11:46 AM, Josef Bacik <jbacik@fb.com> wrote:
>
> So my naive fix would be something like this

Bruce? Josef's patch looks ObviouslyCorrect(tm) to me now that I look
at it - all the other callers of fh_compose() also seem to just drop
the dentry unconditionally, knowing that fh_compose() took a ref to it
if needed.

In fact, the only thing I'd do differently would be to not even put
the comment there at all, since this call site isn't any different
from any of the others. If anything, it could go on fh_compose() if we
want to add comments.

             Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Aug. 10, 2016, 7:06 p.m. UTC | #2
On Wed, 2016-08-10 at 14:46 -0400, Josef Bacik wrote:
> On 08/10/2016 02:25 PM, Linus Torvalds wrote:
> > 
> > > > On Wed, Aug 10, 2016 at 11:22 AM, Josef Bacik <jbacik@fb.com> wrote:
> > > 
> > > On 08/10/2016 02:06 PM, Linus Torvalds wrote:
> > > > 
> > > > 
> > > > More information in the original email on lkml.
> > > 
> > > I'm not subscribed to lkml and for some reason I can't find the original
> > > email in any of the lkml/linux-nfs archives.  Could you forward more of the
> > > details?
> > 
> > Done.
> > 
> 
> So my naive fix would be something like this
> 
> 
> > From: Josef Bacik <jbacik@fb.com>
> Date: Wed, 10 Aug 2016 14:43:08 -0400
> Subject: [PATCH] nfsd: fix dentry refcounting problem
> 
> b44061d0b9 introduced a dentry ref counting bug, previously we were grabbing one
> ref to dchild in nfsd_create(), but with the creation of nfsd_create_locked() we
> have a ref for dchild from the lookup in nfsd_create(), and then another ref in
> nfsd_create_locked().  The ref from the lookup in nfsd_create() is never dropped
> and results in dentries still in use at unmount.
> 
> > Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>   fs/nfsd/vfs.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index ba944123..ff476e6 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1252,10 +1252,13 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >   	if (IS_ERR(dchild))
> >   		return nfserrno(host_err);
> >   	err = fh_compose(resfhp, fhp->fh_export, dchild, fhp);
> > -	if (err) {
> > -		dput(dchild);
> > +	/*
> > +	 * We unconditionally drop our ref to dchild as fh_compose will have
> > +	 * already grabbed its own ref for it.
> > +	 */
> > +	dput(dchild);
> > +	if (err)
> >   		return err;
> > -	}
> >   	return nfsd_create_locked(rqstp, fhp, fname, flen, iap, type,
> >   					rdev, resfhp);
>   }

Looks correct to me:

Reviewed-by: Jeff Layton <jlayton@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields Aug. 10, 2016, 7:09 p.m. UTC | #3
On Wed, Aug 10, 2016 at 11:56:15AM -0700, Linus Torvalds wrote:
> On Wed, Aug 10, 2016 at 11:46 AM, Josef Bacik <jbacik@fb.com> wrote:
> >
> > So my naive fix would be something like this
> 
> Bruce? Josef's patch looks ObviouslyCorrect(tm) to me now that I look
> at it - all the other callers of fh_compose() also seem to just drop
> the dentry unconditionally, knowing that fh_compose() took a ref to it
> if needed.
> 
> In fact, the only thing I'd do differently would be to not even put
> the comment there at all, since this call site isn't any different
> from any of the others. If anything, it could go on fh_compose() if we
> want to add comments.

Yep, makes sense to me too.

OK with me if you want to take it, otherwise I'll run it through my
usual tests and send you a pull request probably today or tomorrow.

Thanks, Josef!  (And kernel test robot folks--the speed with which
they're catching stuff, and bisecting down to individual commits, is
really amazing to me.)

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds Aug. 10, 2016, 7:15 p.m. UTC | #4
On Wed, Aug 10, 2016 at 12:09 PM, J. Bruce Fields <bfields@redhat.com> wrote:
>
> OK with me if you want to take it, otherwise I'll run it through my
> usual tests and send you a pull request probably today or tomorrow.

I'll let it go through your tree and your usual tests - it's not like
this seems to be a "needs to be in my tree *IMMEDIATELY* or the world
will end!!!11!" kind of issue.

Thanks everybody,

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro Aug. 11, 2016, 2:19 a.m. UTC | #5
On Wed, Aug 10, 2016 at 02:46:27PM -0400, Josef Bacik wrote:
> On 08/10/2016 02:25 PM, Linus Torvalds wrote:
> > On Wed, Aug 10, 2016 at 11:22 AM, Josef Bacik <jbacik@fb.com> wrote:
> > > On 08/10/2016 02:06 PM, Linus Torvalds wrote:
> > > > 
> > > > More information in the original email on lkml.
> > > 
> > > I'm not subscribed to lkml and for some reason I can't find the original
> > > email in any of the lkml/linux-nfs archives.  Could you forward more of the
> > > details?
> > 
> > Done.
> > 
> 
> So my naive fix would be something like this
> 
> 
> From: Josef Bacik <jbacik@fb.com>
> Date: Wed, 10 Aug 2016 14:43:08 -0400
> Subject: [PATCH] nfsd: fix dentry refcounting problem
> 
> b44061d0b9 introduced a dentry ref counting bug, previously we were grabbing one
> ref to dchild in nfsd_create(), but with the creation of nfsd_create_locked() we
> have a ref for dchild from the lookup in nfsd_create(), and then another ref in
> nfsd_create_locked().  The ref from the lookup in nfsd_create() is never dropped
> and results in dentries still in use at unmount.
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>

[sorry, had been off-line since yesterday]

Patch looks sane; feel free to slap Acked-by: Al Viro <viro@zeniv.linux.org.uk>
on it.  I think it should go through nfsd tree.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index ba944123..ff476e6 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1252,10 +1252,13 @@  nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
  	if (IS_ERR(dchild))
  		return nfserrno(host_err);
  	err = fh_compose(resfhp, fhp->fh_export, dchild, fhp);
-	if (err) {
-		dput(dchild);
+	/*
+	 * We unconditionally drop our ref to dchild as fh_compose will have
+	 * already grabbed its own ref for it.
+	 */
+	dput(dchild);
+	if (err)
  		return err;
-	}
  	return nfsd_create_locked(rqstp, fhp, fname, flen, iap, type,
  					rdev, resfhp);
  }