On Sun, 2012-07-08 at 11:15 +0200, Julia Lawall wrote:
> The following code, in the function nfs_direct_write_reschedule, looks
> strange to me:
>
> list_for_each_entry_safe(req, tmp, &reqs, wb_list) {
> if (!nfs_pageio_add_request(&desc, req)) {
> nfs_list_add_request(req, &failed);
> spin_lock(cinfo.lock);
> dreq->flags = 0;
> dreq->error = -EIO;
> spin_unlock(cinfo.lock);
> }
> nfs_release_request(req);
> }
> nfs_pageio_complete(&desc);
>
> while (!list_empty(&failed))
> nfs_unlock_and_release_request(req);
>
> After the list_for_each_entry_safe, req is an address at some offset from
> the list head. So it does not seem like an appropriate argument to
> nfs_unlock_and_release_request.
Doh!... That's a bug that crept in via commit
1763da1234cba663b849476d451bdccac5147859 (NFS: rewrite directio write to
use async coalesce code) and has been "polished" until it gleans several
times with assorted cleanups...
How about something like the following fix?
8<---------------------------------------------------------------------
>From 4035c2487f179327fae87af3477659402b797584 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
Date: Sun, 8 Jul 2012 10:24:10 -0400
Subject: [PATCH] NFS: Fix list manipulation snafus in fs/nfs/direct.c
Fix 2 bugs in nfs_direct_write_reschedule:
- The request needs to be removed from the 'reqs' list before it can
be added to 'failed'.
- Fix an infinite loop if the 'failed' list is non-empty.
Reported-by: Julia Lawall <julia.lawall@xxxxxxx>
Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
---
fs/nfs/direct.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 9a4cbfc..4825337 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -484,6 +484,7 @@ static void nfs_direct_write_reschedule(struct nfs_direct_req *dreq)
list_for_each_entry_safe(req, tmp, &reqs, wb_list) {
if (!nfs_pageio_add_request(&desc, req)) {
+ nfs_list_remove_request(req);
nfs_list_add_request(req, &failed);
spin_lock(cinfo.lock);
dreq->flags = 0;
@@ -494,8 +495,11 @@ static void nfs_direct_write_reschedule(struct nfs_direct_req *dreq)
}
nfs_pageio_complete(&desc);
- while (!list_empty(&failed))
+ while (!list_empty(&failed)) {
+ req = nfs_list_entry(failed.next);
+ nfs_list_remove_request(req);
nfs_unlock_and_release_request(req);
+ }
if (put_dreq(dreq))
nfs_direct_write_complete(dreq, dreq->inode);
--
1.7.10.4
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
