I've split the issues introduced by the 2.6.21-rcX write code up into 4
subproblems.
The first patch is just a cleanup in order to ease review.
Patch number 2 ensures that we never release the PG_writeback flag until
_after_ we've either discarded the unstable request altogether, or put it
on the nfs_inode's commit or dirty lists.
Patch number 3 fixes the 'desynchronized value of nfs_i.ncommit' error. It
uses the PG_NEED_COMMIT flag as an indicator for whether or not the request
may be redirtied.
Patch number 4 protects the NFS '.set_page_dirty' address_space operation
against races with nfs_inode_add_request.
Finally, patch number 5 fixes an issue with the RPC code that is supposed
ensure that NFSv4 disconnects before resending a request. The current code
will disconnect for every request it resends (and has a bunch of false
positive cases), instead of just ensuring that it disconnects once every
time a timeout or a garbage reply occurs.
My thanks to the various patient victim^Wpeople who helped with extensive
testing.
Cheers
Trond
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
From: Trond Myklebust <[email protected]>
Get rid of the inlined #ifdefs.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/write.c | 117 ++++++++++++++++++++++++++++------------------
include/linux/nfs_page.h | 30 ------------
2 files changed, 71 insertions(+), 76 deletions(-)
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index ad2e91b..3ed4feb 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -460,6 +460,43 @@ nfs_mark_request_commit(struct nfs_page *req)
inc_zone_page_state(req->wb_page, NR_UNSTABLE_NFS);
__mark_inode_dirty(inode, I_DIRTY_DATASYNC);
}
+
+static inline
+int nfs_write_need_commit(struct nfs_write_data *data)
+{
+ return data->verf.committed != NFS_FILE_SYNC;
+}
+
+static inline
+int nfs_reschedule_unstable_write(struct nfs_page *req)
+{
+ if (test_and_clear_bit(PG_NEED_COMMIT, &req->wb_flags)) {
+ nfs_mark_request_commit(req);
+ return 1;
+ }
+ if (test_and_clear_bit(PG_NEED_RESCHED, &req->wb_flags)) {
+ nfs_redirty_request(req);
+ return 1;
+ }
+ return 0;
+}
+#else
+static inline void
+nfs_mark_request_commit(struct nfs_page *req)
+{
+}
+
+static inline
+int nfs_write_need_commit(struct nfs_write_data *data)
+{
+ return 0;
+}
+
+static inline
+int nfs_reschedule_unstable_write(struct nfs_page *req)
+{
+ return 0;
+}
#endif
/*
@@ -746,26 +783,12 @@ int nfs_updatepage(struct file *file, struct page *page,
static void nfs_writepage_release(struct nfs_page *req)
{
- nfs_end_page_writeback(req->wb_page);
-#if defined(CONFIG_NFS_V3) || defined(CONFIG_NFS_V4)
- if (!PageError(req->wb_page)) {
- if (NFS_NEED_RESCHED(req)) {
- nfs_redirty_request(req);
- goto out;
- } else if (NFS_NEED_COMMIT(req)) {
- nfs_mark_request_commit(req);
- goto out;
- }
- }
- nfs_inode_remove_request(req);
-
-out:
- nfs_clear_commit(req);
- nfs_clear_reschedule(req);
-#else
- nfs_inode_remove_request(req);
-#endif
+ if (PageError(req->wb_page) || !nfs_reschedule_unstable_write(req)) {
+ nfs_end_page_writeback(req->wb_page);
+ nfs_inode_remove_request(req);
+ } else
+ nfs_end_page_writeback(req->wb_page);
nfs_clear_page_writeback(req);
}
@@ -1008,22 +1031,28 @@ static void nfs_writeback_done_partial(struct rpc_task *task, void *calldata)
nfs_set_pageerror(page);
req->wb_context->error = task->tk_status;
dprintk(", error = %d\n", task->tk_status);
- } else {
-#if defined(CONFIG_NFS_V3) || defined(CONFIG_NFS_V4)
- if (data->verf.committed < NFS_FILE_SYNC) {
- if (!NFS_NEED_COMMIT(req)) {
- nfs_defer_commit(req);
- memcpy(&req->wb_verf, &data->verf, sizeof(req->wb_verf));
- dprintk(" defer commit\n");
- } else if (memcmp(&req->wb_verf, &data->verf, sizeof(req->wb_verf))) {
- nfs_defer_reschedule(req);
- dprintk(" server reboot detected\n");
- }
- } else
-#endif
- dprintk(" OK\n");
+ goto out;
}
+ if (nfs_write_need_commit(data)) {
+ spinlock_t *req_lock = &NFS_I(page->mapping->host)->req_lock;
+
+ spin_lock(req_lock);
+ if (test_bit(PG_NEED_RESCHED, &req->wb_flags)) {
+ /* Do nothing we need to resend the writes */
+ } else if (!test_and_set_bit(PG_NEED_COMMIT, &req->wb_flags)) {
+ memcpy(&req->wb_verf, &data->verf, sizeof(req->wb_verf));
+ dprintk(" defer commit\n");
+ } else if (memcmp(&req->wb_verf, &data->verf, sizeof(req->wb_verf))) {
+ set_bit(PG_NEED_RESCHED, &req->wb_flags);
+ clear_bit(PG_NEED_COMMIT, &req->wb_flags);
+ dprintk(" server reboot detected\n");
+ }
+ spin_unlock(req_lock);
+ } else
+ dprintk(" OK\n");
+
+out:
if (atomic_dec_and_test(&req->wb_complete))
nfs_writepage_release(req);
}
@@ -1064,25 +1093,21 @@ static void nfs_writeback_done_full(struct rpc_task *task, void *calldata)
if (task->tk_status < 0) {
nfs_set_pageerror(page);
req->wb_context->error = task->tk_status;
- nfs_end_page_writeback(page);
- nfs_inode_remove_request(req);
dprintk(", error = %d\n", task->tk_status);
- goto next;
+ goto remove_request;
}
- nfs_end_page_writeback(page);
-#if defined(CONFIG_NFS_V3) || defined(CONFIG_NFS_V4)
- if (data->args.stable != NFS_UNSTABLE || data->verf.committed == NFS_FILE_SYNC) {
- nfs_inode_remove_request(req);
- dprintk(" OK\n");
+ if (nfs_write_need_commit(data)) {
+ memcpy(&req->wb_verf, &data->verf, sizeof(req->wb_verf));
+ nfs_mark_request_commit(req);
+ nfs_end_page_writeback(page);
+ dprintk(" marked for commit\n");
goto next;
}
- memcpy(&req->wb_verf, &data->verf, sizeof(req->wb_verf));
- nfs_mark_request_commit(req);
- dprintk(" marked for commit\n");
-#else
+ dprintk(" OK\n");
+remove_request:
+ nfs_end_page_writeback(page);
nfs_inode_remove_request(req);
-#endif
next:
nfs_clear_page_writeback(req);
}
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index d111be6..16b0266 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -49,8 +49,6 @@ struct nfs_page {
};
#define NFS_WBACK_BUSY(req) (test_bit(PG_BUSY,&(req)->wb_flags))
-#define NFS_NEED_COMMIT(req) (test_bit(PG_NEED_COMMIT,&(req)->wb_flags))
-#define NFS_NEED_RESCHED(req) (test_bit(PG_NEED_RESCHED,&(req)->wb_flags))
extern struct nfs_page *nfs_create_request(struct nfs_open_context *ctx,
struct inode *inode,
@@ -121,34 +119,6 @@ nfs_list_remove_request(struct nfs_page *req)
req->wb_list_head = NULL;
}
-static inline int
-nfs_defer_commit(struct nfs_page *req)
-{
- return !test_and_set_bit(PG_NEED_COMMIT, &req->wb_flags);
-}
-
-static inline void
-nfs_clear_commit(struct nfs_page *req)
-{
- smp_mb__before_clear_bit();
- clear_bit(PG_NEED_COMMIT, &req->wb_flags);
- smp_mb__after_clear_bit();
-}
-
-static inline int
-nfs_defer_reschedule(struct nfs_page *req)
-{
- return !test_and_set_bit(PG_NEED_RESCHED, &req->wb_flags);
-}
-
-static inline void
-nfs_clear_reschedule(struct nfs_page *req)
-{
- smp_mb__before_clear_bit();
- clear_bit(PG_NEED_RESCHED, &req->wb_flags);
- smp_mb__after_clear_bit();
-}
-
static inline struct nfs_page *
nfs_list_entry(struct list_head *head)
{
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
From: Trond Myklebust <[email protected]>
Ensure that we don't release the PG_writeback lock until after the page has
either been redirtied, or queued on the nfs_inode 'commit' list.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/write.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 3ed4feb..8e94246 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -920,8 +920,8 @@ out_bad:
list_del(&data->pages);
nfs_writedata_release(data);
}
- nfs_end_page_writeback(req->wb_page);
nfs_redirty_request(req);
+ nfs_end_page_writeback(req->wb_page);
nfs_clear_page_writeback(req);
return -ENOMEM;
}
@@ -966,8 +966,8 @@ static int nfs_flush_one(struct inode *inode, struct list_head *head, int how)
while (!list_empty(head)) {
struct nfs_page *req = nfs_list_entry(head->next);
nfs_list_remove_request(req);
- nfs_end_page_writeback(req->wb_page);
nfs_redirty_request(req);
+ nfs_end_page_writeback(req->wb_page);
nfs_clear_page_writeback(req);
}
return -ENOMEM;
@@ -1002,8 +1002,8 @@ out_err:
while (!list_empty(head)) {
req = nfs_list_entry(head->next);
nfs_list_remove_request(req);
- nfs_end_page_writeback(req->wb_page);
nfs_redirty_request(req);
+ nfs_end_page_writeback(req->wb_page);
nfs_clear_page_writeback(req);
}
return error;
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
From: Trond Myklebust <[email protected]>
Redirtying a request that is already marked for commit will screw up the
accounting for NR_UNSTABLE_NFS as well as nfs_i.ncommit.
Ensure that all requests on the commit queue are labelled with the
PG_NEED_COMMIT flag, and avoid moving them onto the dirty list inside
nfs_page_mark_flush().
Also inline nfs_mark_request_dirty() into nfs_page_mark_flush() for
atomicity reasons. Avoid dropping the spinlock until we're done marking the
request in the radix tree and have added it to the ->dirty list.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/write.c | 47 ++++++++++++++++++++++-------------------------
1 files changed, 22 insertions(+), 25 deletions(-)
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 8e94246..ce5b4a9 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -38,7 +38,6 @@
static struct nfs_page * nfs_update_request(struct nfs_open_context*,
struct page *,
unsigned int, unsigned int);
-static void nfs_mark_request_dirty(struct nfs_page *req);
static long nfs_flush_mapping(struct address_space *mapping, struct writeback_control *wbc, int how);
static const struct rpc_call_ops nfs_write_partial_ops;
static const struct rpc_call_ops nfs_write_full_ops;
@@ -255,7 +254,8 @@ static void nfs_end_page_writeback(struct page *page)
static int nfs_page_mark_flush(struct page *page)
{
struct nfs_page *req;
- spinlock_t *req_lock = &NFS_I(page->mapping->host)->req_lock;
+ struct nfs_inode *nfsi = NFS_I(page->mapping->host);
+ spinlock_t *req_lock = &nfsi->req_lock;
int ret;
spin_lock(req_lock);
@@ -279,11 +279,23 @@ static int nfs_page_mark_flush(struct page *page)
return ret;
spin_lock(req_lock);
}
- spin_unlock(req_lock);
+ if (test_bit(PG_NEED_COMMIT, &req->wb_flags)) {
+ /* This request is marked for commit */
+ spin_unlock(req_lock);
+ nfs_unlock_request(req);
+ return 1;
+ }
if (nfs_set_page_writeback(page) == 0) {
nfs_list_remove_request(req);
- nfs_mark_request_dirty(req);
- }
+ /* add the request to the inode's dirty list. */
+ radix_tree_tag_set(&nfsi->nfs_page_tree,
+ req->wb_index, NFS_PAGE_TAG_DIRTY);
+ nfs_list_add_request(req, &nfsi->dirty);
+ nfsi->ndirty++;
+ spin_unlock(req_lock);
+ __mark_inode_dirty(page->mapping->host, I_DIRTY_PAGES);
+ } else
+ spin_unlock(req_lock);
ret = test_bit(PG_NEED_FLUSH, &req->wb_flags);
nfs_unlock_request(req);
return ret;
@@ -406,24 +418,6 @@ static void nfs_inode_remove_request(struct nfs_page *req)
nfs_release_request(req);
}
-/*
- * Add a request to the inode's dirty list.
- */
-static void
-nfs_mark_request_dirty(struct nfs_page *req)
-{
- struct inode *inode = req->wb_context->dentry->d_inode;
- struct nfs_inode *nfsi = NFS_I(inode);
-
- spin_lock(&nfsi->req_lock);
- radix_tree_tag_set(&nfsi->nfs_page_tree,
- req->wb_index, NFS_PAGE_TAG_DIRTY);
- nfs_list_add_request(req, &nfsi->dirty);
- nfsi->ndirty++;
- spin_unlock(&nfsi->req_lock);
- __mark_inode_dirty(inode, I_DIRTY_PAGES);
-}
-
static void
nfs_redirty_request(struct nfs_page *req)
{
@@ -438,7 +432,7 @@ nfs_dirty_request(struct nfs_page *req)
{
struct page *page = req->wb_page;
- if (page == NULL)
+ if (page == NULL || test_bit(PG_NEED_COMMIT, &req->wb_flags))
return 0;
return !PageWriteback(req->wb_page);
}
@@ -456,6 +450,7 @@ nfs_mark_request_commit(struct nfs_page *req)
spin_lock(&nfsi->req_lock);
nfs_list_add_request(req, &nfsi->commit);
nfsi->ncommit++;
+ set_bit(PG_NEED_COMMIT, &(req)->wb_flags);
spin_unlock(&nfsi->req_lock);
inc_zone_page_state(req->wb_page, NR_UNSTABLE_NFS);
__mark_inode_dirty(inode, I_DIRTY_DATASYNC);
@@ -470,7 +465,7 @@ int nfs_write_need_commit(struct nfs_write_data *data)
static inline
int nfs_reschedule_unstable_write(struct nfs_page *req)
{
- if (test_and_clear_bit(PG_NEED_COMMIT, &req->wb_flags)) {
+ if (test_bit(PG_NEED_COMMIT, &req->wb_flags)) {
nfs_mark_request_commit(req);
return 1;
}
@@ -557,6 +552,7 @@ static void nfs_cancel_commit_list(struct list_head *head)
req = nfs_list_entry(head->next);
dec_zone_page_state(req->wb_page, NR_UNSTABLE_NFS);
nfs_list_remove_request(req);
+ clear_bit(PG_NEED_COMMIT, &(req)->wb_flags);
nfs_inode_remove_request(req);
nfs_unlock_request(req);
}
@@ -1295,6 +1291,7 @@ static void nfs_commit_done(struct rpc_task *task, void *calldata)
while (!list_empty(&data->pages)) {
req = nfs_list_entry(data->pages.next);
nfs_list_remove_request(req);
+ clear_bit(PG_NEED_COMMIT, &(req)->wb_flags);
dec_zone_page_state(req->wb_page, NR_UNSTABLE_NFS);
dprintk("NFS: commit (%s/%Ld %d@%Ld)",
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
From: Trond Myklebust <[email protected]>
Protect nfs_set_page_dirty() against races with nfs_inode_add_request.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/write.c | 17 ++++++++++++++---
1 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index ce5b4a9..7975589 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -388,6 +388,8 @@ static int nfs_inode_add_request(struct inode *inode, struct nfs_page *req)
}
SetPagePrivate(req->wb_page);
set_page_private(req->wb_page, (unsigned long)req);
+ if (PageDirty(req->wb_page))
+ set_bit(PG_NEED_FLUSH, &req->wb_flags);
nfsi->npages++;
atomic_inc(&req->wb_count);
return 0;
@@ -407,6 +409,8 @@ static void nfs_inode_remove_request(struct nfs_page *req)
set_page_private(req->wb_page, 0);
ClearPagePrivate(req->wb_page);
radix_tree_delete(&nfsi->nfs_page_tree, req->wb_index);
+ if (test_and_clear_bit(PG_NEED_FLUSH, &req->wb_flags))
+ __set_page_dirty_nobuffers(req->wb_page);
nfsi->npages--;
if (!nfsi->npages) {
spin_unlock(&nfsi->req_lock);
@@ -1527,15 +1531,22 @@ int nfs_wb_page(struct inode *inode, struct page* page)
int nfs_set_page_dirty(struct page *page)
{
+ spinlock_t *req_lock = &NFS_I(page->mapping->host)->req_lock;
struct nfs_page *req;
+ int ret;
- req = nfs_page_find_request(page);
+ spin_lock(req_lock);
+ req = nfs_page_find_request_locked(page);
if (req != NULL) {
/* Mark any existing write requests for flushing */
- set_bit(PG_NEED_FLUSH, &req->wb_flags);
+ ret = !test_and_set_bit(PG_NEED_FLUSH, &req->wb_flags);
+ spin_unlock(req_lock);
nfs_release_request(req);
+ return ret;
}
- return __set_page_dirty_nobuffers(page);
+ ret = __set_page_dirty_nobuffers(page);
+ spin_unlock(req_lock);
+ return ret;
}
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
From: Trond Myklebust <[email protected]>
Fix a regression due to the patch "NFS: disconnect before retrying NFSv4
requests over TCP"
The assumption made in xprt_transmit() that the condition
"req->rq_bytes_sent == 0 and request is on the receive list"
should imply that we're dealing with a retransmission is false.
Firstly, it may simply happen that the socket send queue was full
at the time the request was initially sent through xprt_transmit().
Secondly, doing this for each request that was retransmitted implies
that we disconnect and reconnect for _every_ request that happened to
be retransmitted irrespective of whether or not a disconnection has
already occurred.
Fix is to move this logic into the call_status request timeout handler.
Signed-off-by: Trond Myklebust <[email protected]>
---
net/sunrpc/clnt.c | 4 ++++
net/sunrpc/xprt.c | 10 ----------
2 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 6d7221f..396cdbe 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1046,6 +1046,8 @@ call_status(struct rpc_task *task)
rpc_delay(task, 3*HZ);
case -ETIMEDOUT:
task->tk_action = call_timeout;
+ if (task->tk_client->cl_discrtry)
+ xprt_disconnect(task->tk_xprt);
break;
case -ECONNREFUSED:
case -ENOTCONN:
@@ -1169,6 +1171,8 @@ call_decode(struct rpc_task *task)
out_retry:
req->rq_received = req->rq_private_buf.len = 0;
task->tk_status = 0;
+ if (task->tk_client->cl_discrtry)
+ xprt_disconnect(task->tk_xprt);
}
/*
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index ee6ffa0..456a145 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -735,16 +735,6 @@ void xprt_transmit(struct rpc_task *task)
xprt_reset_majortimeo(req);
/* Turn off autodisconnect */
del_singleshot_timer_sync(&xprt->timer);
- } else {
- /* If all request bytes have been sent,
- * then we must be retransmitting this one */
- if (!req->rq_bytes_sent) {
- if (task->tk_client->cl_discrtry) {
- xprt_disconnect(xprt);
- task->tk_status = -ENOTCONN;
- return;
- }
- }
}
} else if (!req->rq_bytes_sent)
return;
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Fri, Apr 20, 2007 at 04:03:58PM -0400, Trond Myklebust wrote:
> I've split the issues introduced by the 2.6.21-rcX write code up into 4
> subproblems.
[snip]
> My thanks to the various patient victim^Wpeople who helped with extensive
> testing.
I've pulled the tree this morning
(0f851021c0f91e5073fa89f26b5ac68e23df8e11) and ran big-copy from a
gnome session. All is well.
Thanks,
florin
--
Bruce Schneier expects the Spanish Inquisition.
http://geekz.co.uk/schneierfacts/fact/163