| Message ID | e6fb50d4-2a27-13cd-02c8-5b8439a16b3f@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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); }