* [PATCH v3 0/8] NFS file I/O bugfixes and cleanups @ 2019-02-19 13:53 Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 1/8] NFS: Fix I/O request leakages Trond Myklebust 0 siblings, 1 reply; 9+ messages in thread From: Trond Myklebust @ 2019-02-19 13:53 UTC (permalink / raw) To: linux-nfs The main focus of this patchset is to address a set of I/O request leakages that crept in during the NFSv4.0 file I/O changes. The first 3 patches in the set are intended as stable patches in order to address these issues. In addition, there are a couple of clean ups, and some lesser bugfixes. Trond Myklebust (8): NFS: Fix I/O request leakages NFS: Fix an I/O request leakage in nfs_do_recoalesce NFS: Don't recoalesce on error in nfs_pageio_complete_mirror() NFS: Clean up list moves of struct nfs_page NFS: Pass error information to the pgio error cleanup routine NFS: Ensure NFS writeback allocations don't recurse back into NFS. NFS: EINTR is also a fatal error. NFS: ENOMEM should also be a fatal error. fs/nfs/direct.c | 7 +++---- fs/nfs/internal.h | 2 ++ fs/nfs/pagelist.c | 40 ++++++++++++++++++++++++++-------------- fs/nfs/read.c | 2 +- fs/nfs/write.c | 18 +++++++++++++++--- include/linux/nfs_page.h | 10 ++++++++++ include/linux/nfs_xdr.h | 2 +- 7 files changed, 58 insertions(+), 23 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/8] NFS: Fix I/O request leakages 2019-02-19 13:53 [PATCH v3 0/8] NFS file I/O bugfixes and cleanups Trond Myklebust @ 2019-02-19 13:53 ` Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 2/8] NFS: Fix an I/O request leakage in nfs_do_recoalesce Trond Myklebust 0 siblings, 1 reply; 9+ messages in thread From: Trond Myklebust @ 2019-02-19 13:53 UTC (permalink / raw) To: linux-nfs When we fail to add the request to the I/O queue, we currently leave it to the caller to free the failed request. However since some of the requests that fail are actually created by nfs_pageio_add_request() itself, and are not passed back the caller, this leads to a leakage issue, which can again cause page locks to leak. This commit addresses the leakage by freeing the created requests on error, using desc->pg_completion_ops->error_cleanup() Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> Fixes: a7d42ddb30997 ("nfs: add mirroring support to pgio layer") Cc: stable@vger.kernel.org # v4.0: c18b96a1b862: nfs: clean up rest of reqs Cc: stable@vger.kernel.org # v4.0: d600ad1f2bdb: NFS41: pop some layoutget Cc: stable@vger.kernel.org # v4.0+ --- fs/nfs/pagelist.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c index e54d899c1848..301880a3ad8e 100644 --- a/fs/nfs/pagelist.c +++ b/fs/nfs/pagelist.c @@ -988,6 +988,17 @@ static void nfs_pageio_doio(struct nfs_pageio_descriptor *desc) } } +static void +nfs_pageio_cleanup_request(struct nfs_pageio_descriptor *desc, + struct nfs_page *req) +{ + LIST_HEAD(head); + + nfs_list_remove_request(req); + nfs_list_add_request(req, &head); + desc->pg_completion_ops->error_cleanup(&head); +} + /** * nfs_pageio_add_request - Attempt to coalesce a request into a page list. * @desc: destination io descriptor @@ -1025,10 +1036,8 @@ static int __nfs_pageio_add_request(struct nfs_pageio_descriptor *desc, nfs_page_group_unlock(req); desc->pg_moreio = 1; nfs_pageio_doio(desc); - if (desc->pg_error < 0) - return 0; - if (mirror->pg_recoalesce) - return 0; + if (desc->pg_error < 0 || mirror->pg_recoalesce) + goto out_cleanup_subreq; /* retry add_request for this subreq */ nfs_page_group_lock(req); continue; @@ -1061,6 +1070,10 @@ static int __nfs_pageio_add_request(struct nfs_pageio_descriptor *desc, desc->pg_error = PTR_ERR(subreq); nfs_page_group_unlock(req); return 0; +out_cleanup_subreq: + if (req != subreq) + nfs_pageio_cleanup_request(desc, subreq); + return 0; } static int nfs_do_recoalesce(struct nfs_pageio_descriptor *desc) @@ -1168,11 +1181,14 @@ int nfs_pageio_add_request(struct nfs_pageio_descriptor *desc, if (nfs_pgio_has_mirroring(desc)) desc->pg_mirror_idx = midx; if (!nfs_pageio_add_request_mirror(desc, dupreq)) - goto out_failed; + goto out_cleanup_subreq; } return 1; +out_cleanup_subreq: + if (req != dupreq) + nfs_pageio_cleanup_request(desc, dupreq); out_failed: nfs_pageio_error_cleanup(desc); return 0; -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 2/8] NFS: Fix an I/O request leakage in nfs_do_recoalesce 2019-02-19 13:53 ` [PATCH v3 1/8] NFS: Fix I/O request leakages Trond Myklebust @ 2019-02-19 13:53 ` Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 3/8] NFS: Don't recoalesce on error in nfs_pageio_complete_mirror() Trond Myklebust 0 siblings, 1 reply; 9+ messages in thread From: Trond Myklebust @ 2019-02-19 13:53 UTC (permalink / raw) To: linux-nfs Whether we need to exit early, or just reprocess the list, we must not lost track of the request which failed to get recoalesced. Fixes: 03d5eb65b538 ("NFS: Fix a memory leak in nfs_do_recoalesce") Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> Cc: stable@vger.kernel.org # v4.0+ --- fs/nfs/pagelist.c | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c index 301880a3ad8e..03bde9a41451 100644 --- a/fs/nfs/pagelist.c +++ b/fs/nfs/pagelist.c @@ -1092,7 +1092,6 @@ static int nfs_do_recoalesce(struct nfs_pageio_descriptor *desc) struct nfs_page *req; req = list_first_entry(&head, struct nfs_page, wb_list); - nfs_list_remove_request(req); if (__nfs_pageio_add_request(desc, req)) continue; if (desc->pg_error < 0) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 3/8] NFS: Don't recoalesce on error in nfs_pageio_complete_mirror() 2019-02-19 13:53 ` [PATCH v3 2/8] NFS: Fix an I/O request leakage in nfs_do_recoalesce Trond Myklebust @ 2019-02-19 13:53 ` Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 4/8] NFS: Clean up list moves of struct nfs_page Trond Myklebust 0 siblings, 1 reply; 9+ messages in thread From: Trond Myklebust @ 2019-02-19 13:53 UTC (permalink / raw) To: linux-nfs If the I/O completion failed with a fatal error, then we should just exit nfs_pageio_complete_mirror() rather than try to recoalesce. Fixes: a7d42ddb3099 ("nfs: add mirroring support to pgio layer") Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> Cc: stable@vger.kernel.org # v4.0+ --- fs/nfs/pagelist.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c index 03bde9a41451..a8951f1f7b4e 100644 --- a/fs/nfs/pagelist.c +++ b/fs/nfs/pagelist.c @@ -1209,7 +1209,7 @@ static void nfs_pageio_complete_mirror(struct nfs_pageio_descriptor *desc, desc->pg_mirror_idx = mirror_idx; for (;;) { nfs_pageio_doio(desc); - if (!mirror->pg_recoalesce) + if (desc->pg_error < 0 || !mirror->pg_recoalesce) break; if (!nfs_do_recoalesce(desc)) break; -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 4/8] NFS: Clean up list moves of struct nfs_page 2019-02-19 13:53 ` [PATCH v3 3/8] NFS: Don't recoalesce on error in nfs_pageio_complete_mirror() Trond Myklebust @ 2019-02-19 13:53 ` Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 5/8] NFS: Pass error information to the pgio error cleanup routine Trond Myklebust 0 siblings, 1 reply; 9+ messages in thread From: Trond Myklebust @ 2019-02-19 13:53 UTC (permalink / raw) To: linux-nfs In several places we're just moving the struct nfs_page from one list to another by first removing from the existing list, then adding to the new one. Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> --- fs/nfs/direct.c | 3 +-- fs/nfs/pagelist.c | 12 ++++-------- include/linux/nfs_page.h | 10 ++++++++++ 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c index 33824a0a57bf..1377ee20ecf9 100644 --- a/fs/nfs/direct.c +++ b/fs/nfs/direct.c @@ -664,8 +664,7 @@ static void nfs_direct_write_reschedule(struct nfs_direct_req *dreq) list_for_each_entry_safe(req, tmp, &reqs, wb_list) { if (!nfs_pageio_add_request(&desc, req)) { - nfs_list_remove_request(req); - nfs_list_add_request(req, &failed); + nfs_list_move_request(req, &failed); spin_lock(&cinfo.inode->i_lock); dreq->flags = 0; if (desc.pg_error < 0) diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c index a8951f1f7b4e..9cbfdb979992 100644 --- a/fs/nfs/pagelist.c +++ b/fs/nfs/pagelist.c @@ -768,8 +768,7 @@ int nfs_generic_pgio(struct nfs_pageio_descriptor *desc, pageused = 0; while (!list_empty(head)) { req = nfs_list_entry(head->next); - nfs_list_remove_request(req); - nfs_list_add_request(req, &hdr->pages); + nfs_list_move_request(req, &hdr->pages); if (!last_page || last_page != req->wb_page) { pageused++; @@ -961,8 +960,7 @@ static int nfs_pageio_do_add_request(struct nfs_pageio_descriptor *desc, } if (!nfs_can_coalesce_requests(prev, req, desc)) return 0; - nfs_list_remove_request(req); - nfs_list_add_request(req, &mirror->pg_list); + nfs_list_move_request(req, &mirror->pg_list); mirror->pg_count += req->wb_bytes; return 1; } @@ -994,8 +992,7 @@ nfs_pageio_cleanup_request(struct nfs_pageio_descriptor *desc, { LIST_HEAD(head); - nfs_list_remove_request(req); - nfs_list_add_request(req, &head); + nfs_list_move_request(req, &head); desc->pg_completion_ops->error_cleanup(&head); } @@ -1237,9 +1234,8 @@ int nfs_pageio_resend(struct nfs_pageio_descriptor *desc, while (!list_empty(&hdr->pages)) { struct nfs_page *req = nfs_list_entry(hdr->pages.next); - nfs_list_remove_request(req); if (!nfs_pageio_add_request(desc, req)) - nfs_list_add_request(req, &failed); + nfs_list_move_request(req, &failed); } nfs_pageio_complete(desc); if (!list_empty(&failed)) { diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h index e27572d30d97..ad69430fd0eb 100644 --- a/include/linux/nfs_page.h +++ b/include/linux/nfs_page.h @@ -164,6 +164,16 @@ nfs_list_add_request(struct nfs_page *req, struct list_head *head) list_add_tail(&req->wb_list, head); } +/** + * nfs_list_move_request - Move a request to a new list + * @req: request + * @head: head of list into which to insert the request. + */ +static inline void +nfs_list_move_request(struct nfs_page *req, struct list_head *head) +{ + list_move_tail(&req->wb_list, head); +} /** * nfs_list_remove_request - Remove a request from its wb_list -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 5/8] NFS: Pass error information to the pgio error cleanup routine 2019-02-19 13:53 ` [PATCH v3 4/8] NFS: Clean up list moves of struct nfs_page Trond Myklebust @ 2019-02-19 13:53 ` Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 6/8] NFS: Ensure NFS writeback allocations don't recurse back into NFS Trond Myklebust 0 siblings, 1 reply; 9+ messages in thread From: Trond Myklebust @ 2019-02-19 13:53 UTC (permalink / raw) To: linux-nfs Allow the caller to pass error information when cleaning up a failed I/O request so that we can conditionally take action to cancel the request altogether if the error turned out to be fatal. Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> --- fs/nfs/direct.c | 4 ++-- fs/nfs/pagelist.c | 5 +++-- fs/nfs/read.c | 2 +- fs/nfs/write.c | 11 +++++++++-- include/linux/nfs_xdr.h | 2 +- 5 files changed, 16 insertions(+), 8 deletions(-) diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c index 1377ee20ecf9..0fd811ac08b5 100644 --- a/fs/nfs/direct.c +++ b/fs/nfs/direct.c @@ -428,7 +428,7 @@ static void nfs_direct_read_completion(struct nfs_pgio_header *hdr) hdr->release(hdr); } -static void nfs_read_sync_pgio_error(struct list_head *head) +static void nfs_read_sync_pgio_error(struct list_head *head, int error) { struct nfs_page *req; @@ -820,7 +820,7 @@ static void nfs_direct_write_completion(struct nfs_pgio_header *hdr) hdr->release(hdr); } -static void nfs_write_sync_pgio_error(struct list_head *head) +static void nfs_write_sync_pgio_error(struct list_head *head, int error) { struct nfs_page *req; diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c index 9cbfdb979992..695afb7de3a7 100644 --- a/fs/nfs/pagelist.c +++ b/fs/nfs/pagelist.c @@ -993,7 +993,7 @@ nfs_pageio_cleanup_request(struct nfs_pageio_descriptor *desc, LIST_HEAD(head); nfs_list_move_request(req, &head); - desc->pg_completion_ops->error_cleanup(&head); + desc->pg_completion_ops->error_cleanup(&head, desc->pg_error); } /** @@ -1129,7 +1129,8 @@ static void nfs_pageio_error_cleanup(struct nfs_pageio_descriptor *desc) for (midx = 0; midx < desc->pg_mirror_count; midx++) { mirror = &desc->pg_mirrors[midx]; - desc->pg_completion_ops->error_cleanup(&mirror->pg_list); + desc->pg_completion_ops->error_cleanup(&mirror->pg_list, + desc->pg_error); } } diff --git a/fs/nfs/read.c b/fs/nfs/read.c index f9f19784db82..1d95a60b2586 100644 --- a/fs/nfs/read.c +++ b/fs/nfs/read.c @@ -205,7 +205,7 @@ static void nfs_initiate_read(struct nfs_pgio_header *hdr, } static void -nfs_async_read_error(struct list_head *head) +nfs_async_read_error(struct list_head *head, int error) { struct nfs_page *req; diff --git a/fs/nfs/write.c b/fs/nfs/write.c index d09c9f878141..11df9f03245f 100644 --- a/fs/nfs/write.c +++ b/fs/nfs/write.c @@ -1412,20 +1412,27 @@ static void nfs_redirty_request(struct nfs_page *req) nfs_release_request(req); } -static void nfs_async_write_error(struct list_head *head) +static void nfs_async_write_error(struct list_head *head, int error) { struct nfs_page *req; while (!list_empty(head)) { req = nfs_list_entry(head->next); nfs_list_remove_request(req); + if (nfs_error_is_fatal(error)) { + nfs_context_set_write_error(req->wb_context, error); + if (nfs_error_is_fatal_on_server(error)) { + nfs_write_error_remove_page(req); + continue; + } + } nfs_redirty_request(req); } } static void nfs_async_write_reschedule_io(struct nfs_pgio_header *hdr) { - nfs_async_write_error(&hdr->pages); + nfs_async_write_error(&hdr->pages, 0); filemap_fdatawrite_range(hdr->inode->i_mapping, hdr->args.offset, hdr->args.offset + hdr->args.count - 1); } diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h index 441a93ebcac0..b4bd2bf5f585 100644 --- a/include/linux/nfs_xdr.h +++ b/include/linux/nfs_xdr.h @@ -1549,7 +1549,7 @@ struct nfs_commit_data { }; struct nfs_pgio_completion_ops { - void (*error_cleanup)(struct list_head *head); + void (*error_cleanup)(struct list_head *head, int); void (*init_hdr)(struct nfs_pgio_header *hdr); void (*completion)(struct nfs_pgio_header *hdr); void (*reschedule_io)(struct nfs_pgio_header *hdr); -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 6/8] NFS: Ensure NFS writeback allocations don't recurse back into NFS. 2019-02-19 13:53 ` [PATCH v3 5/8] NFS: Pass error information to the pgio error cleanup routine Trond Myklebust @ 2019-02-19 13:53 ` Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 7/8] NFS: EINTR is also a fatal error Trond Myklebust 0 siblings, 1 reply; 9+ messages in thread From: Trond Myklebust @ 2019-02-19 13:53 UTC (permalink / raw) To: linux-nfs All the allocations that we can hit in the NFS layer and sunrpc layers themselves are already marked as GFP_NOFS, but we need to ensure that any calls to generic kernel functionality do the right thing as well. Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> --- fs/nfs/write.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/fs/nfs/write.c b/fs/nfs/write.c index 11df9f03245f..d1bc0384ac95 100644 --- a/fs/nfs/write.c +++ b/fs/nfs/write.c @@ -26,6 +26,7 @@ #include <linux/iversion.h> #include <linux/uaccess.h> +#include <linux/sched/mm.h> #include "delegation.h" #include "internal.h" @@ -712,11 +713,13 @@ int nfs_writepages(struct address_space *mapping, struct writeback_control *wbc) { struct inode *inode = mapping->host; struct nfs_pageio_descriptor pgio; - struct nfs_io_completion *ioc = nfs_io_completion_alloc(GFP_NOFS); + struct nfs_io_completion *ioc; + unsigned int pflags = memalloc_nofs_save(); int err; nfs_inc_stats(inode, NFSIOS_VFSWRITEPAGES); + ioc = nfs_io_completion_alloc(GFP_NOFS); if (ioc) nfs_io_completion_init(ioc, nfs_io_completion_commit, inode); @@ -727,6 +730,8 @@ int nfs_writepages(struct address_space *mapping, struct writeback_control *wbc) nfs_pageio_complete(&pgio); nfs_io_completion_put(ioc); + memalloc_nofs_restore(pflags); + if (err < 0) goto out_err; err = pgio.pg_error; -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 7/8] NFS: EINTR is also a fatal error. 2019-02-19 13:53 ` [PATCH v3 6/8] NFS: Ensure NFS writeback allocations don't recurse back into NFS Trond Myklebust @ 2019-02-19 13:53 ` Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 8/8] NFS: ENOMEM should also be " Trond Myklebust 0 siblings, 1 reply; 9+ messages in thread From: Trond Myklebust @ 2019-02-19 13:53 UTC (permalink / raw) To: linux-nfs Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> --- fs/nfs/internal.h | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h index b1e577302518..1cb9670bb8b5 100644 --- a/fs/nfs/internal.h +++ b/fs/nfs/internal.h @@ -755,6 +755,7 @@ static inline bool nfs_error_is_fatal(int err) { switch (err) { case -ERESTARTSYS: + case -EINTR: case -EACCES: case -EDQUOT: case -EFBIG: -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 8/8] NFS: ENOMEM should also be a fatal error. 2019-02-19 13:53 ` [PATCH v3 7/8] NFS: EINTR is also a fatal error Trond Myklebust @ 2019-02-19 13:53 ` Trond Myklebust 0 siblings, 0 replies; 9+ messages in thread From: Trond Myklebust @ 2019-02-19 13:53 UTC (permalink / raw) To: linux-nfs Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> --- fs/nfs/internal.h | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h index 1cb9670bb8b5..b26622d9686f 100644 --- a/fs/nfs/internal.h +++ b/fs/nfs/internal.h @@ -764,6 +764,7 @@ static inline bool nfs_error_is_fatal(int err) case -EROFS: case -ESTALE: case -E2BIG: + case -ENOMEM: return true; default: return false; -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-02-19 13:55 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-19 13:53 [PATCH v3 0/8] NFS file I/O bugfixes and cleanups Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 1/8] NFS: Fix I/O request leakages Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 2/8] NFS: Fix an I/O request leakage in nfs_do_recoalesce Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 3/8] NFS: Don't recoalesce on error in nfs_pageio_complete_mirror() Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 4/8] NFS: Clean up list moves of struct nfs_page Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 5/8] NFS: Pass error information to the pgio error cleanup routine Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 6/8] NFS: Ensure NFS writeback allocations don't recurse back into NFS Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 7/8] NFS: EINTR is also a fatal error Trond Myklebust 2019-02-19 13:53 ` [PATCH v3 8/8] NFS: ENOMEM should also be " Trond Myklebust
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).