From patchwork Thu Oct 16 18:43:48 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Olga Kornievskaia X-Patchwork-Id: 5093591 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 3231FC11AC for ; Thu, 16 Oct 2014 18:43:54 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 1B879201CD for ; Thu, 16 Oct 2014 18:43:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C017820166 for ; Thu, 16 Oct 2014 18:43:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751696AbaJPSnu (ORCPT ); Thu, 16 Oct 2014 14:43:50 -0400 Received: from mail-ig0-f176.google.com ([209.85.213.176]:60768 "EHLO mail-ig0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751633AbaJPSnt (ORCPT ); Thu, 16 Oct 2014 14:43:49 -0400 Received: by mail-ig0-f176.google.com with SMTP id hn15so184693igb.3 for ; Thu, 16 Oct 2014 11:43:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=6sBkRgAraG8aWw3ZW09NLMfjZuJS0WtiVwETMWy0/Zc=; b=GMUaHTA+XPiqsnYMkSjgQ6An3CbqNoPxGeqlznPxbcuicj8KW2uczL18VE7C8pfzyq jKIREMd70ILWvJm7HOfaVhUbDGhk8tm3Nmfx+78S5eTJkfasw8o8WTVbpekH9u69GJpZ 2WgJYw627Vv0+k2P6u0xbmPzEl97kA4SIH+8+D+r0i9G9L0BFYWKsgb1EOatU5W5HRh/ x6Nk6oyL/NXwLMfM7Iz/kSbkjVfkLo486TYg+rK8+bB0yDl9mYQjPvlxyVVWKbsrfOt8 2/OF3GcZRkFB48Dsjx15JVKxap7+n4lUIQR92p2PsJwDkFc+ivUtt5JG1paP3NuCZMku CwlA== MIME-Version: 1.0 X-Received: by 10.50.154.97 with SMTP id vn1mr7145280igb.13.1413485029051; Thu, 16 Oct 2014 11:43:49 -0700 (PDT) Received: by 10.107.134.80 with HTTP; Thu, 16 Oct 2014 11:43:48 -0700 (PDT) In-Reply-To: References: Date: Thu, 16 Oct 2014 14:43:48 -0400 X-Google-Sender-Auth: bc4-g4Pq_5ZyqfaiHeaObXTb_tw Message-ID: Subject: Re: how to properly handle failures during delegation recall process From: Olga Kornievskaia To: Trond Myklebust Cc: linux-nfs 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_SIGNED, 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 Tue, Oct 14, 2014 at 11:48 AM, Olga Kornievskaia wrote: > On Mon, Oct 13, 2014 at 6:24 PM, Trond Myklebust > wrote: >> On Mon, Oct 13, 2014 at 6:13 PM, Olga Kornievskaia wrote: >>> On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust >>> wrote: >>>> On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust >>>> wrote: >>>>> On Mon, Oct 13, 2014 at 4:23 PM, Olga Kornievskaia wrote: >>>>>> On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust >>>>>> wrote: >>>>>>> On Mon, Oct 13, 2014 at 2:51 PM, Olga Kornievskaia wrote: >>>>>>>> + } >>>>>>>> + } >>>>>>>> return nfs4_handle_delegation_recall_error(server, state, stateid, err); >>>>>>>> } >>>>>>>> >>>>>>>> @@ -5957,6 +5973,10 @@ int nfs4_lock_delegation_recall(struct >>>>>>>> file_lock *fl, struct nfs4_state *state, >>>>>>>> err = nfs4_set_lock_state(state, fl); >>>>>>>> if (err != 0) >>>>>>>> return err; >>>>>>>> + if (test_bit(NFS_LOCK_LOST, &fl->fl_u.nfs4_fl.owner->ls_flags)) { >>>>>>>> + pr_warn_ratelimited("NFS: %s: Lock reclaim failed!\n", >>>>>>>> __func__); >>>>>>>> + return -EIO; >>>>>>>> + } >>>>>>>> err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW); >>>>>>> >>>>>>> Note that there is a potential race here if the server is playing with >>>>>>> NFS4ERR_DELEG_REVOKED or NFS4ERR_ADMIN_REVOKED. Since we do not >>>>>>> present the delegation as part of the LOCK request, we have no way of >>>>>>> knowing if the delegation is still in effect. I guess we can check the >>>>>>> return value of DELEGRETURN, but if it returns NFS4ERR_DELEG_REVOKED >>>>>>> or NFS4ERR_ADMIN_REVOKED, then we still do not know whether or not the >>>>>>> LOCK is safe. >>>>>> >>>>>> I'm not following you. We send LOCK before we send DELEGRETURN? Also, >>>>>> we normally send in LOCK the open_stateid returned by the open with >>>>>> cur so do we know that delegation is still in effect. >>>>> >>>>> How so? The open stateid doesn't tell you that the delegation is still >>>>> in effect. If the DELEGRETURN returns NFS4ERR_DELEG_REVOKED, how can >>>>> you tell if the delegation was revoked before or after the LOCK >>>>> request was handled? >>>> >>>> Actually, let me answer that myself. You can sort of figure things out >>>> in NFSv4.1 if you look at the SEQ4_STATUS_RECALLABLE_STATE_REVOKED >>>> flag. If it is set, you should probably distrust the lock stateid that >>>> you just attempted to recover, since you now know that at least one >>>> delegation was just revoked. >>>> >>>> In that case, we probably also have to do a TEST_STATEID+FREE_STATEID >>>> on the delegation stateid. >>> >>> I think we are mis-communicating here by talking about different >>> nuances. I agree with you that when an operation is sent there is no >>> way of knowing if in the mean while the server has decided to revoke >>> the delegation. However, this is not what I'm confused about regarding >>> your comment. I'm noticing that in the flow of operations, we send (1) >>> open with cur, then (2) lock, then (3) delegreturn. So I was confused >>> about how can we check return of delegreturn, step 3, if we are in >>> step 2. >>> >>> I think the LOCK is safe if the reply to the LOCK is successful. >> >> It needs to be concurrent with the delegation as well, otherwise >> general lock atomicity is broken. >> >>> Let me just step back from this to note that your solution to "lost >>> locks during delegation" is to recognize the open with cure failure >>> and skip locking and delegreturn. I can work on a patch for that. >>> >>> Do you agree that the state recovery should not be initiated in case >>> we get those errors? >> >> State recovery _should_ be initiated so that we do reclaim opens (I >> don't care about share lock atomicity on Linux). However >> nfs_delegation_claim_locks() _should_not_ be called. >> >> I'll give some more thought as to how we can ensure the missing lock atomicity. > > If state recover is initiated, it will go into an infinite loop. There > is no way to stop it once it's initiated. A "recovery" open will get a > BAD_STATEID which will again initiated state recovery. > > Are proposing to add smarts to the state manager when it should not > recover from a BAD_STATEID? Ok. How about something like this? [PATCH 1/1] Fixing infinite state recovery loop due to failed delegation return If we get a bad-stateid-type of error when we send OPEN with delegate_cur to return currently held delegation, we shouldn't be trying to reclaim locks associated with that delegation state_id because we don't have an open_stateid to be used for the LOCK operation. Thus, we should return an error from the nfs4_open_delegation_recall() in that case. Furthermore, if an error occurs the delegation code will call nfs_abort_delegation_return() which sets again the NFS4CLNT_DELEGRETURN flags in the state and it leads the state manager to into an infinite loop for trying to reclaim the delegated state. Signed-off-by: Olga Kornievskaia --- fs/nfs/delegation.c | 5 +++-- fs/nfs/nfs4proc.c | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) set_bit(NFS_DELEGATED_STATE, &state->flags); -- 1.7.1 > >> >> -- >> Trond Myklebust >> >> Linux NFS client maintainer, PrimaryData >> >> trond.myklebust@primarydata.com -- 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/delegation.c b/fs/nfs/delegation.c index 5853f53..8016d89 100644 --- a/fs/nfs/delegation.c +++ b/fs/nfs/delegation.c @@ -394,7 +394,7 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation err = nfs4_wait_clnt_recover(clp); } while (err == 0); - if (err) { + if (err && err != -EIO) { nfs_abort_delegation_return(delegation, clp); goto out; } @@ -458,7 +458,8 @@ restart: iput(inode); if (!err) goto restart; - set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state); + if (err != -EIO) + set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state); return err; } } diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 5aa55c1..6871055 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -1655,7 +1655,7 @@ static int nfs4_handle_delegation_recall_error(struct nfs_server *server, struct nfs_inode_find_state_and_recover(state->inode, stateid); nfs4_schedule_stateid_recovery(server, state); - return 0; + return -EIO; case -NFS4ERR_DELAY: case -NFS4ERR_GRACE: