* Re: [PATCH] NFS: Handle exceptions for SEEK
2015-06-14 21:35 [PATCH] NFS: Handle exceptions for SEEK Benjamin Coddington
@ 2015-06-15 17:26 ` J. Bruce Fields
2015-06-16 12:34 ` Anna Schumaker
2015-07-22 8:19 ` Benjamin Coddington
2 siblings, 0 replies; 5+ messages in thread
From: J. Bruce Fields @ 2015-06-15 17:26 UTC (permalink / raw)
To: Benjamin Coddington; +Cc: linux-nfs, Trond Myklebust, Anna Schumaker
On Sun, Jun 14, 2015 at 05:35:05PM -0400, Benjamin Coddington wrote:
> Don't pass along NFS4ERR_* exceptions to VFS and do recovery if needed.
> These exceptions might look like offsets (even though they are negative) and
> applications that used the previous behavior of seeking to eof for
> SEEK_HOLE, and current offset for SEEK_DATA may be caught off-guard if the server
> responds with transient errors such as NFS4ERR_GRACE.
Whoops. Yes, looks like that should go to stable too?
--b.
>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
> fs/nfs/nfs42proc.c | 23 +++++++++++++++--------
> 1 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> index 3a9e752..9c3c9d2 100644
> --- a/fs/nfs/nfs42proc.c
> +++ b/fs/nfs/nfs42proc.c
> @@ -146,20 +146,27 @@ loff_t nfs42_proc_llseek(struct file *filep, loff_t offset, int whence)
> .rpc_resp = &res,
> };
> struct nfs_server *server = NFS_SERVER(inode);
> + struct nfs4_exception exception = { };
> int status;
>
> if (!nfs_server_capable(inode, NFS_CAP_SEEK))
> return -ENOTSUPP;
>
> - status = nfs42_set_rw_stateid(&args.sa_stateid, filep, FMODE_READ);
> - if (status)
> - return status;
> -
> nfs_wb_all(inode);
> - status = nfs4_call_sync(server->client, server, &msg,
> - &args.seq_args, &res.seq_res, 0);
> - if (status == -ENOTSUPP)
> - server->caps &= ~NFS_CAP_SEEK;
> + do {
> + status = nfs42_set_rw_stateid(&args.sa_stateid, filep, FMODE_READ);
> + if (status)
> + return status;
> +
> + status = nfs4_call_sync(server->client, server, &msg,
> + &args.seq_args, &res.seq_res, 0);
> + if (status == -ENOTSUPP) {
> + server->caps &= ~NFS_CAP_SEEK;
> + return status;
> + }
> + status = nfs4_handle_exception(server, status, &exception);
> + } while (exception.retry);
> +
> if (status)
> return status;
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] NFS: Handle exceptions for SEEK
2015-06-14 21:35 [PATCH] NFS: Handle exceptions for SEEK Benjamin Coddington
2015-06-15 17:26 ` J. Bruce Fields
@ 2015-06-16 12:34 ` Anna Schumaker
2015-06-16 12:49 ` Benjamin Coddington
2015-07-22 8:19 ` Benjamin Coddington
2 siblings, 1 reply; 5+ messages in thread
From: Anna Schumaker @ 2015-06-16 12:34 UTC (permalink / raw)
To: Benjamin Coddington, linux-nfs
Thanks for finding this!
On 06/14/2015 05:35 PM, Benjamin Coddington wrote:
> Don't pass along NFS4ERR_* exceptions to VFS and do recovery if needed.
> These exceptions might look like offsets (even though they are negative) and
> applications that used the previous behavior of seeking to eof for
> SEEK_HOLE, and current offset for SEEK_DATA may be caught off-guard if the server
> responds with transient errors such as NFS4ERR_GRACE.
>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
> fs/nfs/nfs42proc.c | 23 +++++++++++++++--------
> 1 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> index 3a9e752..9c3c9d2 100644
> --- a/fs/nfs/nfs42proc.c
> +++ b/fs/nfs/nfs42proc.c
> @@ -146,20 +146,27 @@ loff_t nfs42_proc_llseek(struct file *filep, loff_t offset, int whence)
> .rpc_resp = &res,
> };
> struct nfs_server *server = NFS_SERVER(inode);
> + struct nfs4_exception exception = { };
> int status;
>
> if (!nfs_server_capable(inode, NFS_CAP_SEEK))
> return -ENOTSUPP;
>
> - status = nfs42_set_rw_stateid(&args.sa_stateid, filep, FMODE_READ);
> - if (status)
> - return status;
> -
> nfs_wb_all(inode);
> - status = nfs4_call_sync(server->client, server, &msg,
> - &args.seq_args, &res.seq_res, 0);
> - if (status == -ENOTSUPP)
> - server->caps &= ~NFS_CAP_SEEK;
> + do {
> + status = nfs42_set_rw_stateid(&args.sa_stateid, filep, FMODE_READ);
> + if (status)
> + return status;
> +
> + status = nfs4_call_sync(server->client, server, &msg,
> + &args.seq_args, &res.seq_res, 0);
> + if (status == -ENOTSUPP) {
> + server->caps &= ~NFS_CAP_SEEK;
> + return status;
> + }
> + status = nfs4_handle_exception(server, status, &exception);
> + } while (exception.retry);
> +
Other functions using the nfs4_exception structure have the exception handling in a single function, and then call something like _nfs4_proc_llseek() to set up the RPC call. Is there a reason you're not doing it that way here?
Thanks,
Anna
> if (status)
> return status;
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] NFS: Handle exceptions for SEEK
2015-06-16 12:34 ` Anna Schumaker
@ 2015-06-16 12:49 ` Benjamin Coddington
0 siblings, 0 replies; 5+ messages in thread
From: Benjamin Coddington @ 2015-06-16 12:49 UTC (permalink / raw)
To: Anna Schumaker; +Cc: linux-nfs
On Tue, 16 Jun 2015, Anna Schumaker wrote:
> Thanks for finding this!
>
> On 06/14/2015 05:35 PM, Benjamin Coddington wrote:
> > Don't pass along NFS4ERR_* exceptions to VFS and do recovery if needed.
> > These exceptions might look like offsets (even though they are negative) and
> > applications that used the previous behavior of seeking to eof for
> > SEEK_HOLE, and current offset for SEEK_DATA may be caught off-guard if the server
> > responds with transient errors such as NFS4ERR_GRACE.
> >
> > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > ---
> > fs/nfs/nfs42proc.c | 23 +++++++++++++++--------
> > 1 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> > index 3a9e752..9c3c9d2 100644
> > --- a/fs/nfs/nfs42proc.c
> > +++ b/fs/nfs/nfs42proc.c
> > @@ -146,20 +146,27 @@ loff_t nfs42_proc_llseek(struct file *filep, loff_t offset, int whence)
> > .rpc_resp = &res,
> > };
> > struct nfs_server *server = NFS_SERVER(inode);
> > + struct nfs4_exception exception = { };
> > int status;
> >
> > if (!nfs_server_capable(inode, NFS_CAP_SEEK))
> > return -ENOTSUPP;
> >
> > - status = nfs42_set_rw_stateid(&args.sa_stateid, filep, FMODE_READ);
> > - if (status)
> > - return status;
> > -
> > nfs_wb_all(inode);
> > - status = nfs4_call_sync(server->client, server, &msg,
> > - &args.seq_args, &res.seq_res, 0);
> > - if (status == -ENOTSUPP)
> > - server->caps &= ~NFS_CAP_SEEK;
> > + do {
> > + status = nfs42_set_rw_stateid(&args.sa_stateid, filep, FMODE_READ);
> > + if (status)
> > + return status;
> > +
> > + status = nfs4_call_sync(server->client, server, &msg,
> > + &args.seq_args, &res.seq_res, 0);
> > + if (status == -ENOTSUPP) {
> > + server->caps &= ~NFS_CAP_SEEK;
> > + return status;
> > + }
> > + status = nfs4_handle_exception(server, status, &exception);
> > + } while (exception.retry);
> > +
>
> Other functions using the nfs4_exception structure have the exception handling in a single function, and then call something like _nfs4_proc_llseek() to set up the RPC call. Is there a reason you're not doing it that way here?
I made a first pass at it that way, but because the response's offset needs
to be passed back to vfs_setpos, it meant passing in the nfs42_seek_res * to
_nfs4_proc_llseek(), or passing it back via the retun, or other fiddling.
It ended up being much smaller and simpler to do it this way.
Let me know and I'll send it with _nfs4_proc_llseek() instead if you'd like.
Ben
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] NFS: Handle exceptions for SEEK
2015-06-14 21:35 [PATCH] NFS: Handle exceptions for SEEK Benjamin Coddington
2015-06-15 17:26 ` J. Bruce Fields
2015-06-16 12:34 ` Anna Schumaker
@ 2015-07-22 8:19 ` Benjamin Coddington
2 siblings, 0 replies; 5+ messages in thread
From: Benjamin Coddington @ 2015-07-22 8:19 UTC (permalink / raw)
To: Anna.Schumaker, trond.myklebust; +Cc: linux-nfs
Hi Trond and Anna,
Will you take this one? I think I've been forgetting to use CC correctly..
Ben
On Sun, 14 Jun 2015, Benjamin Coddington wrote:
> Don't pass along NFS4ERR_* exceptions to VFS and do recovery if needed.
> These exceptions might look like offsets (even though they are negative) and
> applications that used the previous behavior of seeking to eof for
> SEEK_HOLE, and current offset for SEEK_DATA may be caught off-guard if the server
> responds with transient errors such as NFS4ERR_GRACE.
>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
> fs/nfs/nfs42proc.c | 23 +++++++++++++++--------
> 1 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> index 3a9e752..9c3c9d2 100644
> --- a/fs/nfs/nfs42proc.c
> +++ b/fs/nfs/nfs42proc.c
> @@ -146,20 +146,27 @@ loff_t nfs42_proc_llseek(struct file *filep, loff_t offset, int whence)
> .rpc_resp = &res,
> };
> struct nfs_server *server = NFS_SERVER(inode);
> + struct nfs4_exception exception = { };
> int status;
>
> if (!nfs_server_capable(inode, NFS_CAP_SEEK))
> return -ENOTSUPP;
>
> - status = nfs42_set_rw_stateid(&args.sa_stateid, filep, FMODE_READ);
> - if (status)
> - return status;
> -
> nfs_wb_all(inode);
> - status = nfs4_call_sync(server->client, server, &msg,
> - &args.seq_args, &res.seq_res, 0);
> - if (status == -ENOTSUPP)
> - server->caps &= ~NFS_CAP_SEEK;
> + do {
> + status = nfs42_set_rw_stateid(&args.sa_stateid, filep, FMODE_READ);
> + if (status)
> + return status;
> +
> + status = nfs4_call_sync(server->client, server, &msg,
> + &args.seq_args, &res.seq_res, 0);
> + if (status == -ENOTSUPP) {
> + server->caps &= ~NFS_CAP_SEEK;
> + return status;
> + }
> + status = nfs4_handle_exception(server, status, &exception);
> + } while (exception.retry);
> +
> if (status)
> return status;
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 5+ messages in thread