NFS buffered write cleanup

NFS buffered write cleanup

[Christoph Hellwig]

Hi all,

this series cleans up the nfs_page handling in the buffer write path.

The first patch was already sent independently but hasn't been picked up
and this included here again.

The last patch fixes a bug where a request could get incorrectly reused.
It would require the flexfiles layout and odd I/O timings, and without
a flexfiles server I can't actually hit it.  I'd appreciate a careful
review of that one.

The series is against Trond's testing branch.

Diffstat:
 fs/nfs/file.c                  |    6 
 fs/nfs/filelayout/filelayout.c |    1 
 fs/nfs/fscache.c               |    2 
 fs/nfs/internal.h              |    8 -
 fs/nfs/pagelist.c              |  117 ---------------
 fs/nfs/pnfs.h                  |   22 --
 fs/nfs/pnfs_nfs.c              |   47 ------
 fs/nfs/read.c                  |    2 
 fs/nfs/write.c                 |  316 ++++++++++++++++++-----------------------
 include/linux/nfs_page.h       |    7 
 10 files changed, 157 insertions(+), 371 deletions(-)

[PATCH 1/7] nfs: remove dead code for the old swap over NFS implementation

[Christoph Hellwig]

Remove the code testing folio_test_swapcache either explicitly or
implicitly in pagemap.h headers, as is now handled using the direct I/O
path and not the buffered I/O path that these helpers are located in.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/file.c                  |   6 +-
 fs/nfs/filelayout/filelayout.c |   1 -
 fs/nfs/fscache.c               |   2 +-
 fs/nfs/internal.h              |   8 +--
 fs/nfs/pagelist.c              |   2 +-
 fs/nfs/pnfs.h                  |  22 ------
 fs/nfs/pnfs_nfs.c              |  47 ------------
 fs/nfs/read.c                  |   2 +-
 fs/nfs/write.c                 | 128 +++++++--------------------------
 include/linux/nfs_page.h       |   4 +-
 10 files changed, 36 insertions(+), 186 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 834e612262e62b..0e2f87120cb840 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -427,7 +427,7 @@ static int nfs_write_end(struct file *file, struct address_space *mapping,
 static void nfs_invalidate_folio(struct folio *folio, size_t offset,
 				size_t length)
 {
-	struct inode *inode = folio_file_mapping(folio)->host;
+	struct inode *inode = folio->mapping->host;
 	dfprintk(PAGECACHE, "NFS: invalidate_folio(%lu, %zu, %zu)\n",
 		 folio->index, offset, length);
 
@@ -454,7 +454,7 @@ static bool nfs_release_folio(struct folio *folio, gfp_t gfp)
 		if ((current_gfp_context(gfp) & GFP_KERNEL) != GFP_KERNEL ||
 		    current_is_kswapd())
 			return false;
-		if (nfs_wb_folio(folio_file_mapping(folio)->host, folio) < 0)
+		if (nfs_wb_folio(folio->mapping->host, folio) < 0)
 			return false;
 	}
 	return nfs_fscache_release_folio(folio, gfp);
@@ -606,7 +606,7 @@ static vm_fault_t nfs_vm_page_mkwrite(struct vm_fault *vmf)
 			   TASK_KILLABLE|TASK_FREEZABLE_UNSAFE);
 
 	folio_lock(folio);
-	mapping = folio_file_mapping(folio);
+	mapping = folio->mapping;
 	if (mapping != inode->i_mapping)
 		goto out_unlock;
 
diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
index 29d84dc66ca391..b6e9aeaf4ce289 100644
--- a/fs/nfs/filelayout/filelayout.c
+++ b/fs/nfs/filelayout/filelayout.c
@@ -1110,7 +1110,6 @@ static const struct pnfs_commit_ops filelayout_commit_ops = {
 	.clear_request_commit	= pnfs_generic_clear_request_commit,
 	.scan_commit_lists	= pnfs_generic_scan_commit_lists,
 	.recover_commit_reqs	= pnfs_generic_recover_commit_reqs,
-	.search_commit_reqs	= pnfs_generic_search_commit_reqs,
 	.commit_pagelist	= filelayout_commit_pagelist,
 };
 
diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
index ddc1ee0319554c..7202ce84d0eb03 100644
--- a/fs/nfs/fscache.c
+++ b/fs/nfs/fscache.c
@@ -341,7 +341,7 @@ void nfs_netfs_initiate_read(struct nfs_pgio_header *hdr)
 
 int nfs_netfs_folio_unlock(struct folio *folio)
 {
-	struct inode *inode = folio_file_mapping(folio)->host;
+	struct inode *inode = folio->mapping->host;
 
 	/*
 	 * If fscache is enabled, netfs will unlock pages.
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 9f0f4534744ba4..87ebc4608c316a 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -785,7 +785,7 @@ static inline void nfs_folio_mark_unstable(struct folio *folio,
 					   struct nfs_commit_info *cinfo)
 {
 	if (folio && !cinfo->dreq) {
-		struct inode *inode = folio_file_mapping(folio)->host;
+		struct inode *inode = folio->mapping->host;
 		long nr = folio_nr_pages(folio);
 
 		/* This page is really still in write-back - just that the
@@ -803,7 +803,7 @@ static inline void nfs_folio_mark_unstable(struct folio *folio,
 static inline
 unsigned int nfs_page_length(struct page *page)
 {
-	loff_t i_size = i_size_read(page_file_mapping(page)->host);
+	loff_t i_size = i_size_read(page->mapping->host);
 
 	if (i_size > 0) {
 		pgoff_t index = page_index(page);
@@ -821,10 +821,10 @@ unsigned int nfs_page_length(struct page *page)
  */
 static inline size_t nfs_folio_length(struct folio *folio)
 {
-	loff_t i_size = i_size_read(folio_file_mapping(folio)->host);
+	loff_t i_size = i_size_read(folio->mapping->host);
 
 	if (i_size > 0) {
-		pgoff_t index = folio_index(folio) >> folio_order(folio);
+		pgoff_t index = folio->index >> folio_order(folio);
 		pgoff_t end_index = (i_size - 1) >> folio_shift(folio);
 		if (index < end_index)
 			return folio_size(folio);
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 040b6b79c75e59..3b006bcbcc87a2 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -569,7 +569,7 @@ struct nfs_page *nfs_page_create_from_folio(struct nfs_open_context *ctx,
 
 	if (IS_ERR(l_ctx))
 		return ERR_CAST(l_ctx);
-	ret = nfs_page_create(l_ctx, offset, folio_index(folio), offset, count);
+	ret = nfs_page_create(l_ctx, offset, folio->index, offset, count);
 	if (!IS_ERR(ret)) {
 		nfs_page_assign_folio(ret, folio);
 		nfs_page_group_init(ret, NULL);
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index bb5142b4e67a59..1fc40afcbf1f50 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -199,8 +199,6 @@ struct pnfs_commit_ops {
 				  int max);
 	void (*recover_commit_reqs) (struct list_head *list,
 				     struct nfs_commit_info *cinfo);
-	struct nfs_page * (*search_commit_reqs)(struct nfs_commit_info *cinfo,
-						struct folio *folio);
 };
 
 struct pnfs_layout_hdr {
@@ -409,8 +407,6 @@ void pnfs_generic_prepare_to_resend_writes(struct nfs_commit_data *data);
 void pnfs_generic_rw_release(void *data);
 void pnfs_generic_recover_commit_reqs(struct list_head *dst,
 				      struct nfs_commit_info *cinfo);
-struct nfs_page *pnfs_generic_search_commit_reqs(struct nfs_commit_info *cinfo,
-						 struct folio *folio);
 int pnfs_generic_commit_pagelist(struct inode *inode,
 				 struct list_head *mds_pages,
 				 int how,
@@ -570,17 +566,6 @@ pnfs_recover_commit_reqs(struct list_head *head, struct nfs_commit_info *cinfo)
 		fl_cinfo->ops->recover_commit_reqs(head, cinfo);
 }
 
-static inline struct nfs_page *
-pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo,
-			struct folio *folio)
-{
-	struct pnfs_ds_commit_info *fl_cinfo = cinfo->ds;
-
-	if (!fl_cinfo->ops || !fl_cinfo->ops->search_commit_reqs)
-		return NULL;
-	return fl_cinfo->ops->search_commit_reqs(cinfo, folio);
-}
-
 /* Should the pNFS client commit and return the layout upon a setattr */
 static inline bool
 pnfs_ld_layoutret_on_setattr(struct inode *inode)
@@ -882,13 +867,6 @@ pnfs_recover_commit_reqs(struct list_head *head, struct nfs_commit_info *cinfo)
 {
 }
 
-static inline struct nfs_page *
-pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo,
-			struct folio *folio)
-{
-	return NULL;
-}
-
 static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 {
 	return 0;
diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c
index 88e061bd711b74..a74ee69a2fa638 100644
--- a/fs/nfs/pnfs_nfs.c
+++ b/fs/nfs/pnfs_nfs.c
@@ -351,53 +351,6 @@ void pnfs_generic_recover_commit_reqs(struct list_head *dst,
 }
 EXPORT_SYMBOL_GPL(pnfs_generic_recover_commit_reqs);
 
-static struct nfs_page *
-pnfs_bucket_search_commit_reqs(struct pnfs_commit_bucket *buckets,
-			       unsigned int nbuckets, struct folio *folio)
-{
-	struct nfs_page *req;
-	struct pnfs_commit_bucket *b;
-	unsigned int i;
-
-	/* Linearly search the commit lists for each bucket until a matching
-	 * request is found */
-	for (i = 0, b = buckets; i < nbuckets; i++, b++) {
-		list_for_each_entry(req, &b->written, wb_list) {
-			if (nfs_page_to_folio(req) == folio)
-				return req->wb_head;
-		}
-		list_for_each_entry(req, &b->committing, wb_list) {
-			if (nfs_page_to_folio(req) == folio)
-				return req->wb_head;
-		}
-	}
-	return NULL;
-}
-
-/* pnfs_generic_search_commit_reqs - Search lists in @cinfo for the head request
- *				   for @folio
- * @cinfo - commit info for current inode
- * @folio - page to search for matching head request
- *
- * Return: the head request if one is found, otherwise %NULL.
- */
-struct nfs_page *pnfs_generic_search_commit_reqs(struct nfs_commit_info *cinfo,
-						 struct folio *folio)
-{
-	struct pnfs_ds_commit_info *fl_cinfo = cinfo->ds;
-	struct pnfs_commit_array *array;
-	struct nfs_page *req;
-
-	list_for_each_entry(array, &fl_cinfo->commits, cinfo_list) {
-		req = pnfs_bucket_search_commit_reqs(array->buckets,
-						     array->nbuckets, folio);
-		if (req)
-			return req;
-	}
-	return NULL;
-}
-EXPORT_SYMBOL_GPL(pnfs_generic_search_commit_reqs);
-
 static struct pnfs_layout_segment *
 pnfs_bucket_get_committing(struct list_head *head,
 			   struct pnfs_commit_bucket *bucket,
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 1b0e06c11983cb..036ede4875cab6 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -289,7 +289,7 @@ int nfs_read_add_folio(struct nfs_pageio_descriptor *pgio,
 		       struct nfs_open_context *ctx,
 		       struct folio *folio)
 {
-	struct inode *inode = folio_file_mapping(folio)->host;
+	struct inode *inode = folio->mapping->host;
 	struct nfs_server *server = NFS_SERVER(inode);
 	size_t fsize = folio_size(folio);
 	unsigned int rsize = server->rsize;
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index f5414c96381ac8..641bdddeaad331 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -63,9 +63,6 @@ static void nfs_clear_request_commit(struct nfs_commit_info *cinfo,
 				     struct nfs_page *req);
 static void nfs_init_cinfo_from_inode(struct nfs_commit_info *cinfo,
 				      struct inode *inode);
-static struct nfs_page *
-nfs_page_search_commits_for_head_request_locked(struct nfs_inode *nfsi,
-						struct folio *folio);
 
 static struct kmem_cache *nfs_wdata_cachep;
 static mempool_t *nfs_wdata_mempool;
@@ -178,16 +175,16 @@ static struct nfs_page *nfs_folio_private_request(struct folio *folio)
 }
 
 /**
- * nfs_folio_find_private_request - find head request associated with a folio
+ * nfs_folio_find_head_request - find head request associated with a folio
  * @folio: pointer to folio
  *
  * must be called while holding the inode lock.
  *
  * returns matching head request with reference held, or NULL if not found.
  */
-static struct nfs_page *nfs_folio_find_private_request(struct folio *folio)
+static struct nfs_page *nfs_folio_find_head_request(struct folio *folio)
 {
-	struct address_space *mapping = folio_file_mapping(folio);
+	struct address_space *mapping = folio->mapping;
 	struct nfs_page *req;
 
 	if (!folio_test_private(folio))
@@ -202,45 +199,9 @@ static struct nfs_page *nfs_folio_find_private_request(struct folio *folio)
 	return req;
 }
 
-static struct nfs_page *nfs_folio_find_swap_request(struct folio *folio)
-{
-	struct inode *inode = folio_file_mapping(folio)->host;
-	struct nfs_inode *nfsi = NFS_I(inode);
-	struct nfs_page *req = NULL;
-	if (!folio_test_swapcache(folio))
-		return NULL;
-	mutex_lock(&nfsi->commit_mutex);
-	if (folio_test_swapcache(folio)) {
-		req = nfs_page_search_commits_for_head_request_locked(nfsi,
-								      folio);
-		if (req) {
-			WARN_ON_ONCE(req->wb_head != req);
-			kref_get(&req->wb_kref);
-		}
-	}
-	mutex_unlock(&nfsi->commit_mutex);
-	return req;
-}
-
-/**
- * nfs_folio_find_head_request - find head request associated with a folio
- * @folio: pointer to folio
- *
- * returns matching head request with reference held, or NULL if not found.
- */
-static struct nfs_page *nfs_folio_find_head_request(struct folio *folio)
-{
-	struct nfs_page *req;
-
-	req = nfs_folio_find_private_request(folio);
-	if (!req)
-		req = nfs_folio_find_swap_request(folio);
-	return req;
-}
-
 static struct nfs_page *nfs_folio_find_and_lock_request(struct folio *folio)
 {
-	struct inode *inode = folio_file_mapping(folio)->host;
+	struct inode *inode = folio->mapping->host;
 	struct nfs_page *req, *head;
 	int ret;
 
@@ -261,8 +222,6 @@ static struct nfs_page *nfs_folio_find_and_lock_request(struct folio *folio)
 		/* Ensure that nobody removed the request before we locked it */
 		if (head == nfs_folio_private_request(folio))
 			break;
-		if (folio_test_swapcache(folio))
-			break;
 		nfs_unlock_and_release_request(head);
 	}
 	return head;
@@ -272,14 +231,14 @@ static struct nfs_page *nfs_folio_find_and_lock_request(struct folio *folio)
 static void nfs_grow_file(struct folio *folio, unsigned int offset,
 			  unsigned int count)
 {
-	struct inode *inode = folio_file_mapping(folio)->host;
+	struct inode *inode = folio->mapping->host;
 	loff_t end, i_size;
 	pgoff_t end_index;
 
 	spin_lock(&inode->i_lock);
 	i_size = i_size_read(inode);
 	end_index = ((i_size - 1) >> folio_shift(folio)) << folio_order(folio);
-	if (i_size > 0 && folio_index(folio) < end_index)
+	if (i_size > 0 && folio->index < end_index)
 		goto out;
 	end = folio_file_pos(folio) + (loff_t)offset + (loff_t)count;
 	if (i_size >= end)
@@ -311,7 +270,7 @@ static void nfs_set_pageerror(struct address_space *mapping)
 
 static void nfs_mapping_set_error(struct folio *folio, int error)
 {
-	struct address_space *mapping = folio_file_mapping(folio);
+	struct address_space *mapping = folio->mapping;
 
 	folio_set_error(folio);
 	filemap_set_wb_err(mapping, error);
@@ -412,7 +371,7 @@ int nfs_congestion_kb;
 
 static void nfs_folio_set_writeback(struct folio *folio)
 {
-	struct nfs_server *nfss = NFS_SERVER(folio_file_mapping(folio)->host);
+	struct nfs_server *nfss = NFS_SERVER(folio->mapping->host);
 
 	folio_start_writeback(folio);
 	if (atomic_long_inc_return(&nfss->writeback) > NFS_CONGESTION_ON_THRESH)
@@ -421,7 +380,7 @@ static void nfs_folio_set_writeback(struct folio *folio)
 
 static void nfs_folio_end_writeback(struct folio *folio)
 {
-	struct nfs_server *nfss = NFS_SERVER(folio_file_mapping(folio)->host);
+	struct nfs_server *nfss = NFS_SERVER(folio->mapping->host);
 
 	folio_end_writeback(folio);
 	if (atomic_long_dec_return(&nfss->writeback) <
@@ -567,7 +526,7 @@ void nfs_join_page_group(struct nfs_page *head, struct nfs_commit_info *cinfo,
  */
 static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
 {
-	struct inode *inode = folio_file_mapping(folio)->host;
+	struct inode *inode = folio->mapping->host;
 	struct nfs_page *head;
 	struct nfs_commit_info cinfo;
 	int ret;
@@ -643,7 +602,7 @@ static int nfs_page_async_flush(struct folio *folio,
 		nfs_redirty_request(req);
 		pgio->pg_error = 0;
 	} else
-		nfs_add_stats(folio_file_mapping(folio)->host,
+		nfs_add_stats(folio->mapping->host,
 			      NFSIOS_WRITEPAGES, 1);
 out:
 	return ret;
@@ -655,7 +614,7 @@ static int nfs_page_async_flush(struct folio *folio,
 static int nfs_do_writepage(struct folio *folio, struct writeback_control *wbc,
 			    struct nfs_pageio_descriptor *pgio)
 {
-	nfs_pageio_cond_complete(pgio, folio_index(folio));
+	nfs_pageio_cond_complete(pgio, folio->index);
 	return nfs_page_async_flush(folio, wbc, pgio);
 }
 
@@ -666,7 +625,7 @@ static int nfs_writepage_locked(struct folio *folio,
 				struct writeback_control *wbc)
 {
 	struct nfs_pageio_descriptor pgio;
-	struct inode *inode = folio_file_mapping(folio)->host;
+	struct inode *inode = folio->mapping->host;
 	int err;
 
 	nfs_inc_stats(inode, NFSIOS_VFSWRITEPAGE);
@@ -744,24 +703,17 @@ int nfs_writepages(struct address_space *mapping, struct writeback_control *wbc)
 static void nfs_inode_add_request(struct nfs_page *req)
 {
 	struct folio *folio = nfs_page_to_folio(req);
-	struct address_space *mapping = folio_file_mapping(folio);
+	struct address_space *mapping = folio->mapping;
 	struct nfs_inode *nfsi = NFS_I(mapping->host);
 
 	WARN_ON_ONCE(req->wb_this_page != req);
 
 	/* Lock the request! */
 	nfs_lock_request(req);
-
-	/*
-	 * Swap-space should not get truncated. Hence no need to plug the race
-	 * with invalidate/truncate.
-	 */
 	spin_lock(&mapping->i_private_lock);
-	if (likely(!folio_test_swapcache(folio))) {
-		set_bit(PG_MAPPED, &req->wb_flags);
-		folio_set_private(folio);
-		folio->private = req;
-	}
+	set_bit(PG_MAPPED, &req->wb_flags);
+	folio_set_private(folio);
+	folio->private = req;
 	spin_unlock(&mapping->i_private_lock);
 	atomic_long_inc(&nfsi->nrequests);
 	/* this a head request for a page group - mark it as having an
@@ -781,10 +733,10 @@ static void nfs_inode_remove_request(struct nfs_page *req)
 
 	if (nfs_page_group_sync_on_bit(req, PG_REMOVE)) {
 		struct folio *folio = nfs_page_to_folio(req->wb_head);
-		struct address_space *mapping = folio_file_mapping(folio);
+		struct address_space *mapping = folio->mapping;
 
 		spin_lock(&mapping->i_private_lock);
-		if (likely(folio && !folio_test_swapcache(folio))) {
+		if (likely(folio)) {
 			folio->private = NULL;
 			folio_clear_private(folio);
 			clear_bit(PG_MAPPED, &req->wb_head->wb_flags);
@@ -805,38 +757,6 @@ static void nfs_mark_request_dirty(struct nfs_page *req)
 		filemap_dirty_folio(folio_mapping(folio), folio);
 }
 
-/*
- * nfs_page_search_commits_for_head_request_locked
- *
- * Search through commit lists on @inode for the head request for @folio.
- * Must be called while holding the inode (which is cinfo) lock.
- *
- * Returns the head request if found, or NULL if not found.
- */
-static struct nfs_page *
-nfs_page_search_commits_for_head_request_locked(struct nfs_inode *nfsi,
-						struct folio *folio)
-{
-	struct nfs_page *freq, *t;
-	struct nfs_commit_info cinfo;
-	struct inode *inode = &nfsi->vfs_inode;
-
-	nfs_init_cinfo_from_inode(&cinfo, inode);
-
-	/* search through pnfs commit lists */
-	freq = pnfs_search_commit_reqs(inode, &cinfo, folio);
-	if (freq)
-		return freq->wb_head;
-
-	/* Linearly search the commit list for the correct request */
-	list_for_each_entry_safe(freq, t, &cinfo.mds->list, wb_list) {
-		if (nfs_page_to_folio(freq) == folio)
-			return freq->wb_head;
-	}
-
-	return NULL;
-}
-
 /**
  * nfs_request_add_commit_list_locked - add request to a commit list
  * @req: pointer to a struct nfs_page
@@ -943,7 +863,7 @@ static void nfs_folio_clear_commit(struct folio *folio)
 		long nr = folio_nr_pages(folio);
 
 		node_stat_mod_folio(folio, NR_WRITEBACK, -nr);
-		wb_stat_mod(&inode_to_bdi(folio_file_mapping(folio)->host)->wb,
+		wb_stat_mod(&inode_to_bdi(folio->mapping->host)->wb,
 			    WB_WRITEBACK, -nr);
 	}
 }
@@ -1128,7 +1048,7 @@ static struct nfs_page *nfs_try_to_update_request(struct folio *folio,
 	 */
 	nfs_mark_request_dirty(req);
 	nfs_unlock_and_release_request(req);
-	error = nfs_wb_folio(folio_file_mapping(folio)->host, folio);
+	error = nfs_wb_folio(folio->mapping->host, folio);
 	return (error < 0) ? ERR_PTR(error) : NULL;
 }
 
@@ -1204,7 +1124,7 @@ int nfs_flush_incompatible(struct file *file, struct folio *folio)
 		nfs_release_request(req);
 		if (!do_flush)
 			return 0;
-		status = nfs_wb_folio(folio_file_mapping(folio)->host, folio);
+		status = nfs_wb_folio(folio->mapping->host, folio);
 	} while (status == 0);
 	return status;
 }
@@ -1278,7 +1198,7 @@ bool nfs_ctx_key_to_expire(struct nfs_open_context *ctx, struct inode *inode)
  */
 static bool nfs_folio_write_uptodate(struct folio *folio, unsigned int pagelen)
 {
-	struct inode *inode = folio_file_mapping(folio)->host;
+	struct inode *inode = folio->mapping->host;
 	struct nfs_inode *nfsi = NFS_I(inode);
 
 	if (nfs_have_delegated_attributes(inode))
@@ -1356,7 +1276,7 @@ int nfs_update_folio(struct file *file, struct folio *folio,
 		     unsigned int offset, unsigned int count)
 {
 	struct nfs_open_context *ctx = nfs_file_open_context(file);
-	struct address_space *mapping = folio_file_mapping(folio);
+	struct address_space *mapping = folio->mapping;
 	struct inode *inode = mapping->host;
 	unsigned int pagelen = nfs_folio_length(folio);
 	int		status = 0;
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index 1c315f854ea801..7bc31df457ea58 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -208,8 +208,8 @@ static inline struct inode *nfs_page_to_inode(const struct nfs_page *req)
 	struct folio *folio = nfs_page_to_folio(req);
 
 	if (folio == NULL)
-		return page_file_mapping(req->wb_page)->host;
-	return folio_file_mapping(folio)->host;
+		return req->wb_page->mapping->host;
+	return folio->mapping->host;
 }
 
 /**
-- 
2.43.0

Re: [PATCH 1/7] nfs: remove dead code for the old swap over NFS implementation

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 2/7] nfs: remove nfs_folio_private_request

[Christoph Hellwig]

nfs_folio_private_request is a trivial wrapper around, which itself has
fallen out of favor and has been replaced with plain ->private
dereferences in recent folio conversions.  Do the same for nfs.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/write.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 641bdddeaad331..5410c18a006937 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -169,11 +169,6 @@ nfs_cancel_remove_inode(struct nfs_page *req, struct inode *inode)
 	return 0;
 }
 
-static struct nfs_page *nfs_folio_private_request(struct folio *folio)
-{
-	return folio_get_private(folio);
-}
-
 /**
  * nfs_folio_find_head_request - find head request associated with a folio
  * @folio: pointer to folio
@@ -190,7 +185,7 @@ static struct nfs_page *nfs_folio_find_head_request(struct folio *folio)
 	if (!folio_test_private(folio))
 		return NULL;
 	spin_lock(&mapping->i_private_lock);
-	req = nfs_folio_private_request(folio);
+	req = folio->private;
 	if (req) {
 		WARN_ON_ONCE(req->wb_head != req);
 		kref_get(&req->wb_kref);
@@ -220,7 +215,7 @@ static struct nfs_page *nfs_folio_find_and_lock_request(struct folio *folio)
 			return ERR_PTR(ret);
 		}
 		/* Ensure that nobody removed the request before we locked it */
-		if (head == nfs_folio_private_request(folio))
+		if (head == folio->private)
 			break;
 		nfs_unlock_and_release_request(head);
 	}
-- 
2.43.0

Re: [PATCH 2/7] nfs: remove nfs_folio_private_request

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 3/7] nfs: simplify nfs_folio_find_and_lock_request

[Christoph Hellwig]

nfs_folio_find_and_lock_request and the nfs_page_group_lock_head helper
called by it spend quite some effort to deal with head vs subrequests.
But given that only the head request can be stashed in the folio private
data, non of that is required.

Fold the locking logic from nfs_page_group_lock_head into
nfs_folio_find_and_lock_request and simplify the result based on the
invariant that we always find the head request in the folio private data.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/pagelist.c        | 19 -------------------
 fs/nfs/write.c           | 38 +++++++++++++++++++++-----------------
 include/linux/nfs_page.h |  1 -
 3 files changed, 21 insertions(+), 37 deletions(-)

diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 3b006bcbcc87a2..e48cc69a2361aa 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -187,25 +187,6 @@ nfs_async_iocounter_wait(struct rpc_task *task, struct nfs_lock_context *l_ctx)
 }
 EXPORT_SYMBOL_GPL(nfs_async_iocounter_wait);
 
-/*
- * nfs_page_lock_head_request - page lock the head of the page group
- * @req: any member of the page group
- */
-struct nfs_page *
-nfs_page_group_lock_head(struct nfs_page *req)
-{
-	struct nfs_page *head = req->wb_head;
-
-	while (!nfs_lock_request(head)) {
-		int ret = nfs_wait_on_request(head);
-		if (ret < 0)
-			return ERR_PTR(ret);
-	}
-	if (head != req)
-		kref_get(&head->wb_kref);
-	return head;
-}
-
 /*
  * nfs_unroll_locks -  unlock all newly locked reqs and wait on @req
  * @head: head request of page group, must be holding head lock
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 5410c18a006937..58e5b78ff436b9 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -197,28 +197,32 @@ static struct nfs_page *nfs_folio_find_head_request(struct folio *folio)
 static struct nfs_page *nfs_folio_find_and_lock_request(struct folio *folio)
 {
 	struct inode *inode = folio->mapping->host;
-	struct nfs_page *req, *head;
+	struct nfs_page *head;
 	int ret;
 
-	for (;;) {
-		req = nfs_folio_find_head_request(folio);
-		if (!req)
-			return req;
-		head = nfs_page_group_lock_head(req);
-		if (head != req)
-			nfs_release_request(req);
-		if (IS_ERR(head))
-			return head;
-		ret = nfs_cancel_remove_inode(head, inode);
-		if (ret < 0) {
-			nfs_unlock_and_release_request(head);
+retry:
+	head = nfs_folio_find_head_request(folio);
+	if (!head)
+		return NULL;
+
+	while (!nfs_lock_request(head)) {
+		ret = nfs_wait_on_request(head);
+		if (ret < 0)
 			return ERR_PTR(ret);
-		}
-		/* Ensure that nobody removed the request before we locked it */
-		if (head == folio->private)
-			break;
+	}
+
+	/* Ensure that nobody removed the request before we locked it */
+	if (head != folio->private) {
 		nfs_unlock_and_release_request(head);
+		goto retry;
 	}
+
+	ret = nfs_cancel_remove_inode(head, inode);
+	if (ret < 0) {
+		nfs_unlock_and_release_request(head);
+		return ERR_PTR(ret);
+	}
+
 	return head;
 }
 
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index 7bc31df457ea58..e799d93626f117 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -155,7 +155,6 @@ extern size_t nfs_generic_pg_test(struct nfs_pageio_descriptor *desc,
 extern  int nfs_wait_on_request(struct nfs_page *);
 extern	void nfs_unlock_request(struct nfs_page *req);
 extern	void nfs_unlock_and_release_request(struct nfs_page *);
-extern	struct nfs_page *nfs_page_group_lock_head(struct nfs_page *req);
 extern	int nfs_page_group_lock_subrequests(struct nfs_page *head);
 extern void nfs_join_page_group(struct nfs_page *head,
 				struct nfs_commit_info *cinfo,
-- 
2.43.0

Re: [PATCH 3/7] nfs: simplify nfs_folio_find_and_lock_request

[Sagi Grimberg]

On 01/07/2024 8:26, Christoph Hellwig wrote:
> nfs_folio_find_and_lock_request and the nfs_page_group_lock_head helper
> called by it spend quite some effort to deal with head vs subrequests.
> But given that only the head request can be stashed in the folio private
> data, non of that is required.
>
> Fold the locking logic from nfs_page_group_lock_head into
> nfs_folio_find_and_lock_request and simplify the result based on the
> invariant that we always find the head request in the folio private data.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/nfs/pagelist.c        | 19 -------------------
>   fs/nfs/write.c           | 38 +++++++++++++++++++++-----------------
>   include/linux/nfs_page.h |  1 -
>   3 files changed, 21 insertions(+), 37 deletions(-)
>
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index 3b006bcbcc87a2..e48cc69a2361aa 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -187,25 +187,6 @@ nfs_async_iocounter_wait(struct rpc_task *task, struct nfs_lock_context *l_ctx)
>   }
>   EXPORT_SYMBOL_GPL(nfs_async_iocounter_wait);
>   
> -/*
> - * nfs_page_lock_head_request - page lock the head of the page group
> - * @req: any member of the page group
> - */
> -struct nfs_page *
> -nfs_page_group_lock_head(struct nfs_page *req)
> -{
> -	struct nfs_page *head = req->wb_head;
> -
> -	while (!nfs_lock_request(head)) {
> -		int ret = nfs_wait_on_request(head);
> -		if (ret < 0)
> -			return ERR_PTR(ret);
> -	}
> -	if (head != req)
> -		kref_get(&head->wb_kref);
> -	return head;
> -}
> -
>   /*
>    * nfs_unroll_locks -  unlock all newly locked reqs and wait on @req
>    * @head: head request of page group, must be holding head lock
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 5410c18a006937..58e5b78ff436b9 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -197,28 +197,32 @@ static struct nfs_page *nfs_folio_find_head_request(struct folio *folio)
>   static struct nfs_page *nfs_folio_find_and_lock_request(struct folio *folio)
>   {
>   	struct inode *inode = folio->mapping->host;
> -	struct nfs_page *req, *head;
> +	struct nfs_page *head;
>   	int ret;
>   
> -	for (;;) {
> -		req = nfs_folio_find_head_request(folio);
> -		if (!req)
> -			return req;
> -		head = nfs_page_group_lock_head(req);
> -		if (head != req)
> -			nfs_release_request(req);
> -		if (IS_ERR(head))
> -			return head;
> -		ret = nfs_cancel_remove_inode(head, inode);
> -		if (ret < 0) {
> -			nfs_unlock_and_release_request(head);
> +retry:
> +	head = nfs_folio_find_head_request(folio);

Is nfs_folio_find_head_reques() an appropriate name here? it doesn't 
search and find afaict (aside from internally the pnfs commits search). 
Anyways, Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 3/7] nfs: simplify nfs_folio_find_and_lock_request

[Christoph Hellwig]

On Tue, Jul 02, 2024 at 10:54:17AM +0300, Sagi Grimberg wrote:
> Is nfs_folio_find_head_reques() an appropriate name here? it doesn't search 
> and find afaict (aside from internally the pnfs commits search). Anyways, 

I could use a rename.  If everyone agrees I can do that incrementally.

[PATCH 4/7] nfs: fold nfs_folio_find_and_lock_request into nfs_lock_and_join_requests

[Christoph Hellwig]

Fold nfs_folio_find_and_lock_request into the only caller to prepare
for changes to this code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/write.c | 68 ++++++++++++++++++++------------------------------
 1 file changed, 27 insertions(+), 41 deletions(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 58e5b78ff436b9..2b139493168d87 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -194,38 +194,6 @@ static struct nfs_page *nfs_folio_find_head_request(struct folio *folio)
 	return req;
 }
 
-static struct nfs_page *nfs_folio_find_and_lock_request(struct folio *folio)
-{
-	struct inode *inode = folio->mapping->host;
-	struct nfs_page *head;
-	int ret;
-
-retry:
-	head = nfs_folio_find_head_request(folio);
-	if (!head)
-		return NULL;
-
-	while (!nfs_lock_request(head)) {
-		ret = nfs_wait_on_request(head);
-		if (ret < 0)
-			return ERR_PTR(ret);
-	}
-
-	/* Ensure that nobody removed the request before we locked it */
-	if (head != folio->private) {
-		nfs_unlock_and_release_request(head);
-		goto retry;
-	}
-
-	ret = nfs_cancel_remove_inode(head, inode);
-	if (ret < 0) {
-		nfs_unlock_and_release_request(head);
-		return ERR_PTR(ret);
-	}
-
-	return head;
-}
-
 /* Adjust the file length if we're writing beyond the end */
 static void nfs_grow_file(struct folio *folio, unsigned int offset,
 			  unsigned int count)
@@ -530,26 +498,44 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
 	struct nfs_commit_info cinfo;
 	int ret;
 
-	nfs_init_cinfo_from_inode(&cinfo, inode);
 	/*
 	 * A reference is taken only on the head request which acts as a
 	 * reference to the whole page group - the group will not be destroyed
 	 * until the head reference is released.
 	 */
-	head = nfs_folio_find_and_lock_request(folio);
-	if (IS_ERR_OR_NULL(head))
-		return head;
+retry:
+	head = nfs_folio_find_head_request(folio);
+	if (!head)
+		return NULL;
 
-	/* lock each request in the page group */
-	ret = nfs_page_group_lock_subrequests(head);
-	if (ret < 0) {
+	while (!nfs_lock_request(head)) {
+		ret = nfs_wait_on_request(head);
+		if (ret < 0)
+			return ERR_PTR(ret);
+	}
+
+	/* Ensure that nobody removed the request before we locked it */
+	if (head != folio->private) {
 		nfs_unlock_and_release_request(head);
-		return ERR_PTR(ret);
+		goto retry;
 	}
 
-	nfs_join_page_group(head, &cinfo, inode);
+	ret = nfs_cancel_remove_inode(head, inode);
+	if (ret < 0)
+		goto out_unlock;
 
+	/* lock each request in the page group */
+	ret = nfs_page_group_lock_subrequests(head);
+	if (ret < 0)
+		goto out_unlock;
+
+	nfs_init_cinfo_from_inode(&cinfo, inode);
+	nfs_join_page_group(head, &cinfo, inode);
 	return head;
+
+out_unlock:
+	nfs_unlock_and_release_request(head);
+	return ERR_PTR(ret);
 }
 
 static void nfs_write_error(struct nfs_page *req, int error)
-- 
2.43.0

Re: [PATCH 4/7] nfs: fold nfs_folio_find_and_lock_request into nfs_lock_and_join_requests

[Sagi Grimberg]

On 01/07/2024 8:26, Christoph Hellwig wrote:
> Fold nfs_folio_find_and_lock_request into the only caller to prepare
> for changes to this code.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/nfs/write.c | 68 ++++++++++++++++++++------------------------------
>   1 file changed, 27 insertions(+), 41 deletions(-)
>
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 58e5b78ff436b9..2b139493168d87 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -194,38 +194,6 @@ static struct nfs_page *nfs_folio_find_head_request(struct folio *folio)
>   	return req;
>   }
>   
> -static struct nfs_page *nfs_folio_find_and_lock_request(struct folio *folio)
> -{
> -	struct inode *inode = folio->mapping->host;
> -	struct nfs_page *head;
> -	int ret;
> -
> -retry:
> -	head = nfs_folio_find_head_request(folio);
> -	if (!head)
> -		return NULL;
> -
> -	while (!nfs_lock_request(head)) {
> -		ret = nfs_wait_on_request(head);
> -		if (ret < 0)
> -			return ERR_PTR(ret);
> -	}
> -
> -	/* Ensure that nobody removed the request before we locked it */
> -	if (head != folio->private) {
> -		nfs_unlock_and_release_request(head);
> -		goto retry;
> -	}
> -
> -	ret = nfs_cancel_remove_inode(head, inode);
> -	if (ret < 0) {
> -		nfs_unlock_and_release_request(head);
> -		return ERR_PTR(ret);
> -	}
> -
> -	return head;
> -}
> -
>   /* Adjust the file length if we're writing beyond the end */
>   static void nfs_grow_file(struct folio *folio, unsigned int offset,
>   			  unsigned int count)
> @@ -530,26 +498,44 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
>   	struct nfs_commit_info cinfo;
>   	int ret;
>   
> -	nfs_init_cinfo_from_inode(&cinfo, inode);
>   	/*
>   	 * A reference is taken only on the head request which acts as a
>   	 * reference to the whole page group - the group will not be destroyed
>   	 * until the head reference is released.
>   	 */
> -	head = nfs_folio_find_and_lock_request(folio);
> -	if (IS_ERR_OR_NULL(head))
> -		return head;
> +retry:
> +	head = nfs_folio_find_head_request(folio);
> +	if (!head)
> +		return NULL;
>   
> -	/* lock each request in the page group */
> -	ret = nfs_page_group_lock_subrequests(head);
> -	if (ret < 0) {
> +	while (!nfs_lock_request(head)) {
> +		ret = nfs_wait_on_request(head);
> +		if (ret < 0)
> +			return ERR_PTR(ret);
> +	}
> +
> +	/* Ensure that nobody removed the request before we locked it */
> +	if (head != folio->private) {
>   		nfs_unlock_and_release_request(head);
> -		return ERR_PTR(ret);
> +		goto retry;
>   	}
>   
> -	nfs_join_page_group(head, &cinfo, inode);
> +	ret = nfs_cancel_remove_inode(head, inode);
> +	if (ret < 0)
> +		goto out_unlock;
>   
> +	/* lock each request in the page group */
> +	ret = nfs_page_group_lock_subrequests(head);
> +	if (ret < 0)
> +		goto out_unlock;
> +
> +	nfs_init_cinfo_from_inode(&cinfo, inode);

Any reason why nfs_init_cinfo_from_inode() changed location?

Otherwise,
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 4/7] nfs: fold nfs_folio_find_and_lock_request into nfs_lock_and_join_requests

[Christoph Hellwig]

On Tue, Jul 02, 2024 at 10:57:36AM +0300, Sagi Grimberg wrote:
>> +	nfs_init_cinfo_from_inode(&cinfo, inode);
>
> Any reason why nfs_init_cinfo_from_inode() changed location?

I moved it early enough toward where it is being used to better reason
about the code.  But as a nice little advantage that also means it is
only called when actually needed now.

[PATCH 5/7] nfs: fold nfs_page_group_lock_subrequests into nfs_lock_and_join_requests

[Christoph Hellwig]

Fold nfs_page_group_lock_subrequests into nfs_lock_and_join_requests to
prepare for future changes to this code, and move the helpers to write.c
as well.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/pagelist.c        | 77 ----------------------------------------
 fs/nfs/write.c           | 67 ++++++++++++++++++++++++++++++++--
 include/linux/nfs_page.h |  1 -
 3 files changed, 64 insertions(+), 81 deletions(-)

diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index e48cc69a2361aa..fa7971072900b3 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -187,83 +187,6 @@ nfs_async_iocounter_wait(struct rpc_task *task, struct nfs_lock_context *l_ctx)
 }
 EXPORT_SYMBOL_GPL(nfs_async_iocounter_wait);
 
-/*
- * nfs_unroll_locks -  unlock all newly locked reqs and wait on @req
- * @head: head request of page group, must be holding head lock
- * @req: request that couldn't lock and needs to wait on the req bit lock
- *
- * This is a helper function for nfs_lock_and_join_requests
- * returns 0 on success, < 0 on error.
- */
-static void
-nfs_unroll_locks(struct nfs_page *head, struct nfs_page *req)
-{
-	struct nfs_page *tmp;
-
-	/* relinquish all the locks successfully grabbed this run */
-	for (tmp = head->wb_this_page ; tmp != req; tmp = tmp->wb_this_page) {
-		if (!kref_read(&tmp->wb_kref))
-			continue;
-		nfs_unlock_and_release_request(tmp);
-	}
-}
-
-/*
- * nfs_page_group_lock_subreq -  try to lock a subrequest
- * @head: head request of page group
- * @subreq: request to lock
- *
- * This is a helper function for nfs_lock_and_join_requests which
- * must be called with the head request and page group both locked.
- * On error, it returns with the page group unlocked.
- */
-static int
-nfs_page_group_lock_subreq(struct nfs_page *head, struct nfs_page *subreq)
-{
-	int ret;
-
-	if (!kref_get_unless_zero(&subreq->wb_kref))
-		return 0;
-	while (!nfs_lock_request(subreq)) {
-		nfs_page_group_unlock(head);
-		ret = nfs_wait_on_request(subreq);
-		if (!ret)
-			ret = nfs_page_group_lock(head);
-		if (ret < 0) {
-			nfs_unroll_locks(head, subreq);
-			nfs_release_request(subreq);
-			return ret;
-		}
-	}
-	return 0;
-}
-
-/*
- * nfs_page_group_lock_subrequests -  try to lock the subrequests
- * @head: head request of page group
- *
- * This is a helper function for nfs_lock_and_join_requests which
- * must be called with the head request locked.
- */
-int nfs_page_group_lock_subrequests(struct nfs_page *head)
-{
-	struct nfs_page *subreq;
-	int ret;
-
-	ret = nfs_page_group_lock(head);
-	if (ret < 0)
-		return ret;
-	/* lock each request in the page group */
-	for (subreq = head->wb_this_page; subreq != head;
-			subreq = subreq->wb_this_page) {
-		ret = nfs_page_group_lock_subreq(head, subreq);
-		if (ret < 0)
-			return ret;
-	}
-	nfs_page_group_unlock(head);
-	return 0;
-}
-
 /*
  * nfs_page_set_headlock - set the request PG_HEADLOCK
  * @req: request that is to be locked
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 2b139493168d87..3ba298ebb0be14 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -476,6 +476,57 @@ void nfs_join_page_group(struct nfs_page *head, struct nfs_commit_info *cinfo,
 	nfs_destroy_unlinked_subrequests(destroy_list, head, inode);
 }
 
+/*
+ * nfs_unroll_locks -  unlock all newly locked reqs and wait on @req
+ * @head: head request of page group, must be holding head lock
+ * @req: request that couldn't lock and needs to wait on the req bit lock
+ *
+ * This is a helper function for nfs_lock_and_join_requests
+ * returns 0 on success, < 0 on error.
+ */
+static void
+nfs_unroll_locks(struct nfs_page *head, struct nfs_page *req)
+{
+	struct nfs_page *tmp;
+
+	/* relinquish all the locks successfully grabbed this run */
+	for (tmp = head->wb_this_page ; tmp != req; tmp = tmp->wb_this_page) {
+		if (!kref_read(&tmp->wb_kref))
+			continue;
+		nfs_unlock_and_release_request(tmp);
+	}
+}
+
+/*
+ * nfs_page_group_lock_subreq -  try to lock a subrequest
+ * @head: head request of page group
+ * @subreq: request to lock
+ *
+ * This is a helper function for nfs_lock_and_join_requests which
+ * must be called with the head request and page group both locked.
+ * On error, it returns with the page group unlocked.
+ */
+static int
+nfs_page_group_lock_subreq(struct nfs_page *head, struct nfs_page *subreq)
+{
+	int ret;
+
+	if (!kref_get_unless_zero(&subreq->wb_kref))
+		return 0;
+	while (!nfs_lock_request(subreq)) {
+		nfs_page_group_unlock(head);
+		ret = nfs_wait_on_request(subreq);
+		if (!ret)
+			ret = nfs_page_group_lock(head);
+		if (ret < 0) {
+			nfs_unroll_locks(head, subreq);
+			nfs_release_request(subreq);
+			return ret;
+		}
+	}
+	return 0;
+}
+
 /*
  * nfs_lock_and_join_requests - join all subreqs to the head req
  * @folio: the folio used to lookup the "page group" of nfs_page structures
@@ -494,7 +545,7 @@ void nfs_join_page_group(struct nfs_page *head, struct nfs_commit_info *cinfo,
 static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
 {
 	struct inode *inode = folio->mapping->host;
-	struct nfs_page *head;
+	struct nfs_page *head, *subreq;
 	struct nfs_commit_info cinfo;
 	int ret;
 
@@ -524,11 +575,21 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
 	if (ret < 0)
 		goto out_unlock;
 
-	/* lock each request in the page group */
-	ret = nfs_page_group_lock_subrequests(head);
+	ret = nfs_page_group_lock(head);
 	if (ret < 0)
 		goto out_unlock;
 
+	/* lock each request in the page group */
+	for (subreq = head->wb_this_page;
+	     subreq != head;
+	     subreq = subreq->wb_this_page) {
+		ret = nfs_page_group_lock_subreq(head, subreq);
+		if (ret < 0)
+			goto out_unlock;
+	}
+
+	nfs_page_group_unlock(head);
+
 	nfs_init_cinfo_from_inode(&cinfo, inode);
 	nfs_join_page_group(head, &cinfo, inode);
 	return head;
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index e799d93626f117..63eed97a18ade9 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -155,7 +155,6 @@ extern size_t nfs_generic_pg_test(struct nfs_pageio_descriptor *desc,
 extern  int nfs_wait_on_request(struct nfs_page *);
 extern	void nfs_unlock_request(struct nfs_page *req);
 extern	void nfs_unlock_and_release_request(struct nfs_page *);
-extern	int nfs_page_group_lock_subrequests(struct nfs_page *head);
 extern void nfs_join_page_group(struct nfs_page *head,
 				struct nfs_commit_info *cinfo,
 				struct inode *inode);
-- 
2.43.0

Re: [PATCH 5/7] nfs: fold nfs_page_group_lock_subrequests into nfs_lock_and_join_requests

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 6/7] nfs: move nfs_wait_on_request to write.c

[Christoph Hellwig]

nfs_wait_on_request is now only used in write.c.  Move it there
and mark it static.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/pagelist.c        | 19 -------------------
 fs/nfs/write.c           | 17 +++++++++++++++++
 include/linux/nfs_page.h |  1 -
 3 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index fa7971072900b3..04124f22666530 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -598,25 +598,6 @@ void nfs_release_request(struct nfs_page *req)
 }
 EXPORT_SYMBOL_GPL(nfs_release_request);
 
-/**
- * nfs_wait_on_request - Wait for a request to complete.
- * @req: request to wait upon.
- *
- * Interruptible by fatal signals only.
- * The user is responsible for holding a count on the request.
- */
-int
-nfs_wait_on_request(struct nfs_page *req)
-{
-	if (!test_bit(PG_BUSY, &req->wb_flags))
-		return 0;
-	set_bit(PG_CONTENDED2, &req->wb_flags);
-	smp_mb__after_atomic();
-	return wait_on_bit_io(&req->wb_flags, PG_BUSY,
-			      TASK_UNINTERRUPTIBLE);
-}
-EXPORT_SYMBOL_GPL(nfs_wait_on_request);
-
 /*
  * nfs_generic_pg_test - determine if requests can be coalesced
  * @desc: pointer to descriptor
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 3ba298ebb0be14..2c089444303982 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -476,6 +476,23 @@ void nfs_join_page_group(struct nfs_page *head, struct nfs_commit_info *cinfo,
 	nfs_destroy_unlinked_subrequests(destroy_list, head, inode);
 }
 
+/**
+ * nfs_wait_on_request - Wait for a request to complete.
+ * @req: request to wait upon.
+ *
+ * Interruptible by fatal signals only.
+ * The user is responsible for holding a count on the request.
+ */
+static int nfs_wait_on_request(struct nfs_page *req)
+{
+	if (!test_bit(PG_BUSY, &req->wb_flags))
+		return 0;
+	set_bit(PG_CONTENDED2, &req->wb_flags);
+	smp_mb__after_atomic();
+	return wait_on_bit_io(&req->wb_flags, PG_BUSY,
+			      TASK_UNINTERRUPTIBLE);
+}
+
 /*
  * nfs_unroll_locks -  unlock all newly locked reqs and wait on @req
  * @head: head request of page group, must be holding head lock
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index 63eed97a18ade9..169b4ae30ff479 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -152,7 +152,6 @@ extern	void nfs_pageio_cond_complete(struct nfs_pageio_descriptor *, pgoff_t);
 extern size_t nfs_generic_pg_test(struct nfs_pageio_descriptor *desc,
 				struct nfs_page *prev,
 				struct nfs_page *req);
-extern  int nfs_wait_on_request(struct nfs_page *);
 extern	void nfs_unlock_request(struct nfs_page *req);
 extern	void nfs_unlock_and_release_request(struct nfs_page *);
 extern void nfs_join_page_group(struct nfs_page *head,
-- 
2.43.0

Re: [PATCH 6/7] nfs: move nfs_wait_on_request to write.c

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 7/7] nfs: don't reuse partially completed requests in nfs_lock_and_join_requests

[Christoph Hellwig]

When NFS requests are split into sub-requests, nfs_inode_remove_request
calls nfs_page_group_sync_on_bit to set PG_REMOVE on this sub-request and
only completes the head requests once PG_REMOVE is set on all requests.
This means that when nfs_lock_and_join_requests sees a PG_REMOVE bit, I/O
on the request is in progress and has partially completed.   If such a
request is returned to nfs_try_to_update_request, it could be extended
with the newly dirtied region and I/O for the combined range will be
re-scheduled, leading to extra I/O.

Change the logic to instead restart the search for a request when any
PG_REMOVE bit is set, as the completion handler will remove the request
as soon as it can take the page group lock.  This not only avoid
extending the I/O but also does the right thing for the callers that
want to cancel or flush the request.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/write.c | 49 ++++++++++++++++++++-----------------------------
 1 file changed, 20 insertions(+), 29 deletions(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 2c089444303982..4dffdc5aadb2e2 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -144,31 +144,6 @@ static void nfs_io_completion_put(struct nfs_io_completion *ioc)
 		kref_put(&ioc->refcount, nfs_io_completion_release);
 }
 
-static void
-nfs_page_set_inode_ref(struct nfs_page *req, struct inode *inode)
-{
-	if (!test_and_set_bit(PG_INODE_REF, &req->wb_flags)) {
-		kref_get(&req->wb_kref);
-		atomic_long_inc(&NFS_I(inode)->nrequests);
-	}
-}
-
-static int
-nfs_cancel_remove_inode(struct nfs_page *req, struct inode *inode)
-{
-	int ret;
-
-	if (!test_bit(PG_REMOVE, &req->wb_flags))
-		return 0;
-	ret = nfs_page_group_lock(req);
-	if (ret)
-		return ret;
-	if (test_and_clear_bit(PG_REMOVE, &req->wb_flags))
-		nfs_page_set_inode_ref(req, inode);
-	nfs_page_group_unlock(req);
-	return 0;
-}
-
 /**
  * nfs_folio_find_head_request - find head request associated with a folio
  * @folio: pointer to folio
@@ -564,6 +539,7 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
 	struct inode *inode = folio->mapping->host;
 	struct nfs_page *head, *subreq;
 	struct nfs_commit_info cinfo;
+	bool removed;
 	int ret;
 
 	/*
@@ -588,18 +564,18 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
 		goto retry;
 	}
 
-	ret = nfs_cancel_remove_inode(head, inode);
-	if (ret < 0)
-		goto out_unlock;
-
 	ret = nfs_page_group_lock(head);
 	if (ret < 0)
 		goto out_unlock;
 
+	removed = test_bit(PG_REMOVE, &head->wb_flags);
+
 	/* lock each request in the page group */
 	for (subreq = head->wb_this_page;
 	     subreq != head;
 	     subreq = subreq->wb_this_page) {
+		if (test_bit(PG_REMOVE, &subreq->wb_flags))
+			removed = true;
 		ret = nfs_page_group_lock_subreq(head, subreq);
 		if (ret < 0)
 			goto out_unlock;
@@ -607,6 +583,21 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
 
 	nfs_page_group_unlock(head);
 
+	/*
+	 * If PG_REMOVE is set on any request, I/O on that request has
+	 * completed, but some requests were still under I/O at the time
+	 * we locked the head request.
+	 *
+	 * In that case the above wait for all requests means that all I/O
+	 * has now finished, and we can restart from a clean slate.  Let the
+	 * old requests go away and start from scratch instead.
+	 */
+	if (removed) {
+		nfs_unroll_locks(head, head);
+		nfs_unlock_and_release_request(head);
+		goto retry;
+	}
+
 	nfs_init_cinfo_from_inode(&cinfo, inode);
 	nfs_join_page_group(head, &cinfo, inode);
 	return head;
-- 
2.43.0

Re: [PATCH 7/7] nfs: don't reuse partially completed requests in nfs_lock_and_join_requests

[Sagi Grimberg]

On 01/07/2024 8:26, Christoph Hellwig wrote:
> When NFS requests are split into sub-requests, nfs_inode_remove_request
> calls nfs_page_group_sync_on_bit to set PG_REMOVE on this sub-request and
> only completes the head requests once PG_REMOVE is set on all requests.
> This means that when nfs_lock_and_join_requests sees a PG_REMOVE bit, I/O
> on the request is in progress and has partially completed.   If such a
> request is returned to nfs_try_to_update_request, it could be extended
> with the newly dirtied region and I/O for the combined range will be
> re-scheduled, leading to extra I/O.

Probably worth noting in the change log that large folios makes this 
potentially much
worse?

>
> Change the logic to instead restart the search for a request when any
> PG_REMOVE bit is set, as the completion handler will remove the request
> as soon as it can take the page group lock.  This not only avoid
> extending the I/O but also does the right thing for the callers that
> want to cancel or flush the request.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/nfs/write.c | 49 ++++++++++++++++++++-----------------------------
>   1 file changed, 20 insertions(+), 29 deletions(-)
>
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 2c089444303982..4dffdc5aadb2e2 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -144,31 +144,6 @@ static void nfs_io_completion_put(struct nfs_io_completion *ioc)
>   		kref_put(&ioc->refcount, nfs_io_completion_release);
>   }
>   
> -static void
> -nfs_page_set_inode_ref(struct nfs_page *req, struct inode *inode)
> -{
> -	if (!test_and_set_bit(PG_INODE_REF, &req->wb_flags)) {
> -		kref_get(&req->wb_kref);
> -		atomic_long_inc(&NFS_I(inode)->nrequests);
> -	}
> -}
> -
> -static int
> -nfs_cancel_remove_inode(struct nfs_page *req, struct inode *inode)
> -{
> -	int ret;
> -
> -	if (!test_bit(PG_REMOVE, &req->wb_flags))
> -		return 0;
> -	ret = nfs_page_group_lock(req);
> -	if (ret)
> -		return ret;
> -	if (test_and_clear_bit(PG_REMOVE, &req->wb_flags))
> -		nfs_page_set_inode_ref(req, inode);
> -	nfs_page_group_unlock(req);
> -	return 0;
> -}
> -
>   /**
>    * nfs_folio_find_head_request - find head request associated with a folio
>    * @folio: pointer to folio
> @@ -564,6 +539,7 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
>   	struct inode *inode = folio->mapping->host;
>   	struct nfs_page *head, *subreq;
>   	struct nfs_commit_info cinfo;
> +	bool removed;
>   	int ret;
>   
>   	/*
> @@ -588,18 +564,18 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
>   		goto retry;
>   	}
>   
> -	ret = nfs_cancel_remove_inode(head, inode);
> -	if (ret < 0)
> -		goto out_unlock;
> -
>   	ret = nfs_page_group_lock(head);
>   	if (ret < 0)
>   		goto out_unlock;
>   
> +	removed = test_bit(PG_REMOVE, &head->wb_flags);
> +
>   	/* lock each request in the page group */
>   	for (subreq = head->wb_this_page;
>   	     subreq != head;
>   	     subreq = subreq->wb_this_page) {
> +		if (test_bit(PG_REMOVE, &subreq->wb_flags))
> +			removed = true;
>   		ret = nfs_page_group_lock_subreq(head, subreq);
>   		if (ret < 0)
>   			goto out_unlock;
> @@ -607,6 +583,21 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
>   
>   	nfs_page_group_unlock(head);
>   
> +	/*
> +	 * If PG_REMOVE is set on any request, I/O on that request has
> +	 * completed, but some requests were still under I/O at the time
> +	 * we locked the head request.
> +	 *
> +	 * In that case the above wait for all requests means that all I/O
> +	 * has now finished, and we can restart from a clean slate.  Let the
> +	 * old requests go away and start from scratch instead.
> +	 */
> +	if (removed) {
> +		nfs_unroll_locks(head, head);
> +		nfs_unlock_and_release_request(head);
> +		goto retry;
> +	}

Don't you need a waitqueue or something to avoid excessive restarts 
until the
IO completes?

Re: [PATCH 7/7] nfs: don't reuse partially completed requests in nfs_lock_and_join_requests

[Christoph Hellwig]

On Tue, Jul 02, 2024 at 11:07:13AM +0300, Sagi Grimberg wrote:
> On 01/07/2024 8:26, Christoph Hellwig wrote:
>> When NFS requests are split into sub-requests, nfs_inode_remove_request
>> calls nfs_page_group_sync_on_bit to set PG_REMOVE on this sub-request and
>> only completes the head requests once PG_REMOVE is set on all requests.
>> This means that when nfs_lock_and_join_requests sees a PG_REMOVE bit, I/O
>> on the request is in progress and has partially completed.   If such a
>> request is returned to nfs_try_to_update_request, it could be extended
>> with the newly dirtied region and I/O for the combined range will be
>> re-scheduled, leading to extra I/O.
>
> Probably worth noting in the change log that large folios makes this 
> potentially much
> worse?

That assumes large folios actually create more subrequest.  One big
reason to create subrequests is flexfiles mirroring, which of course
doesn't change with large folio.  The other is that if ->pg_test
doesn't allow the nfs_page to cover everything, which is roughly
bound by a page array allocation and for PNFS the layout segment
size, and the chance for that to fail could very slightly increase.

Re: NFS buffered write cleanup

[Christoph Hellwig]

On Mon, Jul 01, 2024 at 07:26:47AM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series cleans up the nfs_page handling in the buffer write path.
> 
> The first patch was already sent independently but hasn't been picked up
> and this included here again.
> 
> The last patch fixes a bug where a request could get incorrectly reused.
> It would require the flexfiles layout and odd I/O timings, and without
> a flexfiles server I can't actually hit it.  I'd appreciate a careful
> review of that one.
> 
> The series is against Trond's testing branch.

Looks like Anna is doing this merge window despite the earlier testing
branch from Trond.  That already has the first patch applied, and the
rest applies fine to it as well.