Re: [PATCH v1 1/7] nfs: make nfs_page pin-aware

On Wed, Jun 03, 2026 at 12:39:47PM -0400, Anna Schumaker wrote:
> Hi Pranjal,
> 
> On Wed, Jun 3, 2026, at 1:30 AM, Pranjal Shrivastava wrote:
> > 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 <[email protected]>
> > ---
> >  fs/nfs/direct.c          | 22 +++++++++++++++-------
> >  fs/nfs/pagelist.c        | 36 ++++++++++++++++++++++++++----------
> >  fs/nfs/read.c            |  2 +-
> >  fs/nfs/write.c           |  2 +-
> >  include/linux/nfs_page.h |  3 +++
> >  5 files changed, 46 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> > index 48d89716193a..80319c5eeed4 100644
> > --- a/fs/nfs/direct.c
> > +++ b/fs/nfs/direct.c

Hi Anna,

[...]

> > 
> > @@ -435,6 +441,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,
> 
> Can you add a description for "pinned" to the kernel doc comment right
> above this function? I'm seeing:
> 
> Warning: fs/nfs/pagelist.c:446 function parameter 'pinned' not described in 'nfs_page_create_from_page'

Ack. I'll add a description in v2.

> 
> 
> >  					   unsigned int pgbase, loff_t offset,
> >  					   unsigned int count)
> >  {
> > @@ -446,7 +453,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);
> > @@ -466,6 +473,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,
> 
> Same here:
> 
> Warning: fs/nfs/pagelist.c:478 function parameter 'pinned' not described in 'nfs_page_create_from_folio'

Ack. I'll add a description in v2.

[...]

Thanks,
Praan