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

[Mike Snitzer <snitzer@kernel.org>]

On Mon, Jan 05, 2026 at 01:09:50PM -0500, Trond Myklebust wrote:
> On Mon, 2026-01-05 at 13:04 -0500, Mike Snitzer wrote:
> > 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()?
> > 
> 
> That's how restarts work in the RPC code: after the rpc_call_done
> callback is done, rpc_exit_task() will check for whether or not the
> task->tk_action got reset, and if so, it will prepare a new RPC call.
> 
> > 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?
> > 
> 
> I considered a BUG_ON(), but it shouldn't really matter. All this does
> now is cancel the restart.

OK, thanks.

Reviewed-by: Mike Snitzer <snitzer@kernel.org>