[PATCH] NFS: Handle exceptions for SEEK

[PATCH] NFS: Handle exceptions for SEEK

[Benjamin Coddington]

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

Re: [PATCH] NFS: Handle exceptions for SEEK

[J. Bruce Fields]

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

Re: [PATCH] NFS: Handle exceptions for SEEK

[Anna Schumaker]

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;
>  
> 

Re: [PATCH] NFS: Handle exceptions for SEEK

[Benjamin Coddington]

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

Re: [PATCH] NFS: Handle exceptions for SEEK

[Benjamin Coddington]

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
>