| Message ID | 1407594162-28342-4-git-send-email-jlayton@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 09, 2014 at 10:22:42AM -0400, Jeff Layton wrote: > It's possible that we'll have a nfs4_file that has nothing in its fds > array, but that has a populated fi_deleg_file field. Is it really possible? On a quick skim it looks like this is only used in the presence of lock stateid's, when we should have an open. OK with the cleanup I'm just not seeing a reason either one way or the other for the fi_deleg_file change. --b. > Since that function > is generally happy with any file reference, allow it to find and take > a reference to it in that situation. Also, clean up find_any_file for > better readability. > > Signed-off-by: Jeff Layton <jlayton@primarydata.com> > --- > fs/nfsd/nfs4state.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 4356d32479b2..5c5873811bd9 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -355,11 +355,20 @@ find_any_file(struct nfs4_file *f) > > spin_lock(&f->fi_lock); > ret = __nfs4_get_fd(f, O_RDWR); > - if (!ret) { > - ret = __nfs4_get_fd(f, O_WRONLY); > - if (!ret) > - ret = __nfs4_get_fd(f, O_RDONLY); > - } > + if (ret) > + goto out; > + > + ret = __nfs4_get_fd(f, O_WRONLY); > + if (ret) > + goto out; > + > + ret = __nfs4_get_fd(f, O_RDONLY); > + if (ret) > + goto out; > + > + if (f->fi_deleg_file) > + ret = get_file(f->fi_deleg_file); > +out: > spin_unlock(&f->fi_lock); > return ret; > } > -- > 1.9.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 11 Aug 2014 12:08:40 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Sat, Aug 09, 2014 at 10:22:42AM -0400, Jeff Layton wrote: > > It's possible that we'll have a nfs4_file that has nothing in its fds > > array, but that has a populated fi_deleg_file field. > > Is it really possible? On a quick skim it looks like this is only used > in the presence of lock stateid's, when we should have an open. > > OK with the cleanup I'm just not seeing a reason either one way or the > other for the fi_deleg_file change. > > --b. > You're correct. The existing code doesn't specifically require this patch since find_any_file is only used with lock stateids. It should be harmless but it won't hurt anything to drop it. I did however need this when I rebased some pnfsd patches on top of the state overhaul, and it seemed like a reasonable change from a "future-proofing" standpoint. Do you intend to the take the first two in the series? I would like to see those go in since they move the lease removal outside of spinlocks. > > Since that function > > is generally happy with any file reference, allow it to find and take > > a reference to it in that situation. Also, clean up find_any_file for > > better readability. > > > > Signed-off-by: Jeff Layton <jlayton@primarydata.com> > > --- > > fs/nfsd/nfs4state.c | 19 ++++++++++++++----- > > 1 file changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 4356d32479b2..5c5873811bd9 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -355,11 +355,20 @@ find_any_file(struct nfs4_file *f) > > > > spin_lock(&f->fi_lock); > > ret = __nfs4_get_fd(f, O_RDWR); > > - if (!ret) { > > - ret = __nfs4_get_fd(f, O_WRONLY); > > - if (!ret) > > - ret = __nfs4_get_fd(f, O_RDONLY); > > - } > > + if (ret) > > + goto out; > > + > > + ret = __nfs4_get_fd(f, O_WRONLY); > > + if (ret) > > + goto out; > > + > > + ret = __nfs4_get_fd(f, O_RDONLY); > > + if (ret) > > + goto out; > > + > > + if (f->fi_deleg_file) > > + ret = get_file(f->fi_deleg_file); > > +out: > > spin_unlock(&f->fi_lock); > > return ret; > > } > > -- > > 1.9.3 > >
On Mon, Aug 11, 2014 at 12:40:39PM -0400, Jeff Layton wrote: > On Mon, 11 Aug 2014 12:08:40 -0400 > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > On Sat, Aug 09, 2014 at 10:22:42AM -0400, Jeff Layton wrote: > > > It's possible that we'll have a nfs4_file that has nothing in its fds > > > array, but that has a populated fi_deleg_file field. > > > > Is it really possible? On a quick skim it looks like this is only used > > in the presence of lock stateid's, when we should have an open. > > > > OK with the cleanup I'm just not seeing a reason either one way or the > > other for the fi_deleg_file change. > > > > --b. > > > > You're correct. The existing code doesn't specifically require this > patch since find_any_file is only used with lock stateids. It > should be harmless but it won't hurt anything to drop it. > > I did however need this when I rebased some pnfsd patches on top of the > state overhaul, and it seemed like a reasonable change from a > "future-proofing" standpoint. So layout operations depend on this somehow? (But layouts can outlast delegations, so that must not be it.) I'm not opposed to future-proofing as long as we have some evidence about the future. > Do you intend to the take the first two in the series? I would like to > see those go in since they move the lease removal outside of spinlocks. Yes, just waiting for -rc1 comes out to push out a for-3.18 tree. --b. > > > > > Since that function > > > is generally happy with any file reference, allow it to find and take > > > a reference to it in that situation. Also, clean up find_any_file for > > > better readability. > > > > > > Signed-off-by: Jeff Layton <jlayton@primarydata.com> > > > --- > > > fs/nfsd/nfs4state.c | 19 ++++++++++++++----- > > > 1 file changed, 14 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > > index 4356d32479b2..5c5873811bd9 100644 > > > --- a/fs/nfsd/nfs4state.c > > > +++ b/fs/nfsd/nfs4state.c > > > @@ -355,11 +355,20 @@ find_any_file(struct nfs4_file *f) > > > > > > spin_lock(&f->fi_lock); > > > ret = __nfs4_get_fd(f, O_RDWR); > > > - if (!ret) { > > > - ret = __nfs4_get_fd(f, O_WRONLY); > > > - if (!ret) > > > - ret = __nfs4_get_fd(f, O_RDONLY); > > > - } > > > + if (ret) > > > + goto out; > > > + > > > + ret = __nfs4_get_fd(f, O_WRONLY); > > > + if (ret) > > > + goto out; > > > + > > > + ret = __nfs4_get_fd(f, O_RDONLY); > > > + if (ret) > > > + goto out; > > > + > > > + if (f->fi_deleg_file) > > > + ret = get_file(f->fi_deleg_file); > > > +out: > > > spin_unlock(&f->fi_lock); > > > return ret; > > > } > > > -- > > > 1.9.3 > > > > > > -- > Jeff Layton <jlayton@primarydata.com> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 11 Aug 2014 13:09:37 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Mon, Aug 11, 2014 at 12:40:39PM -0400, Jeff Layton wrote: > > On Mon, 11 Aug 2014 12:08:40 -0400 > > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > > > On Sat, Aug 09, 2014 at 10:22:42AM -0400, Jeff Layton wrote: > > > > It's possible that we'll have a nfs4_file that has nothing in its fds > > > > array, but that has a populated fi_deleg_file field. > > > > > > Is it really possible? On a quick skim it looks like this is only used > > > in the presence of lock stateid's, when we should have an open. > > > > > > OK with the cleanup I'm just not seeing a reason either one way or the > > > other for the fi_deleg_file change. > > > > > > --b. > > > > > > > You're correct. The existing code doesn't specifically require this > > patch since find_any_file is only used with lock stateids. It > > should be harmless but it won't hurt anything to drop it. > > > > I did however need this when I rebased some pnfsd patches on top of the > > state overhaul, and it seemed like a reasonable change from a > > "future-proofing" standpoint. > > So layout operations depend on this somehow? (But layouts can outlast > delegations, so that must not be it.) > > I'm not opposed to future-proofing as long as we have some evidence > about the future. > The code I'm working on does depend on this, but it's may not be correct for any arbitrary filesystem. This filesystem sets return_on_close, and we automatically return the layout to the fs when all of the open and deleg stateids go away. If the last one to drop its reference happens to be the delegation stateid, then you may have nothing in the fi_fds slots, and no way to get to the inode in order to return the layout. I've been toying with the idea of keeping an inode reference in the layout state, but I'm not sure if it's the right thing to do... > > Do you intend to the take the first two in the series? I would like to > > see those go in since they move the lease removal outside of spinlocks. > > Yes, just waiting for -rc1 comes out to push out a for-3.18 tree. > Sounds good -- no rush on them. Thanks!
On Mon, Aug 11, 2014 at 01:20:53PM -0400, Jeff Layton wrote: > On Mon, 11 Aug 2014 13:09:37 -0400 > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > On Mon, Aug 11, 2014 at 12:40:39PM -0400, Jeff Layton wrote: > > > On Mon, 11 Aug 2014 12:08:40 -0400 > > > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > > > > > On Sat, Aug 09, 2014 at 10:22:42AM -0400, Jeff Layton wrote: > > > > > It's possible that we'll have a nfs4_file that has nothing in its fds > > > > > array, but that has a populated fi_deleg_file field. > > > > > > > > Is it really possible? On a quick skim it looks like this is only used > > > > in the presence of lock stateid's, when we should have an open. > > > > > > > > OK with the cleanup I'm just not seeing a reason either one way or the > > > > other for the fi_deleg_file change. > > > > > > > > --b. > > > > > > > > > > You're correct. The existing code doesn't specifically require this > > > patch since find_any_file is only used with lock stateids. It > > > should be harmless but it won't hurt anything to drop it. > > > > > > I did however need this when I rebased some pnfsd patches on top of the > > > state overhaul, and it seemed like a reasonable change from a > > > "future-proofing" standpoint. > > > > So layout operations depend on this somehow? (But layouts can outlast > > delegations, so that must not be it.) > > > > I'm not opposed to future-proofing as long as we have some evidence > > about the future. > > > > The code I'm working on does depend on this, but it's may not be > correct for any arbitrary filesystem. > > This filesystem sets return_on_close, and we automatically return the > layout to the fs when all of the open and deleg stateids go away. If > the last one to drop its reference happens to be the delegation > stateid, then you may have nothing in the fi_fds slots, and no way to > get to the inode in order to return the layout. > > I've been toying with the idea of keeping an inode reference in the > layout state, but I'm not sure if it's the right thing to do... OK, makes sense, thanks for the explanation. But I think I'll drop this third patch for now. --b. > > > > Do you intend to the take the first two in the series? I would like to > > > see those go in since they move the lease removal outside of spinlocks. > > > > Yes, just waiting for -rc1 comes out to push out a for-3.18 tree. > > > > Sounds good -- no rush on them. > > Thanks! > -- > Jeff Layton <jlayton@primarydata.com> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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/nfs4state.c b/fs/nfsd/nfs4state.c index 4356d32479b2..5c5873811bd9 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -355,11 +355,20 @@ find_any_file(struct nfs4_file *f) spin_lock(&f->fi_lock); ret = __nfs4_get_fd(f, O_RDWR); - if (!ret) { - ret = __nfs4_get_fd(f, O_WRONLY); - if (!ret) - ret = __nfs4_get_fd(f, O_RDONLY); - } + if (ret) + goto out; + + ret = __nfs4_get_fd(f, O_WRONLY); + if (ret) + goto out; + + ret = __nfs4_get_fd(f, O_RDONLY); + if (ret) + goto out; + + if (f->fi_deleg_file) + ret = get_file(f->fi_deleg_file); +out: spin_unlock(&f->fi_lock); return ret; }
It's possible that we'll have a nfs4_file that has nothing in its fds array, but that has a populated fi_deleg_file field. Since that function is generally happy with any file reference, allow it to find and take a reference to it in that situation. Also, clean up find_any_file for better readability. Signed-off-by: Jeff Layton <jlayton@primarydata.com> --- fs/nfsd/nfs4state.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)