Re: [PATCH v4 1/4] NFSD: avoid using iov_iter_is_aligned() in nfsd_iter_read()

[Chuck Lever <chuck.lever@oracle.com>]

On 8/6/25 11:57 AM, Mike Snitzer wrote:
> On Wed, Aug 06, 2025 at 09:18:51AM -0400, Chuck Lever wrote:
>> On 8/5/25 2:44 PM, Mike Snitzer wrote:
>>> From: Mike Snitzer <snitzer@hammerspace.com>
>>>
>>> Check the bvec is DIO-aligned while creating it, saves CPU cycles by
>>> avoiding iterating the bvec elements a second time using
>>> iov_iter_is_aligned().
>>>
>>> This prepares for Keith Busch's near-term removal of the
>>> iov_iter_is_aligned() interface.  This fixes cel/nfsd-testing commit
>>> 5d78ac1e674b4 ("NFSD: issue READs using O_DIRECT even if IO is
>>> misaligned") and it should be folded into that commit so that NFSD
>>> doesn't require iov_iter_is_aligned() while it is being removed
>>> upstream in parallel.
>>>
>>> Fixes: cel/nfsd-testing 5d78ac1e674b4 ("NFSD: issue READs using O_DIRECT even if IO is misaligned")
>>> Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
>>> ---
>>>  fs/nfsd/vfs.c | 29 +++++++++++++++--------------
>>>  1 file changed, 15 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>> index 46189020172fb..e1751d3715264 100644
>>> --- a/fs/nfsd/vfs.c
>>> +++ b/fs/nfsd/vfs.c
>>> @@ -1226,7 +1226,10 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>  			 */
>>>  			offset = read_dio.start;
>>>  			in_count = read_dio.end - offset;
>>> -			kiocb.ki_flags = IOCB_DIRECT;
>>> +			/* Verify ondisk DIO alignment, memory addrs checked below */
>>> +			if (likely(((offset | in_count) &
>>> +				    (nf->nf_dio_read_offset_align - 1)) == 0))
>>> +				kiocb.ki_flags = IOCB_DIRECT;
>>>  		}
>>>  	} else if (nfsd_io_cache_read == NFSD_IO_DONTCACHE)
>>>  		kiocb.ki_flags = IOCB_DONTCACHE;
>>> @@ -1236,16 +1239,24 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>  	v = 0;
>>>  	total = in_count;
>>>  	if (read_dio.start_extra) {
>>> -		bvec_set_page(&rqstp->rq_bvec[v++], read_dio.start_extra_page,
>>> +		bvec_set_page(&rqstp->rq_bvec[v], read_dio.start_extra_page,
>>>  			      read_dio.start_extra, PAGE_SIZE - read_dio.start_extra);
>>> +		if (unlikely((kiocb.ki_flags & IOCB_DIRECT) &&
>>> +			     rqstp->rq_bvec[v].bv_offset & (nf->nf_dio_mem_align - 1)))
>>> +			kiocb.ki_flags &= ~IOCB_DIRECT;
>>>  		total -= read_dio.start_extra;
>>> +		v++;
>>>  	}
>>>  	while (total) {
>>>  		len = min_t(size_t, total, PAGE_SIZE - base);
>>> -		bvec_set_page(&rqstp->rq_bvec[v++], *(rqstp->rq_next_page++),
>>> -			      len, base);
>>> +		bvec_set_page(&rqstp->rq_bvec[v], *(rqstp->rq_next_page++), len, base);
>>> +		/* No need to verify memory is DIO-aligned since bv_offset is 0 */
>>> +		if (unlikely((kiocb.ki_flags & IOCB_DIRECT) && base &&
>>> +			     (base & (nf->nf_dio_mem_align - 1))))
>>> +			kiocb.ki_flags &= ~IOCB_DIRECT;
>>>  		total -= len;
>>>  		base = 0;
>>> +		v++;
>>>  	}
>>>  	if (WARN_ONCE(v > rqstp->rq_maxpages,
>>>  		      "%s: v=%lu exceeds rqstp->rq_maxpages=%lu\n", __func__,
>>> @@ -1256,16 +1267,6 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>  	if (!host_err) {
>>>  		trace_nfsd_read_vector(rqstp, fhp, offset, in_count);
>>>  		iov_iter_bvec(&iter, ITER_DEST, rqstp->rq_bvec, v, in_count);
>>> -
>>> -		/* Double check nfsd_analyze_read_dio's DIO-aligned result */
>>> -		if (unlikely((kiocb.ki_flags & IOCB_DIRECT) &&
>>> -			     !iov_iter_is_aligned(&iter,
>>> -				nf->nf_dio_mem_align - 1,
>>> -				nf->nf_dio_read_offset_align - 1))) {
>>> -			/* Fallback to buffered IO */
>>> -			kiocb.ki_flags &= ~IOCB_DIRECT;
>>> -		}
>>> -
>>>  		host_err = vfs_iocb_iter_read(file, &kiocb, &iter);
>>>  	}
>>>  
>>
>> Hi Mike,
>>
>> In cases where the SQUASHME patch is this large, I usually drop the
>> patch (or series) in nfsd-testing and ask the contributor to rebase and
>> repost. This gets the new version of the patch properly archived on
>> lore, for one thing.
> 
> Yeah, make sense, I missed that iov_iter_is_aligned() was used early
> on in the series too, so I'll fixup further back.
>  
>> Before reposting, please do run checkpatch.pl on the series.
> 
> Will do, will also ensure bisect safe and that sparse is happy.

Thanks for the changes, sounds like the right path forward.


-- 
Chuck Lever