From patchwork Sun Aug 24 17:49:16 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Boaz Harrosh X-Patchwork-Id: 4770741 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 2D134C0338 for ; Sun, 24 Aug 2014 17:49:25 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 01DBB20121 for ; Sun, 24 Aug 2014 17:49:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C959D2012B for ; Sun, 24 Aug 2014 17:49:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752643AbaHXRtV (ORCPT ); Sun, 24 Aug 2014 13:49:21 -0400 Received: from mail-wi0-f181.google.com ([209.85.212.181]:52836 "EHLO mail-wi0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752343AbaHXRtU (ORCPT ); Sun, 24 Aug 2014 13:49:20 -0400 Received: by mail-wi0-f181.google.com with SMTP id bs8so1583429wib.14 for ; Sun, 24 Aug 2014 10:49:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:content-type:content-transfer-encoding; bh=WNTan8wX3KAEWHxtWtFkWJiU60DcclSPBxT8IgL+jBg=; b=RrvdSoXVAJIKRPVeNUeLPyxgsztY9m7nvwZ8OzJL8qYKJ3VvAPxZgyQU/9+pa+mNn8 Y0KqegVQKiy0UlRBriJj/5j3V4/lAqJVaB3cSW/AVPNqX7etlVQj2U8AQ5jES/7aayFR XGBBW2XuBelrbOeeI16lJIUsFsOBvCvN7JyuU2YmLvwP6zwte9gEUp7CXCe/T9DCrZeH U6kgkNMAAxo2udPxl2EFvBnGWtN49C6lk8ENp112hBa8QIkxPi5syC5tzylzIhA4oArq zS+smYzeU79LFidIN3mjCcS+83SFykobvCvbJaRWODJj95if9vdH0YEEWoNo9TlK2bnc qm0Q== X-Received: by 10.180.91.234 with SMTP id ch10mr10485033wib.48.1408902559140; Sun, 24 Aug 2014 10:49:19 -0700 (PDT) Received: from [10.0.0.5] ([207.232.55.62]) by mx.google.com with ESMTPSA id la6sm30190541wjc.12.2014.08.24.10.49.17 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Sun, 24 Aug 2014 10:49:18 -0700 (PDT) Message-ID: <53FA259C.9050807@gmail.com> Date: Sun, 24 Aug 2014 20:49:16 +0300 From: Boaz Harrosh User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.7.0 MIME-Version: 1.0 To: Christoph Hellwig , linux-nfs@vger.kernel.org Subject: Re: [PATCH 03/19] pnfs: force a layout commit when encountering busy segments during recall References: <1408637375-11343-1-git-send-email-hch@lst.de> <1408637375-11343-4-git-send-email-hch@lst.de> In-Reply-To: <1408637375-11343-4-git-send-email-hch@lst.de> Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 08/21/2014 07:09 PM, Christoph Hellwig wrote: > Expedite layout recall processing by forcing a layout commit when > we see busy segments. Without it the layout recall might have to wait > until the VM decided to start writeback for the file, which can introduce > long delays. > > Signed-off-by: Christoph Hellwig Good god, Hi Christoph I've been sitting on client RECALL bugs over a year NOW. I have you scenario but actually a real DEAD-LOCK instead of an annoying delay. You have the same deadlock only it is harder for you to hit, with objects layout it is very easy to reproduce. (Files layout would have the same bug if it would support segments) The scenario is as follows: * Client is doing a LAYOUT_GET and is returned RECALL_CONFLICT Comment: If your server is serious about it's recalls, then all the while a recall is in progress it will return RECALL_CONFLICT on any segment in conflict with the RECALL. In objects layout this is easy to hit, because the LAYOUT_GET itself may cause the issue of the RECALL, because if the objects map grows do to the current LAYOUT_GET then all clients are RECALLed including the one issuing the call. But this can also happen when one client caused an operation that sends a RECALL on our client while our client is in the middle of issuing a LAYOUT_GET. So our client is stuck in LAYOUT_GET until RECALL from self is satisfied. * The RECALL is received but LAYOUTs are busy because they need a LAYOUTCOMMIT. ERR_DELAY is returned. Note the server will busy loop on RECALLs until success (NO_MATCHING_LAYOUT) * Ha ha. LAYOUTCOMMIT will never be called because our client is stuck inside LAYOUTGET, and we only call LAYOUTCOMMIT from update_inode() but LAYOUTGET is already in an update_inode and VFS will not concurrently call update_inode() twice, it will always wait for one to finish in order to notice the inode_dirty flag and issue a new one. So now we are dead-locked, LAYOUT_GET will wait for the Server to finish the RECALL, and will pole for LAYOUT. Server is stuck on Polling RECALL, waiting for the client to do a LO_COMMIT but this one will never happen because it is waiting for the LAYOUT_GET to return. * The way to try and solve this is like you did below by pushing an immediate LAYOUTCOMMIT as part of the recall thread and thous releasing the segments. I had a slight different solution though > --- > fs/nfs/callback_proc.c | 16 +++++++++++----- > fs/nfs/pnfs.c | 3 +++ > 2 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c > index 41db525..bf017b0 100644 > --- a/fs/nfs/callback_proc.c > +++ b/fs/nfs/callback_proc.c > @@ -164,6 +164,7 @@ static u32 initiate_file_draining(struct nfs_client *clp, > struct inode *ino; > struct pnfs_layout_hdr *lo; > u32 rv = NFS4ERR_NOMATCHING_LAYOUT; > + bool need_commit = false; > LIST_HEAD(free_me_list); > > lo = get_layout_by_fh(clp, &args->cbl_fh, &args->cbl_stateid); > @@ -172,16 +173,21 @@ static u32 initiate_file_draining(struct nfs_client *clp, > > ino = lo->plh_inode; > spin_lock(&ino->i_lock); > - if (test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags) || > - pnfs_mark_matching_lsegs_invalid(lo, &free_me_list, > - &args->cbl_range)) > + if (test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags)) { > rv = NFS4ERR_DELAY; > - else > - rv = NFS4ERR_NOMATCHING_LAYOUT; > + } else if (pnfs_mark_matching_lsegs_invalid(lo, &free_me_list, > + &args->cbl_range)) { > + need_commit = true; > + rv = NFS4ERR_DELAY; > + } > + > pnfs_set_layout_stateid(lo, &args->cbl_stateid, true); > spin_unlock(&ino->i_lock); > pnfs_free_lseg_list(&free_me_list); > pnfs_put_layout_hdr(lo); > + > + if (need_commit) > + pnfs_layoutcommit_inode(ino, false); > iput(ino); > out: > return rv; I did this like below: Comments: 1. I do the pnfs_layoutcommit_inode() regrdless of busy segments because if it has-nothing-to-do it returns right-away. Segments may be busy because of need-to-commit but also because they are used by in-flight-IO So busy segments are not an exact indication. In any way we can always do pnfs_layoutcommit_inode() to kick a LAYOUTCOMMIT it will never do any harm. 2. This has a performance advantage, any segments held by LAYOUTCOMMIT will now be freed, and the RECALL will return success instead of forcing the server to one or more RECALL rounds with ERR_DELAY. It is allowed by the protocol to issue a LAYOUTCOMMIT while in recall because RECALL is governed by the BACK-CHANNEL seq_id and LAYOUTCOMMIT by the for-channel seq_id and they need not wait for each other to finish. (Like for example LAYOUT_GET and LAYOUT_COMMIT which are serialized by the seq_id) > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > index 6e0fa71..242e73f 100644 > --- a/fs/nfs/pnfs.c > +++ b/fs/nfs/pnfs.c > @@ -604,6 +604,9 @@ pnfs_layout_free_bulk_destroy_list(struct list_head *layout_list, > spin_unlock(&inode->i_lock); > pnfs_free_lseg_list(&lseg_list); > pnfs_put_layout_hdr(lo); > + > + if (ret) > + pnfs_layoutcommit_inode(inode, false); > iput(inode); > } > return ret; > With My patch I could go farther on but hit some of the other stuff you have fixes for with the state_ids and other protocol stuff. Also with my patch I hit races in state management, because my patch waits for LAYOUT_COMMIT to execute synchronously from the RECALL thread, your patch of asynchronous LAYOUT_COMMIT has a lower chance of hitting. But I think Trond might have fixed these races, as I have tested this code like 6 month a go. If you are up to it you might want to test my synchronous way and see if you like things better. I'm testing your code as well to see how it looks. BTW: It looks like the hch-pnfs/getdeviceinfo has some of the pnfs fixes but that the hch-pnfs/blocklayout-for-3.18 has newer fixes but without the getdeviceinfo stuff. I'm testing with the older getdeviceinfo branch. [hch-pnfs == git://git.infradead.org/users/hch/pnfs.git] [Testing is not so easy because I need to merge in my pnfs-server as well as this here and I needed to do some forward porting as newest code was stuck on like 6 month ago. That was easy, now I need to go figure out what Ganesha to use. Kernel-pnfs-server is out of the question because it is stuck on 3.12 and will not merge very well with this here, But I'm stupid I can just run a 3.12 based Server, and this here as client, Ye I'll go do this tomorrow. See who gets stuck sooner Ganesha or Kpnfsd ] Thanks for working on this Boaz --- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c index 41db525..59f76bf 100644 --- a/fs/nfs/callback_proc.c +++ b/fs/nfs/callback_proc.c @@ -171,6 +171,14 @@ static u32 initiate_file_draining(struct nfs_client *clp, goto out; ino = lo->plh_inode; + + spin_lock(&ino->i_lock); + pnfs_set_layout_stateid(lo, &args->cbl_stateid, true); + spin_unlock(&ino->i_lock); + + /* kick out any segs held by need to commit */ + pnfs_layoutcommit_inode(ino, true); + spin_lock(&ino->i_lock); if (test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags) || pnfs_mark_matching_lsegs_invalid(lo, &free_me_list, @@ -178,7 +186,7 @@ static u32 initiate_file_draining(struct nfs_client *clp, rv = NFS4ERR_DELAY; else rv = NFS4ERR_NOMATCHING_LAYOUT; - pnfs_set_layout_stateid(lo, &args->cbl_stateid, true); spin_unlock(&ino->i_lock); pnfs_free_lseg_list(&free_me_list); pnfs_put_layout_hdr(lo);