| Message ID | 20220901183341.1543827-1-anna@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On Sep 1, 2022, at 2:33 PM, Anna Schumaker <anna@kernel.org> wrote: > > Chuck, I tried to add in sparse read support by adding this extra > change. Unfortunately it leads to a bunch of new failing xfstests. Do > you have any thoughts about what might be going on? Is the patch okay > without the splice support? > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index adbff7737c14..e21e6cfd1c6d 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -4733,6 +4733,7 @@ static __be32 > nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, > struct nfsd4_read *read) > { > + bool splice_ok = test_bit(RQ_SPLICE_OK, &resp->rqstp->rq_flags); > unsigned long maxcount; > struct xdr_stream *xdr = resp->xdr; > struct file *file = read->rd_nf->nf_file; > @@ -4747,7 +4748,10 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, > maxcount = min_t(unsigned long, read->rd_length, > (xdr->buf->buflen - xdr->buf->len)); > > - nfserr = nfsd4_encode_readv(resp, read, file, maxcount); > + if (file->f_op->splice_read && splice_ok) > + nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount); > + else > + nfserr = nfsd4_encode_readv(resp, read, file, maxcount) > if (nfserr) > return nfserr; I applied the above change to a test server, and was able to reproduce a bunch of new test failures when using NFSv4.2. I confirmed using nfsd tracepoints that splice read and READ_PLUS is being used. I then expanded the test. When using an XFS-based export, I reproduced the failures. But I was not able to reproduce these failures with exports based on tmpfs, btrfs, or ext4. Again, I confirmed using nfsd tracepoints that splice read was being used, and mountstats on my client showed READ_PLUS is being used. Then I tried testing the XFS-backed export with NFSv4.1, and found that most of the failures appeared again. Once again, I confirmed using nfsd tracepoints that splice read is being used during the tests. Can you confirm that you see test failures with NFSv4.1 and XFS but not with NFSv4.2 / READ_PLUS with btrfs, ext4, or tmpfs? -- Chuck Lever
Hi Chuck, On Sat, Sep 3, 2022 at 1:36 PM Chuck Lever III <chuck.lever@oracle.com> wrote: > > > > > On Sep 1, 2022, at 2:33 PM, Anna Schumaker <anna@kernel.org> wrote: > > > > Chuck, I tried to add in sparse read support by adding this extra > > change. Unfortunately it leads to a bunch of new failing xfstests. Do > > you have any thoughts about what might be going on? Is the patch okay > > without the splice support? > > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > index adbff7737c14..e21e6cfd1c6d 100644 > > --- a/fs/nfsd/nfs4xdr.c > > +++ b/fs/nfsd/nfs4xdr.c > > @@ -4733,6 +4733,7 @@ static __be32 > > nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, > > struct nfsd4_read *read) > > { > > + bool splice_ok = test_bit(RQ_SPLICE_OK, &resp->rqstp->rq_flags); > > unsigned long maxcount; > > struct xdr_stream *xdr = resp->xdr; > > struct file *file = read->rd_nf->nf_file; > > @@ -4747,7 +4748,10 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, > > maxcount = min_t(unsigned long, read->rd_length, > > (xdr->buf->buflen - xdr->buf->len)); > > > > - nfserr = nfsd4_encode_readv(resp, read, file, maxcount); > > + if (file->f_op->splice_read && splice_ok) > > + nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount); > > + else > > + nfserr = nfsd4_encode_readv(resp, read, file, maxcount) > > if (nfserr) > > return nfserr; > > I applied the above change to a test server, and was able to reproduce > a bunch of new test failures when using NFSv4.2. I confirmed using nfsd > tracepoints that splice read and READ_PLUS is being used. > > I then expanded the test. When using an XFS-based export, I reproduced > the failures. But I was not able to reproduce these failures with > exports based on tmpfs, btrfs, or ext4. Again, I confirmed using nfsd > tracepoints that splice read was being used, and mountstats on my > client showed READ_PLUS is being used. > > Then I tried testing the XFS-backed export with NFSv4.1, and found > that most of the failures appeared again. Once again, I confirmed > using nfsd tracepoints that splice read is being used during the tests. > > Can you confirm that you see test failures with NFSv4.1 and XFS but > not with NFSv4.2 / READ_PLUS with btrfs, ext4, or tmpfs? I can confirm that I'm seeing the same failures with NFS v4.1 and xfs, but not with v4.2 and ext4. I didn't test btrfs or tmpfs, since the ext4 test passed. Should I re-add the splice change for v2 of this patch, in addition to addressing the other comments you had? Anna > > > -- > Chuck Lever > > >
> On Sep 6, 2022, at 2:17 PM, Anna Schumaker <anna@kernel.org> wrote: > > Hi Chuck, > > On Sat, Sep 3, 2022 at 1:36 PM Chuck Lever III <chuck.lever@oracle.com> wrote: >> >> >> >>> On Sep 1, 2022, at 2:33 PM, Anna Schumaker <anna@kernel.org> wrote: >>> >>> Chuck, I tried to add in sparse read support by adding this extra >>> change. Unfortunately it leads to a bunch of new failing xfstests. Do >>> you have any thoughts about what might be going on? Is the patch okay >>> without the splice support? >>> >>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c >>> index adbff7737c14..e21e6cfd1c6d 100644 >>> --- a/fs/nfsd/nfs4xdr.c >>> +++ b/fs/nfsd/nfs4xdr.c >>> @@ -4733,6 +4733,7 @@ static __be32 >>> nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, >>> struct nfsd4_read *read) >>> { >>> + bool splice_ok = test_bit(RQ_SPLICE_OK, &resp->rqstp->rq_flags); >>> unsigned long maxcount; >>> struct xdr_stream *xdr = resp->xdr; >>> struct file *file = read->rd_nf->nf_file; >>> @@ -4747,7 +4748,10 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, >>> maxcount = min_t(unsigned long, read->rd_length, >>> (xdr->buf->buflen - xdr->buf->len)); >>> >>> - nfserr = nfsd4_encode_readv(resp, read, file, maxcount); >>> + if (file->f_op->splice_read && splice_ok) >>> + nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount); >>> + else >>> + nfserr = nfsd4_encode_readv(resp, read, file, maxcount) >>> if (nfserr) >>> return nfserr; >> >> I applied the above change to a test server, and was able to reproduce >> a bunch of new test failures when using NFSv4.2. I confirmed using nfsd >> tracepoints that splice read and READ_PLUS is being used. >> >> I then expanded the test. When using an XFS-based export, I reproduced >> the failures. But I was not able to reproduce these failures with >> exports based on tmpfs, btrfs, or ext4. Again, I confirmed using nfsd >> tracepoints that splice read was being used, and mountstats on my >> client showed READ_PLUS is being used. >> >> Then I tried testing the XFS-backed export with NFSv4.1, and found >> that most of the failures appeared again. Once again, I confirmed >> using nfsd tracepoints that splice read is being used during the tests. >> >> Can you confirm that you see test failures with NFSv4.1 and XFS but >> not with NFSv4.2 / READ_PLUS with btrfs, ext4, or tmpfs? > > I can confirm that I'm seeing the same failures with NFS v4.1 and xfs, > but not with v4.2 and ext4. I didn't test btrfs or tmpfs, since the > ext4 test passed. > > Should I re-add the splice change for v2 of this patch, in addition to > addressing the other comments you had? Yes. Given the comment in nfsd4_read() I think READ_PLUS can't use splice read for anything but the last data segment and only if this READ_PLUS is the final operation in the COMPOUND. That will need some attention at some later point. Meanwhile I'm going to try to bisect the XFS READ failures right now. -- Chuck Lever
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index adbff7737c14..e21e6cfd1c6d 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -4733,6 +4733,7 @@ static __be32 nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, struct nfsd4_read *read) { + bool splice_ok = test_bit(RQ_SPLICE_OK, &resp->rqstp->rq_flags); unsigned long maxcount; struct xdr_stream *xdr = resp->xdr; struct file *file = read->rd_nf->nf_file; @@ -4747,7 +4748,10 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp, maxcount = min_t(unsigned long, read->rd_length, (xdr->buf->buflen - xdr->buf->len)); - nfserr = nfsd4_encode_readv(resp, read, file, maxcount); + if (file->f_op->splice_read && splice_ok) + nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount); + else + nfserr = nfsd4_encode_readv(resp, read, file, maxcount) if (nfserr) return nfserr;
From: Anna Schumaker <Anna.Schumaker@Netapp.com> When we left off with READ_PLUS, Chuck had suggested reverting the server to reply with a single NFS4_CONTENT_DATA segment essentially mimicing how the READ operation behaves. Then, a future sparse read function can be added and the server modified to support it without needing to rip out the old READ_PLUS code at the same time. This patch takes that first step. I was even able to re-use the nfsd4_encode_readv() function to remove some duplicate code. Chuck, I tried to add in sparse read support by adding this extra change. Unfortunately it leads to a bunch of new failing xfstests. Do you have any thoughts about what might be going on? Is the patch okay without the splice support? Thanks, Anna Anna Schumaker (1): NFSD: Simplify READ_PLUS fs/nfsd/nfs4xdr.c | 122 ++++++++-------------------------------------- 1 file changed, 20 insertions(+), 102 deletions(-)