* [PATCH v6 1/5] nfs: make nfs_page pin-aware
2026-08-14 14:32 [PATCH v6 0/5] nfs: modernize direct I/O path Pranjal Shrivastava
@ 2026-08-14 14:32 ` Pranjal Shrivastava
2026-08-14 14:46 ` sashiko-bot
2026-08-14 14:32 ` [PATCH v6 2/5] nfs: track number of pinned pages in nfs_page Pranjal Shrivastava
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Pranjal Shrivastava @ 2026-08-14 14:32 UTC (permalink / raw)
To: linux-nfs, Trond Myklebust, Anna Schumaker
Cc: Chuck Lever, Jeff Layton, linux-kernel, Christoph Hellwig,
Logan Gunthorpe, Jason Gunthorpe, linux-pci, linux-rdma,
Shivaji Kant, Pranjal Shrivastava
Modernizing the NFS Direct I/O path to use iov_iter_extract_pages()
introduces page pinning (GUP) instead of standard page referencing.
To handle this correctly, nfs_page must track whether it holds a
pin or a standard reference.
Introduce a new flag, PG_PINNED, to struct nfs_page. Update the creation
path (nfs_page_create_from_page and nfs_page_create_from_folio) to
accept a pinned bool and set the flag accordingly. If the page is pinned,
we skip the existing reference increment (get_page/folio_get) as the pin
itself acts as a reference.
Update nfs_clear_request() & nfs_direct_release_pages() to use
unpin_user_page() or unpin_user_folio() instead of only refcount
decrement (put_page) when PG_PINNED flag is set. Finally, ensure
subrequests inherit the pinning status from their parent request.
Signed-off-by: Pranjal Shrivastava <praan@google.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/nfs/direct.c | 22 +++++++++++++-------
fs/nfs/pagelist.c | 44 +++++++++++++++++++++++++++++++---------
fs/nfs/read.c | 2 +-
fs/nfs/write.c | 2 +-
include/linux/nfs_page.h | 3 +++
5 files changed, 54 insertions(+), 19 deletions(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index e626c72495e6..19792a38c924 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -165,11 +165,17 @@ int nfs_swap_rw(struct kiocb *iocb, struct iov_iter *iter)
return 0;
}
-static void nfs_direct_release_pages(struct page **pages, unsigned int npages)
+static void nfs_direct_release_pages(struct page **pages, unsigned int npages,
+ bool pinned)
{
unsigned int i;
- for (i = 0; i < npages; i++)
- put_page(pages[i]);
+
+ if (pinned) {
+ unpin_user_pages(pages, npages);
+ } else {
+ for (i = 0; i < npages; i++)
+ put_page(pages[i]);
+ }
}
void nfs_init_cinfo_from_dreq(struct nfs_commit_info *cinfo,
@@ -371,7 +377,8 @@ 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],
- pgbase, pos, req_len);
+ false, pgbase, pos,
+ req_len);
if (IS_ERR(req)) {
result = PTR_ERR(req);
break;
@@ -386,7 +393,7 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
requested_bytes += req_len;
pos += req_len;
}
- nfs_direct_release_pages(pagevec, npages);
+ nfs_direct_release_pages(pagevec, npages, false);
kvfree(pagevec);
if (result < 0)
break;
@@ -907,7 +914,8 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
unsigned int req_len = min_t(size_t, bytes, PAGE_SIZE - pgbase);
req = nfs_page_create_from_page(dreq->ctx, pagevec[i],
- pgbase, pos, req_len);
+ false, pgbase, pos,
+ req_len);
if (IS_ERR(req)) {
result = PTR_ERR(req);
break;
@@ -950,7 +958,7 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
desc.pg_error = 0;
defer = true;
}
- nfs_direct_release_pages(pagevec, npages);
+ nfs_direct_release_pages(pagevec, npages, false);
kvfree(pagevec);
if (result < 0)
break;
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 7dd478ffc2fa..a562cfe2a126 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -404,20 +404,28 @@ static struct nfs_page *nfs_page_create(struct nfs_lock_context *l_ctx,
return req;
}
-static void nfs_page_assign_folio(struct nfs_page *req, struct folio *folio)
+static void nfs_page_assign_folio(struct nfs_page *req, struct folio *folio,
+ bool pinned)
{
if (folio != NULL) {
req->wb_folio = folio;
- folio_get(folio);
+ if (pinned)
+ set_bit(PG_PINNED, &req->wb_flags);
+ else
+ folio_get(folio);
set_bit(PG_FOLIO, &req->wb_flags);
}
}
-static void nfs_page_assign_page(struct nfs_page *req, struct page *page)
+static void nfs_page_assign_page(struct nfs_page *req, struct page *page,
+ bool pinned)
{
if (page != NULL) {
req->wb_page = page;
- get_page(page);
+ if (pinned)
+ set_bit(PG_PINNED, &req->wb_flags);
+ else
+ get_page(page);
}
}
@@ -425,6 +433,7 @@ static void nfs_page_assign_page(struct nfs_page *req, struct page *page)
* nfs_page_create_from_page - Create an NFS read/write request.
* @ctx: open context to use
* @page: page to write
+ * @pinned: true if page is pinned
* @pgbase: starting offset within the page for the write
* @offset: file offset for the write
* @count: number of bytes to read/write
@@ -435,6 +444,7 @@ static void nfs_page_assign_page(struct nfs_page *req, struct page *page)
*/
struct nfs_page *nfs_page_create_from_page(struct nfs_open_context *ctx,
struct page *page,
+ bool pinned,
unsigned int pgbase, loff_t offset,
unsigned int count)
{
@@ -446,7 +456,7 @@ struct nfs_page *nfs_page_create_from_page(struct nfs_open_context *ctx,
ret = nfs_page_create(l_ctx, pgbase, offset >> PAGE_SHIFT,
offset_in_page(offset), count);
if (!IS_ERR(ret)) {
- nfs_page_assign_page(ret, page);
+ nfs_page_assign_page(ret, page, pinned);
nfs_page_group_init(ret, NULL);
}
nfs_put_lock_context(l_ctx);
@@ -457,6 +467,7 @@ struct nfs_page *nfs_page_create_from_page(struct nfs_open_context *ctx,
* nfs_page_create_from_folio - Create an NFS read/write request.
* @ctx: open context to use
* @folio: folio to write
+ * @pinned: true if folio is pinned
* @offset: starting offset within the folio for the write
* @count: number of bytes to read/write
*
@@ -466,6 +477,7 @@ struct nfs_page *nfs_page_create_from_page(struct nfs_open_context *ctx,
*/
struct nfs_page *nfs_page_create_from_folio(struct nfs_open_context *ctx,
struct folio *folio,
+ bool pinned,
unsigned int offset,
unsigned int count)
{
@@ -476,7 +488,7 @@ struct nfs_page *nfs_page_create_from_folio(struct nfs_open_context *ctx,
return ERR_CAST(l_ctx);
ret = nfs_page_create(l_ctx, offset, folio->index, offset, count);
if (!IS_ERR(ret)) {
- nfs_page_assign_folio(ret, folio);
+ nfs_page_assign_folio(ret, folio, pinned);
nfs_page_group_init(ret, NULL);
}
nfs_put_lock_context(l_ctx);
@@ -498,9 +510,11 @@ nfs_create_subreq(struct nfs_page *req,
offset, count);
if (!IS_ERR(ret)) {
if (folio)
- nfs_page_assign_folio(ret, folio);
+ nfs_page_assign_folio(ret, folio,
+ test_bit(PG_PINNED, &req->wb_flags));
else
- nfs_page_assign_page(ret, page);
+ nfs_page_assign_page(ret, page,
+ test_bit(PG_PINNED, &req->wb_flags));
/* find the last request */
for (last = req->wb_head;
last->wb_this_page != req->wb_head;
@@ -552,11 +566,21 @@ static void nfs_clear_request(struct nfs_page *req)
struct nfs_open_context *ctx;
if (folio != NULL) {
- folio_put(folio);
+ if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
+ if (req == req->wb_head)
+ unpin_user_folio(folio, 1);
+ } else {
+ folio_put(folio);
+ }
req->wb_folio = NULL;
clear_bit(PG_FOLIO, &req->wb_flags);
} else if (page != NULL) {
- put_page(page);
+ if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
+ if (req == req->wb_head)
+ unpin_user_page(page);
+ } else {
+ put_page(page);
+ }
req->wb_page = NULL;
}
if (l_ctx != NULL) {
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 2b70bd2b934b..e7497b029d6c 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -324,7 +324,7 @@ int nfs_read_add_folio(struct nfs_pageio_descriptor *pgio,
aligned_len = min_t(unsigned int, ALIGN(len, rsize), fsize);
- new = nfs_page_create_from_folio(ctx, folio, 0, aligned_len);
+ new = nfs_page_create_from_folio(ctx, folio, false, 0, aligned_len);
if (IS_ERR(new)) {
error = PTR_ERR(new);
if (nfs_netfs_folio_unlock(folio))
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index d2b03ceaeb4f..e565b811d8b7 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1088,7 +1088,7 @@ static struct nfs_page *nfs_setup_write_request(struct nfs_open_context *ctx,
req = nfs_try_to_update_request(folio, offset, bytes);
if (req != NULL)
goto out;
- req = nfs_page_create_from_folio(ctx, folio, offset, bytes);
+ req = nfs_page_create_from_folio(ctx, folio, false, offset, bytes);
if (IS_ERR(req))
goto out;
nfs_inode_add_request(req);
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index 4b9a35dbc062..fd7aafe7cb54 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -38,6 +38,7 @@ enum {
PG_REMOVE, /* page group sync bit in write path */
PG_CONTENDED1, /* Is someone waiting for a lock? */
PG_CONTENDED2, /* Is someone waiting for a lock? */
+ PG_PINNED, /* page is pinned by GUP */
};
struct nfs_inode;
@@ -125,11 +126,13 @@ struct nfs_pageio_descriptor {
extern struct nfs_page *nfs_page_create_from_page(struct nfs_open_context *ctx,
struct page *page,
+ bool pinned,
unsigned int pgbase,
loff_t offset,
unsigned int count);
extern struct nfs_page *nfs_page_create_from_folio(struct nfs_open_context *ctx,
struct folio *folio,
+ bool pinned,
unsigned int offset,
unsigned int count);
extern void nfs_release_request(struct nfs_page *);
--
2.55.0.691.gc56d675ccc-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v6 1/5] nfs: make nfs_page pin-aware
2026-08-14 14:32 ` [PATCH v6 1/5] nfs: make nfs_page pin-aware Pranjal Shrivastava
@ 2026-08-14 14:46 ` sashiko-bot
2026-08-14 15:04 ` Pranjal Shrivastava
0 siblings, 1 reply; 13+ messages in thread
From: sashiko-bot @ 2026-08-14 14:46 UTC (permalink / raw)
To: Pranjal Shrivastava; +Cc: Anna Schumaker, Chuck Lever, linux-pci, Jeff Layton
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Subrequests incorrectly drop GUP pins multiple times because `req == req->wb_head` evaluates to true for all requests during teardown, causing pin count underflow and Use-After-Free.
- [Medium] Hardcoded unpin count of 1 in `unpin_user_folio()` leaks GUP pins for large folios spanning multiple pages.
--
--- Patch [1]: [PATCH v6 1/5] nfs: make nfs_page pin-aware ---
commit 21b0f62730c29dca3085e0b77d441b62786919e8
Author: Pranjal Shrivastava <praan@google.com>
nfs: make nfs_page pin-aware
Modernizing the NFS Direct I/O path to use iov_iter_extract_pages()
introduces page pinning (GUP) instead of standard page referencing.
To handle this correctly, nfs_page must track whether it holds a
pin or a standard reference.
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index 7dd478ffc2fab..a562cfe2a126d 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
[ ... ]
> @@ -552,11 +566,21 @@ static void nfs_clear_request(struct nfs_page *req)
> struct nfs_open_context *ctx;
>
> if (folio != NULL) {
> - folio_put(folio);
> + if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
> + if (req == req->wb_head)
> + unpin_user_folio(folio, 1);
[Severity: Medium]
Will this leak GUP pins for large folios spanning multiple pages?
If a caller uses nfs_page_create_from_folio() with pinned=true for a folio
spanning multiple pages, hardcoding 1 releases only one pin and leaks the rest.
Also, does this check for the head request evaluate to true for all
subrequests?
During teardown, nfs_page_group_destroy() breaks the page group linkage by
setting tmp->wb_head = tmp before freeing the request:
nfs_page_group_destroy()
tmp->wb_head = tmp;
nfs_free_request(tmp);
Because of this, when nfs_clear_request() is called, req == req->wb_head will
evaluate to true for all subrequests, causing each subrequest to unconditionally
invoke unpin_user_folio() and potentially resulting in a pin count underflow
and Use-After-Free.
> + } else {
> + folio_put(folio);
> + }
> req->wb_folio = NULL;
> clear_bit(PG_FOLIO, &req->wb_flags);
> } else if (page != NULL) {
> - put_page(page);
> + if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
> + if (req == req->wb_head)
> + unpin_user_page(page);
[Severity: Medium]
Similar to the issue above, because nfs_page_group_destroy() severs the
linkage and sets tmp->wb_head = tmp, will this unconditionally call
unpin_user_page() for all subrequests and cause a pin count underflow?
> + } else {
> + put_page(page);
> + }
> req->wb_page = NULL;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260814143255.861084-1-praan@google.com?part=1
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v6 1/5] nfs: make nfs_page pin-aware
2026-08-14 14:46 ` sashiko-bot
@ 2026-08-14 15:04 ` Pranjal Shrivastava
0 siblings, 0 replies; 13+ messages in thread
From: Pranjal Shrivastava @ 2026-08-14 15:04 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Anna Schumaker, Chuck Lever, linux-pci, Jeff Layton
On Fri, Aug 14, 2026 at 02:46:57PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Subrequests incorrectly drop GUP pins multiple times because `req == req->wb_head` evaluates to true for all requests during teardown, causing pin count underflow and Use-After-Free.
> - [Medium] Hardcoded unpin count of 1 in `unpin_user_folio()` leaks GUP pins for large folios spanning multiple pages.
> --
>
> --- Patch [1]: [PATCH v6 1/5] nfs: make nfs_page pin-aware ---
> commit 21b0f62730c29dca3085e0b77d441b62786919e8
> Author: Pranjal Shrivastava <praan@google.com>
>
> nfs: make nfs_page pin-aware
>
> Modernizing the NFS Direct I/O path to use iov_iter_extract_pages()
> introduces page pinning (GUP) instead of standard page referencing.
> To handle this correctly, nfs_page must track whether it holds a
> pin or a standard reference.
>
> > diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> > index 7dd478ffc2fab..a562cfe2a126d 100644
> > --- a/fs/nfs/pagelist.c
> > +++ b/fs/nfs/pagelist.c
> [ ... ]
> > @@ -552,11 +566,21 @@ static void nfs_clear_request(struct nfs_page *req)
> > struct nfs_open_context *ctx;
> >
> > if (folio != NULL) {
> > - folio_put(folio);
> > + if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
> > + if (req == req->wb_head)
> > + unpin_user_folio(folio, 1);
>
> [Severity: Medium]
> Will this leak GUP pins for large folios spanning multiple pages?
>
> If a caller uses nfs_page_create_from_folio() with pinned=true for a folio
> spanning multiple pages, hardcoding 1 releases only one pin and leaks the rest.
>
> Also, does this check for the head request evaluate to true for all
> subrequests?
>
> During teardown, nfs_page_group_destroy() breaks the page group linkage by
> setting tmp->wb_head = tmp before freeing the request:
>
> nfs_page_group_destroy()
> tmp->wb_head = tmp;
> nfs_free_request(tmp);
>
> Because of this, when nfs_clear_request() is called, req == req->wb_head will
> evaluate to true for all subrequests, causing each subrequest to unconditionally
> invoke unpin_user_folio() and potentially resulting in a pin count underflow
> and Use-After-Free.
>
> > + } else {
> > + folio_put(folio);
> > + }
> > req->wb_folio = NULL;
> > clear_bit(PG_FOLIO, &req->wb_flags);
> > } else if (page != NULL) {
> > - put_page(page);
> > + if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
> > + if (req == req->wb_head)
> > + unpin_user_page(page);
>
> [Severity: Medium]
> Similar to the issue above, because nfs_page_group_destroy() severs the
> linkage and sets tmp->wb_head = tmp, will this unconditionally call
> unpin_user_page() for all subrequests and cause a pin count underflow?
>
These are fixed by patch 2 with the introduction of wb_nr_pinned.
Thanks,
Praan
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v6 2/5] nfs: track number of pinned pages in nfs_page
2026-08-14 14:32 [PATCH v6 0/5] nfs: modernize direct I/O path Pranjal Shrivastava
2026-08-14 14:32 ` [PATCH v6 1/5] nfs: make nfs_page pin-aware Pranjal Shrivastava
@ 2026-08-14 14:32 ` Pranjal Shrivastava
2026-08-14 14:47 ` sashiko-bot
2026-08-14 14:32 ` [PATCH v6 3/5] nfs: introduce nfs_release_request_list helper Pranjal Shrivastava
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Pranjal Shrivastava @ 2026-08-14 14:32 UTC (permalink / raw)
To: linux-nfs, Trond Myklebust, Anna Schumaker
Cc: Chuck Lever, Jeff Layton, linux-kernel, Christoph Hellwig,
Logan Gunthorpe, Jason Gunthorpe, linux-pci, linux-rdma,
Shivaji Kant, Pranjal Shrivastava
Track the number of pinned pages in nfs_page to handle unpinning
correctly, ensuring that only primary requests perform the final
unpinning operation, preventing subrequests from incorrectly
performing unpinning on behalf of their parent requests.
Add wb_nr_pinned to struct nfs_page to store the count of pinned pages
owned by the request. Update request creation and cleanup helpers to
initialize and use wb_nr_pinned for primary requests. Use the
nfs_page_array_len() helper to calculate the number of pages spanned
by a request's offset and length.
Signed-off-by: Pranjal Shrivastava <praan@google.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/nfs/pagelist.c | 13 +++++++++----
include/linux/nfs_page.h | 1 +
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index a562cfe2a126..b9ccf2a87e3c 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -457,6 +457,8 @@ struct nfs_page *nfs_page_create_from_page(struct nfs_open_context *ctx,
offset_in_page(offset), count);
if (!IS_ERR(ret)) {
nfs_page_assign_page(ret, page, pinned);
+ if (pinned)
+ ret->wb_nr_pinned = 1;
nfs_page_group_init(ret, NULL);
}
nfs_put_lock_context(l_ctx);
@@ -489,6 +491,9 @@ struct nfs_page *nfs_page_create_from_folio(struct nfs_open_context *ctx,
ret = nfs_page_create(l_ctx, offset, folio->index, offset, count);
if (!IS_ERR(ret)) {
nfs_page_assign_folio(ret, folio, pinned);
+ if (pinned)
+ ret->wb_nr_pinned = nfs_page_array_len(offset_in_page(offset),
+ count);
nfs_page_group_init(ret, NULL);
}
nfs_put_lock_context(l_ctx);
@@ -567,8 +572,8 @@ static void nfs_clear_request(struct nfs_page *req)
if (folio != NULL) {
if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
- if (req == req->wb_head)
- unpin_user_folio(folio, 1);
+ if (req->wb_nr_pinned > 0)
+ unpin_user_folio(folio, req->wb_nr_pinned);
} else {
folio_put(folio);
}
@@ -576,8 +581,8 @@ static void nfs_clear_request(struct nfs_page *req)
clear_bit(PG_FOLIO, &req->wb_flags);
} else if (page != NULL) {
if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
- if (req == req->wb_head)
- unpin_user_page(page);
+ if (req->wb_nr_pinned > 0)
+ unpin_user_pages(&page, req->wb_nr_pinned);
} else {
put_page(page);
}
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index fd7aafe7cb54..080fa3e23580 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -59,6 +59,7 @@ struct nfs_page {
struct nfs_page *wb_this_page; /* list of reqs for this page */
struct nfs_page *wb_head; /* head pointer for req list */
unsigned short wb_nio; /* Number of I/O attempts */
+ unsigned int wb_nr_pinned; /* Number of pinned pages */
};
struct nfs_pgio_mirror;
--
2.55.0.691.gc56d675ccc-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v6 2/5] nfs: track number of pinned pages in nfs_page
2026-08-14 14:32 ` [PATCH v6 2/5] nfs: track number of pinned pages in nfs_page Pranjal Shrivastava
@ 2026-08-14 14:47 ` sashiko-bot
2026-08-14 15:14 ` Pranjal Shrivastava
0 siblings, 1 reply; 13+ messages in thread
From: sashiko-bot @ 2026-08-14 14:47 UTC (permalink / raw)
To: Pranjal Shrivastava; +Cc: Chuck Lever, Anna Schumaker, Jeff Layton, linux-pci
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Latent stack out-of-bounds read by passing a single local stack variable to unpin_user_pages() with a dynamically loaded length.
--
--- Patch [2]: [PATCH v6 2/5] nfs: track number of pinned pages in nfs_page ---
commit 1fad80f498aa716421accabc5c2572235b589709
Author: Pranjal Shrivastava <praan@google.com>
nfs: track number of pinned pages in nfs_page
This commit adds wb_nr_pinned to struct nfs_page to track the number
of pinned pages. It updates request creation and cleanup helpers to
ensure that only primary requests perform the final unpinning operation.
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index a562cfe2a126d..b9ccf2a87e3c9 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
[ ... ]
> @@ -567,8 +572,8 @@ static void nfs_clear_request(struct nfs_page *req)
>
> if (folio != NULL) {
> if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
> - if (req == req->wb_head)
> - unpin_user_folio(folio, 1);
> + if (req->wb_nr_pinned > 0)
> + unpin_user_folio(folio, req->wb_nr_pinned);
> } else {
> folio_put(folio);
> }
> @@ -576,8 +581,8 @@ static void nfs_clear_request(struct nfs_page *req)
> clear_bit(PG_FOLIO, &req->wb_flags);
> } else if (page != NULL) {
> if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
> - if (req == req->wb_head)
> - unpin_user_page(page);
> + if (req->wb_nr_pinned > 0)
> + unpin_user_pages(&page, req->wb_nr_pinned);
[Severity: Medium]
Could this lead to an out-of-bounds stack read if wb_nr_pinned is ever
greater than 1?
Looking at nfs_clear_request(), page is declared as a single local pointer
on the stack:
static void nfs_clear_request(struct nfs_page *req)
{
...
struct page *page = req->wb_page;
If req->wb_nr_pinned exceeds 1, unpin_user_pages() will read adjacent stack
memory to interpret as struct page pointers. While wb_nr_pinned is currently
hardcoded to 1 when a request is created from a page, passing a dynamically
loaded length alongside a pointer to a single local stack variable seems
prone to future regressions.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260814143255.861084-1-praan@google.com?part=2
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v6 2/5] nfs: track number of pinned pages in nfs_page
2026-08-14 14:47 ` sashiko-bot
@ 2026-08-14 15:14 ` Pranjal Shrivastava
0 siblings, 0 replies; 13+ messages in thread
From: Pranjal Shrivastava @ 2026-08-14 15:14 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Chuck Lever, Anna Schumaker, Jeff Layton, linux-pci
On Fri, Aug 14, 2026 at 02:47:13PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Latent stack out-of-bounds read by passing a single local stack variable to unpin_user_pages() with a dynamically loaded length.
> --
>
> --- Patch [2]: [PATCH v6 2/5] nfs: track number of pinned pages in nfs_page ---
> commit 1fad80f498aa716421accabc5c2572235b589709
> Author: Pranjal Shrivastava <praan@google.com>
>
> nfs: track number of pinned pages in nfs_page
>
> This commit adds wb_nr_pinned to struct nfs_page to track the number
> of pinned pages. It updates request creation and cleanup helpers to
> ensure that only primary requests perform the final unpinning operation.
>
> > diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> > index a562cfe2a126d..b9ccf2a87e3c9 100644
> > --- a/fs/nfs/pagelist.c
> > +++ b/fs/nfs/pagelist.c
>
> [ ... ]
>
> > @@ -567,8 +572,8 @@ static void nfs_clear_request(struct nfs_page *req)
> >
> > if (folio != NULL) {
> > if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
> > - if (req == req->wb_head)
> > - unpin_user_folio(folio, 1);
> > + if (req->wb_nr_pinned > 0)
> > + unpin_user_folio(folio, req->wb_nr_pinned);
> > } else {
> > folio_put(folio);
> > }
> > @@ -576,8 +581,8 @@ static void nfs_clear_request(struct nfs_page *req)
> > clear_bit(PG_FOLIO, &req->wb_flags);
> > } else if (page != NULL) {
> > if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
> > - if (req == req->wb_head)
> > - unpin_user_page(page);
> > + if (req->wb_nr_pinned > 0)
> > + unpin_user_pages(&page, req->wb_nr_pinned);
>
> [Severity: Medium]
> Could this lead to an out-of-bounds stack read if wb_nr_pinned is ever
> greater than 1?
>
> Looking at nfs_clear_request(), page is declared as a single local pointer
> on the stack:
>
> static void nfs_clear_request(struct nfs_page *req)
> {
> ...
> struct page *page = req->wb_page;
>
> If req->wb_nr_pinned exceeds 1, unpin_user_pages() will read adjacent stack
> memory to interpret as struct page pointers. While wb_nr_pinned is currently
> hardcoded to 1 when a request is created from a page, passing a dynamically
> loaded length alongside a pointer to a single local stack variable seems
> prone to future regressions.
>
In this (non-folio) path wb_nr_pinned would always ever be 1. Although,
I think we could use unpin_user_page (singular) here.
Thanks,
Praan
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260814143255.861084-1-praan@google.com?part=2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v6 3/5] nfs: introduce nfs_release_request_list helper
2026-08-14 14:32 [PATCH v6 0/5] nfs: modernize direct I/O path Pranjal Shrivastava
2026-08-14 14:32 ` [PATCH v6 1/5] nfs: make nfs_page pin-aware Pranjal Shrivastava
2026-08-14 14:32 ` [PATCH v6 2/5] nfs: track number of pinned pages in nfs_page Pranjal Shrivastava
@ 2026-08-14 14:32 ` Pranjal Shrivastava
2026-08-14 14:38 ` sashiko-bot
2026-08-14 14:32 ` [PATCH v6 4/5] nfs: migrate direct I/O to iov_iter_extract_pages Pranjal Shrivastava
2026-08-14 14:32 ` [PATCH v6 5/5] nfs: introduce nfs_direct_extract_pages helper Pranjal Shrivastava
4 siblings, 1 reply; 13+ messages in thread
From: Pranjal Shrivastava @ 2026-08-14 14:32 UTC (permalink / raw)
To: linux-nfs, Trond Myklebust, Anna Schumaker
Cc: Chuck Lever, Jeff Layton, linux-kernel, Christoph Hellwig,
Logan Gunthorpe, Jason Gunthorpe, linux-pci, linux-rdma,
Shivaji Kant, Pranjal Shrivastava
Introduce a centralized helper, nfs_release_request_list, to handle
the bulk release of nfs_page requests from a list.
This serves as a preparatory step for two upcoming improvements:
1. Pin-Aware Cleanup: As we migrate to iov_iter_extract_* API,
requests will hold pins (GUP) instead of standard references. The
helper ensures that the correct unpinning logic gets applied
consistently across all requests in a list.
2. Folio Support: In subsequent patches where nfs_page structures
will cover multi-page folios, this helper provides a clean
infrastructure to unlock these larger units of I/O in bulk during
completion, similar to the pattern in bio_release_pages.
Additionally, refactor nfs_read_sync_pgio_error() to utilize this new
helper.
Signed-off-by: Pranjal Shrivastava <praan@google.com>
Reviewed-by: Shivaji Kant <shivajikant@google.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/nfs/direct.c | 8 +-------
fs/nfs/pagelist.c | 17 +++++++++++++++++
include/linux/nfs_page.h | 4 ++--
3 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 19792a38c924..96995736fac2 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -314,13 +314,7 @@ static void nfs_direct_read_completion(struct nfs_pgio_header *hdr)
static void nfs_read_sync_pgio_error(struct list_head *head, int error)
{
- struct nfs_page *req;
-
- while (!list_empty(head)) {
- req = nfs_list_entry(head->next);
- nfs_list_remove_request(req);
- nfs_release_request(req);
- }
+ nfs_release_request_list(head);
}
static void nfs_direct_pgio_init(struct nfs_pgio_header *hdr)
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index b9ccf2a87e3c..71f0ce2bc4ea 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -628,6 +628,23 @@ void nfs_release_request(struct nfs_page *req)
}
EXPORT_SYMBOL_GPL(nfs_release_request);
+/*
+ * nfs_release_request_list - Release a list of NFS read/write requests
+ * @head: list of requests to release
+ *
+ * Removes each request from the list and drops it's refcount.
+ */
+void nfs_release_request_list(struct list_head *head)
+{
+ while (!list_empty(head)) {
+ struct nfs_page *req = nfs_list_entry(head->next);
+
+ nfs_list_remove_request(req);
+ nfs_release_request(req);
+ }
+}
+EXPORT_SYMBOL_GPL(nfs_release_request_list);
+
/*
* nfs_generic_pg_test - determine if requests can be coalesced
* @desc: pointer to descriptor
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index 080fa3e23580..c38e4b380be5 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -136,8 +136,8 @@ extern struct nfs_page *nfs_page_create_from_folio(struct nfs_open_context *ctx,
bool pinned,
unsigned int offset,
unsigned int count);
-extern void nfs_release_request(struct nfs_page *);
-
+void nfs_release_request(struct nfs_page *req);
+void nfs_release_request_list(struct list_head *head);
extern void nfs_pageio_init(struct nfs_pageio_descriptor *desc,
struct inode *inode,
--
2.55.0.691.gc56d675ccc-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v6 3/5] nfs: introduce nfs_release_request_list helper
2026-08-14 14:32 ` [PATCH v6 3/5] nfs: introduce nfs_release_request_list helper Pranjal Shrivastava
@ 2026-08-14 14:38 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-08-14 14:38 UTC (permalink / raw)
To: Pranjal Shrivastava; +Cc: Chuck Lever, Jeff Layton, Anna Schumaker, linux-pci
> Introduce a centralized helper, nfs_release_request_list, to handle
> the bulk release of nfs_page requests from a list.
>
> This serves as a preparatory step for two upcoming improvements:
>
> 1. Pin-Aware Cleanup: As we migrate to iov_iter_extract_* API,
> requests will hold pins (GUP) instead of standard references. The
> helper ensures that the correct unpinning logic gets applied
> consistently across all requests in a list.
>
> 2. Folio Support: In subsequent patches where nfs_page structures
> will cover multi-page folios, this helper provides a clean
> infrastructure to unlock these larger units of I/O in bulk during
> completion, similar to the pattern in bio_release_pages.
>
> Additionally, refactor nfs_read_sync_pgio_error() to utilize this new
> helper.
>
> Signed-off-by: Pranjal Shrivastava <praan@google.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260814143255.861084-1-praan@google.com?part=3
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v6 4/5] nfs: migrate direct I/O to iov_iter_extract_pages
2026-08-14 14:32 [PATCH v6 0/5] nfs: modernize direct I/O path Pranjal Shrivastava
` (2 preceding siblings ...)
2026-08-14 14:32 ` [PATCH v6 3/5] nfs: introduce nfs_release_request_list helper Pranjal Shrivastava
@ 2026-08-14 14:32 ` Pranjal Shrivastava
2026-08-14 14:48 ` sashiko-bot
2026-08-14 14:32 ` [PATCH v6 5/5] nfs: introduce nfs_direct_extract_pages helper Pranjal Shrivastava
4 siblings, 1 reply; 13+ messages in thread
From: Pranjal Shrivastava @ 2026-08-14 14:32 UTC (permalink / raw)
To: linux-nfs, Trond Myklebust, Anna Schumaker
Cc: Chuck Lever, Jeff Layton, linux-kernel, Christoph Hellwig,
Logan Gunthorpe, Jason Gunthorpe, linux-pci, linux-rdma,
Shivaji Kant, Pranjal Shrivastava
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.
The migration fixes a bug in the Direct I/O loop where pages were being
unpinned immediately after request creation. With the new extraction
model, pins are held until the I/O is complete. Manual release in the
loop is correspondingly updated to only clean up failed pages.
Signed-off-by: Pranjal Shrivastava <praan@google.com>
Reviewed-by: Shivaji Kant <shivajikant@google.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/nfs/direct.c | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 96995736fac2..a1479fbf4f9e 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -168,14 +168,8 @@ int nfs_swap_rw(struct kiocb *iocb, struct iov_iter *iter)
static void nfs_direct_release_pages(struct page **pages, unsigned int npages,
bool pinned)
{
- unsigned int i;
-
- if (pinned) {
+ if (pinned)
unpin_user_pages(pages, npages);
- } else {
- for (i = 0; i < npages; i++)
- put_page(pages[i]);
- }
}
void nfs_init_cinfo_from_dreq(struct nfs_commit_info *cinfo,
@@ -354,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;
-
+
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;
}
pgbase = 0;
@@ -387,7 +383,8 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
requested_bytes += req_len;
pos += req_len;
}
- nfs_direct_release_pages(pagevec, npages, false);
+ if (i < npages)
+ nfs_direct_release_pages(pagevec + i, npages - i, pinned);
kvfree(pagevec);
if (result < 0)
break;
@@ -891,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;
@@ -908,7 +906,7 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
unsigned int req_len = min_t(size_t, bytes, PAGE_SIZE - pgbase);
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);
@@ -918,6 +916,7 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
if (desc.pg_error < 0) {
nfs_free_request(req);
result = desc.pg_error;
+ i++;
break;
}
@@ -939,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;
}
@@ -952,7 +952,8 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
desc.pg_error = 0;
defer = true;
}
- nfs_direct_release_pages(pagevec, npages, false);
+ if (i < npages)
+ nfs_direct_release_pages(pagevec + i, npages - i, pinned);
kvfree(pagevec);
if (result < 0)
break;
--
2.55.0.691.gc56d675ccc-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v6 4/5] nfs: migrate direct I/O to iov_iter_extract_pages
2026-08-14 14:32 ` [PATCH v6 4/5] nfs: migrate direct I/O to iov_iter_extract_pages Pranjal Shrivastava
@ 2026-08-14 14:48 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-08-14 14:48 UTC (permalink / raw)
To: Pranjal Shrivastava; +Cc: Anna Schumaker, Jeff Layton, linux-pci, Chuck Lever
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 <praan@google.com>
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/20260814143255.861084-1-praan@google.com?part=4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v6 5/5] nfs: introduce nfs_direct_extract_pages helper
2026-08-14 14:32 [PATCH v6 0/5] nfs: modernize direct I/O path Pranjal Shrivastava
` (3 preceding siblings ...)
2026-08-14 14:32 ` [PATCH v6 4/5] nfs: migrate direct I/O to iov_iter_extract_pages Pranjal Shrivastava
@ 2026-08-14 14:32 ` Pranjal Shrivastava
2026-08-14 14:40 ` sashiko-bot
4 siblings, 1 reply; 13+ messages in thread
From: Pranjal Shrivastava @ 2026-08-14 14:32 UTC (permalink / raw)
To: linux-nfs, Trond Myklebust, Anna Schumaker
Cc: Chuck Lever, Jeff Layton, linux-kernel, Christoph Hellwig,
Logan Gunthorpe, Jason Gunthorpe, linux-pci, linux-rdma,
Shivaji Kant, Pranjal Shrivastava
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>
Reviewed-by: Shivaji Kant <shivajikant@google.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/nfs/direct.c | 130 ++++++++++++++++++++++++------------------------
1 file changed, 64 insertions(+), 66 deletions(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index a1479fbf4f9e..76c954bbc9af 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -172,6 +172,50 @@ static void nfs_direct_release_pages(struct page **pages, unsigned int npages,
unpin_user_pages(pages, 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;
+ int err = 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)) {
+ err = 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 ? bytes : err;
+}
+
void nfs_init_cinfo_from_dreq(struct nfs_commit_info *cinfo,
struct nfs_direct_req *dreq)
{
@@ -340,6 +384,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);
@@ -348,44 +393,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);
- i++;
+ 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;
}
@@ -876,6 +900,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);
@@ -888,57 +913,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;
- i++;
- 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);
- i++;
+ nfs_release_request_list(&nfs_page_list);
break;
}
@@ -949,12 +949,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.691.gc56d675ccc-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread