Resend of patches, with Trond's suggested renaming. Also fix a compile
problem with the second patch due to forward reference.
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 | 6 +++++-
2 files changed, 9 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 df85b7b..a146089 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -39,6 +39,7 @@ static struct nfs_page * nfs_update_request(struct nfs_open_context*,
unsigned int, unsigned int);
static void nfs_pageio_init_write(struct nfs_pageio_descriptor *desc,
struct inode *inode, int ioflags);
+static void nfs_redirty_request(struct nfs_page *req);
static const struct rpc_call_ops nfs_write_partial_ops;
static const struct rpc_call_ops nfs_write_full_ops;
static const struct rpc_call_ops nfs_commit_ops;
@@ -288,7 +289,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_redirty_request(req);
+ return pgio->pg_error;
+ }
return 0;
}
--
1.5.3.3
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 | 21 ++++++++++++++-------
1 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 80c61fd..df85b7b 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -407,7 +407,7 @@ static void nfs_inode_remove_request(struct nfs_page *req)
}
static void
-nfs_redirty_request(struct nfs_page *req)
+nfs_mark_request_dirty(struct nfs_page *req)
{
__set_page_dirty_nobuffers(req->wb_page);
}
@@ -461,7 +461,7 @@ int nfs_reschedule_unstable_write(struct nfs_page *req)
return 1;
}
if (test_and_clear_bit(PG_NEED_RESCHED, &req->wb_flags)) {
- nfs_redirty_request(req);
+ nfs_mark_request_dirty(req);
return 1;
}
return 0;
@@ -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_redirty_request(struct nfs_page *req)
+{
+ nfs_mark_request_dirty(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.
@@ -900,8 +911,6 @@ out_bad:
nfs_writedata_release(data);
}
nfs_redirty_request(req);
- nfs_end_page_writeback(req->wb_page);
- nfs_clear_page_tag_locked(req);
return -ENOMEM;
}
@@ -942,8 +951,6 @@ static int nfs_flush_one(struct inode *inode, struct list_head *head, unsigned i
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);
}
return -ENOMEM;
}
@@ -1291,7 +1298,7 @@ static void nfs_commit_done(struct rpc_task *task, void *calldata)
}
/* We have a mismatch. Write the page again */
dprintk(" mismatch\n");
- nfs_redirty_request(req);
+ nfs_mark_request_dirty(req);
next:
nfs_clear_page_tag_locked(req);
}
--
1.5.3.3
On Wed, 2008-03-19 at 11:24 -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]>
> ---
> fs/nfs/read.c | 5 ++++-
> fs/nfs/write.c | 6 +++++-
> 2 files changed, 9 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;
^^^^^^^^^^^^^ this has never been
compile-tested.
> + goto out_unlock;
> + }
> return 0;
> out_error:
> error = PTR_ERR(new);
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index df85b7b..a146089 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -39,6 +39,7 @@ static struct nfs_page * nfs_update_request(struct nfs_open_context*,
> unsigned int, unsigned int);
> static void nfs_pageio_init_write(struct nfs_pageio_descriptor *desc,
> struct inode *inode, int ioflags);
> +static void nfs_redirty_request(struct nfs_page *req);
> static const struct rpc_call_ops nfs_write_partial_ops;
> static const struct rpc_call_ops nfs_write_full_ops;
> static const struct rpc_call_ops nfs_commit_ops;
> @@ -288,7 +289,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_redirty_request(req);
Please could you reverse the order of these two patches. I'd like to
send this one in for 2.6.25, but I can't do that as long as it depends
on the previous patch.
> + return pgio->pg_error;
> + }
> return 0;
> }
>
On Wed, 2008-03-19 at 16:44 -0400, Trond Myklebust wrote:
> Please could you reverse the order of these two patches. I'd like to
> send this one in for 2.6.25, but I can't do that as long as it depends
> on the previous patch.
I ended up doing this myself, since I'd like to prepare this for merging
as soon as possible. Please could you ack the following updated patch.
Cheers
Trond
---------------------------------
From: Fred Isaman <[email protected]>
Date: Wed, 19 Mar 2008 11:24:39 -0400
nfs: don't ignore return value from nfs_pageio_add_request
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]>
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/read.c | 5 ++++-
fs/nfs/write.c | 8 +++++++-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index be9e827..ab2f7d2 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -531,7 +531,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->pgio->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 3051302..7a2ba26 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -39,6 +39,7 @@ static struct nfs_page * nfs_update_request(struct nfs_open_context*,
unsigned int, unsigned int);
static void nfs_pageio_init_write(struct nfs_pageio_descriptor *desc,
struct inode *inode, int ioflags);
+static void nfs_redirty_request(struct nfs_page *req);
static const struct rpc_call_ops nfs_write_partial_ops;
static const struct rpc_call_ops nfs_write_full_ops;
static const struct rpc_call_ops nfs_commit_ops;
@@ -279,7 +280,12 @@ 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_redirty_request(req);
+ nfs_end_page_writeback(page);
+ nfs_clear_page_tag_locked(req);
+ return pgio->pg_error;
+ }
return 0;
}
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
ACK.
Fred
On Wed, 19 Mar 2008, Trond Myklebust wrote:
>
> On Wed, 2008-03-19 at 16:44 -0400, Trond Myklebust wrote:
>> Please could you reverse the order of these two patches. I'd like to
>> send this one in for 2.6.25, but I can't do that as long as it depends
>> on the previous patch.
>
> I ended up doing this myself, since I'd like to prepare this for merging
> as soon as possible. Please could you ack the following updated patch.
>
> Cheers
> Trond
>
> ---------------------------------
> From: Fred Isaman <[email protected]>
> Date: Wed, 19 Mar 2008 11:24:39 -0400
> nfs: don't ignore return value from nfs_pageio_add_request
>
> 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]>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
>
> fs/nfs/read.c | 5 ++++-
> fs/nfs/write.c | 8 +++++++-
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/read.c b/fs/nfs/read.c
> index be9e827..ab2f7d2 100644
> --- a/fs/nfs/read.c
> +++ b/fs/nfs/read.c
> @@ -531,7 +531,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->pgio->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 3051302..7a2ba26 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -39,6 +39,7 @@ static struct nfs_page * nfs_update_request(struct nfs_open_context*,
> unsigned int, unsigned int);
> static void nfs_pageio_init_write(struct nfs_pageio_descriptor *desc,
> struct inode *inode, int ioflags);
> +static void nfs_redirty_request(struct nfs_page *req);
> static const struct rpc_call_ops nfs_write_partial_ops;
> static const struct rpc_call_ops nfs_write_full_ops;
> static const struct rpc_call_ops nfs_commit_ops;
> @@ -279,7 +280,12 @@ 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_redirty_request(req);
> + nfs_end_page_writeback(page);
> + nfs_clear_page_tag_locked(req);
> + return pgio->pg_error;
> + }
> return 0;
> }
>
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
>