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 <[email protected]>
Fixes: a7d42ddb30997 ("nfs: add mirroring support to pgio layer")
Cc: [email protected] # v4.0+
---
fs/nfs/pagelist.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index e54d899c1848..40b90f187eeb 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,9 @@ 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:
+ nfs_pageio_cleanup_request(desc, subreq);
+ return 0;
}
static int nfs_do_recoalesce(struct nfs_pageio_descriptor *desc)
@@ -1168,11 +1180,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
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 <[email protected]>
---
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 33824a0a57bf..aa24a2e10dd7 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;
@@ -821,7 +821,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 40b90f187eeb..9e512c0ab544 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -996,7 +996,7 @@ nfs_pageio_cleanup_request(struct nfs_pageio_descriptor *desc,
nfs_list_remove_request(req);
nfs_list_add_request(req, &head);
- desc->pg_completion_ops->error_cleanup(&head);
+ desc->pg_completion_ops->error_cleanup(&head, desc->pg_error);
}
/**
@@ -1132,7 +1132,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 f12cb31a41e5..480ccad94b9a 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1411,20 +1411,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 c39a29ab0dfd..9b8324ec08f3 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1584,7 +1584,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
Hi Anna,
On Wed, 2019-02-13 at 16:53 -0500, Trond Myklebust wrote:
> 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 <[email protected]>
> Fixes: a7d42ddb30997 ("nfs: add mirroring support to pgio layer")
> Cc: [email protected] # v4.0+
I believe this patch should fix the page lock leak that I mentioned
yesterday. The issue is that we're creating nfs_page structures as part
of a page group, but then abandoning those structures to their fate. It
means that the calls to nfs_page_group_sync_on_bit(req, PG_UNLOCKPAGE),
nfs_page_group_sync_on_bit(req, PG_WB_END), and even
nfs_page_group_sync_on_bit(req, PG_TEARDOWN) can never succeed, and so
we end up leaking, page locks, page writeback locks, and nfs_page
requests...
Cheers
Trond
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
Hi,
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag,
fixing commit: a7d42ddb3099 nfs: add mirroring support to pgio layer.
The bot has tested the following trees: v4.20.10, v4.19.23, v4.14.101, v4.9.158, v4.4.174.
v4.20.10: Build OK!
v4.19.23: Build OK!
v4.14.101: Build OK!
v4.9.158: Build OK!
v4.4.174: Failed to apply! Possible dependencies:
c18b96a1b862 ("nfs: clean up rest of reqs when failing to add one")
d600ad1f2bdb ("NFS41: pop some layoutget errors to application")
How should we proceed with this patch?
--
Thanks,
Sasha