Return-Path: Received: from mail-io0-f169.google.com ([209.85.223.169]:35213 "EHLO mail-io0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751778AbbIQNgd convert rfc822-to-8bit (ORCPT ); Thu, 17 Sep 2015 09:36:33 -0400 Received: by ioiz6 with SMTP id z6so21761869ioi.2 for ; Thu, 17 Sep 2015 06:36:33 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: Date: Thu, 17 Sep 2015 09:36:33 -0400 Message-ID: Subject: Re: Failing to send a CLOSE if file is opened WRONLY and server reboots on a 4.x mount From: Olga Kornievskaia To: Trond Myklebust Cc: linux-nfs Content-Type: text/plain; charset=UTF-8 Sender: linux-nfs-owner@vger.kernel.org List-ID: On Thu, Sep 17, 2015 at 9:34 AM, Trond Myklebust wrote: > On Tue, Sep 15, 2015 at 12:52 PM, Olga Kornievskaia wrote: >> On Tue, Sep 15, 2015 at 11:49 AM, Trond Myklebust >> wrote: >>> On Tue, Sep 15, 2015 at 10:27 AM, Olga Kornievskaia wrote: >>>> On Tue, Sep 15, 2015 at 9:39 AM, Trond Myklebust >>>> wrote: >>>>> On Mon, Sep 14, 2015 at 7:54 PM, Olga Kornievskaia wrote: >>>>>> A test case is as the description says: >>>>>> open(foobar, O_WRONLY); >>>>>> sleep() --> reboot the server >>>>>> close(foobar) >>>>>> >>>>>> The bug is because in nfs4state.c in nfs4_reclaim_open_state() a few >>>>>> line before going to restart, there is >>>>>> clear_bit(NFS4CLNT_RECLAIM_NOGRACE, &state->flags). >>>>>> >>>>>> NFS4CLNT_RECLAIM_NOGRACE is a flag for the client states not open >>>>>> owner states. Value of NFS4CLNT_RECLAIM_NOGRACE is 4 which is the >>>>>> value of NFS_O_WRONLY_STATE in nfs4_state->flags. So clearing it wipes >>>>>> out state and when we go to close it, “call_close” doesn’t get set as >>>>>> state flag is not set and CLOSE doesn’t go on the wire. >>>>>> >>>>>> That line was introduced to fix an infinite loop for OPEN recovery >>>>>> upon receiving a BAD_STATEID error: commit e8d975e73. I have tested >>>>>> injecting BAD_STATEID error using the patch below and the code >>>>>> recovers without problems. However, I'm not sure the clearing of the >>>>>> bit is needed any more. I have tested for infinite loop by reverting >>>>>> the patch and didn't hit the infinite loop. >>>>>> >>>>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c >>>>>> index da73bc4..5db3246 100644 >>>>>> --- a/fs/nfs/nfs4state.c >>>>>> +++ b/fs/nfs/nfs4state.c >>>>>> @@ -1481,7 +1481,7 @@ restart: >>>>>> spin_unlock(&state->state_lock); >>>>>> } >>>>>> nfs4_put_open_state(state); >>>>>> - clear_bit(NFS4CLNT_RECLAIM_NOGRACE, >>>>>> + clear_bit(NFS_STATE_RECLAIM_NOGRACE, >>>>>> &state->flags); >>>>>> spin_lock(&sp->so_lock); >>>>>> goto restart; >>>>> >>>>> That's an obvious typo. Thanks for spotting it! >>>>> >>>>> As for whether or not the bit clear is needed at all, I think it is >>>>> for NFSv4 on older kernels. On newer kernels, we do have the NFSv4 >>>>> state recovery drain the slot table (just like we've always done for >>>>> NFSv4.1) and so I agree that those kernels probably won't be >>>>> afflicted. >>>>> >>>> >>>> Thanks Trond. Do you need me to resubmit it without the last paragraph >>>> or is the patch ok as is? >>>> >>> >>> I can easily remove that paragraph when applying the patch, if you >>> agree that it is superfluous. >> >> Thanks. Works for me. >> > > May I also add a signed-off-by line from you? I can't really apply > this (or any other patches) without it. Of course. > > Cheers > Trond > -- > 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