Commit 5b2b5187 authored by Trond Myklebust's avatar Trond Myklebust

NFS: Fix nfs_page_group_destroy() and nfs_lock_and_join_requests() race cases

Since nfs_page_group_destroy() does not take any locks on the requests
to be freed, we need to ensure that we don't inadvertently free the
request in nfs_destroy_unlinked_subrequests() while the last reference
is being released elsewhere.

Do this by:

1) Taking a reference to the request unless it is already being freed
2) Checking (under the page group lock) if PG_TEARDOWN is already set before
   freeing an unreferenced request in nfs_destroy_unlinked_subrequests()
Signed-off-by: default avatarTrond Myklebust <trond.myklebust@primarydata.com>
parent 74a6d4b5
...@@ -384,10 +384,11 @@ nfs_unroll_locks(struct inode *inode, struct nfs_page *head, ...@@ -384,10 +384,11 @@ nfs_unroll_locks(struct inode *inode, struct nfs_page *head,
struct nfs_page *tmp; struct nfs_page *tmp;
/* relinquish all the locks successfully grabbed this run */ /* relinquish all the locks successfully grabbed this run */
for (tmp = head->wb_this_page ; tmp != req; tmp = tmp->wb_this_page) for (tmp = head->wb_this_page ; tmp != req; tmp = tmp->wb_this_page) {
nfs_unlock_request(tmp); if (!kref_read(&tmp->wb_kref))
continue;
WARN_ON_ONCE(test_bit(PG_TEARDOWN, &req->wb_flags)); nfs_unlock_and_release_request(tmp);
}
} }
/* /*
...@@ -414,36 +415,32 @@ nfs_destroy_unlinked_subrequests(struct nfs_page *destroy_list, ...@@ -414,36 +415,32 @@ nfs_destroy_unlinked_subrequests(struct nfs_page *destroy_list,
WARN_ON_ONCE(old_head != subreq->wb_head); WARN_ON_ONCE(old_head != subreq->wb_head);
/* make sure old group is not used */ /* make sure old group is not used */
subreq->wb_head = subreq;
subreq->wb_this_page = subreq; subreq->wb_this_page = subreq;
/* subreq is now totally disconnected from page group or any /* Note: races with nfs_page_group_destroy() */
* write / commit lists. last chance to wake any waiters */ if (!kref_read(&subreq->wb_kref)) {
nfs_unlock_request(subreq); bool freeme = test_bit(PG_TEARDOWN, &subreq->wb_flags);
if (!test_bit(PG_TEARDOWN, &subreq->wb_flags)) {
/* release ref on old head request */
nfs_release_request(old_head);
nfs_page_group_clear_bits(subreq); nfs_page_group_clear_bits(subreq);
/* Check if we raced with nfs_page_group_destroy() */
if (freeme)
nfs_free_request(subreq);
continue;
}
subreq->wb_head = subreq;
/* release the PG_INODE_REF reference */
if (test_and_clear_bit(PG_INODE_REF, &subreq->wb_flags)) { if (test_and_clear_bit(PG_INODE_REF, &subreq->wb_flags)) {
nfs_release_request(subreq); nfs_release_request(subreq);
spin_lock(&inode->i_lock); spin_lock(&inode->i_lock);
NFS_I(inode)->nrequests--; NFS_I(inode)->nrequests--;
spin_unlock(&inode->i_lock); spin_unlock(&inode->i_lock);
} else
WARN_ON_ONCE(1);
} else {
WARN_ON_ONCE(test_bit(PG_CLEAN, &subreq->wb_flags));
/* zombie requests have already released the last
* reference and were waiting on the rest of the
* group to complete. Since it's no longer part of a
* group, simply free the request */
nfs_page_group_clear_bits(subreq);
nfs_free_request(subreq);
} }
nfs_page_group_clear_bits(subreq);
/* subreq is now totally disconnected from page group or any
* write / commit lists. last chance to wake any waiters */
nfs_unlock_and_release_request(subreq);
} }
} }
...@@ -512,6 +509,8 @@ nfs_lock_and_join_requests(struct page *page) ...@@ -512,6 +509,8 @@ nfs_lock_and_join_requests(struct page *page)
for (subreq = head->wb_this_page; subreq != head; for (subreq = head->wb_this_page; subreq != head;
subreq = subreq->wb_this_page) { subreq = subreq->wb_this_page) {
if (!kref_get_unless_zero(&subreq->wb_kref))
continue;
while (!nfs_lock_request(subreq)) { while (!nfs_lock_request(subreq)) {
/* /*
* Unlock page to allow nfs_page_group_sync_on_bit() * Unlock page to allow nfs_page_group_sync_on_bit()
...@@ -523,6 +522,7 @@ nfs_lock_and_join_requests(struct page *page) ...@@ -523,6 +522,7 @@ nfs_lock_and_join_requests(struct page *page)
ret = nfs_page_group_lock(head, false); ret = nfs_page_group_lock(head, false);
if (ret < 0) { if (ret < 0) {
nfs_unroll_locks(inode, head, subreq); nfs_unroll_locks(inode, head, subreq);
nfs_release_request(subreq);
nfs_unlock_and_release_request(head); nfs_unlock_and_release_request(head);
return ERR_PTR(ret); return ERR_PTR(ret);
} }
...@@ -537,8 +537,8 @@ nfs_lock_and_join_requests(struct page *page) ...@@ -537,8 +537,8 @@ nfs_lock_and_join_requests(struct page *page)
} else if (WARN_ON_ONCE(subreq->wb_offset < head->wb_offset || } else if (WARN_ON_ONCE(subreq->wb_offset < head->wb_offset ||
((subreq->wb_offset + subreq->wb_bytes) > ((subreq->wb_offset + subreq->wb_bytes) >
(head->wb_offset + total_bytes)))) { (head->wb_offset + total_bytes)))) {
nfs_unlock_request(subreq);
nfs_unroll_locks(inode, head, subreq); nfs_unroll_locks(inode, head, subreq);
nfs_unlock_and_release_request(subreq);
nfs_page_group_unlock(head); nfs_page_group_unlock(head);
nfs_unlock_and_release_request(head); nfs_unlock_and_release_request(head);
return ERR_PTR(-EIO); return ERR_PTR(-EIO);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment