From: Fred <[email protected]>
Both flush functions have the same error handling routine. Pull
it out as a function.
Signed-off-by: Fred Isaman <[email protected]>
---
fs/nfs/write.c | 19 +++++++++++++------
1 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 80c61fd..01bf68e 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -845,6 +845,17 @@ static void nfs_write_rpcsetup(struct nfs_page *req,
rpc_put_task(task);
}
+/* If a nfs_flush_* function fails, it should remove reqs from @head and
+ * call this on each, which will prepare them to be retried on next
+ * writeback using standard nfs.
+ */
+static void nfs_retry_request(struct nfs_page *req)
+{
+ nfs_redirty_request(req);
+ nfs_end_page_writeback(req->wb_page);
+ nfs_clear_page_tag_locked(req);
+}
+
/*
* Generate multiple small requests to write out a single
* contiguous dirty area on one page.
@@ -899,9 +910,7 @@ out_bad:
list_del(&data->pages);
nfs_writedata_release(data);
}
- nfs_redirty_request(req);
- nfs_end_page_writeback(req->wb_page);
- nfs_clear_page_tag_locked(req);
+ nfs_retry_request(req);
return -ENOMEM;
}
@@ -941,9 +950,7 @@ static int nfs_flush_one(struct inode *inode, struct list_head *head, unsigned i
while (!list_empty(head)) {
req = nfs_list_entry(head->next);
nfs_list_remove_request(req);
- nfs_redirty_request(req);
- nfs_end_page_writeback(req->wb_page);
- nfs_clear_page_tag_locked(req);
+ nfs_retry_request(req);
}
return -ENOMEM;
}
--
1.5.3.3
On Wed, 2008-03-19 at 10:13 -0400, Fred Isaman wrote:
> Ignoring the return value from nfs_pageio_add_request can cause deadlocks.
>
> In read path:
> call nfs_pageio_add_request from readpage_async_filler
> assume at this point that there are requests already in desc, that
> can't be merged with the current request.
> so nfs_pageio_doio is fired up to clear out desc.
> assume something goes wrong in setting up the io, so desc->pg_error is set.
> This causes nfs_pageio_add_request to return 0, *WITHOUT* adding the original
> request.
> BUT, since return code is ignored, readpage_async_filler assumes it has
> been added, and does nothing further, leaving page locked.
> do_generic_mapping_read will eventually call lock_page, resulting in deadlock
>
> In write path:
> page is marked dirty by generic_perform_write
> nfs_writepages is called
> call nfs_pageio_add_request from nfs_page_async_flush
> assume at this point that there are requests already in desc, that
> can't be merged with the current request.
> so nfs_pageio_doio is fired up to clear out desc.
> assume something goes wrong in setting up the io, so desc->pg_error is set.
> This causes nfs_page_async_flush to return 0, *WITHOUT* adding the original
> request, yet marking the request as locked (PG_BUSY) and in writeback,
> clearing dirty marks.
> The next time a write is done to the page, deadlock will result as
> nfs_write_end calls nfs_update_request
>
> Signed-off-by: Fred Isaman <[email protected]>
Applied. Thanks Fred!
On Wed, 2008-03-19 at 10:13 -0400, Fred Isaman wrote:
> Ignoring the return value from nfs_pageio_add_request can cause deadlocks.
>
> In read path:
> call nfs_pageio_add_request from readpage_async_filler
> assume at this point that there are requests already in desc, that
> can't be merged with the current request.
> so nfs_pageio_doio is fired up to clear out desc.
> assume something goes wrong in setting up the io, so desc->pg_error is set.
> This causes nfs_pageio_add_request to return 0, *WITHOUT* adding the original
> request.
> BUT, since return code is ignored, readpage_async_filler assumes it has
> been added, and does nothing further, leaving page locked.
> do_generic_mapping_read will eventually call lock_page, resulting in deadlock
BTW: the same bug may occur in nfs_readpage_async(). I'll add a fixup
for that to your patch.
Cheers
Trond
On Wed, 2008-03-19 at 10:13 -0400, Fred Isaman wrote:
> From: Fred <[email protected]>
>
> Both flush functions have the same error handling routine. Pull
> it out as a function.
>
> Signed-off-by: Fred Isaman <[email protected]>
> ---
> fs/nfs/write.c | 19 +++++++++++++------
> 1 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 80c61fd..01bf68e 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -845,6 +845,17 @@ static void nfs_write_rpcsetup(struct nfs_page *req,
> rpc_put_task(task);
> }
>
> +/* If a nfs_flush_* function fails, it should remove reqs from @head and
> + * call this on each, which will prepare them to be retried on next
> + * writeback using standard nfs.
> + */
> +static void nfs_retry_request(struct nfs_page *req)
> +{
> + nfs_redirty_request(req);
> + nfs_end_page_writeback(req->wb_page);
> + nfs_clear_page_tag_locked(req);
> +}
> +
> /*
> * Generate multiple small requests to write out a single
> * contiguous dirty area on one page.
> @@ -899,9 +910,7 @@ out_bad:
> list_del(&data->pages);
> nfs_writedata_release(data);
> }
> - nfs_redirty_request(req);
> - nfs_end_page_writeback(req->wb_page);
> - nfs_clear_page_tag_locked(req);
> + nfs_retry_request(req);
> return -ENOMEM;
> }
>
> @@ -941,9 +950,7 @@ static int nfs_flush_one(struct inode *inode, struct list_head *head, unsigned i
> while (!list_empty(head)) {
> req = nfs_list_entry(head->next);
> nfs_list_remove_request(req);
> - nfs_redirty_request(req);
> - nfs_end_page_writeback(req->wb_page);
> - nfs_clear_page_tag_locked(req);
> + nfs_retry_request(req);
> }
> return -ENOMEM;
> }
My one nit is with the name of your function: it isn't really retrying
the flush, but rather requeuing it on the dirty list after an attempt to
flush failed.
How about renaming the current 'nfs_redirty_request()' as
'nfs_mark_request_dirty()', and then using the name
nfs_redirty_request() for your new helper?
Cheers
Trond
Ignoring the return value from nfs_pageio_add_request can cause deadlocks.
In read path:
call nfs_pageio_add_request from readpage_async_filler
assume at this point that there are requests already in desc, that
can't be merged with the current request.
so nfs_pageio_doio is fired up to clear out desc.
assume something goes wrong in setting up the io, so desc->pg_error is set.
This causes nfs_pageio_add_request to return 0, *WITHOUT* adding the original
request.
BUT, since return code is ignored, readpage_async_filler assumes it has
been added, and does nothing further, leaving page locked.
do_generic_mapping_read will eventually call lock_page, resulting in deadlock
In write path:
page is marked dirty by generic_perform_write
nfs_writepages is called
call nfs_pageio_add_request from nfs_page_async_flush
assume at this point that there are requests already in desc, that
can't be merged with the current request.
so nfs_pageio_doio is fired up to clear out desc.
assume something goes wrong in setting up the io, so desc->pg_error is set.
This causes nfs_page_async_flush to return 0, *WITHOUT* adding the original
request, yet marking the request as locked (PG_BUSY) and in writeback,
clearing dirty marks.
The next time a write is done to the page, deadlock will result as
nfs_write_end calls nfs_update_request
Signed-off-by: Fred Isaman <[email protected]>
---
fs/nfs/read.c | 5 ++++-
fs/nfs/write.c | 5 ++++-
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 3d7d963..77f8ac9 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -533,7 +533,10 @@ readpage_async_filler(void *data, struct page *page)
if (len < PAGE_CACHE_SIZE)
zero_user_segment(page, len, PAGE_CACHE_SIZE);
- nfs_pageio_add_request(desc->pgio, new);
+ if (!nfs_pageio_add_request(desc->pgio, new)) {
+ error = desc->pagio->pg_error;
+ goto out_unlock;
+ }
return 0;
out_error:
error = PTR_ERR(new);
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 01bf68e..c8bb767 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -288,7 +288,10 @@ static int nfs_page_async_flush(struct nfs_pageio_descriptor *pgio,
BUG();
}
spin_unlock(&inode->i_lock);
- nfs_pageio_add_request(pgio, req);
+ if (!nfs_pageio_add_request(pgio, req)) {
+ nfs_retry_request(req);
+ return pgio->pg_error;
+ }
return 0;
}
--
1.5.3.3