| Message ID | 20240207182912.30981-1-olga.kornievskaia@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] NFSv4.1/pnfs: error gracefully on partial pnfs layout | expand |
On Wed, 2024-02-07 at 13:29 -0500, Olga Kornievskaia wrote: > From: Olga Kornievskaia <kolga@netapp.com> > > Currently, if the server returns a partial layout, the client gets > stuck asking for a layout indefinitely. Until we add support for > partial layouts, treat partial layout as layout unavailable error. > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > --- > fs/nfs/nfs4proc.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index dae4c1b6cc1c..108bc7f3e8c2 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -9790,6 +9790,12 @@ nfs4_proc_layoutget(struct nfs4_layoutget > *lgp, > if (status != 0) > goto out; > > + /* Since client does not support partial pnfs layout, then > treat > + * getting a partial layout as LAYOUTUNAVAILABLE error > + */ > + if (lgp->args.range.length != lgp->res.range.length) > + task->tk_status = -NFS4ERR_LAYOUTUNAVAILABLE; I think this case is better handled by allowing the caller to set lgp- >args.minlength to an appropriate minimum value. > + > if (task->tk_status < 0) { > exception->retry = 1; > status = nfs4_layoutget_handle_exception(task, lgp, > exception);
On Wed, Feb 7, 2024 at 2:12 PM Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Wed, 2024-02-07 at 13:29 -0500, Olga Kornievskaia wrote: > > From: Olga Kornievskaia <kolga@netapp.com> > > > > Currently, if the server returns a partial layout, the client gets > > stuck asking for a layout indefinitely. Until we add support for > > partial layouts, treat partial layout as layout unavailable error. > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > --- > > fs/nfs/nfs4proc.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > index dae4c1b6cc1c..108bc7f3e8c2 100644 > > --- a/fs/nfs/nfs4proc.c > > +++ b/fs/nfs/nfs4proc.c > > @@ -9790,6 +9790,12 @@ nfs4_proc_layoutget(struct nfs4_layoutget > > *lgp, > > if (status != 0) > > goto out; > > > > + /* Since client does not support partial pnfs layout, then > > treat > > + * getting a partial layout as LAYOUTUNAVAILABLE error > > + */ > > + if (lgp->args.range.length != lgp->res.range.length) > > + task->tk_status = -NFS4ERR_LAYOUTUNAVAILABLE; > > > I think this case is better handled by allowing the caller to set lgp- > >args.minlength to an appropriate minimum value. I do not understand what this suggestion means. What I can think of is that the caller would set an appropriate minimum value and the code here would check that the result is at least as large? If so, can you explain why that's more desirable? Seems to me it'd be more lines for something that would be removed later? > > > + > > if (task->tk_status < 0) { > > exception->retry = 1; > > status = nfs4_layoutget_handle_exception(task, lgp, > > exception); > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
On Wed, Feb 7, 2024 at 2:51 PM Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote: > > On Wed, Feb 7, 2024 at 2:12 PM Trond Myklebust <trondmy@hammerspace.com> wrote: > > > > On Wed, 2024-02-07 at 13:29 -0500, Olga Kornievskaia wrote: > > > From: Olga Kornievskaia <kolga@netapp.com> > > > > > > Currently, if the server returns a partial layout, the client gets > > > stuck asking for a layout indefinitely. Until we add support for > > > partial layouts, treat partial layout as layout unavailable error. > > > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > > --- > > > fs/nfs/nfs4proc.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > index dae4c1b6cc1c..108bc7f3e8c2 100644 > > > --- a/fs/nfs/nfs4proc.c > > > +++ b/fs/nfs/nfs4proc.c > > > @@ -9790,6 +9790,12 @@ nfs4_proc_layoutget(struct nfs4_layoutget > > > *lgp, > > > if (status != 0) > > > goto out; > > > > > > + /* Since client does not support partial pnfs layout, then > > > treat > > > + * getting a partial layout as LAYOUTUNAVAILABLE error > > > + */ > > > + if (lgp->args.range.length != lgp->res.range.length) > > > + task->tk_status = -NFS4ERR_LAYOUTUNAVAILABLE; > > > > > > I think this case is better handled by allowing the caller to set lgp- > > >args.minlength to an appropriate minimum value. > > I do not understand what this suggestion means. What I can think of is > that the caller would set an appropriate minimum value and the code > here would check that the result is at least as large? A follow up question on a "minimum value". It seems that since the client would then need to set it to the same value as the "length" (ie whole file layout value), yes? So it shifts the responsibility to the server, disallowing it from returning a partial layout. > If so, can you explain why that's more desirable? Seems to me it'd be > more lines for something that would be removed later? > > > > > + > > > if (task->tk_status < 0) { > > > exception->retry = 1; > > > status = nfs4_layoutget_handle_exception(task, lgp, > > > exception); > > > > -- > > Trond Myklebust > > Linux NFS client maintainer, Hammerspace > > trond.myklebust@hammerspace.com > > > >
On Wed, 2024-02-07 at 14:58 -0500, Olga Kornievskaia wrote: > On Wed, Feb 7, 2024 at 2:51 PM Olga Kornievskaia > <olga.kornievskaia@gmail.com> wrote: > > > > On Wed, Feb 7, 2024 at 2:12 PM Trond Myklebust > > <trondmy@hammerspace.com> wrote: > > > > > > On Wed, 2024-02-07 at 13:29 -0500, Olga Kornievskaia wrote: > > > > From: Olga Kornievskaia <kolga@netapp.com> > > > > > > > > Currently, if the server returns a partial layout, the client > > > > gets > > > > stuck asking for a layout indefinitely. Until we add support > > > > for > > > > partial layouts, treat partial layout as layout unavailable > > > > error. > > > > > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > > > --- > > > > fs/nfs/nfs4proc.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > > index dae4c1b6cc1c..108bc7f3e8c2 100644 > > > > --- a/fs/nfs/nfs4proc.c > > > > +++ b/fs/nfs/nfs4proc.c > > > > @@ -9790,6 +9790,12 @@ nfs4_proc_layoutget(struct > > > > nfs4_layoutget > > > > *lgp, > > > > if (status != 0) > > > > goto out; > > > > > > > > + /* Since client does not support partial pnfs layout, > > > > then > > > > treat > > > > + * getting a partial layout as LAYOUTUNAVAILABLE error > > > > + */ > > > > + if (lgp->args.range.length != lgp->res.range.length) > > > > + task->tk_status = -NFS4ERR_LAYOUTUNAVAILABLE; > > > > > > > > > I think this case is better handled by allowing the caller to set > > > lgp- > > > > args.minlength to an appropriate minimum value. > > > > I do not understand what this suggestion means. What I can think of > > is > > that the caller would set an appropriate minimum value and the code > > here would check that the result is at least as large? > > A follow up question on a "minimum value". It seems that since the > client would then need to set it to the same value as the "length" > (ie > whole file layout value), yes? So it shifts the responsibility to the > server, disallowing it from returning a partial layout. > > > If so, can you explain why that's more desirable? Seems to me it'd > > be > > more lines for something that would be removed later? > What I'm saying is that the protocol expects the client to send the minimal acceptable layout length as a separate argument from the desired length. Right now, we set the minimal length in pnfs_alloc_init_layoutget_args() to be the smaller of PAGE_SIZE or the length of the I/O segment. The expectation is that all the pnfs drivers should be able to handle that. If you're telling me that there are drivers that do not handle being given a layout with the minimal length that is set in pnfs_alloc_init_layoutget_args(), then we should give them control over that value. > > > > > > > + > > > > if (task->tk_status < 0) { > > > > exception->retry = 1; > > > > status = nfs4_layoutget_handle_exception(task, > > > > lgp, > > > > exception); > > > > > >
On Wed, 2024-02-07 at 14:58 -0500, Olga Kornievskaia wrote: > On Wed, Feb 7, 2024 at 2:51 PM Olga Kornievskaia > <olga.kornievskaia@gmail.com> wrote: > > > > On Wed, Feb 7, 2024 at 2:12 PM Trond Myklebust > > <trondmy@hammerspace.com> wrote: > > > > > > On Wed, 2024-02-07 at 13:29 -0500, Olga Kornievskaia wrote: > > > > From: Olga Kornievskaia <kolga@netapp.com> > > > > > > > > Currently, if the server returns a partial layout, the client > > > > gets > > > > stuck asking for a layout indefinitely. Until we add support > > > > for > > > > partial layouts, treat partial layout as layout unavailable > > > > error. > > > > > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > > > --- > > > > fs/nfs/nfs4proc.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > > index dae4c1b6cc1c..108bc7f3e8c2 100644 > > > > --- a/fs/nfs/nfs4proc.c > > > > +++ b/fs/nfs/nfs4proc.c > > > > @@ -9790,6 +9790,12 @@ nfs4_proc_layoutget(struct > > > > nfs4_layoutget > > > > *lgp, > > > > if (status != 0) > > > > goto out; > > > > > > > > + /* Since client does not support partial pnfs layout, > > > > then > > > > treat > > > > + * getting a partial layout as LAYOUTUNAVAILABLE error > > > > + */ > > > > + if (lgp->args.range.length != lgp->res.range.length) > > > > + task->tk_status = -NFS4ERR_LAYOUTUNAVAILABLE; > > > > > > > > > I think this case is better handled by allowing the caller to set > > > lgp- > > > > args.minlength to an appropriate minimum value. > > > > I do not understand what this suggestion means. What I can think of > > is > > that the caller would set an appropriate minimum value and the code > > here would check that the result is at least as large? > > A follow up question on a "minimum value". It seems that since the > client would then need to set it to the same value as the "length" > (ie > whole file layout value), yes? So it shifts the responsibility to the > server, disallowing it from returning a partial layout. My expectation is that we use 'minimum length' to mean the length that is the smallest value that is acceptable to the client (i.e. the "loga_minlength" as described in RFC5661 Section 18.43). If the client cannot handle a layout that is smaller than a whole file layout, then that's the value we should set for loga_minlength. The server then gets to decide if it can honour the request, or should return NFS4ERR_LAYOUTTRYLATER. The other value is the desired length (i.e. the "loga_length" as described in RFC5661, Section 18.43). Currently, in the flexfiles client, we set that to the length of the I/O request that we're trying to obey. If the server can meet or exceed that value, it will still do so, but it doesn't need to return an error if it cannot meet that value provided that it can still return a layout with a length that meets the "loga_minlength" requirement.
On Wed, Feb 7, 2024 at 3:42 PM Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Wed, 2024-02-07 at 14:58 -0500, Olga Kornievskaia wrote: > > On Wed, Feb 7, 2024 at 2:51 PM Olga Kornievskaia > > <olga.kornievskaia@gmail.com> wrote: > > > > > > On Wed, Feb 7, 2024 at 2:12 PM Trond Myklebust > > > <trondmy@hammerspace.com> wrote: > > > > > > > > On Wed, 2024-02-07 at 13:29 -0500, Olga Kornievskaia wrote: > > > > > From: Olga Kornievskaia <kolga@netapp.com> > > > > > > > > > > Currently, if the server returns a partial layout, the client > > > > > gets > > > > > stuck asking for a layout indefinitely. Until we add support > > > > > for > > > > > partial layouts, treat partial layout as layout unavailable > > > > > error. > > > > > > > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > > > > --- > > > > > fs/nfs/nfs4proc.c | 6 ++++++ > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > > > index dae4c1b6cc1c..108bc7f3e8c2 100644 > > > > > --- a/fs/nfs/nfs4proc.c > > > > > +++ b/fs/nfs/nfs4proc.c > > > > > @@ -9790,6 +9790,12 @@ nfs4_proc_layoutget(struct > > > > > nfs4_layoutget > > > > > *lgp, > > > > > if (status != 0) > > > > > goto out; > > > > > > > > > > + /* Since client does not support partial pnfs layout, > > > > > then > > > > > treat > > > > > + * getting a partial layout as LAYOUTUNAVAILABLE error > > > > > + */ > > > > > + if (lgp->args.range.length != lgp->res.range.length) > > > > > + task->tk_status = -NFS4ERR_LAYOUTUNAVAILABLE; > > > > > > > > > > > > I think this case is better handled by allowing the caller to set > > > > lgp- > > > > > args.minlength to an appropriate minimum value. > > > > > > I do not understand what this suggestion means. What I can think of > > > is > > > that the caller would set an appropriate minimum value and the code > > > here would check that the result is at least as large? > > > > A follow up question on a "minimum value". It seems that since the > > client would then need to set it to the same value as the "length" > > (ie > > whole file layout value), yes? So it shifts the responsibility to the > > server, disallowing it from returning a partial layout. > > My expectation is that we use 'minimum length' to mean the length that > is the smallest value that is acceptable to the client (i.e. the > "loga_minlength" as described in RFC5661 Section 18.43). If the client > cannot handle a layout that is smaller than a whole file layout, then > that's the value we should set for loga_minlength. The server then gets > to decide if it can honour the request, or should return > NFS4ERR_LAYOUTTRYLATER. > > The other value is the desired length (i.e. the "loga_length" as > described in RFC5661, Section 18.43). Currently, in the flexfiles > client, we set that to the length of the I/O request that we're trying > to obey. If the server can meet or exceed that value, it will still do > so, but it doesn't need to return an error if it cannot meet that value > provided that it can still return a layout with a length that meets the > "loga_minlength" requirement. I think I'm misunderstanding a partial layout and the client's ability to support them. Is partial layout support something that generic pnfs code does or is that something that each layout driver supposed to support? I was under the impression that it is or will be generic. I'm specifically confused because in the previous email you said, the minimal (layout) length is set to a PAGE (or I/O request size) and that each driver is supposed to be able to handle that. That seems to imply to me that if the server returned back a layout of 1 page (which is a partial layout) then the layoutdriver should be OK with it (mean handle partial layouts) (which filalyout driver does not, it needs a whole file layout), is that correct? > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index dae4c1b6cc1c..108bc7f3e8c2 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -9790,6 +9790,12 @@ nfs4_proc_layoutget(struct nfs4_layoutget *lgp, if (status != 0) goto out; + /* Since client does not support partial pnfs layout, then treat + * getting a partial layout as LAYOUTUNAVAILABLE error + */ + if (lgp->args.range.length != lgp->res.range.length) + task->tk_status = -NFS4ERR_LAYOUTUNAVAILABLE; + if (task->tk_status < 0) { exception->retry = 1; status = nfs4_layoutget_handle_exception(task, lgp, exception);