| Message ID | 20240905-delstid-v4-1-d3e5fd34d107@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: implement the "delstid" draft | expand |
On Thu, Sep 05, 2024 at 08:41:45AM -0400, Jeff Layton wrote: > At this point in compound processing, currentfh refers to the parent of > the file, not the file itself. Get the correct dentry from the delegation > stateid instead. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> Subject says "fixes ..." so IMO a Fixes: tag is warranted. Suggestions welcome. > --- > fs/nfsd/nfs4state.c | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index df69dc6af467..db90677fc016 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -5914,6 +5914,26 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status) > } > } > > +static bool > +nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh, > + struct kstat *stat) > +{ > + struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file); > + struct path path; > + > + if (!nf) > + return false; > + > + path.mnt = currentfh->fh_export->ex_path.mnt; > + path.dentry = file_dentry(nf->nf_file); > + > + if (vfs_getattr(&path, stat, > + (STATX_INO | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), > + AT_STATX_SYNC_AS_STAT)) > + return false; > + return true; > +} > + > /* > * The Linux NFS server does not offer write delegations to NFSv4.0 > * clients in order to avoid conflicts between write delegations and > @@ -5949,7 +5969,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > int cb_up; > int status = 0; > struct kstat stat; > - struct path path; > > cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client); > open->op_recall = false; > @@ -5985,20 +6004,16 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid)); > > if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { > - open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; > - trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); > - path.mnt = currentfh->fh_export->ex_path.mnt; > - path.dentry = currentfh->fh_dentry; > - if (vfs_getattr(&path, &stat, > - (STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), > - AT_STATX_SYNC_AS_STAT)) { > + if (!nfs4_delegation_stat(dp, currentfh, &stat)) { > nfs4_put_stid(&dp->dl_stid); > destroy_delegation(dp); > goto out_no_deleg; > } > + open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; > dp->dl_cb_fattr.ncf_cur_fsize = stat.size; > dp->dl_cb_fattr.ncf_initial_cinfo = > nfsd4_change_attribute(&stat, d_inode(currentfh->fh_dentry)); > + trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); > } else { > open->op_delegate_type = NFS4_OPEN_DELEGATE_READ; > trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid); > > -- > 2.46.0 >
On Thu, 2024-09-05 at 11:46 -0400, Chuck Lever wrote: > On Thu, Sep 05, 2024 at 08:41:45AM -0400, Jeff Layton wrote: > > At this point in compound processing, currentfh refers to the parent of > > the file, not the file itself. Get the correct dentry from the delegation > > stateid instead. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > Subject says "fixes ..." so IMO a Fixes: tag is warranted. > Suggestions welcome. > > Yes, sorry. I think we want: Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") > > --- > > fs/nfsd/nfs4state.c | 31 +++++++++++++++++++++++-------- > > 1 file changed, 23 insertions(+), 8 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index df69dc6af467..db90677fc016 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -5914,6 +5914,26 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status) > > } > > } > > > > +static bool > > +nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh, > > + struct kstat *stat) > > +{ > > + struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file); > > + struct path path; > > + > > + if (!nf) > > + return false; > > + > > + path.mnt = currentfh->fh_export->ex_path.mnt; > > + path.dentry = file_dentry(nf->nf_file); > > + > > + if (vfs_getattr(&path, stat, > > + (STATX_INO | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), > > + AT_STATX_SYNC_AS_STAT)) > > + return false; > > + return true; > > +} > > + > > /* > > * The Linux NFS server does not offer write delegations to NFSv4.0 > > * clients in order to avoid conflicts between write delegations and > > @@ -5949,7 +5969,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > > int cb_up; > > int status = 0; > > struct kstat stat; > > - struct path path; > > > > cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client); > > open->op_recall = false; > > @@ -5985,20 +6004,16 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > > memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid)); > > > > if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { > > - open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; > > - trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); > > - path.mnt = currentfh->fh_export->ex_path.mnt; > > - path.dentry = currentfh->fh_dentry; > > - if (vfs_getattr(&path, &stat, > > - (STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), > > - AT_STATX_SYNC_AS_STAT)) { > > + if (!nfs4_delegation_stat(dp, currentfh, &stat)) { > > nfs4_put_stid(&dp->dl_stid); > > destroy_delegation(dp); > > goto out_no_deleg; > > } > > + open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; > > dp->dl_cb_fattr.ncf_cur_fsize = stat.size; > > dp->dl_cb_fattr.ncf_initial_cinfo = > > nfsd4_change_attribute(&stat, d_inode(currentfh->fh_dentry)); > > + trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); > > } else { > > open->op_delegate_type = NFS4_OPEN_DELEGATE_READ; > > trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid); > > > > -- > > 2.46.0 > > >
On Thu, 2024-09-05 at 08:41 -0400, Jeff Layton wrote: > At this point in compound processing, currentfh refers to the parent of > the file, not the file itself. Get the correct dentry from the delegation > stateid instead. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4state.c | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index df69dc6af467..db90677fc016 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -5914,6 +5914,26 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status) > } > } > > +static bool > +nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh, > + struct kstat *stat) > +{ > + struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file); > + struct path path; > + > + if (!nf) > + return false; > + > + path.mnt = currentfh->fh_export->ex_path.mnt; > + path.dentry = file_dentry(nf->nf_file); > + > + if (vfs_getattr(&path, stat, > + (STATX_INO | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), Minor oversight here. I added STATX_INO when I was debugging, but we don't need it here. We should probably drop that flag (though it's mostly harmless). Chuck, would you be ok with fixing that up? > + AT_STATX_SYNC_AS_STAT)) > + return false; > + return true; > +} > + > /* > * The Linux NFS server does not offer write delegations to NFSv4.0 > * clients in order to avoid conflicts between write delegations and > @@ -5949,7 +5969,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > int cb_up; > int status = 0; > struct kstat stat; > - struct path path; > > cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client); > open->op_recall = false; > @@ -5985,20 +6004,16 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid)); > > if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { > - open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; > - trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); > - path.mnt = currentfh->fh_export->ex_path.mnt; > - path.dentry = currentfh->fh_dentry; > - if (vfs_getattr(&path, &stat, > - (STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), > - AT_STATX_SYNC_AS_STAT)) { > + if (!nfs4_delegation_stat(dp, currentfh, &stat)) { > nfs4_put_stid(&dp->dl_stid); > destroy_delegation(dp); > goto out_no_deleg; > } > + open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; > dp->dl_cb_fattr.ncf_cur_fsize = stat.size; > dp->dl_cb_fattr.ncf_initial_cinfo = > nfsd4_change_attribute(&stat, d_inode(currentfh->fh_dentry)); > + trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); > } else { > open->op_delegate_type = NFS4_OPEN_DELEGATE_READ; > trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid); > Thanks!
On Fri, Sep 06, 2024 at 10:08:29AM -0400, Jeff Layton wrote: > On Thu, 2024-09-05 at 08:41 -0400, Jeff Layton wrote: > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index df69dc6af467..db90677fc016 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -5914,6 +5914,26 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status) > > } > > } > > > > +static bool > > +nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh, > > + struct kstat *stat) > > +{ > > + struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file); > > + struct path path; > > + > > + if (!nf) > > + return false; > > + > > + path.mnt = currentfh->fh_export->ex_path.mnt; > > + path.dentry = file_dentry(nf->nf_file); > > + > > + if (vfs_getattr(&path, stat, > > + (STATX_INO | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), > > Minor oversight here. > > I added STATX_INO when I was debugging, but we don't need it here. We > should probably drop that flag (though it's mostly harmless). Chuck, > would you be ok with fixing that up? Fixed and squashed into nfsd-next.
On Thu, Sep 05, 2024 at 08:41:45AM -0400, Jeff Layton wrote: > At this point in compound processing, currentfh refers to the parent of > the file, not the file itself. Get the correct dentry from the delegation > stateid instead. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4state.c | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index df69dc6af467..db90677fc016 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -5914,6 +5914,26 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status) > } > } > > +static bool > +nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh, > + struct kstat *stat) > +{ > + struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file); The xfstests workflow on NFSv4.2 exhausts the capacity of both the main and scratch devices (backed by xfs) about half-way through each test run. Deleting all visible files on both devices frees only a little bit of space. The test exports can be unshared but not unmounted (EBUSY). Looks like unlinked but still open files, maybe. Bisected to this here patch. Should there be a matching nfsd_file_put() book-end for the new find_rw_file() call site? > + struct path path; > + > + if (!nf) > + return false; > + > + path.mnt = currentfh->fh_export->ex_path.mnt; > + path.dentry = file_dentry(nf->nf_file); > + > + if (vfs_getattr(&path, stat, > + (STATX_INO | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), > + AT_STATX_SYNC_AS_STAT)) > + return false; > + return true; > +} > + > /* > * The Linux NFS server does not offer write delegations to NFSv4.0 > * clients in order to avoid conflicts between write delegations and > @@ -5949,7 +5969,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > int cb_up; > int status = 0; > struct kstat stat; > - struct path path; > > cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client); > open->op_recall = false; > @@ -5985,20 +6004,16 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid)); > > if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { > - open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; > - trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); > - path.mnt = currentfh->fh_export->ex_path.mnt; > - path.dentry = currentfh->fh_dentry; > - if (vfs_getattr(&path, &stat, > - (STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), > - AT_STATX_SYNC_AS_STAT)) { > + if (!nfs4_delegation_stat(dp, currentfh, &stat)) { > nfs4_put_stid(&dp->dl_stid); > destroy_delegation(dp); > goto out_no_deleg; > } > + open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; > dp->dl_cb_fattr.ncf_cur_fsize = stat.size; > dp->dl_cb_fattr.ncf_initial_cinfo = > nfsd4_change_attribute(&stat, d_inode(currentfh->fh_dentry)); > + trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); > } else { > open->op_delegate_type = NFS4_OPEN_DELEGATE_READ; > trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid); > > -- > 2.46.0 >
On Sun, 2024-09-08 at 14:00 -0400, Chuck Lever wrote: > On Thu, Sep 05, 2024 at 08:41:45AM -0400, Jeff Layton wrote: > > At this point in compound processing, currentfh refers to the parent of > > the file, not the file itself. Get the correct dentry from the delegation > > stateid instead. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/nfs4state.c | 31 +++++++++++++++++++++++-------- > > 1 file changed, 23 insertions(+), 8 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index df69dc6af467..db90677fc016 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -5914,6 +5914,26 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status) > > } > > } > > > > +static bool > > +nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh, > > + struct kstat *stat) > > +{ > > + struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file); > > The xfstests workflow on NFSv4.2 exhausts the capacity of both the > main and scratch devices (backed by xfs) about half-way through > each test run. > > Deleting all visible files on both devices frees only a little bit > of space. The test exports can be unshared but not unmounted > (EBUSY). Looks like unlinked but still open files, maybe. > > Bisected to this here patch. > > Should there be a matching nfsd_file_put() book-end for the new > find_rw_file() call site? > Yes. Braino on my end. I was thinking that find_rw_file didn't take a reference, but it does and we do need to put it. Would you like me to respin, or do you just want to add it in the appropriate spot? > > > + struct path path; > > + > > + if (!nf) > > + return false; > > + > > + path.mnt = currentfh->fh_export->ex_path.mnt; > > + path.dentry = file_dentry(nf->nf_file); > > + > > + if (vfs_getattr(&path, stat, > > + (STATX_INO | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), > > + AT_STATX_SYNC_AS_STAT)) > > + return false; > > + return true; > > +} > > + > > /* > > * The Linux NFS server does not offer write delegations to NFSv4.0 > > * clients in order to avoid conflicts between write delegations and > > @@ -5949,7 +5969,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > > int cb_up; > > int status = 0; > > struct kstat stat; > > - struct path path; > > > > cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client); > > open->op_recall = false; > > @@ -5985,20 +6004,16 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > > memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid)); > > > > if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { > > - open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; > > - trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); > > - path.mnt = currentfh->fh_export->ex_path.mnt; > > - path.dentry = currentfh->fh_dentry; > > - if (vfs_getattr(&path, &stat, > > - (STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), > > - AT_STATX_SYNC_AS_STAT)) { > > + if (!nfs4_delegation_stat(dp, currentfh, &stat)) { > > nfs4_put_stid(&dp->dl_stid); > > destroy_delegation(dp); > > goto out_no_deleg; > > } > > + open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; > > dp->dl_cb_fattr.ncf_cur_fsize = stat.size; > > dp->dl_cb_fattr.ncf_initial_cinfo = > > nfsd4_change_attribute(&stat, d_inode(currentfh->fh_dentry)); > > + trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); > > } else { > > open->op_delegate_type = NFS4_OPEN_DELEGATE_READ; > > trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid); > > > > -- > > 2.46.0 > > >
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index df69dc6af467..db90677fc016 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -5914,6 +5914,26 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status) } } +static bool +nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh, + struct kstat *stat) +{ + struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file); + struct path path; + + if (!nf) + return false; + + path.mnt = currentfh->fh_export->ex_path.mnt; + path.dentry = file_dentry(nf->nf_file); + + if (vfs_getattr(&path, stat, + (STATX_INO | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), + AT_STATX_SYNC_AS_STAT)) + return false; + return true; +} + /* * The Linux NFS server does not offer write delegations to NFSv4.0 * clients in order to avoid conflicts between write delegations and @@ -5949,7 +5969,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, int cb_up; int status = 0; struct kstat stat; - struct path path; cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client); open->op_recall = false; @@ -5985,20 +6004,16 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid)); if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { - open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; - trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); - path.mnt = currentfh->fh_export->ex_path.mnt; - path.dentry = currentfh->fh_dentry; - if (vfs_getattr(&path, &stat, - (STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE), - AT_STATX_SYNC_AS_STAT)) { + if (!nfs4_delegation_stat(dp, currentfh, &stat)) { nfs4_put_stid(&dp->dl_stid); destroy_delegation(dp); goto out_no_deleg; } + open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; dp->dl_cb_fattr.ncf_cur_fsize = stat.size; dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat, d_inode(currentfh->fh_dentry)); + trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); } else { open->op_delegate_type = NFS4_OPEN_DELEGATE_READ; trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
At this point in compound processing, currentfh refers to the parent of the file, not the file itself. Get the correct dentry from the delegation stateid instead. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4state.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)