* [PATCH 1/5] NFS: Prevent a deadlock in the new writeback code
@ 2012-05-09 19:18 Trond Myklebust
2012-05-09 19:18 ` [PATCH 2/5] NFS: nfs_set_page_writeback no longer needs to reference the page Trond Myklebust
0 siblings, 1 reply; 5+ messages in thread
From: Trond Myklebust @ 2012-05-09 19:18 UTC (permalink / raw)
To: linux-nfs; +Cc: Fred Isaman
We have to unlock the nfs_page before we call nfs_end_page_writeback
to avoid races with functions that expect the page to be unlocked
when PG_locked and PG_writeback are not set.
The problem is that nfs_unlock_request also releases the nfs_page,
causing a deadlock if the release of the nfs_open_context
triggers an iput() while the PG_writeback flag is still set...
The solution is to separate the unlocking and release of the nfs_page,
so that we can do the former before nfs_end_page_writeback and the
latter after.
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Fred Isaman <iisaman@netapp.com>
---
fs/nfs/pagelist.c | 12 ++++++++++--
fs/nfs/write.c | 6 ++++--
include/linux/nfs_page.h | 1 +
3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 33a21ca..69146f3 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -128,10 +128,10 @@ nfs_create_request(struct nfs_open_context *ctx, struct inode *inode,
}
/**
- * nfs_unlock_request - Unlock request and wake up sleepers.
+ * nfs_unlock_request_dont_release - Unlock request and wake up sleepers.
* @req:
*/
-void nfs_unlock_request(struct nfs_page *req)
+void nfs_unlock_request_dont_release(struct nfs_page *req)
{
if (!NFS_WBACK_BUSY(req)) {
printk(KERN_ERR "NFS: Invalid unlock attempted\n");
@@ -141,6 +141,14 @@ void nfs_unlock_request(struct nfs_page *req)
clear_bit(PG_BUSY, &req->wb_flags);
smp_mb__after_clear_bit();
wake_up_bit(&req->wb_flags, PG_BUSY);
+}
+
+/**
+ * nfs_unlock_request - Unlock request and release the nfs_page
+ */
+void nfs_unlock_request(struct nfs_page *req)
+{
+ nfs_unlock_request_dont_release(req);
nfs_release_request(req);
}
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 6f263da..fd36b31 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -628,8 +628,9 @@ static void nfs_write_completion(struct nfs_pgio_header *hdr)
remove_req:
nfs_inode_remove_request(req);
next:
- nfs_unlock_request(req);
+ nfs_unlock_request_dont_release(req);
nfs_end_page_writeback(page);
+ nfs_release_request(req);
}
out:
hdr->release(hdr);
@@ -1042,8 +1043,9 @@ static void nfs_redirty_request(struct nfs_page *req)
struct page *page = req->wb_page;
nfs_mark_request_dirty(req);
- nfs_unlock_request(req);
+ nfs_unlock_request_dont_release(req);
nfs_end_page_writeback(page);
+ nfs_release_request(req);
}
static void nfs_async_write_error(struct list_head *head)
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index f9ee9eb..ef75042 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -96,6 +96,7 @@ extern bool nfs_generic_pg_test(struct nfs_pageio_descriptor *desc,
struct nfs_page *req);
extern int nfs_wait_on_request(struct nfs_page *);
extern void nfs_unlock_request(struct nfs_page *req);
+extern void nfs_unlock_request_dont_release(struct nfs_page *req);
/*
* Lock the page of an asynchronous request without getting a new reference
--
1.7.7.6
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/5] NFS: nfs_set_page_writeback no longer needs to reference the page
2012-05-09 19:18 [PATCH 1/5] NFS: Prevent a deadlock in the new writeback code Trond Myklebust
@ 2012-05-09 19:18 ` Trond Myklebust
2012-05-09 19:18 ` [PATCH 3/5] NFS: Clean up - simplify nfs_lock_request() Trond Myklebust
0 siblings, 1 reply; 5+ messages in thread
From: Trond Myklebust @ 2012-05-09 19:18 UTC (permalink / raw)
To: linux-nfs; +Cc: Fred Isaman
We now hold a reference to the nfs_page across the calls to
nfs_set_page_writeback and nfs_end_page_writeback, and that
means we already have a reference to the struct page.
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Fred Isaman <iisaman@netapp.com>
---
fs/nfs/write.c | 11 +++--------
1 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index fd36b31..8382329 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -230,7 +230,6 @@ static int nfs_set_page_writeback(struct page *page)
struct inode *inode = page->mapping->host;
struct nfs_server *nfss = NFS_SERVER(inode);
- page_cache_get(page);
if (atomic_long_inc_return(&nfss->writeback) >
NFS_CONGESTION_ON_THRESH) {
set_bdi_congested(&nfss->backing_dev_info,
@@ -246,7 +245,6 @@ static void nfs_end_page_writeback(struct page *page)
struct nfs_server *nfss = NFS_SERVER(inode);
end_page_writeback(page);
- page_cache_release(page);
if (atomic_long_dec_return(&nfss->writeback) < NFS_CONGESTION_OFF_THRESH)
clear_bdi_congested(&nfss->backing_dev_info, BLK_RW_ASYNC);
}
@@ -607,13 +605,12 @@ static void nfs_write_completion(struct nfs_pgio_header *hdr)
nfs_init_cinfo_from_inode(&cinfo, hdr->inode);
while (!list_empty(&hdr->pages)) {
struct nfs_page *req = nfs_list_entry(hdr->pages.next);
- struct page *page = req->wb_page;
bytes += req->wb_bytes;
nfs_list_remove_request(req);
if (test_bit(NFS_IOHDR_ERROR, &hdr->flags) &&
(hdr->good_bytes < bytes)) {
- nfs_set_pageerror(page);
+ nfs_set_pageerror(req->wb_page);
nfs_context_set_write_error(req->wb_context, hdr->error);
goto remove_req;
}
@@ -629,7 +626,7 @@ remove_req:
nfs_inode_remove_request(req);
next:
nfs_unlock_request_dont_release(req);
- nfs_end_page_writeback(page);
+ nfs_end_page_writeback(req->wb_page);
nfs_release_request(req);
}
out:
@@ -1040,11 +1037,9 @@ static int nfs_do_multiple_writes(struct list_head *head,
*/
static void nfs_redirty_request(struct nfs_page *req)
{
- struct page *page = req->wb_page;
-
nfs_mark_request_dirty(req);
nfs_unlock_request_dont_release(req);
- nfs_end_page_writeback(page);
+ nfs_end_page_writeback(req->wb_page);
nfs_release_request(req);
}
--
1.7.7.6
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/5] NFS: Clean up - simplify nfs_lock_request()
2012-05-09 19:18 ` [PATCH 2/5] NFS: nfs_set_page_writeback no longer needs to reference the page Trond Myklebust
@ 2012-05-09 19:18 ` Trond Myklebust
2012-05-09 19:18 ` [PATCH 4/5] NFS: Clean up - Rename nfs_unlock_request and nfs_unlock_request_dont_release Trond Myklebust
0 siblings, 1 reply; 5+ messages in thread
From: Trond Myklebust @ 2012-05-09 19:18 UTC (permalink / raw)
To: linux-nfs; +Cc: Fred Isaman
We only have two places where we need to grab a reference when trying
to lock the nfs_page. We're better off making that explicit.
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Fred Isaman <iisaman@netapp.com>
---
fs/nfs/direct.c | 1 +
fs/nfs/write.c | 11 ++++++-----
include/linux/nfs_page.h | 14 ++------------
3 files changed, 9 insertions(+), 17 deletions(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 257d009..465ea84 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -657,6 +657,7 @@ static ssize_t nfs_direct_write_schedule_segment(struct nfs_pageio_descriptor *d
break;
}
nfs_lock_request(req);
+ kref_get(&req->wb_kref);
req->wb_index = pos >> PAGE_SHIFT;
req->wb_offset = pos & ~PAGE_MASK;
if (!nfs_pageio_add_request(desc, req)) {
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 8382329..553f7ef 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -260,10 +260,10 @@ static struct nfs_page *nfs_find_and_lock_request(struct page *page, bool nonblo
req = nfs_page_find_request_locked(page);
if (req == NULL)
break;
- if (nfs_lock_request_dontget(req))
+ if (nfs_lock_request(req))
break;
/* Note: If we hold the page lock, as is the case in nfs_writepage,
- * then the call to nfs_lock_request_dontget() will always
+ * then the call to nfs_lock_request() will always
* succeed provided that someone hasn't already marked the
* request as dirty (in which case we don't care).
*/
@@ -406,7 +406,7 @@ static void nfs_inode_add_request(struct inode *inode, struct nfs_page *req)
struct nfs_inode *nfsi = NFS_I(inode);
/* Lock the request! */
- nfs_lock_request_dontget(req);
+ nfs_lock_request(req);
spin_lock(&inode->i_lock);
if (!nfsi->npages && nfs_have_delegation(inode, FMODE_WRITE))
@@ -651,6 +651,7 @@ nfs_scan_commit_list(struct list_head *src, struct list_head *dst,
list_for_each_entry_safe(req, tmp, src, wb_list) {
if (!nfs_lock_request(req))
continue;
+ kref_get(&req->wb_kref);
if (cond_resched_lock(cinfo->lock))
list_safe_reset_next(req, tmp, wb_list);
nfs_request_remove_commit_list(req, cinfo);
@@ -741,7 +742,7 @@ static struct nfs_page *nfs_try_to_update_request(struct inode *inode,
|| end < req->wb_offset)
goto out_flushme;
- if (nfs_lock_request_dontget(req))
+ if (nfs_lock_request(req))
break;
/* The request is locked, so wait and then retry */
@@ -1717,7 +1718,7 @@ int nfs_wb_page_cancel(struct inode *inode, struct page *page)
req = nfs_page_find_request(page);
if (req == NULL)
break;
- if (nfs_lock_request_dontget(req)) {
+ if (nfs_lock_request(req)) {
nfs_clear_request_commit(req);
nfs_inode_remove_request(req);
/*
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index ef75042..263f30a 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -99,24 +99,14 @@ extern void nfs_unlock_request(struct nfs_page *req);
extern void nfs_unlock_request_dont_release(struct nfs_page *req);
/*
- * Lock the page of an asynchronous request without getting a new reference
+ * Lock the page of an asynchronous request
*/
static inline int
-nfs_lock_request_dontget(struct nfs_page *req)
-{
- return !test_and_set_bit(PG_BUSY, &req->wb_flags);
-}
-
-static inline int
nfs_lock_request(struct nfs_page *req)
{
- if (test_and_set_bit(PG_BUSY, &req->wb_flags))
- return 0;
- kref_get(&req->wb_kref);
- return 1;
+ return !test_and_set_bit(PG_BUSY, &req->wb_flags);
}
-
/**
* nfs_list_add_request - Insert a request into a list
* @req: request
--
1.7.7.6
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 4/5] NFS: Clean up - Rename nfs_unlock_request and nfs_unlock_request_dont_release
2012-05-09 19:18 ` [PATCH 3/5] NFS: Clean up - simplify nfs_lock_request() Trond Myklebust
@ 2012-05-09 19:18 ` Trond Myklebust
2012-05-09 19:18 ` [PATCH 5/5] NFS: Clean up - Simplify reference counting in fs/nfs/direct.c Trond Myklebust
0 siblings, 1 reply; 5+ messages in thread
From: Trond Myklebust @ 2012-05-09 19:18 UTC (permalink / raw)
To: linux-nfs; +Cc: Fred Isaman
Function rename to ensure that the functionality of nfs_unlock_request()
mirrors that of nfs_lock_request(). Then let nfs_unlock_and_release_request()
do the work of what used to be called nfs_unlock_request()...
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Fred Isaman <iisaman@netapp.com>
---
fs/nfs/direct.c | 10 +++++-----
fs/nfs/pagelist.c | 11 ++++++-----
fs/nfs/write.c | 12 ++++++------
include/linux/nfs_page.h | 2 +-
4 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 465ea84..845e201 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -488,7 +488,7 @@ static void nfs_direct_write_reschedule(struct nfs_direct_req *dreq)
while (!list_empty(&failed)) {
nfs_release_request(req);
- nfs_unlock_request(req);
+ nfs_unlock_and_release_request(req);
}
if (put_dreq(dreq))
@@ -521,7 +521,7 @@ static void nfs_direct_commit_complete(struct nfs_commit_data *data)
nfs_mark_request_commit(req, NULL, &cinfo);
} else
nfs_release_request(req);
- nfs_unlock_request(req);
+ nfs_unlock_and_release_request(req);
}
if (atomic_dec_and_test(&cinfo.mds->rpcs_out))
@@ -662,7 +662,7 @@ static ssize_t nfs_direct_write_schedule_segment(struct nfs_pageio_descriptor *d
req->wb_offset = pos & ~PAGE_MASK;
if (!nfs_pageio_add_request(desc, req)) {
result = desc->pg_error;
- nfs_unlock_request(req);
+ nfs_unlock_and_release_request(req);
nfs_release_request(req);
break;
}
@@ -739,7 +739,7 @@ static void nfs_direct_write_completion(struct nfs_pgio_header *hdr)
default:
nfs_release_request(req);
}
- nfs_unlock_request(req);
+ nfs_unlock_and_release_request(req);
}
out_put:
@@ -756,7 +756,7 @@ static void nfs_write_sync_pgio_error(struct list_head *head)
req = nfs_list_entry(head->next);
nfs_list_remove_request(req);
nfs_release_request(req);
- nfs_unlock_request(req);
+ nfs_unlock_and_release_request(req);
}
}
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 69146f3..aed913c 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -128,10 +128,10 @@ nfs_create_request(struct nfs_open_context *ctx, struct inode *inode,
}
/**
- * nfs_unlock_request_dont_release - Unlock request and wake up sleepers.
+ * nfs_unlock_request - Unlock request and wake up sleepers.
* @req:
*/
-void nfs_unlock_request_dont_release(struct nfs_page *req)
+void nfs_unlock_request(struct nfs_page *req)
{
if (!NFS_WBACK_BUSY(req)) {
printk(KERN_ERR "NFS: Invalid unlock attempted\n");
@@ -144,11 +144,12 @@ void nfs_unlock_request_dont_release(struct nfs_page *req)
}
/**
- * nfs_unlock_request - Unlock request and release the nfs_page
+ * nfs_unlock_and_release_request - Unlock request and release the nfs_page
+ * @req:
*/
-void nfs_unlock_request(struct nfs_page *req)
+void nfs_unlock_and_release_request(struct nfs_page *req)
{
- nfs_unlock_request_dont_release(req);
+ nfs_unlock_request(req);
nfs_release_request(req);
}
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 553f7ef..8ffd7d5 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -625,7 +625,7 @@ static void nfs_write_completion(struct nfs_pgio_header *hdr)
remove_req:
nfs_inode_remove_request(req);
next:
- nfs_unlock_request_dont_release(req);
+ nfs_unlock_request(req);
nfs_end_page_writeback(req->wb_page);
nfs_release_request(req);
}
@@ -812,7 +812,7 @@ static int nfs_writepage_setup(struct nfs_open_context *ctx, struct page *page,
nfs_grow_file(page, offset, count);
nfs_mark_uptodate(page, req->wb_pgbase, req->wb_bytes);
nfs_mark_request_dirty(req);
- nfs_unlock_request(req);
+ nfs_unlock_and_release_request(req);
return 0;
}
@@ -1039,7 +1039,7 @@ static int nfs_do_multiple_writes(struct list_head *head,
static void nfs_redirty_request(struct nfs_page *req)
{
nfs_mark_request_dirty(req);
- nfs_unlock_request_dont_release(req);
+ nfs_unlock_request(req);
nfs_end_page_writeback(req->wb_page);
nfs_release_request(req);
}
@@ -1477,7 +1477,7 @@ void nfs_retry_commit(struct list_head *page_list,
dec_bdi_stat(req->wb_page->mapping->backing_dev_info,
BDI_RECLAIMABLE);
}
- nfs_unlock_request(req);
+ nfs_unlock_and_release_request(req);
}
}
EXPORT_SYMBOL_GPL(nfs_retry_commit);
@@ -1555,7 +1555,7 @@ static void nfs_commit_release_pages(struct nfs_commit_data *data)
dprintk(" mismatch\n");
nfs_mark_request_dirty(req);
next:
- nfs_unlock_request(req);
+ nfs_unlock_and_release_request(req);
}
nfs_init_cinfo(&cinfo, data->inode, data->dreq);
if (atomic_dec_and_test(&cinfo.mds->rpcs_out))
@@ -1726,7 +1726,7 @@ int nfs_wb_page_cancel(struct inode *inode, struct page *page)
* page as being dirty
*/
cancel_dirty_page(page, PAGE_CACHE_SIZE);
- nfs_unlock_request(req);
+ nfs_unlock_and_release_request(req);
break;
}
ret = nfs_wait_on_request(req);
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index 263f30a..88d166b 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -96,7 +96,7 @@ extern bool nfs_generic_pg_test(struct nfs_pageio_descriptor *desc,
struct nfs_page *req);
extern int nfs_wait_on_request(struct nfs_page *);
extern void nfs_unlock_request(struct nfs_page *req);
-extern void nfs_unlock_request_dont_release(struct nfs_page *req);
+extern void nfs_unlock_and_release_request(struct nfs_page *req);
/*
* Lock the page of an asynchronous request
--
1.7.7.6
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 5/5] NFS: Clean up - Simplify reference counting in fs/nfs/direct.c
2012-05-09 19:18 ` [PATCH 4/5] NFS: Clean up - Rename nfs_unlock_request and nfs_unlock_request_dont_release Trond Myklebust
@ 2012-05-09 19:18 ` Trond Myklebust
0 siblings, 0 replies; 5+ messages in thread
From: Trond Myklebust @ 2012-05-09 19:18 UTC (permalink / raw)
To: linux-nfs; +Cc: Fred Isaman
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Fred Isaman <iisaman@netapp.com>
---
fs/nfs/direct.c | 15 ++++-----------
1 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 845e201..c47a46e 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -486,10 +486,8 @@ static void nfs_direct_write_reschedule(struct nfs_direct_req *dreq)
}
nfs_pageio_complete(&desc);
- while (!list_empty(&failed)) {
- nfs_release_request(req);
+ while (!list_empty(&failed))
nfs_unlock_and_release_request(req);
- }
if (put_dreq(dreq))
nfs_direct_write_complete(dreq, dreq->inode);
@@ -518,9 +516,9 @@ static void nfs_direct_commit_complete(struct nfs_commit_data *data)
nfs_list_remove_request(req);
if (dreq->flags == NFS_ODIRECT_RESCHED_WRITES) {
/* Note the rewrite will go through mds */
+ kref_get(&req->wb_kref);
nfs_mark_request_commit(req, NULL, &cinfo);
- } else
- nfs_release_request(req);
+ }
nfs_unlock_and_release_request(req);
}
@@ -657,13 +655,11 @@ static ssize_t nfs_direct_write_schedule_segment(struct nfs_pageio_descriptor *d
break;
}
nfs_lock_request(req);
- kref_get(&req->wb_kref);
req->wb_index = pos >> PAGE_SHIFT;
req->wb_offset = pos & ~PAGE_MASK;
if (!nfs_pageio_add_request(desc, req)) {
result = desc->pg_error;
nfs_unlock_and_release_request(req);
- nfs_release_request(req);
break;
}
pgbase = 0;
@@ -734,10 +730,8 @@ static void nfs_direct_write_completion(struct nfs_pgio_header *hdr)
switch (bit) {
case NFS_IOHDR_NEED_RESCHED:
case NFS_IOHDR_NEED_COMMIT:
+ kref_get(&req->wb_kref);
nfs_mark_request_commit(req, hdr->lseg, &cinfo);
- break;
- default:
- nfs_release_request(req);
}
nfs_unlock_and_release_request(req);
}
@@ -755,7 +749,6 @@ static void nfs_write_sync_pgio_error(struct list_head *head)
while (!list_empty(head)) {
req = nfs_list_entry(head->next);
nfs_list_remove_request(req);
- nfs_release_request(req);
nfs_unlock_and_release_request(req);
}
}
--
1.7.7.6
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-05-09 19:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-09 19:18 [PATCH 1/5] NFS: Prevent a deadlock in the new writeback code Trond Myklebust
2012-05-09 19:18 ` [PATCH 2/5] NFS: nfs_set_page_writeback no longer needs to reference the page Trond Myklebust
2012-05-09 19:18 ` [PATCH 3/5] NFS: Clean up - simplify nfs_lock_request() Trond Myklebust
2012-05-09 19:18 ` [PATCH 4/5] NFS: Clean up - Rename nfs_unlock_request and nfs_unlock_request_dont_release Trond Myklebust
2012-05-09 19:18 ` [PATCH 5/5] NFS: Clean up - Simplify reference counting in fs/nfs/direct.c Trond Myklebust
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.