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

["Anna Schumaker" <anna@kernel.org>]

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 <praan@google.com>
> ---
>  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
> @@ -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;
> @@ -899,7 +906,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;
> @@ -942,7 +950,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 4a87b2fdb2e6..5c5601a27663 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -404,20 +404,26 @@ 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);
>  	}
>  }
> 
> @@ -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'


>  					   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'

Thanks,
Anna

>  					    unsigned int offset,
>  					    unsigned int count)
>  {
> @@ -476,7 +484,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 +506,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 +562,17 @@ 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))
> +			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))
> +			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 e1fe78d7b8d0..ebfebc47966d 100644
> --- a/fs/nfs/read.c
> +++ b/fs/nfs/read.c
> @@ -301,7 +301,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 3134bb17f3e3..cf56c07ae2ee 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -1086,7 +1086,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 afe1d8f09d89..9ece78f1b788 100644
> --- a/include/linux/nfs_page.h
> +++ b/include/linux/nfs_page.h
> @@ -37,6 +37,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;
> @@ -124,11 +125,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.54.0.1013.g208068f2d8-goog