Re: [PATCH v2 2/4] NFSD: prepare nfsd_vfs_write() to use O_DIRECT on misaligned WRITEs

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Jul 31, 2025 at 04:28:13PM -0400, Jeff Layton wrote:
> On Thu, 2025-07-31 at 15:44 -0400, Mike Snitzer wrote:
> > Refactor nfsd_vfs_write() to support splitting a WRITE into parts
> > (which will be either misaligned or DIO-aligned).  Doing so in a
> > preliminary commit just allows for indentation and slight
> > transformation to be more easily understood and reviewed.
> > 
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > ---
> >  fs/nfsd/vfs.c | 50 ++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 30 insertions(+), 20 deletions(-)
> > 
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 35c29b8ade9c3..e4855c32dad12 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1341,7 +1341,6 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	struct super_block	*sb = file_inode(file)->i_sb;
> >  	struct kiocb		kiocb;
> >  	struct svc_export	*exp;
> > -	struct iov_iter		iter;
> >  	errseq_t		since;
> >  	__be32			nfserr;
> >  	int			host_err;
> > @@ -1349,6 +1348,9 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	unsigned int		pflags = current->flags;
> >  	bool			restore_flags = false;
> >  	unsigned int		nvecs;
> > +	struct iov_iter		iter_stack[1];
> > +	struct iov_iter		*iter = iter_stack;
> > +	unsigned int		n_iters = 0;
> >  
> >  	trace_nfsd_write_opened(rqstp, fhp, offset, *cnt);
> >  
> > @@ -1378,14 +1380,15 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  		kiocb.ki_flags |= IOCB_DSYNC;
> >  
> >  	nvecs = xdr_buf_to_bvec(rqstp->rq_bvec, rqstp->rq_maxpages, payload);
> > -	iov_iter_bvec(&iter, ITER_SOURCE, rqstp->rq_bvec, nvecs, *cnt);
> > +	iov_iter_bvec(&iter[0], ITER_SOURCE, rqstp->rq_bvec, nvecs, *cnt);
> > +	n_iters++;
> >  
> >  	switch (nfsd_io_cache_write) {
> >  	case NFSD_IO_DIRECT:
> >  		/* direct I/O must be aligned to device logical sector size */
> >  		if (nf->nf_dio_mem_align && nf->nf_dio_offset_align &&
> >  		    (((offset | *cnt) & (nf->nf_dio_offset_align-1)) == 0) &&
> > -		    iov_iter_is_aligned(&iter, nf->nf_dio_mem_align - 1,
> > +		    iov_iter_is_aligned(&iter[0], nf->nf_dio_mem_align - 1,
> >  					nf->nf_dio_offset_align - 1))
> >  			kiocb.ki_flags = IOCB_DIRECT;
> >  		break;
> > @@ -1396,25 +1399,32 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  		break;
> >  	}
> >  
> > -	since = READ_ONCE(file->f_wb_err);
> > -	if (verf)
> > -		nfsd_copy_write_verifier(verf, nn);
> > -	host_err = vfs_iocb_iter_write(file, &kiocb, &iter);
> > -	if (host_err < 0) {
> > -		commit_reset_write_verifier(nn, rqstp, host_err);
> > -		goto out_nfserr;
> > -	}
> > -	*cnt = host_err;
> > -	nfsd_stats_io_write_add(nn, exp, *cnt);
> > -	fsnotify_modify(file);
> > -	host_err = filemap_check_wb_err(file->f_mapping, since);
> > -	if (host_err < 0)
> > -		goto out_nfserr;
> > +	*cnt = 0;
> > +	for (int i = 0; i < n_iters; i++) {
> > +		since = READ_ONCE(file->f_wb_err);
> 
> The above assignment can stay outside the loop. No need to resample it
> on every pass, and doing that could cause you to miss errors.
> 
> > +		if (verf)
> > +			nfsd_copy_write_verifier(verf, nn);
> >  
> 
> The verf doesn't need to be copied every time either.

OK, ack to both, will fix.
 
> > -	if (stable && fhp->fh_use_wgather) {
> > -		host_err = wait_for_concurrent_writes(file);
> > -		if (host_err < 0)
> > +		host_err = vfs_iocb_iter_write(file, &kiocb, &iter[i]);
> > +		if (host_err < 0) {
> >  			commit_reset_write_verifier(nn, rqstp, host_err);
> > +			goto out_nfserr;
> > +		}
> > +		*cnt += host_err;
> > +		nfsd_stats_io_write_add(nn, exp, host_err);
> > +
> > +		fsnotify_modify(file);
> > +		host_err = filemap_check_wb_err(file->f_mapping, since);
> > +		if (host_err < 0)
> > +			goto out_nfserr;
> > +
> > +		if (stable && fhp->fh_use_wgather) {
> > +			host_err = wait_for_concurrent_writes(file);
> > +			if (host_err < 0) {
> > +				commit_reset_write_verifier(nn, rqstp, host_err);
> > +				goto out_nfserr;
> > +			}
> > +		}
> >  	}
> >  
> >  out_nfserr:
> 
> The rest looks good though.

Thanks for the review!