[PATCH 0/5] more pgio cleanup

[PATCH 0/5] more pgio cleanup

[Weston Andros Adamson]

There was a mixup and this patchset was posted to Trond, but not the list -
and they've already made it to his testing branch. Oops!

"nfs: check wait_on_bit_lock err in page_group_lock" does not handle blocking
calls correctly and is fixed by a patch in patchset I just posted: 

  [PATCH 2/5] nfs: fix nonblocking calls to nfs_page_group_lock

I don't know if we want to apply it as a fixup or leave them as separate
patches..

Sorry, I did not intend to bypass the list! Have at them and I'll address
any issues ASAP.

Thanks,

 -dros


Weston Andros Adamson (5):
  nfs: check wait_on_bit_lock err in page_group_lock
  nfs: fix comment and add warn_on for PG_INODE_REF
  pnfs: find swapped pages on pnfs commit lists too
  pnfs: add pnfs_put_lseg_async
  nfs: clear_request_commit while holding i_lock

 fs/nfs/filelayout/filelayout.c | 36 +++++++++++++++++++--
 fs/nfs/pagelist.c              | 29 +++++++++++++----
 fs/nfs/pnfs.c                  | 17 ++++++++++
 fs/nfs/pnfs.h                  | 27 ++++++++++++++++
 fs/nfs/write.c                 | 72 +++++++++++++++++++++++++++---------------
 include/linux/nfs_page.h       |  4 +--
 6 files changed, 149 insertions(+), 36 deletions(-)

-- 
1.8.5.2 (Apple Git-48)

[PATCH 1/5] nfs: check wait_on_bit_lock err in page_group_lock

[Weston Andros Adamson]

Return errors from wait_on_bit_lock from nfs_page_group_lock.

Add a bool argument @wait to nfs_page_group_lock. If true, loop over
wait_on_bit_lock until it returns cleanly. If false, return the error
from wait_on_bit_lock.

Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/pagelist.c        | 29 +++++++++++++++++++++++------
 fs/nfs/write.c           |  6 ++++--
 include/linux/nfs_page.h |  2 +-
 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index e76a40e..9425118 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -147,17 +147,25 @@ static int nfs_wait_bit_uninterruptible(void *word)
  * @req - request in group that is to be locked
  *
  * this lock must be held if modifying the page group list
+ *
+ * returns result from wait_on_bit_lock: 0 on success, < 0 on error
  */
-void
-nfs_page_group_lock(struct nfs_page *req)
+int
+nfs_page_group_lock(struct nfs_page *req, bool wait)
 {
 	struct nfs_page *head = req->wb_head;
+	int ret;
 
 	WARN_ON_ONCE(head != head->wb_head);
 
-	wait_on_bit_lock(&head->wb_flags, PG_HEADLOCK,
+	do {
+		ret = wait_on_bit_lock(&head->wb_flags, PG_HEADLOCK,
 			nfs_wait_bit_uninterruptible,
 			TASK_UNINTERRUPTIBLE);
+	} while (wait && ret != 0);
+
+	WARN_ON_ONCE(ret > 0);
+	return ret;
 }
 
 /*
@@ -218,7 +226,7 @@ bool nfs_page_group_sync_on_bit(struct nfs_page *req, unsigned int bit)
 {
 	bool ret;
 
-	nfs_page_group_lock(req);
+	nfs_page_group_lock(req, true);
 	ret = nfs_page_group_sync_on_bit_locked(req, bit);
 	nfs_page_group_unlock(req);
 
@@ -858,8 +866,13 @@ static int __nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,
 	struct nfs_page *subreq;
 	unsigned int bytes_left = 0;
 	unsigned int offset, pgbase;
+	int ret;
 
-	nfs_page_group_lock(req);
+	ret = nfs_page_group_lock(req, false);
+	if (ret < 0) {
+		desc->pg_error = ret;
+		return 0;
+	}
 
 	subreq = req;
 	bytes_left = subreq->wb_bytes;
@@ -881,7 +894,11 @@ static int __nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,
 			if (desc->pg_recoalesce)
 				return 0;
 			/* retry add_request for this subreq */
-			nfs_page_group_lock(req);
+			ret = nfs_page_group_lock(req, false);
+			if (ret < 0) {
+				desc->pg_error = ret;
+				return 0;
+			}
 			continue;
 		}
 
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index d357728..8d1ed2b 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -216,7 +216,7 @@ static bool nfs_page_group_covers_page(struct nfs_page *req)
 	unsigned int pos = 0;
 	unsigned int len = nfs_page_length(req->wb_page);
 
-	nfs_page_group_lock(req);
+	nfs_page_group_lock(req, true);
 
 	do {
 		tmp = nfs_page_group_search_locked(req->wb_head, pos);
@@ -456,7 +456,9 @@ try_again:
 	}
 
 	/* lock each request in the page group */
-	nfs_page_group_lock(head);
+	ret = nfs_page_group_lock(head, false);
+	if (ret < 0)
+		return ERR_PTR(ret);
 	subreq = head;
 	do {
 		/*
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index 4b48548..291924c 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -122,7 +122,7 @@ 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 void nfs_page_group_lock(struct nfs_page *);
+extern int nfs_page_group_lock(struct nfs_page *, bool);
 extern void nfs_page_group_unlock(struct nfs_page *);
 extern bool nfs_page_group_sync_on_bit(struct nfs_page *, unsigned int);
 
-- 
1.8.5.2 (Apple Git-48)

[PATCH 2/5] nfs: fix comment and add warn_on for PG_INODE_REF

[Weston Andros Adamson]

Fix the comment in nfs_page.h for PG_INODE_REF to reflect that it's no longer
set only on head requests. Also add a WARN_ON_ONCE in nfs_inode_remove_request
as PG_INODE_REF should always be set.

Suggested-by: Peng Tao <tao.peng@primarydata.com>
Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/write.c           | 2 ++
 include/linux/nfs_page.h | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 8d1ed2b..e6bc5b5 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -707,6 +707,8 @@ static void nfs_inode_remove_request(struct nfs_page *req)
 
 	if (test_and_clear_bit(PG_INODE_REF, &req->wb_flags))
 		nfs_release_request(req);
+	else
+		WARN_ON_ONCE(1);
 }
 
 static void
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index 291924c..6ad2bbc 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -26,7 +26,7 @@ enum {
 	PG_MAPPED,		/* page private set for buffered io */
 	PG_CLEAN,		/* write succeeded */
 	PG_COMMIT_TO_DS,	/* used by pnfs layouts */
-	PG_INODE_REF,		/* extra ref held by inode (head req only) */
+	PG_INODE_REF,		/* extra ref held by inode when in writeback */
 	PG_HEADLOCK,		/* page group lock of wb_head */
 	PG_TEARDOWN,		/* page group sync for destroy */
 	PG_UNLOCKPAGE,		/* page group sync bit in read path */
-- 
1.8.5.2 (Apple Git-48)

[PATCH 3/5] pnfs: find swapped pages on pnfs commit lists too

[Weston Andros Adamson]

nfs_page_find_head_request_locked looks through the regular nfs commit lists
when the page is swapped out, but doesn't look through the pnfs commit lists.

I'm not sure if anyone has hit any issues caused by this.

Suggested-by: Peng Tao <tao.peng@primarydata.com>
Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/filelayout/filelayout.c | 31 ++++++++++++++++++++++++++
 fs/nfs/pnfs.h                  | 20 +++++++++++++++++
 fs/nfs/write.c                 | 49 +++++++++++++++++++++++++++++++-----------
 3 files changed, 88 insertions(+), 12 deletions(-)

diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
index 2576d28b..524e66f 100644
--- a/fs/nfs/filelayout/filelayout.c
+++ b/fs/nfs/filelayout/filelayout.c
@@ -1237,6 +1237,36 @@ restart:
 	spin_unlock(cinfo->lock);
 }
 
+/* filelayout_search_commit_reqs - Search lists in @cinfo for the head reqest
+ *				   for @page
+ * @cinfo - commit info for current inode
+ * @page - page to search for matching head request
+ *
+ * Returns a the head request if one is found, otherwise returns NULL.
+ */
+static struct nfs_page *
+filelayout_search_commit_reqs(struct nfs_commit_info *cinfo, struct page *page)
+{
+	struct nfs_page *freq, *t;
+	struct pnfs_commit_bucket *b;
+	int i;
+
+	/* Linearly search the commit lists for each bucket until a matching
+	 * request is found */
+	for (i = 0, b = cinfo->ds->buckets; i < cinfo->ds->nbuckets; i++, b++) {
+		list_for_each_entry_safe(freq, t, &b->written, wb_list) {
+			if (freq->wb_page == page)
+				return freq->wb_head;
+		}
+		list_for_each_entry_safe(freq, t, &b->committing, wb_list) {
+			if (freq->wb_page == page)
+				return freq->wb_head;
+		}
+	}
+
+	return NULL;
+}
+
 static void filelayout_retry_commit(struct nfs_commit_info *cinfo, int idx)
 {
 	struct pnfs_ds_commit_info *fl_cinfo = cinfo->ds;
@@ -1386,6 +1416,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
 	.clear_request_commit	= filelayout_clear_request_commit,
 	.scan_commit_lists	= filelayout_scan_commit_lists,
 	.recover_commit_reqs	= filelayout_recover_commit_reqs,
+	.search_commit_reqs	= filelayout_search_commit_reqs,
 	.commit_pagelist	= filelayout_commit_pagelist,
 	.read_pagelist		= filelayout_read_pagelist,
 	.write_pagelist		= filelayout_write_pagelist,
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 27ddecd..203b6c9 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -104,6 +104,8 @@ struct pnfs_layoutdriver_type {
 				  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 page *page);
 	int (*commit_pagelist)(struct inode *inode,
 			       struct list_head *mds_pages,
 			       int how,
@@ -341,6 +343,17 @@ pnfs_recover_commit_reqs(struct inode *inode, struct list_head *list,
 	NFS_SERVER(inode)->pnfs_curr_ld->recover_commit_reqs(list, cinfo);
 }
 
+static inline struct nfs_page *
+pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo,
+			struct page *page)
+{
+	struct pnfs_layoutdriver_type *ld = NFS_SERVER(inode)->pnfs_curr_ld;
+
+	if (ld == NULL || ld->search_commit_reqs == NULL)
+		return NULL;
+	return ld->search_commit_reqs(cinfo, page);
+}
+
 /* Should the pNFS client commit and return the layout upon a setattr */
 static inline bool
 pnfs_ld_layoutret_on_setattr(struct inode *inode)
@@ -492,6 +505,13 @@ pnfs_recover_commit_reqs(struct inode *inode, struct list_head *list,
 {
 }
 
+static inline struct nfs_page *
+pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo,
+			struct page *page)
+{
+	return NULL;
+}
+
 static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 {
 	return 0;
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index e6bc5b5..ba41769 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -47,6 +47,8 @@ static const struct nfs_pgio_completion_ops nfs_async_write_completion_ops;
 static const struct nfs_commit_completion_ops nfs_commit_completion_ops;
 static const struct nfs_rw_ops nfs_rw_write_ops;
 static void nfs_clear_request_commit(struct nfs_page *req);
+static void nfs_init_cinfo_from_inode(struct nfs_commit_info *cinfo,
+				      struct inode *inode);
 
 static struct kmem_cache *nfs_wdata_cachep;
 static mempool_t *nfs_wdata_mempool;
@@ -93,6 +95,38 @@ static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error)
 }
 
 /*
+ * nfs_page_search_commits_for_head_request_locked
+ *
+ * Search through commit lists on @inode for the head request for @page.
+ * 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 page *page)
+{
+	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, page);
+	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 (freq->wb_page == page)
+			return freq->wb_head;
+	}
+
+	return NULL;
+}
+
+/*
  * nfs_page_find_head_request_locked - find head request associated with @page
  *
  * must be called while holding the inode lock.
@@ -106,21 +140,12 @@ nfs_page_find_head_request_locked(struct nfs_inode *nfsi, struct page *page)
 
 	if (PagePrivate(page))
 		req = (struct nfs_page *)page_private(page);
-	else if (unlikely(PageSwapCache(page))) {
-		struct nfs_page *freq, *t;
-
-		/* Linearly search the commit list for the correct req */
-		list_for_each_entry_safe(freq, t, &nfsi->commit_info.list, wb_list) {
-			if (freq->wb_page == page) {
-				req = freq->wb_head;
-				break;
-			}
-		}
-	}
+	else if (unlikely(PageSwapCache(page)))
+		req = nfs_page_search_commits_for_head_request_locked(nfsi,
+			page);
 
 	if (req) {
 		WARN_ON_ONCE(req->wb_head != req);
-
 		kref_get(&req->wb_kref);
 	}
 
-- 
1.8.5.2 (Apple Git-48)

[PATCH 4/5] pnfs: add pnfs_put_lseg_async

[Weston Andros Adamson]

This is useful when lsegs need to be released while holding locks.

Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/pnfs.c | 17 +++++++++++++++++
 fs/nfs/pnfs.h |  7 +++++++
 2 files changed, 24 insertions(+)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 83ff8a0..4e85315 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -361,6 +361,23 @@ pnfs_put_lseg(struct pnfs_layout_segment *lseg)
 }
 EXPORT_SYMBOL_GPL(pnfs_put_lseg);
 
