Re: [PATCH v3 5/5] nfs: introduce nfs_direct_extract_pages helper

[Pranjal Shrivastava <praan@google.com>]

On Wed, Aug 05, 2026 at 01:12:28PM -0700, Trond Myklebust wrote:

Hi Trond,

> On Wed, 2026-07-15 at 14:35 +0000, Pranjal Shrivastava wrote:
> > Introduce nfs_direct_extract_pages() in direct.c to centralize page
> > extraction and request creation for the Direct I/O path. The helper
> > manages extraction from the iters and builds a list of nfs_page
> > requests
> > 
> > Refactor nfs_direct_read_schedule_iovec() and
> > nfs_direct_write_schedule_iovec() to utilize the new helper, unifying
> > the extraction logic on both paths.
> > 
> > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > ---
> >  fs/nfs/direct.c | 127 ++++++++++++++++++++++++----------------------
> > --
> >  1 file changed, 64 insertions(+), 63 deletions(-)
> > 
> > diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> > index b9ac0a67693c..d31e4720ffff 100644
> > --- a/fs/nfs/direct.c
> > +++ b/fs/nfs/direct.c
> > @@ -178,6 +178,50 @@ static void nfs_direct_release_pages(struct page
> > **pages, unsigned int npages,
> >  	}
> >  }
> >  
> > +static ssize_t nfs_direct_extract_pages(struct nfs_direct_req *dreq,
> > +					 struct iov_iter *iter,
> > +					 size_t size, loff_t *pos,
> > +					 struct list_head *list)
> > +{
> > +	bool pinned = iov_iter_extract_will_pin(iter);
> > +	struct page **pagevec = NULL;
> > +	ssize_t result, bytes = 0;
> > +	unsigned int npages, i;
> > +	size_t pgbase;
> > +
> > +	result = iov_iter_extract_pages(iter, &pagevec, size, ~0U,
> > 0, &pgbase);
> > +	if (result <= 0)
> > +		return result;
> > +
> > +	npages = (result + pgbase + PAGE_SIZE - 1) >> PAGE_SHIFT;
> > +	for (i = 0; i < npages; i++) {
> > +		struct nfs_page *req;
> > +		unsigned int req_len = min_t(size_t, result - bytes,
> > PAGE_SIZE - pgbase);
> > +
> > +		req = nfs_page_create_from_page(dreq->ctx,
> > pagevec[i],
> > +						pinned, pgbase,
> > *pos,
> > +						req_len);
> > +		if (IS_ERR(req)) {
> > +			if (!bytes)
> > +				bytes = PTR_ERR(req);
> > +			break;
> > +		}
> > +
> > +		list_add_tail(&req->wb_list, list);
> > +		pgbase = 0;
> > +		bytes += req_len;
> > +		*pos += req_len;
> > +	}
> > +
> > +	if (i < npages) {
> > +		iov_iter_revert(iter, result - bytes);
> 
> Oopsable if bytes == PTR_ERR(req)
> 

Ack. I'll catch errors in a separate variable something like:

 ...
    for (i = 0; i < npages; i++) {
 ...
        req = nfs_page_create_from_page(...);
        if (IS_ERR(req)) {
            err = PTR_ERR(req);
            break;
        }
 ...
        bytes += req_len;
    }

    if (i < npages) {
        iov_iter_revert(iter, result - bytes);
        nfs_direct_release_pages(pagevec + i, npages - i, pinned);
    }

    kvfree(pagevec);
    return bytes ? bytes : err;

> > +		nfs_direct_release_pages(pagevec + i, npages - i,
> > pinned);
> 
> See what happens above with swap over NFS (which sets pinned = false).
> 

I see what you mean, we never got a ref (after patch 4) but we'll call
put_page() if pinned == false. I'll reduce the nfs_direct_release_pages
to the following in patch 4 (when we start using the new extract API):

static void nfs_direct_release_pages(struct page **pages, unsigned int npages,
				     bool pinned)
{
	if (pinned) 
		unpin_user_pages(pages, npages);
}


Sounds good for v5? 

Thanks,
Praan