Re: [PATCH v7 12/14] NFSD: Introduce struct nfsd_write_dio_seg

[Mike Snitzer <snitzer@kernel.org>]

On Fri, Oct 24, 2025 at 10:43:04AM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Passing iter arrays by reference is a little risky. Instead, pass a
> struct with a fixed-size array so bounds checking can be done.
> 
> Name each item in the array a "segment", as the term "extent"
> generally refers to a set of blocks on storage, not to a buffer.

Well both ondisk and memory are in play when setting up DIO and
checking alignment.  So length of associated ondisk is an extent.  SO
this is just to say, this header might do well to not get hung up on
past naming.

Not a big deal either way, we're ensuring the DIO-aligned middle
_extent_ ondisk, our alignment additional alignment checking can be
constrained to focus purely on memory segment checking.. so I agree
with your desire to flip the focus to "segment".

> Each segment is processed via a single vfs_iocb_iter_write() call,
> and is either IOCB_DIRECT or buffered.
> 
> Introduce a segment constructor function so each segment is
> initialized identically.
> 
> Each segment has its own length. The loop that iterates over the
> segment array can simply skip over the segments of zero length.
> A count of segments is not needed.
> 
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Code looks great, matches how I read Christoph's suggestion, nice
done.  Thanks for running with this.

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

ps. super small nit below.

> @@ -1373,36 +1369,33 @@ nfsd_issue_write_dio(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		     unsigned int nvecs, unsigned long *cnt)
>  {
>  	struct file *file = args->nf->nf_file;
> -	bool iter_is_dio_aligned[3];
> -	struct iov_iter iter_stack[3];
> -	struct iov_iter *iter = iter_stack;
> -	unsigned int n_iters = 0;
> -	unsigned long in_count = *cnt;
> -	loff_t in_offset = kiocb->ki_pos;
> +	struct nfsd_write_dio_seg *segment;
>  	ssize_t host_err;
> +	size_t i;
>  
> -	n_iters = nfsd_setup_write_dio_iters(&iter, iter_is_dio_aligned,
> -					     rqstp->rq_bvec, nvecs, *cnt,
> -					     args);
> -	if (unlikely(!n_iters))
> +	if (!nfsd_setup_write_dio_iters(args, rqstp->rq_bvec, nvecs, *cnt))
>  		return nfsd_iocb_write(file, rqstp->rq_bvec, nvecs,
>  				       cnt, kiocb);
>  
> -	trace_nfsd_write_direct(rqstp, fhp, in_offset, in_count);
> -
>  	*cnt = 0;
> -	for (int i = 0; i < n_iters; i++) {
> -		if (iter_is_dio_aligned[i])
> +	segment = args->segment;
> +	for (i = 0; i < ARRAY_SIZE(args->segment); i++) {
> +		if (segment->len == 0)
> +			continue;
> +		if (segment->use_dio) {
>  			kiocb->ki_flags |= IOCB_DIRECT;
> -		else
> +			trace_nfsd_write_direct(rqstp, fhp, kiocb->ki_pos,
> +						segment->len);
> +		} else
>  			kiocb->ki_flags &= ~IOCB_DIRECT;
>  
> -		host_err = vfs_iocb_iter_write(file, kiocb, &iter[i]);
> +		host_err = vfs_iocb_iter_write(file, kiocb, &segment->iter);
>  		if (host_err < 0)
>  			return host_err;
>  		*cnt += host_err;
> -		if (host_err < iter[i].count) /* partial write? */
> +		if (host_err < segment->iter.count)
>  			break;
> +		++segment;
>  	}

I think the /* partial write? */ comment helps a bit. Maybe switch to:

 		if (host_err < segment->iter.count)
  			break; /* partial write */

Or not, up to you... ;)