Re: [PATCH v6 4/5] nfs: migrate direct I/O to iov_iter_extract_pages

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] If request creation or insertion fails mid-chunk, the iterator is not reverted for the remaining un-scheduled bytes, leading to iterator desync.
- [High] If `iov_iter_extract_pages` returns 0, the scheduling loops will spin infinitely without making progress.
--

--- Patch [4]: [PATCH v6 4/5] nfs: migrate direct I/O to iov_iter_extract_pages ---
commit 5228f408a5f9a33be680decc8231db65a3427585
Author: Pranjal Shrivastava <[email protected]>

nfs: migrate direct I/O to iov_iter_extract_pages
    
Migrate the NFS Direct I/O path away from the legacy
iov_iter_get_pages_alloc2() API to the modern iov_iter_extract_pages API.
The transition aligns NFS with the modern VFS extraction model and serves
as a preparatory step for supporting requirements such as page pinning
via GUP for DMA.

> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> index 96995736fac2a..a1479fbf4f9e1 100644
> --- a/fs/nfs/direct.c
> +++ b/fs/nfs/direct.c
[ ... ]
> @@ -348,16 +348,17 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
>  	inode_dio_begin(inode);
>  
>  	while (iov_iter_count(iter)) {
> -		struct page **pagevec;
> +		struct page **pagevec = NULL;
>  		size_t bytes;
>  		size_t pgbase;
>  		unsigned npages, i;
> +		bool pinned = iov_iter_extract_will_pin(iter);
>  
> -		result = iov_iter_get_pages_alloc2(iter, &pagevec,
> -						  rsize, &pgbase);
> +		result = iov_iter_extract_pages(iter, &pagevec,
> +						rsize, ~0U, 0, &pgbase);
>  		if (result < 0)
>  			break;

[Severity: High]
This is a pre-existing issue, but if iov_iter_extract_pages() returns 0,
will this loop spin indefinitely?

The outer loop in nfs_direct_read_schedule_iovec() relies on
iov_iter_count(iter) > 0 to terminate. If iov_iter_extract_pages() returns
0 (which can happen if zero pages are pinned), the result < 0 check won't
catch it.

This leaves the iterator unadvanced and bytes = 0, causing the loop to
repeat indefinitely and potentially leading to a kernel hang.

Should the check be updated to if (result <= 0)?

> -	
> +
>  		bytes = result;
>  		npages = (result + pgbase + PAGE_SIZE - 1) / PAGE_SIZE;
>  		for (i = 0; i < npages; i++) {
> @@ -371,7 +366,7 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
>  			unsigned int req_len = min_t(size_t, bytes, PAGE_SIZE - pgbase);
>  			/* XXX do we need to do the eof zeroing found in async_filler? */
>  			req = nfs_page_create_from_page(dreq->ctx, pagevec[i],
> -							false, pgbase, pos,
> +							pinned, pgbase, pos,
>  							req_len);
>  			if (IS_ERR(req)) {
>  				result = PTR_ERR(req);
> @@ -380,6 +375,7 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
>  			if (!nfs_pageio_add_request(&desc, req)) {
>  				result = desc.pg_error;
>  				nfs_release_request(req);
> +				i++;
>  				break;
>  			}

[Severity: High]
This is a pre-existing issue, but does this early break cause the iterator
to become desynchronized?

iov_iter_extract_pages() advances the iterator by the total extracted bytes.
If the inner loop breaks early here (e.g. because nfs_pageio_add_request()
fails), the remaining unprocessed bytes in the chunk are skipped, but the
iterator is never reverted for those skipped bytes.

The caller calculates the revert amount based on successfully processed
bytes, so the iterator remains incorrectly advanced. If the VFS retries the
remaining data, it might resume from the wrong offset and silently skip
data.

Does this path need to call iov_iter_revert() for the unprocessed bytes?

[ ... ]
> @@ -888,13 +888,14 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
>  
>  	NFS_I(inode)->write_io += iov_iter_count(iter);
>  	while (iov_iter_count(iter)) {
> -		struct page **pagevec;
> +		struct page **pagevec = NULL;
>  		size_t bytes;
>  		size_t pgbase;
>  		unsigned npages, i;
> +		bool pinned = iov_iter_extract_will_pin(iter);
>  
> -		result = iov_iter_get_pages_alloc2(iter, &pagevec,
> -						  wsize, &pgbase);
> +		result = iov_iter_extract_pages(iter, &pagevec,
> +						wsize, ~0U, 0, &pgbase);
>  		if (result < 0)
>  			break;

[Severity: High]
This isn't a bug introduced by this patch, but similar to the read path,
if iov_iter_extract_pages() returns 0 here, will it result in an infinite
loop in nfs_direct_write_schedule_iovec()?

[ ... ]
> @@ -938,6 +938,7 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
>  			if (desc.pg_error < 0 && desc.pg_error != -EAGAIN) {
>  				result = desc.pg_error;
>  				nfs_unlock_and_release_request(req);
> +				i++;
>  				break;
>  			}

[Severity: High]
This is a pre-existing issue, but this write path seems to have the same
iterator advancement leak as the read path when breaking early on hard
errors. Should it also revert the iterator for the unprocessed bytes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4