more pgio cleanup
Subsystems (2)
cover more pgio cleanup
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(-)
Signature · 1 line
1.8.5.2 (Apple Git-48)
PATCH 1/5 nfs: check wait_on_bit_lock err in page_group_lock
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.
--- fs/nfs/pagelist.c | 29 +++++++++++++++++++++++------ fs/nfs/write.c | 6 ++++-- include/linux/nfs_page.h | 2 +- 3 files changed, 28 insertions(+), 9 deletions(-)
fs/nfs/pagelist.c +23 -6
@@ -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 ; }
fs/nfs/write.c +4 -2
@@ -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 { /*
include/linux/nfs_page.h +1 -1
@@ -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 );
Signature · 1 line
1.8.5.2 (Apple Git-48)
PATCH 2/5 nfs: fix comment and add warn_on for PG_INODE_REF
In reply to Weston Andros AdamsonFix 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.
--- fs/nfs/write.c | 2 ++ include/linux/nfs_page.h | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
fs/nfs/write.c +2 -0
@@ -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
include/linux/nfs_page.h +1 -1
@@ -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 */
Signature · 1 line
1.8.5.2 (Apple Git-48)
PATCH 3/5 pnfs: find swapped pages on pnfs commit lists too
In reply to Weston Andros Adamsonnfs_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.
--- fs/nfs/filelayout/filelayout.c | 31 ++++++++++++++++++++++++++ fs/nfs/pnfs.h | 20 +++++++++++++++++ fs/nfs/write.c | 49 +++++++++++++++++++++++++++++++----------- 3 files changed, 88 insertions(+), 12 deletions(-)
fs/nfs/filelayout/filelayout.c +31 -0
@@ -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,
fs/nfs/pnfs.h +20 -0
@@ -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 ;
fs/nfs/write.c +37 -12
70 lines not on this page · raw message
Signature · 1 line
1.8.5.2 (Apple Git-48)
Re: [PATCH 3/5] pnfs: find swapped pages on pnfs commit lists too
Quoted 131 lines · Weston Andros Adamson
nfs_page_find_head_request_lockedlooks 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 <[email protected]>Signed-off-by Weston Andros Adamson <[email protected]>Signed-off-by Trond Myklebust <[email protected]>--- fs/nfs/filelayout/filelayout.c | 31 ++++++++++++++++++++++++++ fs/nfs/pnfs.h | 20 +++++++++++++++++ fs/nfs/write.c | 49 +++++++++++++++++++++++++++++++----------- 3 files changed, 88 insertions(+), 12 deletions(-)fs/nfs/filelayout/filelayout.c +31
-0@@ -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,fs/nfs/pnfs.h +20
-0@@ -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 ;fs/nfs/write.c +8
-0@@ -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.
fs/nfs/write.c static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error) · line 113
Quoted 10 lines
fs/nfs/write.c +10
-0+*/ +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
Quoted 44 lines
fs/nfs/write.c +19
-12+ +/* 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
Thanks Anna, I’ll fix it up.
-dros
Quoted 197 lines · Anna Schumaker
Quoted 131 lines · Weston Andros Adamson
nfs_page_find_head_request_lockedlooks 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 <[email protected]>Signed-off-by Weston Andros Adamson <[email protected]>Signed-off-by Trond Myklebust <[email protected]>---
fs/nfs/filelayout/filelayout.c | 31 ++++++++++++++++++++++++++ fs/nfs/pnfs.h | 20 +++++++++++++++++ fs/nfs/write.c | 49 +++++++++++++++++++++++++++++++----------- 3 files changed, 88 insertions(+), 12 deletions(-)fs/nfs/filelayout/filelayout.c +31
-0@@ -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,fs/nfs/pnfs.h +13
-0@@ -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)fs/nfs/pnfs.h +7
-0@@ -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 ;fs/nfs/write.c +8
-0@@ -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. fs/nfs/write.c static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error) · line 113
Quoted 10 lines
fs/nfs/write.c +10
-0+*/ +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
structnfs_commit_infocinfo;Probably because
nfs_init_cinfo_from_inode()is empty without v3 or v4.Anna
Quoted 43 lines
fs/nfs/write.c +19
-12+ +/* 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
For some reason I can’t reproduce this with:
CONFIG_NFS_FS=mCONFIG_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
Quoted 206 lines · Weston Andros Adamson
Thanks Anna, I’ll fix it up.
-dros
Quoted 197 lines · Anna Schumaker
Quoted 131 lines · Weston Andros Adamson
nfs_page_find_head_request_lockedlooks 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 <[email protected]>Signed-off-by Weston Andros Adamson <[email protected]>Signed-off-by Trond Myklebust <[email protected]>---
fs/nfs/filelayout/filelayout.c | 31 ++++++++++++++++++++++++++ fs/nfs/pnfs.h | 20 +++++++++++++++++ fs/nfs/write.c | 49 +++++++++++++++++++++++++++++++----------- 3 files changed, 88 insertions(+), 12 deletions(-)fs/nfs/filelayout/filelayout.c +31
-0@@ -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,fs/nfs/pnfs.h +13
-0@@ -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)fs/nfs/pnfs.h +7
-0@@ -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 ;fs/nfs/write.c +8
-0@@ -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. fs/nfs/write.c static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error) · line 113
Quoted 10 lines
fs/nfs/write.c +10
-0+*/ +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 structnfs_commit_infocinfo;Probably because
nfs_init_cinfo_from_inode()is empty without v3 or v4.Anna
Quoted 43 lines
fs/nfs/write.c +19
-12+ +/* 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
Quoted 7 lines · Weston Andros Adamson
For some reason I can�t reproduce this with:
CONFIG_NFS_FS=mCONFIG_NFS_V2=m
#CONFIG_NFS_V3is not set
#CONFIG_NFS_V4is not set
Quoted 5 lines
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_inodewithout 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=mCONFIG_NFS_V2=m
# CONFIG_NFS_V3 is not set
# CONFIG_NFS_V4 is not setCONFIG_NFS_SWAP=yCONFIG_NFS_FSCACHE=yCONFIG_NFS_ACL_SUPPORT=mCONFIG_NFS_COMMON=yCONFIG_SUNRPC_GSS=mCONFIG_SUNRPC_SWAP=y
# CONFIG_SUNRPC_DEBUG is not set
Anna
Quoted 212 lines
-dros
Quoted 205 lines · Weston Andros Adamson
Thanks Anna, I�ll fix it up.
-dros
Quoted 197 lines · Anna Schumaker
Quoted 131 lines · Weston Andros Adamson
nfs_page_find_head_request_lockedlooks 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 <[email protected]>Signed-off-by Weston Andros Adamson <[email protected]>Signed-off-by Trond Myklebust <[email protected]>---
fs/nfs/filelayout/filelayout.c | 31 ++++++++++++++++++++++++++ fs/nfs/pnfs.h | 20 +++++++++++++++++ fs/nfs/write.c | 49 +++++++++++++++++++++++++++++++----------- 3 files changed, 88 insertions(+), 12 deletions(-)fs/nfs/filelayout/filelayout.c +31
-0@@ -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,fs/nfs/pnfs.h +13
-0@@ -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)fs/nfs/pnfs.h +7
-0@@ -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 ;fs/nfs/write.c +8
-0@@ -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. fs/nfs/write.c static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error) · line 113
Quoted 10 lines
fs/nfs/write.c +10
-0+*/ +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 structnfs_commit_infocinfo;Probably because
nfs_init_cinfo_from_inode()is empty without v3 or v4.Anna
Quoted 43 lines
fs/nfs/write.c +19
-12+ +/* 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); }
PATCH 4/5 pnfs: add pnfs_put_lseg_async
In reply to Weston Andros AdamsonThis is useful when lsegs need to be released while holding locks.
--- fs/nfs/pnfs.c | 17 +++++++++++++++++ fs/nfs/pnfs.h | 7 +++++++ 2 files changed, 24 insertions(+)
fs/nfs/pnfs.c +17 -0
@@ -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) {
fs/nfs/pnfs.h +7 -0
@@ -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 ;
Signature · 1 line
1.8.5.2 (Apple Git-48)
PATCH 5/5 nfs: clear_request_commit while holding i_lock
In reply to Weston Andros Adamson--- fs/nfs/filelayout/filelayout.c | 5 ++--- fs/nfs/write.c | 15 ++++----------- 2 files changed, 6 insertions(+), 14 deletions(-)
fs/nfs/filelayout/filelayout.c +2 -3
@@ -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
fs/nfs/write.c +4 -11
@@ -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);
Signature · 1 line
1.8.5.2 (Apple Git-48)