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

On Wed, Jul 15, 2026 at 8:05 PM Pranjal Shrivastava <[email protected]> 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 <[email protected]>
> ---
>  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);
> +               nfs_direct_release_pages(pagevec + i, npages - i, pinned);
> +       }
> +
> +       kvfree(pagevec);
> +       return bytes;
> +}
> +
>  void nfs_init_cinfo_from_dreq(struct nfs_commit_info *cinfo,
>                               struct nfs_direct_req *dreq)
>  {
> @@ -346,6 +390,7 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
>         ssize_t result = -EINVAL;
>         size_t requested_bytes = 0;
>         size_t rsize = max_t(size_t, NFS_SERVER(inode)->rsize, PAGE_SIZE);
> +       LIST_HEAD(nfs_page_list);
>
>         nfs_pageio_init_read(&desc, dreq->inode, false,
>                              &nfs_direct_read_completion_ops);
> @@ -354,43 +399,23 @@ 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 = NULL;
> -               size_t bytes;
> -               size_t pgbase;
> -               unsigned npages, i;
> -               bool pinned = iov_iter_extract_will_pin(iter);
> -
> -               result = iov_iter_extract_pages(iter, &pagevec,
> -                                               rsize, ~0U, 0, &pgbase);
> +               result = nfs_direct_extract_pages(dreq, iter, rsize, &pos, &nfs_page_list);
>                 if (result < 0)
>                         break;
>
> -               bytes = result;
> -               npages = (result + pgbase + PAGE_SIZE - 1) / PAGE_SIZE;
> -               for (i = 0; i < npages; i++) {
> -                       struct nfs_page *req;
> -                       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],
> -                                                       pinned, pgbase, pos,
> -                                                       req_len);
> -                       if (IS_ERR(req)) {
> -                               result = PTR_ERR(req);
> -                               break;
> -                       }
> +               while (!list_empty(&nfs_page_list)) {
> +                       struct nfs_page *req = nfs_list_entry(nfs_page_list.next);
> +                       size_t req_len = req->wb_bytes;
> +
> +                       nfs_list_remove_request(req);
>                         if (!nfs_pageio_add_request(&desc, req)) {
>                                 result = desc.pg_error;
>                                 nfs_release_request(req);
> +                               nfs_release_request_list(&nfs_page_list);
>                                 break;
>                         }
> -                       pgbase = 0;
> -                       bytes -= req_len;
>                         requested_bytes += req_len;
> -                       pos += req_len;
>                 }
> -               if (i < npages)
> -                       nfs_direct_release_pages(pagevec + i, npages - i, pinned);
> -               kvfree(pagevec);
>                 if (result < 0)
>                         break;
>         }
> @@ -881,6 +906,7 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
>         ssize_t result = 0;
>         size_t requested_bytes = 0;
>         size_t wsize = max_t(size_t, NFS_SERVER(inode)->wsize, PAGE_SIZE);
> +       LIST_HEAD(nfs_page_list);
>         bool defer = false;
>
>         trace_nfs_direct_write_schedule_iovec(dreq);
> @@ -893,55 +919,32 @@ 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 = NULL;
> -               size_t bytes;
> -               size_t pgbase;
> -               unsigned npages, i;
> -               bool pinned = iov_iter_extract_will_pin(iter);
> -
> -               result = iov_iter_extract_pages(iter, &pagevec,
> -                                               wsize, ~0U, 0, &pgbase);
> +               result = nfs_direct_extract_pages(dreq, iter, wsize, &pos, &nfs_page_list);
>                 if (result < 0)
>                         break;
>
> -               bytes = result;
> -               npages = (result + pgbase + PAGE_SIZE - 1) / PAGE_SIZE;
> -               for (i = 0; i < npages; i++) {
> -                       struct nfs_page *req;
> -                       unsigned int req_len = min_t(size_t, bytes, PAGE_SIZE - pgbase);
> -
> -                       req = nfs_page_create_from_page(dreq->ctx, pagevec[i],
> -                                                       pinned, pgbase, pos,
> -                                                       req_len);
> -                       if (IS_ERR(req)) {
> -                               result = PTR_ERR(req);
> -                               break;
> -                       }
> -
> -                       if (desc.pg_error < 0) {
> -                               nfs_free_request(req);
> -                               result = desc.pg_error;
> -                               break;
> -                       }
> -
> -                       pgbase = 0;
> -                       bytes -= req_len;
> -                       requested_bytes += req_len;
> -                       pos += req_len;
> +               while (!list_empty(&nfs_page_list)) {
> +                       struct nfs_page *req = nfs_list_entry(nfs_page_list.next);
> +                       size_t req_len = req->wb_bytes;
>
> +                       nfs_list_remove_request(req);
>                         if (defer) {
>                                 nfs_mark_request_commit(req, NULL, &cinfo, 0);
> +                               requested_bytes += req_len;
>                                 continue;
>                         }
>
>                         nfs_lock_request(req);
> -                       if (nfs_pageio_add_request(&desc, req))
> +                       if (nfs_pageio_add_request(&desc, req)) {
> +                               requested_bytes += req_len;
>                                 continue;
> +                       }
>
>                         /* Exit on hard errors */
>                         if (desc.pg_error < 0 && desc.pg_error != -EAGAIN) {
>                                 result = desc.pg_error;
>                                 nfs_unlock_and_release_request(req);
> +                               nfs_release_request_list(&nfs_page_list);
>                                 break;
>                         }
>
> @@ -952,12 +955,10 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
>                         spin_unlock(&dreq->lock);
>                         nfs_unlock_request(req);
>                         nfs_mark_request_commit(req, NULL, &cinfo, 0);
> +                       requested_bytes += req_len;
>                         desc.pg_error = 0;
>                         defer = true;
>                 }
> -               if (i < npages)
> -                       nfs_direct_release_pages(pagevec + i, npages - i, pinned);
> -               kvfree(pagevec);
>                 if (result < 0)
>                         break;
>         }
> --
> 2.55.0.229.g6434b31f56-goog
>

Refactoring looks good. Consolidates extraction, error handling, and
iov_iter_revert() into a single helper shared by both read and write
paths.

Reviewed-by: Shivaji Kant <[email protected]>