[PATCH] nfsd/blocklayout: accept any minlength

[PATCH] nfsd/blocklayout: accept any minlength

[Christoph Hellwig]

Recent Linux clients have started to send less than blocksize minglength
requests.  Given that minlength is just a hint except for the magic value
of zero just don't check for it except for same zero value.  Without
this we'll hang forever during fsx runs.

Cc: stable@vger.kernel.org
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/blocklayout.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
index cdefaa3..c29d942 100644
--- a/fs/nfsd/blocklayout.c
+++ b/fs/nfsd/blocklayout.c
@@ -56,14 +56,6 @@ nfsd4_block_proc_layoutget(struct inode *inode, const struct svc_fh *fhp,
 	u32 device_generation = 0;
 	int error;
 
-	/*
-	 * We do not attempt to support I/O smaller than the fs block size,
-	 * or not aligned to it.
-	 */
-	if (args->lg_minlength < block_size) {
-		dprintk("pnfsd: I/O too small\n");
-		goto out_layoutunavailable;
-	}
 	if (seg->offset & (block_size - 1)) {
 		dprintk("pnfsd: I/O misaligned\n");
 		goto out_layoutunavailable;
-- 
1.9.1

Re: [PATCH] nfsd/blocklayout: accept any minlength

[J. Bruce Fields]

On Fri, Oct 09, 2015 at 03:03:26PM +0200, Christoph Hellwig wrote:
> Recent Linux clients have started to send less than blocksize minglength
> requests.  Given that minlength is just a hint except for the magic value
> of zero just don't check for it except for same zero value.  Without
> this we'll hang forever during fsx runs.

OK, planning to apply for 4.3 just on the assumption that you know what
you're doing, but: I don't get it--it looks like the worst that can
happen here is we just reuturn LAYOUTUNAVAILABLE to LAYOUTGET.
Shouldn't the client then just fall back on normal NFS IO?  Why the
hang?

--b.

> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfsd/blocklayout.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
> index cdefaa3..c29d942 100644
> --- a/fs/nfsd/blocklayout.c
> +++ b/fs/nfsd/blocklayout.c
> @@ -56,14 +56,6 @@ nfsd4_block_proc_layoutget(struct inode *inode, const struct svc_fh *fhp,
>  	u32 device_generation = 0;
>  	int error;
>  
> -	/*
> -	 * We do not attempt to support I/O smaller than the fs block size,
> -	 * or not aligned to it.
> -	 */
> -	if (args->lg_minlength < block_size) {
> -		dprintk("pnfsd: I/O too small\n");
> -		goto out_layoutunavailable;
> -	}
>  	if (seg->offset & (block_size - 1)) {
>  		dprintk("pnfsd: I/O misaligned\n");
>  		goto out_layoutunavailable;
> -- 
> 1.9.1

Re: [PATCH] nfsd/blocklayout: accept any minlength

[Christoph Hellwig]

On Fri, Oct 09, 2015 at 11:28:03AM -0400, J. Bruce Fields wrote:
> OK, planning to apply for 4.3 just on the assumption that you know what
> you're doing, but: I don't get it--it looks like the worst that can
> happen here is we just reuturn LAYOUTUNAVAILABLE to LAYOUTGET.
> Shouldn't the client then just fall back on normal NFS IO?  Why the
> hang?

I've just retested with Trond's latest tree and can't reproduce the
hang anymore.  It used to fence the client due to a lack of response,
but that might have been a different client bug that has now been fixed.

Re: [PATCH] nfsd/blocklayout: accept any minlength

[J. Bruce Fields]

On Fri, Oct 09, 2015 at 07:04:00PM +0200, Christoph Hellwig wrote:
> On Fri, Oct 09, 2015 at 11:28:03AM -0400, J. Bruce Fields wrote:
> > OK, planning to apply for 4.3 just on the assumption that you know what
> > you're doing, but: I don't get it--it looks like the worst that can
> > happen here is we just reuturn LAYOUTUNAVAILABLE to LAYOUTGET.
> > Shouldn't the client then just fall back on normal NFS IO?  Why the
> > hang?
> 
> I've just retested with Trond's latest tree and can't reproduce the
> hang anymore.  It used to fence the client due to a lack of response,
> but that might have been a different client bug that has now been fixed.

OK, makes sense.

This still looks like a harmless enough change, but is it still stable
and 4.3 material?

If it affected a released client then it's probably worth it even if
it's really a client bug.  If it's just something you saw once against
an -rc1, I'd rather leave it for 4.4.

--b.

Re: [PATCH] nfsd/blocklayout: accept any minlength

[Trond Myklebust]

On Fri, Oct 9, 2015 at 1:45 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 09, 2015 at 07:04:00PM +0200, Christoph Hellwig wrote:
> > On Fri, Oct 09, 2015 at 11:28:03AM -0400, J. Bruce Fields wrote:
> > > OK, planning to apply for 4.3 just on the assumption that you know what
> > > you're doing, but: I don't get it--it looks like the worst that can
> > > happen here is we just reuturn LAYOUTUNAVAILABLE to LAYOUTGET.
> > > Shouldn't the client then just fall back on normal NFS IO?  Why the
> > > hang?
> >
> > I've just retested with Trond's latest tree and can't reproduce the
> > hang anymore.  It used to fence the client due to a lack of response,
> > but that might have been a different client bug that has now been fixed.
>
> OK, makes sense.
>
> This still looks like a harmless enough change, but is it still stable
> and 4.3 material?
>
> If it affected a released client then it's probably worth it even if
> it's really a client bug.  If it's just something you saw once against
> an -rc1, I'd rather leave it for 4.4.

It's not a client bug.

The server is supposed to comply with the requirements in table 13 of
https://tools.ietf.org/html/rfc5661#section-18.43.3
Note that it should also be returning NFS4ERR_BADLAYOUT (or
NFS4ERR_LAYOUTTRYLATER if loga_minlength == 0) instead of
layoutunavailable if the loga_minlength request cannot be met.

Cheers
  Trond

Re: [PATCH] nfsd/blocklayout: accept any minlength

[J. Bruce Fields]

On Fri, Oct 09, 2015 at 01:54:22PM -0400, Trond Myklebust wrote:
> On Fri, Oct 9, 2015 at 1:45 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Fri, Oct 09, 2015 at 07:04:00PM +0200, Christoph Hellwig wrote:
> > > On Fri, Oct 09, 2015 at 11:28:03AM -0400, J. Bruce Fields wrote:
> > > > OK, planning to apply for 4.3 just on the assumption that you know what
> > > > you're doing, but: I don't get it--it looks like the worst that can
> > > > happen here is we just reuturn LAYOUTUNAVAILABLE to LAYOUTGET.
> > > > Shouldn't the client then just fall back on normal NFS IO?  Why the
> > > > hang?
> > >
> > > I've just retested with Trond's latest tree and can't reproduce the
> > > hang anymore.  It used to fence the client due to a lack of response,
> > > but that might have been a different client bug that has now been fixed.
> >
> > OK, makes sense.
> >
> > This still looks like a harmless enough change, but is it still stable
> > and 4.3 material?
> >
> > If it affected a released client then it's probably worth it even if
> > it's really a client bug.  If it's just something you saw once against
> > an -rc1, I'd rather leave it for 4.4.
> 
> It's not a client bug.
> 
> The server is supposed to comply with the requirements in table 13 of
> https://tools.ietf.org/html/rfc5661#section-18.43.3
> Note that it should also be returning NFS4ERR_BADLAYOUT (or
> NFS4ERR_LAYOUTTRYLATER if loga_minlength == 0) instead of
> layoutunavailable if the loga_minlength request cannot be met.

I had some ideas that layouts were something a server could decline just
on random whim.  Rereading that section.... OK, looks like I was
confused, TRYLATER is the closest we come to random whim.

So the following condition on the alignments of the offset also looks
wrong.  Christoph, should it be rounding the offset down instead of
rejecting in that case?

And other layoutunavailable cases might need review too.

--b.

Re: [PATCH] nfsd/blocklayout: accept any minlength

[J. Bruce Fields]

On Fri, Oct 09, 2015 at 04:04:38PM -0400, J. Bruce Fields wrote:
> On Fri, Oct 09, 2015 at 01:54:22PM -0400, Trond Myklebust wrote:
> > On Fri, Oct 9, 2015 at 1:45 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > >
> > > On Fri, Oct 09, 2015 at 07:04:00PM +0200, Christoph Hellwig wrote:
> > > > On Fri, Oct 09, 2015 at 11:28:03AM -0400, J. Bruce Fields wrote:
> > > > > OK, planning to apply for 4.3 just on the assumption that you know what
> > > > > you're doing, but: I don't get it--it looks like the worst that can
> > > > > happen here is we just reuturn LAYOUTUNAVAILABLE to LAYOUTGET.
> > > > > Shouldn't the client then just fall back on normal NFS IO?  Why the
> > > > > hang?
> > > >
> > > > I've just retested with Trond's latest tree and can't reproduce the
> > > > hang anymore.  It used to fence the client due to a lack of response,
> > > > but that might have been a different client bug that has now been fixed.
> > >
> > > OK, makes sense.
> > >
> > > This still looks like a harmless enough change, but is it still stable
> > > and 4.3 material?
> > >
> > > If it affected a released client then it's probably worth it even if
> > > it's really a client bug.  If it's just something you saw once against
> > > an -rc1, I'd rather leave it for 4.4.
> > 
> > It's not a client bug.
> > 
> > The server is supposed to comply with the requirements in table 13 of
> > https://tools.ietf.org/html/rfc5661#section-18.43.3
> > Note that it should also be returning NFS4ERR_BADLAYOUT (or
> > NFS4ERR_LAYOUTTRYLATER if loga_minlength == 0) instead of
> > layoutunavailable if the loga_minlength request cannot be met.
> 
> I had some ideas that layouts were something a server could decline just
> on random whim.  Rereading that section.... OK, looks like I was
> confused, TRYLATER is the closest we come to random whim.

So, updated the changelog as follows, let me know if I've got anything
wrong.

--b.

commit 8c3ad9cb7343
Author: Christoph Hellwig <hch@lst.de>
Date:   Fri Oct 9 15:03:26 2015 +0200

    nfsd/blocklayout: accept any minlength
    
    Recent Linux clients have started to send GETLAYOUT requests with
    minlength less than blocksize.
    
    Servers aren't really allowed to impose this kind of restriction on
    layouts; see RFC 5661 section 18.43.3 for details.
    
    This has been observed to cause indefinite hangs on fsx runs on some
    clients.
    
    Cc: stable@vger.kernel.org
    Signed-off-by: Christoph Hellwig <hch@lst.de>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
index cdefaa331a07..c29d9421bd5e 100644
--- a/fs/nfsd/blocklayout.c
+++ b/fs/nfsd/blocklayout.c
@@ -56,14 +56,6 @@ nfsd4_block_proc_layoutget(struct inode *inode, const struct svc_fh *fhp,
 	u32 device_generation = 0;
 	int error;
 
-	/*
-	 * We do not attempt to support I/O smaller than the fs block size,
-	 * or not aligned to it.
-	 */
-	if (args->lg_minlength < block_size) {
-		dprintk("pnfsd: I/O too small\n");
-		goto out_layoutunavailable;
-	}
 	if (seg->offset & (block_size - 1)) {
 		dprintk("pnfsd: I/O misaligned\n");
 		goto out_layoutunavailable;

Re: [PATCH] nfsd/blocklayout: accept any minlength

[Christoph Hellwig]

On Fri, Oct 09, 2015 at 04:04:38PM -0400, J. Bruce Fields wrote:
> I had some ideas that layouts were something a server could decline just
> on random whim.  Rereading that section.... OK, looks like I was
> confused, TRYLATER is the closest we come to random whim.
> 
> So the following condition on the alignments of the offset also looks
> wrong.  Christoph, should it be rounding the offset down instead of
> rejecting in that case?

RFC5663 is very explicit about the extents being aligned, but doesn't
say anything about LAYOUTGET requests.  It's a bit of a gray area, but
I think not handing out a layout is still the best thing to do as a client
has to be really confused to ask for an unaligned layout and expect to get
an aligned extent back.  I just need to check for the best possible error
value.

> And other layoutunavailable cases might need review too.

Will do.

Re: [PATCH] nfsd/blocklayout: accept any minlength

[J. Bruce Fields]

On Sun, Oct 11, 2015 at 03:08:22PM +0200, Christoph Hellwig wrote:
> On Fri, Oct 09, 2015 at 04:04:38PM -0400, J. Bruce Fields wrote:
> > I had some ideas that layouts were something a server could decline just
> > on random whim.  Rereading that section.... OK, looks like I was
> > confused, TRYLATER is the closest we come to random whim.
> > 
> > So the following condition on the alignments of the offset also looks
> > wrong.  Christoph, should it be rounding the offset down instead of
> > rejecting in that case?
> 
> RFC5663 is very explicit about the extents being aligned, but doesn't
> say anything about LAYOUTGET requests.  It's a bit of a gray area, but
> I think not handing out a layout is still the best thing to do as a client
> has to be really confused to ask for an unaligned layout and expect to get
> an aligned extent back.

http://tools.ietf.org/html/rfc5661#section-18.43.3 always allows a
server to round down the offset, is there any downside to doing that?

> I just need to check for the best possible error value.

INVAL or BADLAYOUT, I guess.  But just rounding down seems harmless and
compatible with the letter of the spec.

Though without any reason or a client to request an unaligned offset I
guess it's a bit academic.

--b.

> 
> > And other layoutunavailable cases might need review too.
> 
> Will do.

Re: [PATCH] nfsd/blocklayout: accept any minlength

[Christoph Hellwig]

On Fri, Oct 09, 2015 at 01:45:56PM -0400, J. Bruce Fields wrote:
> If it affected a released client then it's probably worth it even if
> it's really a client bug.  If it's just something you saw once against
> an -rc1, I'd rather leave it for 4.4.

This one is a server bug uncovered by a client change that went into
4.3-rc.  Additionally when I first reproduced this in Trond's tree it
also caused a hang, but that is now gone.  Given how little activity
the server had around time I'm confident to blame that part on an unknown
client bug.

So yes, we really need it in ASAP and in stable all the way back.