nfs_page_group_lock cleanup
Subsystems (2)
cover nfs_page_group_lock cleanup
These patches clean up some issues surrouding nfs_page_group_lock:
- normalize wait/nonblock argument
- make nonblocking calls really nonblocking
- handle errors
- ensure that we don't call blocking nfs_page_group_lock when holding the inode spinlock
This cleanup was inspired by Fengguang Wu's report that we were sleeping with locks held in nfs_lock_and_join_requests.
Weston Andros Adamson (5):
nfs: change nfs_page_group_lock argument
nfs: fix nonblocking calls to nfs_page_group_lock
nfs: use blocking page_group_lock in add_request
nfs: fix error handling in lock_and_join_requests
nfs: don't sleep with inode lock in lock_and_join_requests
fs/nfs/pagelist.c | 59 ++++++++++++++++++++++++++++++------------------ fs/nfs/write.c | 21 +++++++++++++---- include/linux/nfs_page.h | 1 + 3 files changed, 55 insertions(+), 26 deletions(-)
Signature · 1 line
1.8.5.2 (Apple Git-48)
PATCH 1/5 nfs: change nfs_page_group_lock argument
Flip the meaning of the second argument from 'wait' to 'nonblock' to match related functions. Update all five calls to reflect this change.
--- fs/nfs/pagelist.c | 11 ++++++----- fs/nfs/write.c | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-)
fs/nfs/pagelist.c +6 -5
@@ -145,13 +145,14 @@ static int nfs_wait_bit_uninterruptible(void *word)/* * nfs_page_group_lock - lock the head of the page group * @req - request in group that is to be locked +* @nonblock - if true don't block waiting for lock * * this lock must be held if modifying the page group list * * returns result from wait_on_bit_lock: 0 on success, < 0 on error */ int -nfs_page_group_lock (struct nfs_page *req,bool wait) +nfs_page_group_lock (struct nfs_page *req,bool nonblock) {struct nfs_page *head = req->wb_head;int ret;
@@ -162,7 +163,7 @@ nfs_page_group_lock(struct nfs_page *req, bool wait) ret =wait_on_bit_lock (&head->wb_flags,PG_HEADLOCK , nfs_wait_bit_uninterruptible,TASK_UNINTERRUPTIBLE ); - }while (wait && ret !=0 ); + }while (!nonblock && ret !=0 );WARN_ON_ONCE (ret >0 );return ret;
@@ -226,7 +227,7 @@ bool nfs_page_group_sync_on_bit(struct nfs_page *req, unsigned int bit) {bool ret; -nfs_page_group_lock (req, true); +nfs_page_group_lock (req, false); ret =nfs_page_group_sync_on_bit_locked (req, bit);nfs_page_group_unlock (req);
@@ -868,7 +869,7 @@ static int __nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,unsigned int offset, pgbase;int ret; - ret =nfs_page_group_lock (req, false); + ret =nfs_page_group_lock (req, true);if (ret <0 ) { desc->pg_error = ret;return 0 ;
@@ -894,7 +895,7 @@ static int __nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,if (desc->pg_recoalesce)return 0 ;/* retry add_request for this subreq */ - ret =nfs_page_group_lock (req, false); + ret =nfs_page_group_lock (req, true);if (ret <0 ) { desc->pg_error = ret;return 0 ;
fs/nfs/write.c +2 -2
@@ -241,7 +241,7 @@ static bool nfs_page_group_covers_page(struct nfs_page *req)unsigned int pos =0 ;unsigned int len =nfs_page_length (req->wb_page); -nfs_page_group_lock (req, true); +nfs_page_group_lock (req, false); do { tmp = nfs_page_group_search_locked(req->wb_head, pos);
@@ -479,7 +479,7 @@ try_again: }/* lock each request in the page group */ - ret =nfs_page_group_lock (head, false); + ret =nfs_page_group_lock (head, true);if (ret <0 )return ERR_PTR (ret); subreq = head;
Signature · 1 line
1.8.5.2 (Apple Git-48)
PATCH 2/5 nfs: fix nonblocking calls to nfs_page_group_lock
In reply to Weston Andros Adamsonnfs_page_group_lock was calling wait_on_bit_lock even when told not to block. Fix by first trying test_and_set_bit, followed by wait_on_bit_lock if and only if blocking is allowed. Return -EAGAIN if nonblocking and the test_and_set of the bit was already locked.
--- fs/nfs/pagelist.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
fs/nfs/pagelist.c +14 -9
@@ -149,24 +149,29 @@ static int nfs_wait_bit_uninterruptible(void *word)* * this lock must be held if modifying the page group list * -* returns result from wait_on_bit_lock: 0 on success, < 0 on error +* return 0 on success, < 0 on error: -EDELAY if nonblocking or the +* result from wait_on_bit_lock +* +* NOTE: calling with nonblock=false should always have set the +* lock bit (see fs/buffer.c and other uses of wait_on_bit_lock +* with TASK_UNINTERRUPTIBLE), so there is no need to check the result. */ int nfs_page_group_lock (struct nfs_page *req,bool nonblock) {struct nfs_page *head = req->wb_head; -int ret;WARN_ON_ONCE (head != head->wb_head); -do { - ret =wait_on_bit_lock (&head->wb_flags,PG_HEADLOCK , - nfs_wait_bit_uninterruptible, -TASK_UNINTERRUPTIBLE ); - }while (!nonblock && ret !=0 ); +if (!test_and_set_bit (PG_HEADLOCK , &head->wb_flags)) +return 0 ; -WARN_ON_ONCE (ret >0 ); -return ret; +if (!nonblock) +return wait_on_bit_lock (&head->wb_flags,PG_HEADLOCK , + nfs_wait_bit_uninterruptible, +TASK_UNINTERRUPTIBLE ); + +return -EAGAIN ; }/*
Signature · 1 line
1.8.5.2 (Apple Git-48)
PATCH 3/5 nfs: use blocking page_group_lock in add_request
In reply to Weston Andros Adamson__nfs_pageio_add_request was calling nfs_page_group_lock nonblocking, but this can return -EAGAIN which would end up passing -EIO to the application.
There is no reason not to block in this path, so change the two calls to do so. Also, there is no need to check the return value of nfs_page_group_lock when nonblock=false, so remove the error handling code.
--- fs/nfs/pagelist.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
fs/nfs/pagelist.c +2 -11
@@ -872,13 +872,8 @@ static int __nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,struct nfs_page *subreq;unsigned int bytes_left =0 ;unsigned int offset, pgbase; -int ret; - ret =nfs_page_group_lock (req, true); -if (ret <0 ) { - desc->pg_error = ret; -return 0 ; - } +nfs_page_group_lock (req, false); subreq = req; bytes_left = subreq->wb_bytes;
@@ -900,11 +895,7 @@ static int __nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,if (desc->pg_recoalesce)return 0 ;/* retry add_request for this subreq */ - ret =nfs_page_group_lock (req, true); -if (ret <0 ) { - desc->pg_error = ret; -return 0 ; - } +nfs_page_group_lock (req, false);continue ; }
Signature · 1 line
1.8.5.2 (Apple Git-48)
PATCH 4/5 nfs: fix error handling in lock_and_join_requests
In reply to Weston Andros AdamsonThis fixes handling of errors from nfs_page_group_lock in nfs_lock_and_join_requests. It now releases the inode lock and the reference to the head request.
--- fs/nfs/write.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
fs/nfs/write.c +4 -1
@@ -480,8 +480,11 @@ try_again:/* lock each request in the page group */ ret =nfs_page_group_lock (head, true); -if (ret <0 ) +if (ret <0 ) { +spin_unlock (&inode->i_lock); +nfs_release_request (head);return ERR_PTR (ret); + } subreq = head; do { /*
Signature · 1 line
1.8.5.2 (Apple Git-48)
PATCH 5/5 nfs: don't sleep with inode lock in lock_and_join_requests
In reply to Weston Andros AdamsonThis handles the 'nonblock=false' case in nfs_lock_and_join_requests. If the group is already locked and blocking is allowed, drop the inode lock and wait for the group lock to be cleared before trying it all again. This should fix warnings found in peterz's tree (sched/wait branch), where might_sleep() checks are added to wait.[ch].
--- fs/nfs/pagelist.c | 18 ++++++++++++++++++ fs/nfs/write.c | 12 +++++++++++- include/linux/nfs_page.h | 1 + 3 files changed, 30 insertions(+), 1 deletion(-)
fs/nfs/pagelist.c +18 -0
@@ -175,6 +175,24 @@ nfs_page_group_lock(struct nfs_page *req, bool nonblock) }/* +* nfs_page_group_lock_wait - wait for the lock to clear, but don't grab it +* @req - a request in the group +* +* This is a blocking call to wait for the group lock to be cleared. +*/ +void +nfs_page_group_lock_wait (struct nfs_page *req) +{ +struct nfs_page *head = req->wb_head; + +WARN_ON_ONCE (head != head->wb_head); + +wait_on_bit (&head->wb_flags,PG_HEADLOCK , + nfs_wait_bit_uninterruptible, +TASK_UNINTERRUPTIBLE ); +} + +/* * nfs_page_group_unlock - unlock the head of the page group * @req - request in group that is to be unlocked */
fs/nfs/write.c +11 -1
@@ -478,13 +478,23 @@ try_again:return NULL ; } -/* lock each request in the page group */ +/* holding inode lock, so always make a non-blocking call to try the +* page group lock */ ret =nfs_page_group_lock (head, true);if (ret <0 ) {spin_unlock (&inode->i_lock); + +if (!nonblock && ret == -EAGAIN ) { +nfs_page_group_lock_wait (head); +nfs_release_request (head); + goto try_again; + } +nfs_release_request (head);return ERR_PTR (ret); } + +/* lock each request in the page group */ subreq = head; do { /*
include/linux/nfs_page.h +1 -0
@@ -123,6 +123,7 @@ extern int nfs_wait_on_request(struct nfs_page *);extern void nfs_unlock_request (struct nfs_page *req);extern void nfs_unlock_and_release_request (struct nfs_page *);extern int nfs_page_group_lock (struct nfs_page *,bool ); +extern void nfs_page_group_lock_wait (struct nfs_page *);extern void nfs_page_group_unlock (struct nfs_page *);extern bool nfs_page_group_sync_on_bit (struct nfs_page *,unsigned int );
Signature · 1 line
1.8.5.2 (Apple Git-48)