From patchwork Sun Jul 8 14:34:03 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Trond Myklebust X-Patchwork-Id: 1169691 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 5A08A40134 for ; Sun, 8 Jul 2012 14:38:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752141Ab2GHOeX (ORCPT ); Sun, 8 Jul 2012 10:34:23 -0400 Received: from mx2.netapp.com ([216.240.18.37]:53138 "EHLO mx2.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751742Ab2GHOeX (ORCPT ); Sun, 8 Jul 2012 10:34:23 -0400 X-IronPort-AV: E=Sophos;i="4.77,547,1336374000"; d="scan'208";a="660693958" Received: from smtp1.corp.netapp.com ([10.57.156.124]) by mx2-out.netapp.com with ESMTP; 08 Jul 2012 07:34:06 -0700 Received: from vmwexceht04-prd.hq.netapp.com (vmwexceht04-prd.hq.netapp.com [10.106.77.34]) by smtp1.corp.netapp.com (8.13.1/8.13.1/NTAP-1.6) with ESMTP id q68EY5p6001218; Sun, 8 Jul 2012 07:34:05 -0700 (PDT) Received: from SACEXCMBX04-PRD.hq.netapp.com ([169.254.6.160]) by vmwexceht04-prd.hq.netapp.com ([10.106.77.34]) with mapi id 14.02.0298.004; Sun, 8 Jul 2012 07:34:04 -0700 From: "Myklebust, Trond" To: Julia Lawall CC: "linux-nfs@vger.kernel.org" Subject: Re: question about fs/nfs/direct.c Thread-Topic: question about fs/nfs/direct.c Thread-Index: AQHNXOpJ4tmWIyNecEWRz2aUZFFGbJcf6PQA Date: Sun, 8 Jul 2012 14:34:03 +0000 Message-ID: <1341758042.3575.6.camel@lade.trondhjem.org> References: In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.104.60.115] Content-ID: <4E0D6CF289CF6A4183278748F662FB2D@tahoe.netapp.com> MIME-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org 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 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 Signed-off-by: Trond Myklebust --- fs/nfs/direct.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) -- 1.7.10.4 -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com 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);