Re: [PATCH 3/4] NFS/localio: Handle short writes by retrying

[Mike Snitzer <snitzer@kernel.org>]

On Sat, Jan 03, 2026 at 12:14:59PM -0500, Trond Myklebust wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> The current code for handling short writes in localio just truncates the
> I/O and then sets an error. While that is close to how the ordinary NFS
> code behaves, it does mean there is a chance the data that got written
> is lost because it isn't persisted.
> To fix this, change localio so that the upper layers can direct the
> behaviour to persist any unstable data by rewriting it, and then
> continuing writing until an ENOSPC is hit.
> 
> Fixes: 70ba381e1a43 ("nfs: add LOCALIO support")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>

This is a pretty subtle fix in that it depends on rpc_call_done
conditionally setting task->tk_action -- is it worth adding a relevant
code comment in nfs_local_pgio_release()?

Additional inline review comment below.

> ---
>  fs/nfs/localio.c | 64 +++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 47 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
> index c5f975bb5a64..87abebbedbab 100644
> --- a/fs/nfs/localio.c
> +++ b/fs/nfs/localio.c
> @@ -58,6 +58,11 @@ struct nfs_local_fsync_ctx {
>  static bool localio_enabled __read_mostly = true;
>  module_param(localio_enabled, bool, 0644);
>  
> +static int nfs_local_do_read(struct nfs_local_kiocb *iocb,
> +			     const struct rpc_call_ops *call_ops);
> +static int nfs_local_do_write(struct nfs_local_kiocb *iocb,
> +			      const struct rpc_call_ops *call_ops);
> +
>  static inline bool nfs_client_is_local(const struct nfs_client *clp)
>  {
>  	return !!rcu_access_pointer(clp->cl_uuid.net);
> @@ -542,13 +547,50 @@ nfs_local_iocb_release(struct nfs_local_kiocb *iocb)
>  	nfs_local_iocb_free(iocb);
>  }
>  
> -static void
> -nfs_local_pgio_release(struct nfs_local_kiocb *iocb)
> +static void nfs_local_pgio_restart(struct nfs_local_kiocb *iocb,
> +				   struct nfs_pgio_header *hdr)
> +{
> +	int status = 0;
> +
> +	iocb->kiocb.ki_pos = hdr->args.offset;
> +	iocb->kiocb.ki_flags &= ~(IOCB_DSYNC | IOCB_SYNC | IOCB_DIRECT);
> +	iocb->kiocb.ki_complete = NULL;
> +	iocb->aio_complete_work = NULL;
> +	iocb->end_iter_index = -1;
> +
> +	switch (hdr->rw_mode) {
> +	case FMODE_READ:
> +		nfs_local_iters_init(iocb, ITER_DEST);
> +		status = nfs_local_do_read(iocb, hdr->task.tk_ops);
> +		break;
> +	case FMODE_WRITE:
> +		nfs_local_iters_init(iocb, ITER_SOURCE);
> +		status = nfs_local_do_write(iocb, hdr->task.tk_ops);
> +		break;
> +	default:
> +		status = -EOPNOTSUPP;
> +	}

If this is a restart, then hdr->rw_mode will never not be FMODE_READ
or FMODE_WRITE.  As such, hdr->task.tk_ops will have been initialized
(as a side-effect of the original nfs_local_do_{read,write}) _and_
reinitialized by the above new calls to them.

My point: "default" case shouldn't ever be possible.  So should a
comment be added?  Switch to BUG_ON?  Do nothing about it?

Mike

> +
> +	if (status != 0) {
> +		nfs_local_iocb_release(iocb);
> +		hdr->task.tk_status = status;
> +		nfs_local_hdr_release(hdr, hdr->task.tk_ops);
> +	}
> +}
> +
> +static void nfs_local_pgio_release(struct nfs_local_kiocb *iocb)
>  {
>  	struct nfs_pgio_header *hdr = iocb->hdr;
> +	struct rpc_task *task = &hdr->task;
> +
> +	task->tk_action = NULL;
> +	task->tk_ops->rpc_call_done(task, hdr);
>  
> -	nfs_local_iocb_release(iocb);
> -	nfs_local_hdr_release(hdr, hdr->task.tk_ops);
> +	if (task->tk_action == NULL) {
> +		nfs_local_iocb_release(iocb);
> +		task->tk_ops->rpc_release(hdr);
> +	} else
> +		nfs_local_pgio_restart(iocb, hdr);
>  }
>  
>  /*
> @@ -776,19 +818,7 @@ static void nfs_local_write_done(struct nfs_local_kiocb *iocb)
>  		pr_info_ratelimited("nfs: Unexpected direct I/O write alignment failure\n");
>  	}
>  
> -	/* Handle short writes as if they are ENOSPC */
> -	status = hdr->res.count;
> -	if (status > 0 && status < hdr->args.count) {
> -		hdr->mds_offset += status;
> -		hdr->args.offset += status;
> -		hdr->args.pgbase += status;
> -		hdr->args.count -= status;
> -		nfs_set_pgio_error(hdr, -ENOSPC, hdr->args.offset);
> -		status = -ENOSPC;
> -		/* record -ENOSPC in terms of nfs_local_pgio_done */
> -		(void) nfs_local_pgio_done(iocb, status, true);
> -	}
> -	if (hdr->task.tk_status < 0)
> +	if (status < 0)
>  		nfs_reset_boot_verifier(hdr->inode);
>  }
>  
> -- 
> 2.52.0
>