| Message ID | 164010014140.6448.18108343631467243001.stgit@klimt.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] NFSD: Fix zero-length NFSv3 WRITEs | expand |
On Tue, Dec 21, 2021 at 10:23:25AM -0500, Chuck Lever wrote: > The Linux NFS server currently responds to a zero-length NFSv3 WRITE > request with NFS3ERR_IO. It responds to a zero-length NFSv4 WRITE > with NFS4_OK and count of zero. > > RFC 1813 says of the WRITE procedure's @count argument: > > count > The number of bytes of data to be written. If count is > 0, the WRITE will succeed and return a count of 0, > barring errors due to permissions checking. > > RFC 8881 has similar language for NFSv4, though NFSv4 removed the > explicit @count argument because that value is already contained in > the opaque payload array. > > The synthetic client pynfs's WRT4 and WRT15 tests do emit zero- > length WRITEs to exercise this spec requirement, but interestingly > the Linux NFS client does not appear to emit zero-length WRITEs, > instead squelching them. > > I'm not aware of a test that can generate such WRITEs for NFSv3, so > I wrote a naive C program to generate a zero-length WRITE and test > this fix. I know it's probably only a few lines, but still may be worth posting somewhere and making it the start of a collection of protocol-level v3 tests. --b. > > Fixes: 14168d678a0f ("NFSD: Remove the RETURN_STATUS() macro") > Reported-by: Trond Myklebust <trond.myklebust@hammerspace.com> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > > Here's an alternate approach to addressing the zero-length NFSv3 > WRITE failures. > > > fs/nfsd/nfs3proc.c | 6 +----- > fs/nfsd/nfsproc.c | 5 ----- > 2 files changed, 1 insertion(+), 10 deletions(-) > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > index 4418517f6f12..2c681785186f 100644 > --- a/fs/nfsd/nfs3proc.c > +++ b/fs/nfsd/nfs3proc.c > @@ -202,15 +202,11 @@ nfsd3_proc_write(struct svc_rqst *rqstp) > fh_copy(&resp->fh, &argp->fh); > resp->committed = argp->stable; > nvecs = svc_fill_write_vector(rqstp, &argp->payload); > - if (!nvecs) { > - resp->status = nfserr_io; > - goto out; > - } > + > resp->status = nfsd_write(rqstp, &resp->fh, argp->offset, > rqstp->rq_vec, nvecs, &cnt, > resp->committed, resp->verf); > resp->count = cnt; > -out: > return rpc_success; > } > > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > index eea5b59b6a6c..1743ed04197e 100644 > --- a/fs/nfsd/nfsproc.c > +++ b/fs/nfsd/nfsproc.c > @@ -235,10 +235,6 @@ nfsd_proc_write(struct svc_rqst *rqstp) > argp->len, argp->offset); > > nvecs = svc_fill_write_vector(rqstp, &argp->payload); > - if (!nvecs) { > - resp->status = nfserr_io; > - goto out; > - } > > resp->status = nfsd_write(rqstp, fh_copy(&resp->fh, &argp->fh), > argp->offset, rqstp->rq_vec, nvecs, > @@ -247,7 +243,6 @@ nfsd_proc_write(struct svc_rqst *rqstp) > resp->status = fh_getattr(&resp->fh, &resp->stat); > else if (resp->status == nfserr_jukebox) > return rpc_drop_reply; > -out: > return rpc_success; > } >
> On Jan 3, 2022, at 4:00 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > > On Tue, Dec 21, 2021 at 10:23:25AM -0500, Chuck Lever wrote: >> The Linux NFS server currently responds to a zero-length NFSv3 WRITE >> request with NFS3ERR_IO. It responds to a zero-length NFSv4 WRITE >> with NFS4_OK and count of zero. >> >> RFC 1813 says of the WRITE procedure's @count argument: >> >> count >> The number of bytes of data to be written. If count is >> 0, the WRITE will succeed and return a count of 0, >> barring errors due to permissions checking. >> >> RFC 8881 has similar language for NFSv4, though NFSv4 removed the >> explicit @count argument because that value is already contained in >> the opaque payload array. >> >> The synthetic client pynfs's WRT4 and WRT15 tests do emit zero- >> length WRITEs to exercise this spec requirement, but interestingly >> the Linux NFS client does not appear to emit zero-length WRITEs, >> instead squelching them. >> >> I'm not aware of a test that can generate such WRITEs for NFSv3, so >> I wrote a naive C program to generate a zero-length WRITE and test >> this fix. > > I know it's probably only a few lines, It is the same kind of code that Dr. Morris has posted recently. I've saved it (somewhere). However... > but still may be worth posting > somewhere and making it the start of a collection of protocol-level v3 > tests. ... I question whether it's worth posting anything until there is a framework for collecting and maintaining such things. I do agree that the community should be working up a set of NFSv3 specific tests like this. I like Frank's idea of making them a part of pynfs, fwiw. > --b. > >> >> Fixes: 14168d678a0f ("NFSD: Remove the RETURN_STATUS() macro") >> Reported-by: Trond Myklebust <trond.myklebust@hammerspace.com> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> >> Here's an alternate approach to addressing the zero-length NFSv3 >> WRITE failures. >> >> >> fs/nfsd/nfs3proc.c | 6 +----- >> fs/nfsd/nfsproc.c | 5 ----- >> 2 files changed, 1 insertion(+), 10 deletions(-) >> >> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c >> index 4418517f6f12..2c681785186f 100644 >> --- a/fs/nfsd/nfs3proc.c >> +++ b/fs/nfsd/nfs3proc.c >> @@ -202,15 +202,11 @@ nfsd3_proc_write(struct svc_rqst *rqstp) >> fh_copy(&resp->fh, &argp->fh); >> resp->committed = argp->stable; >> nvecs = svc_fill_write_vector(rqstp, &argp->payload); >> - if (!nvecs) { >> - resp->status = nfserr_io; >> - goto out; >> - } >> + >> resp->status = nfsd_write(rqstp, &resp->fh, argp->offset, >> rqstp->rq_vec, nvecs, &cnt, >> resp->committed, resp->verf); >> resp->count = cnt; >> -out: >> return rpc_success; >> } >> >> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c >> index eea5b59b6a6c..1743ed04197e 100644 >> --- a/fs/nfsd/nfsproc.c >> +++ b/fs/nfsd/nfsproc.c >> @@ -235,10 +235,6 @@ nfsd_proc_write(struct svc_rqst *rqstp) >> argp->len, argp->offset); >> >> nvecs = svc_fill_write_vector(rqstp, &argp->payload); >> - if (!nvecs) { >> - resp->status = nfserr_io; >> - goto out; >> - } >> >> resp->status = nfsd_write(rqstp, fh_copy(&resp->fh, &argp->fh), >> argp->offset, rqstp->rq_vec, nvecs, >> @@ -247,7 +243,6 @@ nfsd_proc_write(struct svc_rqst *rqstp) >> resp->status = fh_getattr(&resp->fh, &resp->stat); >> else if (resp->status == nfserr_jukebox) >> return rpc_drop_reply; >> -out: >> return rpc_success; >> } -- Chuck Lever
On Tue, Jan 04, 2022 at 01:12:40AM +0000, Chuck Lever III wrote: > > but still may be worth posting > > somewhere and making it the start of a collection of protocol-level v3 > > tests. > > ... I question whether it's worth posting anything until there > is a framework for collecting and maintaining such things. I > do agree that the community should be working up a set of NFSv3 > specific tests like this. I like Frank's idea of making them a > part of pynfs, fwiw. Somebody did actually do a v3 pynfs that I never got around to merging, it'd be worth revisiting: https://github.com/sthaber/pynfs --b.
> On Tue, Jan 04, 2022 at 01:12:40AM +0000, Chuck Lever III wrote: > > > but still may be worth posting > > > somewhere and making it the start of a collection of protocol-level > > > v3 tests. > > > > ... I question whether it's worth posting anything until there is a > > framework for collecting and maintaining such things. I do agree that > > the community should be working up a set of NFSv3 specific tests like > > this. I like Frank's idea of making them a part of pynfs, fwiw. > > Somebody did actually do a v3 pynfs that I never got around to merging, it'd be > worth revisiting: > > https://github.com/sthaber/pynfs I'm working on that... It requires significant effort, but I have made some progress: https://github.com/ffilz/pynfs/commit/d3a1610815117cb6bdf6567e575baedb0d8809 5e I need to get back to it, but it's lower on my priority list. Frank
On Thu, Jan 06, 2022 at 10:28:10AM -0800, Frank Filz wrote: > > On Tue, Jan 04, 2022 at 01:12:40AM +0000, Chuck Lever III wrote: > > > > but still may be worth posting > > > > somewhere and making it the start of a collection of protocol-level > > > > v3 tests. > > > > > > ... I question whether it's worth posting anything until there is a > > > framework for collecting and maintaining such things. I do agree that > > > the community should be working up a set of NFSv3 specific tests like > > > this. I like Frank's idea of making them a part of pynfs, fwiw. > > > > Somebody did actually do a v3 pynfs that I never got around to merging, > it'd be > > worth revisiting: > > > > https://github.com/sthaber/pynfs > > I'm working on that... It requires significant effort, but I have made some > progress: > > https://github.com/ffilz/pynfs/commit/d3a1610815117cb6bdf6567e575baedb0d8809 > 5e > > I need to get back to it, but it's lower on my priority list. Oh, good to know, thanks. --b.
> On Thu, Jan 06, 2022 at 10:28:10AM -0800, Frank Filz wrote: > > > On Tue, Jan 04, 2022 at 01:12:40AM +0000, Chuck Lever III wrote: > > > > > but still may be worth posting > > > > > somewhere and making it the start of a collection of > > > > > protocol-level > > > > > v3 tests. > > > > > > > > ... I question whether it's worth posting anything until there is > > > > a framework for collecting and maintaining such things. I do agree > > > > that the community should be working up a set of NFSv3 specific > > > > tests like this. I like Frank's idea of making them a part of pynfs, fwiw. > > > > > > Somebody did actually do a v3 pynfs that I never got around to > > > merging, > > it'd be > > > worth revisiting: > > > > > > https://github.com/sthaber/pynfs > > > > I'm working on that... It requires significant effort, but I have made > > some > > progress: > > > > https://github.com/ffilz/pynfs/commit/d3a1610815117cb6bdf6567e575baedb > > 0d8809 > > 5e > > > > I need to get back to it, but it's lower on my priority list. > > Oh, good to know, thanks. Are you going to continue to maintain pynfs? Your repo is the one we use as our "gold standard". Frank
On Thu, Jan 06, 2022 at 11:37:46AM -0800, Frank Filz wrote: > > On Thu, Jan 06, 2022 at 10:28:10AM -0800, Frank Filz wrote: > > > > On Tue, Jan 04, 2022 at 01:12:40AM +0000, Chuck Lever III wrote: > > > > > > but still may be worth posting > > > > > > somewhere and making it the start of a collection of > > > > > > protocol-level > > > > > > v3 tests. > > > > > > > > > > ... I question whether it's worth posting anything until there is > > > > > a framework for collecting and maintaining such things. I do agree > > > > > that the community should be working up a set of NFSv3 specific > > > > > tests like this. I like Frank's idea of making them a part of pynfs, > fwiw. > > > > > > > > Somebody did actually do a v3 pynfs that I never got around to > > > > merging, > > > it'd be > > > > worth revisiting: > > > > > > > > https://github.com/sthaber/pynfs > > > > > > I'm working on that... It requires significant effort, but I have made > > > some > > > progress: > > > > > > https://github.com/ffilz/pynfs/commit/d3a1610815117cb6bdf6567e575baedb > > > 0d8809 > > > 5e > > > > > > I need to get back to it, but it's lower on my priority list. > > > > Oh, good to know, thanks. > > Are you going to continue to maintain pynfs? Your repo is the one we use as > our "gold standard". For now, yeah. --b.
diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c index 4418517f6f12..2c681785186f 100644 --- a/fs/nfsd/nfs3proc.c +++ b/fs/nfsd/nfs3proc.c @@ -202,15 +202,11 @@ nfsd3_proc_write(struct svc_rqst *rqstp) fh_copy(&resp->fh, &argp->fh); resp->committed = argp->stable; nvecs = svc_fill_write_vector(rqstp, &argp->payload); - if (!nvecs) { - resp->status = nfserr_io; - goto out; - } + resp->status = nfsd_write(rqstp, &resp->fh, argp->offset, rqstp->rq_vec, nvecs, &cnt, resp->committed, resp->verf); resp->count = cnt; -out: return rpc_success; } diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c index eea5b59b6a6c..1743ed04197e 100644 --- a/fs/nfsd/nfsproc.c +++ b/fs/nfsd/nfsproc.c @@ -235,10 +235,6 @@ nfsd_proc_write(struct svc_rqst *rqstp) argp->len, argp->offset); nvecs = svc_fill_write_vector(rqstp, &argp->payload); - if (!nvecs) { - resp->status = nfserr_io; - goto out; - } resp->status = nfsd_write(rqstp, fh_copy(&resp->fh, &argp->fh), argp->offset, rqstp->rq_vec, nvecs, @@ -247,7 +243,6 @@ nfsd_proc_write(struct svc_rqst *rqstp) resp->status = fh_getattr(&resp->fh, &resp->stat); else if (resp->status == nfserr_jukebox) return rpc_drop_reply; -out: return rpc_success; }
The Linux NFS server currently responds to a zero-length NFSv3 WRITE request with NFS3ERR_IO. It responds to a zero-length NFSv4 WRITE with NFS4_OK and count of zero. RFC 1813 says of the WRITE procedure's @count argument: count The number of bytes of data to be written. If count is 0, the WRITE will succeed and return a count of 0, barring errors due to permissions checking. RFC 8881 has similar language for NFSv4, though NFSv4 removed the explicit @count argument because that value is already contained in the opaque payload array. The synthetic client pynfs's WRT4 and WRT15 tests do emit zero- length WRITEs to exercise this spec requirement, but interestingly the Linux NFS client does not appear to emit zero-length WRITEs, instead squelching them. I'm not aware of a test that can generate such WRITEs for NFSv3, so I wrote a naive C program to generate a zero-length WRITE and test this fix. Fixes: 14168d678a0f ("NFSD: Remove the RETURN_STATUS() macro") Reported-by: Trond Myklebust <trond.myklebust@hammerspace.com> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- Here's an alternate approach to addressing the zero-length NFSv3 WRITE failures. fs/nfsd/nfs3proc.c | 6 +----- fs/nfsd/nfsproc.c | 5 ----- 2 files changed, 1 insertion(+), 10 deletions(-)