+static void pnfs_put_lseg_async_work(struct work_struct *work)
+{
+	struct pnfs_layout_segment *lseg;
+
+	lseg = container_of(work, struct pnfs_layout_segment, pls_work);
+
+	pnfs_put_lseg(lseg);
+}
+
+void
+pnfs_put_lseg_async(struct pnfs_layout_segment *lseg)
+{
+	INIT_WORK(&lseg->pls_work, pnfs_put_lseg_async_work);
+	schedule_work(&lseg->pls_work);
+}
+EXPORT_SYMBOL_GPL(pnfs_put_lseg_async);
+
 static u64
 end_offset(u64 start, u64 len)
 {
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 203b6c9..aca3dff 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -32,6 +32,7 @@
 
 #include <linux/nfs_fs.h>
 #include <linux/nfs_page.h>
+#include <linux/workqueue.h>
 
 enum {
 	NFS_LSEG_VALID = 0,	/* cleared when lseg is recalled/returned */
@@ -46,6 +47,7 @@ struct pnfs_layout_segment {
 	atomic_t pls_refcount;
 	unsigned long pls_flags;
 	struct pnfs_layout_hdr *pls_layout;
+	struct work_struct pls_work;
 };
 
 enum pnfs_try_status {
@@ -181,6 +183,7 @@ extern int nfs4_proc_layoutreturn(struct nfs4_layoutreturn *lrp);
 /* pnfs.c */
 void pnfs_get_layout_hdr(struct pnfs_layout_hdr *lo);
 void pnfs_put_lseg(struct pnfs_layout_segment *lseg);
+void pnfs_put_lseg_async(struct pnfs_layout_segment *lseg);
 
 void set_pnfs_layoutdriver(struct nfs_server *, const struct nfs_fh *, u32);
 void unset_pnfs_layoutdriver(struct nfs_server *);
@@ -419,6 +422,10 @@ static inline void pnfs_put_lseg(struct pnfs_layout_segment *lseg)
 {
 }
 
+static inline void pnfs_put_lseg_async(struct pnfs_layout_segment *lseg)
+{
+}
+
 static inline int pnfs_return_layout(struct inode *ino)
 {
 	return 0;
-- 
1.8.5.2 (Apple Git-48)

[PATCH 5/5] nfs: clear_request_commit while holding i_lock

[Weston Andros Adamson]

Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/filelayout/filelayout.c |  5 ++---
 fs/nfs/write.c                 | 15 ++++-----------
 2 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
index 524e66f..1359c4a 100644
--- a/fs/nfs/filelayout/filelayout.c
+++ b/fs/nfs/filelayout/filelayout.c
@@ -1009,6 +1009,7 @@ static u32 select_bucket_index(struct nfs4_filelayout_segment *fl, u32 j)
 
 /* The generic layer is about to remove the req from the commit list.
  * If this will make the bucket empty, it will need to put the lseg reference.
+ * Note this is must be called holding the inode (/cinfo) lock
  */
 static void
 filelayout_clear_request_commit(struct nfs_page *req,
@@ -1016,7 +1017,6 @@ filelayout_clear_request_commit(struct nfs_page *req,
 {
 	struct pnfs_layout_segment *freeme = NULL;
 
-	spin_lock(cinfo->lock);
 	if (!test_and_clear_bit(PG_COMMIT_TO_DS, &req->wb_flags))
 		goto out;
 	cinfo->ds->nwritten--;
@@ -1031,8 +1031,7 @@ filelayout_clear_request_commit(struct nfs_page *req,
 	}
 out:
 	nfs_request_remove_commit_list(req, cinfo);
-	spin_unlock(cinfo->lock);
-	pnfs_put_lseg(freeme);
+	pnfs_put_lseg_async(freeme);
 }
 
 static void
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index ba41769..1065de2 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -404,8 +404,6 @@ nfs_destroy_unlinked_subrequests(struct nfs_page *destroy_list,
 		subreq->wb_head = subreq;
 		subreq->wb_this_page = subreq;
 
-		nfs_clear_request_commit(subreq);
-
 		/* subreq is now totally disconnected from page group or any
 		 * write / commit lists. last chance to wake any waiters */
 		nfs_unlock_request(subreq);
@@ -515,7 +513,7 @@ try_again:
 	 * Commit list removal accounting is done after locks are dropped */
 	subreq = head;
 	do {
-		nfs_list_remove_request(subreq);
+		nfs_clear_request_commit(subreq);
 		subreq = subreq->wb_this_page;
 	} while (subreq != head);
 
@@ -545,15 +543,11 @@ try_again:
 
 	nfs_page_group_unlock(head);
 
-	/* drop lock to clear_request_commit the head req and clean up
-	 * requests on destroy list */
+	/* drop lock to clean uprequests on destroy list */
 	spin_unlock(&inode->i_lock);
 
 	nfs_destroy_unlinked_subrequests(destroy_list, head);
 
-	/* clean up commit list state */
-	nfs_clear_request_commit(head);
-
 	/* still holds ref on head from nfs_page_find_head_request_locked
 	 * and still has lock on head from lock loop */
 	return head;
@@ -837,6 +831,7 @@ nfs_clear_page_commit(struct page *page)
 	dec_bdi_stat(page_file_mapping(page)->backing_dev_info, BDI_RECLAIMABLE);
 }
 
+/* Called holding inode (/cinfo) lock */
 static void
 nfs_clear_request_commit(struct nfs_page *req)
 {
@@ -846,9 +841,7 @@ nfs_clear_request_commit(struct nfs_page *req)
 
 		nfs_init_cinfo_from_inode(&cinfo, inode);
 		if (!pnfs_clear_request_commit(req, &cinfo)) {
-			spin_lock(cinfo.lock);
 			nfs_request_remove_commit_list(req, &cinfo);
-			spin_unlock(cinfo.lock);
 		}
 		nfs_clear_page_commit(req->wb_page);
 	}
@@ -1061,9 +1054,9 @@ static struct nfs_page *nfs_try_to_update_request(struct inode *inode,
 	else
 		req->wb_bytes = rqend - req->wb_offset;
 out_unlock:
-	spin_unlock(&inode->i_lock);
 	if (req)
 		nfs_clear_request_commit(req);
+	spin_unlock(&inode->i_lock);
 	return req;
 out_flushme:
 	spin_unlock(&inode->i_lock);
-- 
1.8.5.2 (Apple Git-48)

Re: [PATCH 3/5] pnfs: find swapped pages on pnfs commit lists too

[Anna Schumaker]

On 08/08/2014 11:13 AM, Weston Andros Adamson wrote:
> nfs_page_find_head_request_locked looks through the regular nfs commit lists
> when the page is swapped out, but doesn't look through the pnfs commit lists.
>
> I'm not sure if anyone has hit any issues caused by this.
>
> Suggested-by: Peng Tao <tao.peng@primarydata.com>
> Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  fs/nfs/filelayout/filelayout.c | 31 ++++++++++++++++++++++++++
>  fs/nfs/pnfs.h                  | 20 +++++++++++++++++
>  fs/nfs/write.c                 | 49 +++++++++++++++++++++++++++++++-----------
>  3 files changed, 88 insertions(+), 12 deletions(-)
>
> diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
> index 2576d28b..524e66f 100644
> --- a/fs/nfs/filelayout/filelayout.c
> +++ b/fs/nfs/filelayout/filelayout.c
> @@ -1237,6 +1237,36 @@ restart:
>  	spin_unlock(cinfo->lock);
>  }
>  
> +/* filelayout_search_commit_reqs - Search lists in @cinfo for the head reqest
> + *				   for @page
> + * @cinfo - commit info for current inode
> + * @page - page to search for matching head request
> + *
> + * Returns a the head request if one is found, otherwise returns NULL.
> + */
> +static struct nfs_page *
> +filelayout_search_commit_reqs(struct nfs_commit_info *cinfo, struct page *page)
> +{
> +	struct nfs_page *freq, *t;
> +	struct pnfs_commit_bucket *b;
> +	int i;
> +
> +	/* Linearly search the commit lists for each bucket until a matching
> +	 * request is found */
> +	for (i = 0, b = cinfo->ds->buckets; i < cinfo->ds->nbuckets; i++, b++) {
> +		list_for_each_entry_safe(freq, t, &b->written, wb_list) {
> +			if (freq->wb_page == page)
> +				return freq->wb_head;
> +		}
> +		list_for_each_entry_safe(freq, t, &b->committing, wb_list) {
> +			if (freq->wb_page == page)
> +				return freq->wb_head;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
>  static void filelayout_retry_commit(struct nfs_commit_info *cinfo, int idx)
>  {
>  	struct pnfs_ds_commit_info *fl_cinfo = cinfo->ds;
> @@ -1386,6 +1416,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
>  	.clear_request_commit	= filelayout_clear_request_commit,
>  	.scan_commit_lists	= filelayout_scan_commit_lists,
>  	.recover_commit_reqs	= filelayout_recover_commit_reqs,
> +	.search_commit_reqs	= filelayout_search_commit_reqs,
>  	.commit_pagelist	= filelayout_commit_pagelist,
>  	.read_pagelist		= filelayout_read_pagelist,
>  	.write_pagelist		= filelayout_write_pagelist,
> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
> index 27ddecd..203b6c9 100644
> --- a/fs/nfs/pnfs.h
> +++ b/fs/nfs/pnfs.h
> @@ -104,6 +104,8 @@ struct pnfs_layoutdriver_type {
>  				  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 page *page);
>  	int (*commit_pagelist)(struct inode *inode,
>  			       struct list_head *mds_pages,
>  			       int how,
> @@ -341,6 +343,17 @@ pnfs_recover_commit_reqs(struct inode *inode, struct list_head *list,
>  	NFS_SERVER(inode)->pnfs_curr_ld->recover_commit_reqs(list, cinfo);
>  }
>  
> +static inline struct nfs_page *
> +pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo,
> +			struct page *page)
> +{
> +	struct pnfs_layoutdriver_type *ld = NFS_SERVER(inode)->pnfs_curr_ld;
> +
> +	if (ld == NULL || ld->search_commit_reqs == NULL)
> +		return NULL;
> +	return ld->search_commit_reqs(cinfo, page);
> +}
> +
>  /* Should the pNFS client commit and return the layout upon a setattr */
>  static inline bool
>  pnfs_ld_layoutret_on_setattr(struct inode *inode)
> @@ -492,6 +505,13 @@ pnfs_recover_commit_reqs(struct inode *inode, struct list_head *list,
>  {
>  }
>  
> +static inline struct nfs_page *
> +pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo,
> +			struct page *page)
> +{
> +	return NULL;
> +}
> +
>  static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
>  {
>  	return 0;
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index e6bc5b5..ba41769 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -47,6 +47,8 @@ static const struct nfs_pgio_completion_ops nfs_async_write_completion_ops;
>  static const struct nfs_commit_completion_ops nfs_commit_completion_ops;
>  static const struct nfs_rw_ops nfs_rw_write_ops;
>  static void nfs_clear_request_commit(struct nfs_page *req);
> +static void nfs_init_cinfo_from_inode(struct nfs_commit_info *cinfo,
> +				      struct inode *inode);
>  
>  static struct kmem_cache *nfs_wdata_cachep;
>  static mempool_t *nfs_wdata_mempool;
> @@ -93,6 +95,38 @@ static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error)
>  }
>  
>  /*
> + * nfs_page_search_commits_for_head_request_locked
> + *
> + * Search through commit lists on @inode for the head request for @page.
> + * 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 page *page)
> +{
> +	struct nfs_page *freq, *t;
> +	struct nfs_commit_info cinfo;
> +	struct inode *inode = &nfsi->vfs_inode;
> +
> +	nfs_init_cinfo_from_inode(&cinfo, inode);
This has a compiler warning when CONFIG_NFS_V4=n, CONFIG_NFS_V3=n, CONFIG_NFS_V2=y:

fs/nfs/write.c: In function ‘nfs_page_find_head_request_locked.isra.16’:
include/linux/kernel.h:834:39: error: ‘cinfo.mds’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
                                       ^
fs/nfs/write.c:110:25: note: ‘cinfo.mds’ was declared here
  struct nfs_commit_info cinfo;

Probably because nfs_init_cinfo_from_inode() is empty without v3 or v4.

Anna
> +
> +	/* search through pnfs commit lists */
> +	freq = pnfs_search_commit_reqs(inode, &cinfo, page);
> +	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 (freq->wb_page == page)
> +			return freq->wb_head;
> +	}
> +
> +	return NULL;
> +}
> +
> +/*
>   * nfs_page_find_head_request_locked - find head request associated with @page
>   *
>   * must be called while holding the inode lock.
> @@ -106,21 +140,12 @@ nfs_page_find_head_request_locked(struct nfs_inode *nfsi, struct page *page)
>  
>  	if (PagePrivate(page))
>  		req = (struct nfs_page *)page_private(page);
> -	else if (unlikely(PageSwapCache(page))) {
> -		struct nfs_page *freq, *t;
> -
> -		/* Linearly search the commit list for the correct req */
> -		list_for_each_entry_safe(freq, t, &nfsi->commit_info.list, wb_list) {
> -			if (freq->wb_page == page) {
> -				req = freq->wb_head;
> -				break;
> -			}
> -		}
> -	}
> +	else if (unlikely(PageSwapCache(page)))
> +		req = nfs_page_search_commits_for_head_request_locked(nfsi,
> +			page);
>  
>  	if (req) {
>  		WARN_ON_ONCE(req->wb_head != req);
> -
>  		kref_get(&req->wb_kref);
>  	}
>  

Re: [PATCH 3/5] pnfs: find swapped pages on pnfs commit lists too

[Weston Andros Adamson]

Thanks Anna, I’ll fix it up.

-dros



On Aug 11, 2014, at 9:30 AM, Anna Schumaker <schumaker.anna@gmail.com> wrote:

> On 08/08/2014 11:13 AM, Weston Andros Adamson wrote:
>> nfs_page_find_head_request_locked looks through the regular nfs commit lists
>> when the page is swapped out, but doesn't look through the pnfs commit lists.
>> 
>> I'm not sure if anyone has hit any issues caused by this.
>> 
>> Suggested-by: Peng Tao <tao.peng@primarydata.com>
>> Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>> ---
>> fs/nfs/filelayout/filelayout.c | 31 ++++++++++++++++++++++++++
>> fs/nfs/pnfs.h                  | 20 +++++++++++++++++
>> fs/nfs/write.c                 | 49 +++++++++++++++++++++++++++++++-----------
>> 3 files changed, 88 insertions(+), 12 deletions(-)
>> 
>> diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
>> index 2576d28b..524e66f 100644
>> --- a/fs/nfs/filelayout/filelayout.c
>> +++ b/fs/nfs/filelayout/filelayout.c
>> @@ -1237,6 +1237,36 @@ restart:
>> 	spin_unlock(cinfo->lock);
>> }
>> 
>> +/* filelayout_search_commit_reqs - Search lists in @cinfo for the head reqest
>> + *				   for @page
>> + * @cinfo - commit info for current inode
>> + * @page - page to search for matching head request
>> + *
>> + * Returns a the head request if one is found, otherwise returns NULL.
>> + */
>> +static struct nfs_page *
>> +filelayout_search_commit_reqs(struct nfs_commit_info *cinfo, struct page *page)
>> +{
>> +	struct nfs_page *freq, *t;
>> +	struct pnfs_commit_bucket *b;
>> +	int i;
>> +
>> +	/* Linearly search the commit lists for each bucket until a matching
>> +	 * request is found */
>> +	for (i = 0, b = cinfo->ds->buckets; i < cinfo->ds->nbuckets; i++, b++) {
>> +		list_for_each_entry_safe(freq, t, &b->written, wb_list) {
>> +			if (freq->wb_page == page)
>> +				return freq->wb_head;
>> +		}
>> +		list_for_each_entry_safe(freq, t, &b->committing, wb_list) {
>> +			if (freq->wb_page == page)
>> +				return freq->wb_head;
>> +		}
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> static void filelayout_retry_commit(struct nfs_commit_info *cinfo, int idx)
>> {
>> 	struct pnfs_ds_commit_info *fl_cinfo = cinfo->ds;
>> @@ -1386,6 +1416,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
>> 	.clear_request_commit	= filelayout_clear_request_commit,
>> 	.scan_commit_lists	= filelayout_scan_commit_lists,
>> 	.recover_commit_reqs	= filelayout_recover_commit_reqs,
>> +	.search_commit_reqs	= filelayout_search_commit_reqs,
>> 	.commit_pagelist	= filelayout_commit_pagelist,
>> 	.read_pagelist		= filelayout_read_pagelist,
>> 	.write_pagelist		= filelayout_write_pagelist,
>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
>> index 27ddecd..203b6c9 100644
>> --- a/fs/nfs/pnfs.h
>> +++ b/fs/nfs/pnfs.h
>> @@ -104,6 +104,8 @@ struct pnfs_layoutdriver_type {
>> 				  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 page *page);
>> 	int (*commit_pagelist)(struct inode *inode,
>> 			       struct list_head *mds_pages,
>> 			       int how,
>> @@ -341,6 +343,17 @@ pnfs_recover_commit_reqs(struct inode *inode, struct list_head *list,
>> 	NFS_SERVER(inode)->pnfs_curr_ld->recover_commit_reqs(list, cinfo);
>> }
>> 
>> +static inline struct nfs_page *
>> +pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo,
>> +			struct page *page)
>> +{
>> +	struct pnfs_layoutdriver_type *ld = NFS_SERVER(inode)->pnfs_curr_ld;
>> +
>> +	if (ld == NULL || ld->search_commit_reqs == NULL)
>> +		return NULL;
>> +	return ld->search_commit_reqs(cinfo, page);
>> +}
>> +
>> /* Should the pNFS client commit and return the layout upon a setattr */
>> static inline bool
>> pnfs_ld_layoutret_on_setattr(struct inode *inode)
>> @@ -492,6 +505,13 @@ pnfs_recover_commit_reqs(struct inode *inode, struct list_head *list,
>> {
>> }
>> 
>> +static inline struct nfs_page *
>> +pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo,
>> +			struct page *page)
>> +{
>> +	return NULL;
>> +}
>> +
>> static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
>> {
>> 	return 0;
>> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
>> index e6bc5b5..ba41769 100644
>> --- a/fs/nfs/write.c
>> +++ b/fs/nfs/write.c
>> @@ -47,6 +47,8 @@ static const struct nfs_pgio_completion_ops nfs_async_write_completion_ops;
>> static const struct nfs_commit_completion_ops nfs_commit_completion_ops;
>> static const struct nfs_rw_ops nfs_rw_write_ops;
>> static void nfs_clear_request_commit(struct nfs_page *req);
>> +static void nfs_init_cinfo_from_inode(struct nfs_commit_info *cinfo,
>> +				      struct inode *inode);
>> 
>> static struct kmem_cache *nfs_wdata_cachep;
>> static mempool_t *nfs_wdata_mempool;
>> @@ -93,6 +95,38 @@ static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error)
>> }
>> 
>> /*
>> + * nfs_page_search_commits_for_head_request_locked
>> + *
>> + * Search through commit lists on @inode for the head request for @page.
>> + * 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 page *page)
>> +{
>> +	struct nfs_page *freq, *t;
>> +	struct nfs_commit_info cinfo;
>> +	struct inode *inode = &nfsi->vfs_inode;
>> +
>> +	nfs_init_cinfo_from_inode(&cinfo, inode);
> This has a compiler warning when CONFIG_NFS_V4=n, CONFIG_NFS_V3=n, CONFIG_NFS_V2=y:
> 
> fs/nfs/write.c: In function ‘nfs_page_find_head_request_locked.isra.16’:
> include/linux/kernel.h:834:39: error: ‘cinfo.mds’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
>                                       ^
> fs/nfs/write.c:110:25: note: ‘cinfo.mds’ was declared here
>  struct nfs_commit_info cinfo;
> 
> Probably because nfs_init_cinfo_from_inode() is empty without v3 or v4.
> 
> Anna
>> +
>> +	/* search through pnfs commit lists */
>> +	freq = pnfs_search_commit_reqs(inode, &cinfo, page);
>> +	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 (freq->wb_page == page)
>> +			return freq->wb_head;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/*
>>  * nfs_page_find_head_request_locked - find head request associated with @page
>>  *
>>  * must be called while holding the inode lock.
>> @@ -106,21 +140,12 @@ nfs_page_find_head_request_locked(struct nfs_inode *nfsi, struct page *page)
>> 
>> 	if (PagePrivate(page))
>> 		req = (struct nfs_page *)page_private(page);
>> -	else if (unlikely(PageSwapCache(page))) {
>> -		struct nfs_page *freq, *t;
>> -
>> -		/* Linearly search the commit list for the correct req */
>> -		list_for_each_entry_safe(freq, t, &nfsi->commit_info.list, wb_list) {
>> -			if (freq->wb_page == page) {
>> -				req = freq->wb_head;
>> -				break;
>> -			}
>> -		}
>> -	}
>> +	else if (unlikely(PageSwapCache(page)))
>> +		req = nfs_page_search_commits_for_head_request_locked(nfsi,
>> +			page);
>> 
>> 	if (req) {
>> 		WARN_ON_ONCE(req->wb_head != req);
>> -
>> 		kref_get(&req->wb_kref);
>> 	}

Re: [PATCH 3/5] pnfs: find swapped pages on pnfs commit lists too

[Weston Andros Adamson]

For some reason I can’t reproduce this with:

CONFIG_NFS_FS=m
CONFIG_NFS_V2=m
# CONFIG_NFS_V3 is not set
# CONFIG_NFS_V4 is not set

Are you compiling with some special option? It’s not in the output of make W=1, W=3 or W=3.

It looks like there are several places that call nfs_init_cinfo_from_inode without any ifdef logic…

We could just fix all of them and be done with it, but I’m wondering why you get the warning and I don’t.

-dros



On Aug 11, 2014, at 10:54 PM, Weston Andros Adamson <dros@primarydata.com> wrote:

> Thanks Anna, I’ll fix it up.
> 
> -dros
> 
> 
> 
> On Aug 11, 2014, at 9:30 AM, Anna Schumaker <schumaker.anna@gmail.com> wrote:
> 
>> On 08/08/2014 11:13 AM, Weston Andros Adamson wrote:
>>> nfs_page_find_head_request_locked looks through the regular nfs commit lists
>>> when the page is swapped out, but doesn't look through the pnfs commit lists.
>>> 
>>> I'm not sure if anyone has hit any issues caused by this.
>>> 
>>> Suggested-by: Peng Tao <tao.peng@primarydata.com>
>>> Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
>>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>>> ---
>>> fs/nfs/filelayout/filelayout.c | 31 ++++++++++++++++++++++++++
>>> fs/nfs/pnfs.h                  | 20 +++++++++++++++++
>>> fs/nfs/write.c                 | 49 +++++++++++++++++++++++++++++++-----------
>>> 3 files changed, 88 insertions(+), 12 deletions(-)
>>> 
>>> diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
>>> index 2576d28b..524e66f 100644
>>> --- a/fs/nfs/filelayout/filelayout.c
>>> +++ b/fs/nfs/filelayout/filelayout.c
>>> @@ -1237,6 +1237,36 @@ restart:
>>> 	spin_unlock(cinfo->lock);
>>> }
>>> 
>>> +/* filelayout_search_commit_reqs - Search lists in @cinfo for the head reqest
>>> + *				   for @page
>>> + * @cinfo - commit info for current inode
>>> + * @page - page to search for matching head request
>>> + *
>>> + * Returns a the head request if one is found, otherwise returns NULL.
>>> + */
>>> +static struct nfs_page *
>>> +filelayout_search_commit_reqs(struct nfs_commit_info *cinfo, struct page *page)
>>> +{
>>> +	struct nfs_page *freq, *t;
>>> +	struct pnfs_commit_bucket *b;
>>> +	int i;
>>> +
>>> +	/* Linearly search the commit lists for each bucket until a matching
>>> +	 * request is found */
>>> +	for (i = 0, b = cinfo->ds->buckets; i < cinfo->ds->nbuckets; i++, b++) {
>>> +		list_for_each_entry_safe(freq, t, &b->written, wb_list) {
>>> +			if (freq->wb_page == page)
>>> +				return freq->wb_head;
>>> +		}
>>> +		list_for_each_entry_safe(freq, t, &b->committing, wb_list) {
>>> +			if (freq->wb_page == page)
>>> +				return freq->wb_head;
>>> +		}
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> static void filelayout_retry_commit(struct nfs_commit_info *cinfo, int idx)
>>> {
>>> 	struct pnfs_ds_commit_info *fl_cinfo = cinfo->ds;
>>> @@ -1386,6 +1416,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
>>> 	.clear_request_commit	= filelayout_clear_request_commit,
>>> 	.scan_commit_lists	= filelayout_scan_commit_lists,
>>> 	.recover_commit_reqs	= filelayout_recover_commit_reqs,
>>> +	.search_commit_reqs	= filelayout_search_commit_reqs,
>>> 	.commit_pagelist	= filelayout_commit_pagelist,
>>> 	.read_pagelist		= filelayout_read_pagelist,
>>> 	.write_pagelist		= filelayout_write_pagelist,
>>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
>>> index 27ddecd..203b6c9 100644
>>> --- a/fs/nfs/pnfs.h
>>> +++ b/fs/nfs/pnfs.h
>>> @@ -104,6 +104,8 @@ struct pnfs_layoutdriver_type {
>>> 				  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 page *page);
>>> 	int (*commit_pagelist)(struct inode *inode,
>>> 			       struct list_head *mds_pages,
>>> 			       int how,
>>> @@ -341,6 +343,17 @@ pnfs_recover_commit_reqs(struct inode *inode, struct list_head *list,
>>> 	NFS_SERVER(inode)->pnfs_curr_ld->recover_commit_reqs(list, cinfo);
>>> }
>>> 
>>> +static inline struct nfs_page *
>>> +pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo,
>>> +			struct page *page)
>>> +{
>>> +	struct pnfs_layoutdriver_type *ld = NFS_SERVER(inode)->pnfs_curr_ld;
>>> +
>>> +	if (ld == NULL || ld->search_commit_reqs == NULL)
>>> +		return NULL;
>>> +	return ld->search_commit_reqs(cinfo, page);
>>> +}
>>> +
>>> /* Should the pNFS client commit and return the layout upon a setattr */
>>> static inline bool
>>> pnfs_ld_layoutret_on_setattr(struct inode *inode)
>>> @@ -492,6 +505,13 @@ pnfs_recover_commit_reqs(struct inode *inode, struct list_head *list,
>>> {
>>> }
>>> 
>>> +static inline struct nfs_page *
>>> +pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo,
>>> +			struct page *page)
>>> +{
>>> +	return NULL;
>>> +}
>>> +
>>> static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
>>> {
>>> 	return 0;
>>> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
>>> index e6bc5b5..ba41769 100644
>>> --- a/fs/nfs/write.c
>>> +++ b/fs/nfs/write.c
>>> @@ -47,6 +47,8 @@ static const struct nfs_pgio_completion_ops nfs_async_write_completion_ops;
>>> static const struct nfs_commit_completion_ops nfs_commit_completion_ops;
>>> static const struct nfs_rw_ops nfs_rw_write_ops;
>>> static void nfs_clear_request_commit(struct nfs_page *req);
>>> +static void nfs_init_cinfo_from_inode(struct nfs_commit_info *cinfo,
>>> +				      struct inode *inode);
>>> 
>>> static struct kmem_cache *nfs_wdata_cachep;
>>> static mempool_t *nfs_wdata_mempool;
>>> @@ -93,6 +95,38 @@ static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error)
>>> }
>>> 
>>> /*
>>> + * nfs_page_search_commits_for_head_request_locked
>>> + *
>>> + * Search through commit lists on @inode for the head request for @page.
>>> + * 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 page *page)
>>> +{
>>> +	struct nfs_page *freq, *t;
>>> +	struct nfs_commit_info cinfo;
>>> +	struct inode *inode = &nfsi->vfs_inode;
>>> +
>>> +	nfs_init_cinfo_from_inode(&cinfo, inode);
>> This has a compiler warning when CONFIG_NFS_V4=n, CONFIG_NFS_V3=n, CONFIG_NFS_V2=y:
>> 
>> fs/nfs/write.c: In function ‘nfs_page_find_head_request_locked.isra.16’:
>> include/linux/kernel.h:834:39: error: ‘cinfo.mds’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>> const typeof( ((type *)0)->member ) *__mptr = (ptr); \
>>                                      ^
>> fs/nfs/write.c:110:25: note: ‘cinfo.mds’ was declared here
>> struct nfs_commit_info cinfo;
>> 
>> Probably because nfs_init_cinfo_from_inode() is empty without v3 or v4.
>> 
>> Anna
>>> +
>>> +	/* search through pnfs commit lists */
>>> +	freq = pnfs_search_commit_reqs(inode, &cinfo, page);
>>> +	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 (freq->wb_page == page)
>>> +			return freq->wb_head;
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +/*
>>> * nfs_page_find_head_request_locked - find head request associated with @page
>>> *
>>> * must be called while holding the inode lock.
>>> @@ -106,21 +140,12 @@ nfs_page_find_head_request_locked(struct nfs_inode *nfsi, struct page *page)
>>> 
>>> 	if (PagePrivate(page))
>>> 		req = (struct nfs_page *)page_private(page);
>>> -	else if (unlikely(PageSwapCache(page))) {
>>> -		struct nfs_page *freq, *t;
>>> -
>>> -		/* Linearly search the commit list for the correct req */
>>> -		list_for_each_entry_safe(freq, t, &nfsi->commit_info.list, wb_list) {
>>> -			if (freq->wb_page == page) {
>>> -				req = freq->wb_head;
>>> -				break;
>>> -			}
>>> -		}
>>> -	}
>>> +	else if (unlikely(PageSwapCache(page)))
>>> +		req = nfs_page_search_commits_for_head_request_locked(nfsi,
>>> +			page);
>>> 
>>> 	if (req) {
>>> 		WARN_ON_ONCE(req->wb_head != req);
>>> -
>>> 		kref_get(&req->wb_kref);
>>> 	}
> 

Re: [PATCH 3/5] pnfs: find swapped pages on pnfs commit lists too

[Anna Schumaker]

On 08/15/2014 10:48 AM, Weston Andros Adamson wrote:
> For some reason I can�t reproduce this with:
>
> CONFIG_NFS_FS=m
> CONFIG_NFS_V2=m
> # CONFIG_NFS_V3 is not set
> # CONFIG_NFS_V4 is not set
>
> Are you compiling with some special option? It�s not in the output of make W=1, W=3 or W=3.
>
> It looks like there are several places that call nfs_init_cinfo_from_inode without any ifdef logic�
>
> We could just fix all of them and be done with it, but I�m wondering why you get the warning and I don�t.

Whoa, somehow I missed this email (cats probably walked over my keyboard, sorry!).  I don't think I'm setting any special config options, but I can try regenerating my .config.  This is what I have set:

CONFIG_NFS_FS=m
CONFIG_NFS_V2=m
# CONFIG_NFS_V3 is not set
# CONFIG_NFS_V4 is not set
CONFIG_NFS_SWAP=y
CONFIG_NFS_FSCACHE=y
CONFIG_NFS_ACL_SUPPORT=m
CONFIG_NFS_COMMON=y
CONFIG_SUNRPC_GSS=m
CONFIG_SUNRPC_SWAP=y
# CONFIG_SUNRPC_DEBUG is not set

Anna
>
> -dros
>
>
>
> On Aug 11, 2014, at 10:54 PM, Weston Andros Adamson <dros@primarydata.com> wrote:
>
>> Thanks Anna, I�ll fix it up.
>>
>> -dros
>>
>>
>>
>> On Aug 11, 2014, at 9:30 AM, Anna Schumaker <schumaker.anna@gmail.com> wrote:
>>
>>> On 08/08/2014 11:13 AM, Weston Andros Adamson wrote:
>>>> nfs_page_find_head_request_locked looks through the regular nfs commit lists
>>>> when the page is swapped out, but doesn't look through the pnfs commit lists.
>>>>
>>>> I'm not sure if anyone has hit any issues caused by this.
>>>>
>>>> Suggested-by: Peng Tao <tao.peng@primarydata.com>
>>>> Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
>>>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>>>> ---
>>>> fs/nfs/filelayout/filelayout.c | 31 ++++++++++++++++++++++++++
>>>> fs/nfs/pnfs.h                  | 20 +++++++++++++++++
>>>> fs/nfs/write.c                 | 49 +++++++++++++++++++++++++++++++-----------
>>>> 3 files changed, 88 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
>>>> index 2576d28b..524e66f 100644
>>>> --- a/fs/nfs/filelayout/filelayout.c
>>>> +++ b/fs/nfs/filelayout/filelayout.c
>>>> @@ -1237,6 +1237,36 @@ restart:
>>>> 	spin_unlock(cinfo->lock);
>>>> }
>>>>
>>>> +/* filelayout_search_commit_reqs - Search lists in @cinfo for the head reqest
>>>> + *				   for @page
>>>> + * @cinfo - commit info for current inode
>>>> + * @page - page to search for matching head request
>>>> + *
>>>> + * Returns a the head request if one is found, otherwise returns NULL.
>>>> + */
>>>> +static struct nfs_page *
>>>> +filelayout_search_commit_reqs(struct nfs_commit_info *cinfo, struct page *page)
>>>> +{
>>>> +	struct nfs_page *freq, *t;
>>>> +	struct pnfs_commit_bucket *b;
>>>> +	int i;
>>>> +
>>>> +	/* Linearly search the commit lists for each bucket until a matching
>>>> +	 * request is found */
>>>> +	for (i = 0, b = cinfo->ds->buckets; i < cinfo->ds->nbuckets; i++, b++) {
>>>> +		list_for_each_entry_safe(freq, t, &b->written, wb_list) {
>>>> +			if (freq->wb_page == page)
>>>> +				return freq->wb_head;
>>>> +		}
>>>> +		list_for_each_entry_safe(freq, t, &b->committing, wb_list) {
>>>> +			if (freq->wb_page == page)
>>>> +				return freq->wb_head;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> static void filelayout_retry_commit(struct nfs_commit_info *cinfo, int idx)
>>>> {
>>>> 	struct pnfs_ds_commit_info *fl_cinfo = cinfo->ds;
>>>> @@ -1386,6 +1416,7 @@ static struct pnfs_layoutdriver_type filelayout_type = {
>>>> 	.clear_request_commit	= filelayout_clear_request_commit,
>>>> 	.scan_commit_lists	= filelayout_scan_commit_lists,
>>>> 	.recover_commit_reqs	= filelayout_recover_commit_reqs,
>>>> +	.search_commit_reqs	= filelayout_search_commit_reqs,
>>>> 	.commit_pagelist	= filelayout_commit_pagelist,
>>>> 	.read_pagelist		= filelayout_read_pagelist,
>>>> 	.write_pagelist		= filelayout_write_pagelist,
>>>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
>>>> index 27ddecd..203b6c9 100644
>>>> --- a/fs/nfs/pnfs.h
>>>> +++ b/fs/nfs/pnfs.h
>>>> @@ -104,6 +104,8 @@ struct pnfs_layoutdriver_type {
>>>> 				  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 page *page);
>>>> 	int (*commit_pagelist)(struct inode *inode,
>>>> 			       struct list_head *mds_pages,
>>>> 			       int how,
>>>> @@ -341,6 +343,17 @@ pnfs_recover_commit_reqs(struct inode *inode, struct list_head *list,
>>>> 	NFS_SERVER(inode)->pnfs_curr_ld->recover_commit_reqs(list, cinfo);
>>>> }
>>>>
>>>> +static inline struct nfs_page *
>>>> +pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo,
>>>> +			struct page *page)
>>>> +{
>>>> +	struct pnfs_layoutdriver_type *ld = NFS_SERVER(inode)->pnfs_curr_ld;
>>>> +
>>>> +	if (ld == NULL || ld->search_commit_reqs == NULL)
>>>> +		return NULL;
>>>> +	return ld->search_commit_reqs(cinfo, page);
>>>> +}
>>>> +
>>>> /* Should the pNFS client commit and return the layout upon a setattr */
>>>> static inline bool
>>>> pnfs_ld_layoutret_on_setattr(struct inode *inode)
>>>> @@ -492,6 +505,13 @@ pnfs_recover_commit_reqs(struct inode *inode, struct list_head *list,
>>>> {
>>>> }
>>>>
>>>> +static inline struct nfs_page *
>>>> +pnfs_search_commit_reqs(struct inode *inode, struct nfs_commit_info *cinfo,
>>>> +			struct page *page)
>>>> +{
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> static inline int pnfs_layoutcommit_inode(struct inode *inode, bool sync)
>>>> {
>>>> 	return 0;
>>>> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
>>>> index e6bc5b5..ba41769 100644
>>>> --- a/fs/nfs/write.c
>>>> +++ b/fs/nfs/write.c
>>>> @@ -47,6 +47,8 @@ static const struct nfs_pgio_completion_ops nfs_async_write_completion_ops;
>>>> static const struct nfs_commit_completion_ops nfs_commit_completion_ops;
>>>> static const struct nfs_rw_ops nfs_rw_write_ops;
>>>> static void nfs_clear_request_commit(struct nfs_page *req);
>>>> +static void nfs_init_cinfo_from_inode(struct nfs_commit_info *cinfo,
>>>> +				      struct inode *inode);
>>>>
>>>> static struct kmem_cache *nfs_wdata_cachep;
>>>> static mempool_t *nfs_wdata_mempool;
>>>> @@ -93,6 +95,38 @@ static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error)
>>>> }
>>>>
>>>> /*
>>>> + * nfs_page_search_commits_for_head_request_locked
>>>> + *
>>>> + * Search through commit lists on @inode for the head request for @page.
>>>> + * 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 page *page)
>>>> +{
>>>> +	struct nfs_page *freq, *t;
>>>> +	struct nfs_commit_info cinfo;
>>>> +	struct inode *inode = &nfsi->vfs_inode;
>>>> +
>>>> +	nfs_init_cinfo_from_inode(&cinfo, inode);
>>> This has a compiler warning when CONFIG_NFS_V4=n, CONFIG_NFS_V3=n, CONFIG_NFS_V2=y:
>>>
>>> fs/nfs/write.c: In function �nfs_page_find_head_request_locked.isra.16�:
>>> include/linux/kernel.h:834:39: error: �cinfo.mds� may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>> const typeof( ((type *)0)->member ) *__mptr = (ptr); \
>>>                                      ^
>>> fs/nfs/write.c:110:25: note: �cinfo.mds� was declared here
>>> struct nfs_commit_info cinfo;
>>>
>>> Probably because nfs_init_cinfo_from_inode() is empty without v3 or v4.
>>>
>>> Anna
>>>> +
>>>> +	/* search through pnfs commit lists */
>>>> +	freq = pnfs_search_commit_reqs(inode, &cinfo, page);
>>>> +	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 (freq->wb_page == page)
>>>> +			return freq->wb_head;
>>>> +	}
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +/*
>>>> * nfs_page_find_head_request_locked - find head request associated with @page
>>>> *
>>>> * must be called while holding the inode lock.
>>>> @@ -106,21 +140,12 @@ nfs_page_find_head_request_locked(struct nfs_inode *nfsi, struct page *page)
>>>>
>>>> 	if (PagePrivate(page))
>>>> 		req = (struct nfs_page *)page_private(page);
>>>> -	else if (unlikely(PageSwapCache(page))) {
>>>> -		struct nfs_page *freq, *t;
>>>> -
>>>> -		/* Linearly search the commit list for the correct req */
>>>> -		list_for_each_entry_safe(freq, t, &nfsi->commit_info.list, wb_list) {
>>>> -			if (freq->wb_page == page) {
>>>> -				req = freq->wb_head;
>>>> -				break;
>>>> -			}
>>>> -		}
>>>> -	}
>>>> +	else if (unlikely(PageSwapCache(page)))
>>>> +		req = nfs_page_search_commits_for_head_request_locked(nfsi,
>>>> +			page);
>>>>
>>>> 	if (req) {
>>>> 		WARN_ON_ONCE(req->wb_head != req);
>>>> -
>>>> 		kref_get(&req->wb_kref);
>>>> 	}