Re: [lkp] [nfsd] b44061d0b9: BUG: Dentry ffff880027d7c540{i=1846f,n=0a} still in use (1) [unmount of btrfs vda]
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 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 > 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 [sorry, had been off-line since yesterday] Patch looks sane; feel free to slap Acked-by: Al Viro 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lkp] [nfsd] b44061d0b9: BUG: Dentry ffff880027d7c540{i=1846f,n=0a} still in use (1) [unmount of btrfs vda]
On Tue, Aug 9, 2016 at 10:39 PM, kernel test robot wrote: > > [ 1537.558739] nfsd: last server has exited, flushing export cache > [ 1540.627795] BUG: Dentry 880027d7c540{i=1846f,n=0a} still in use (1) > [unmount of btrfs vda] > [ 1540.633915] [ cut here ] > [ 1540.636551] WARNING: CPU: 0 PID: 20552 at fs/dcache.c:1474 > umount_check+0x72/0x80 Hmm. Adding Al and the btrfs people to the cc, and expanding the nfs list. Unlike the flakey-IO warning, this one sounds like a real issue. Whether it's some dentry leak by nfsd or a VFS or btrfs issue, I can't begin to guess. Al, ideas? More information in the original email on lkml. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lkp] [nfsd] b44061d0b9: BUG: Dentry ffff880027d7c540{i=1846f,n=0a} still in use (1) [unmount of btrfs vda]
On Wed, Aug 10, 2016 at 11:22 AM, Josef Bacik 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. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lkp] [nfsd] b44061d0b9: BUG: Dentry ffff880027d7c540{i=1846f,n=0a} still in use (1) [unmount of btrfs vda]
On 08/10/2016 02:06 PM, Linus Torvalds wrote: On Tue, Aug 9, 2016 at 10:39 PM, kernel test robot wrote: [ 1537.558739] nfsd: last server has exited, flushing export cache [ 1540.627795] BUG: Dentry 880027d7c540{i=1846f,n=0a} still in use (1) [unmount of btrfs vda] [ 1540.633915] [ cut here ] [ 1540.636551] WARNING: CPU: 0 PID: 20552 at fs/dcache.c:1474 umount_check+0x72/0x80 Hmm. Adding Al and the btrfs people to the cc, and expanding the nfs list. Unlike the flakey-IO warning, this one sounds like a real issue. Whether it's some dentry leak by nfsd or a VFS or btrfs issue, I can't begin to guess. Al, ideas? 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? Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lkp] [nfsd] b44061d0b9: BUG: Dentry ffff880027d7c540{i=1846f,n=0a} still in use (1) [unmount of btrfs vda]
On Wed, Aug 10, 2016 at 11:46 AM, Josef Bacik 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lkp] [nfsd] b44061d0b9: BUG: Dentry ffff880027d7c540{i=1846f,n=0a} still in use (1) [unmount of btrfs vda]
On Wed, Aug 10, 2016 at 12:09 PM, J. Bruce Fields 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lkp] [nfsd] b44061d0b9: BUG: Dentry ffff880027d7c540{i=1846f,n=0a} still in use (1) [unmount of btrfs vda]
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 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 > 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 > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lkp] [nfsd] b44061d0b9: BUG: Dentry ffff880027d7c540{i=1846f,n=0a} still in use (1) [unmount of btrfs vda]
On Wed, Aug 10, 2016 at 11:56:15AM -0700, Linus Torvalds wrote: > On Wed, Aug 10, 2016 at 11:46 AM, Josef Bacik 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lkp] [nfsd] b44061d0b9: BUG: Dentry ffff880027d7c540{i=1846f,n=0a} still in use (1) [unmount of btrfs vda]
On 08/10/2016 02:25 PM, Linus Torvalds wrote: On Wed, Aug 10, 2016 at 11:22 AM, Josef Bacik 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 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 --- 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); } -- 2.5.5 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lkp] [nfsd] b44061d0b9: BUG: Dentry ffff880027d7c540{i=1846f,n=0a} still in use (1) [unmount of btrfs vda]
On 08/10/2016 02:25 PM, Linus Torvalds wrote: On Wed, Aug 10, 2016 at 11:22 AM, Josef Bacik 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. Looks like an NFS problem. Before in the !resfhp->fh_dentry case we would do a lookup_one_len() and carry on. Now we do the lookup_one_len(), call into nfsd_create_locked() and do a dget on resfhp->f_dentry, which from looks of it is the dentry we just did a lookup_one_len() on, so we have one more ref on the dentry than we did previously. Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html