From patchwork Mon Mar 2 22:50:45 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Olga Kornievskaia X-Patchwork-Id: 5917951 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.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 27571BF440 for ; Mon, 2 Mar 2015 22:50:49 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id DD44620221 for ; Mon, 2 Mar 2015 22:50:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A1DAF20219 for ; Mon, 2 Mar 2015 22:50:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754057AbbCBWuq (ORCPT ); Mon, 2 Mar 2015 17:50:46 -0500 Received: from mail-ie0-f178.google.com ([209.85.223.178]:43159 "EHLO mail-ie0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753924AbbCBWup (ORCPT ); Mon, 2 Mar 2015 17:50:45 -0500 Received: by iebtr6 with SMTP id tr6so52233158ieb.10 for ; Mon, 02 Mar 2015 14:50:45 -0800 (PST) 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=+t3erZYRs8fKUpG78sfj0TC873XqaeJW7/vBDfsz37w=; b=vkMpamxajtefFwECySJQTsAicyIBDhUAvZcZsuvDKOLNGwD7lUExVMgzv92IEUcAsF NG/DmJ3rRzhj9fxQsIX963qarH7R9BEpCLPlWDnyg50Re8mVO8kg0vB96+zzx+dycu0O 2iBusKSOJgyN/+sJ+CqtXNnnS3Q8R1bsZkfee8U8mnDe7tSvifx0JEqxKZzxo1JDyTyI DAhXlMIN0jgjSfeole/fr5KWBgkp65fpj1jiJW0ldiYLJv3RaHX0q5MbOa4t+odXSEvg +bUIFnYRmRHnG3p9KYlEF4fN6AIyicnYk+ZOISfHrctJQ+Q13pvrZqSgGj/fzqNClDvb A3lQ== MIME-Version: 1.0 X-Received: by 10.107.11.17 with SMTP id v17mr39547538ioi.74.1425336645217; Mon, 02 Mar 2015 14:50:45 -0800 (PST) Received: by 10.107.157.73 with HTTP; Mon, 2 Mar 2015 14:50:45 -0800 (PST) In-Reply-To: References: Date: Mon, 2 Mar 2015 17:50:45 -0500 X-Google-Sender-Auth: OtzVVyBvb9G5S4kjuIXTk-eRJnY Message-ID: Subject: Re: buggy CLOSE in the "testing" branch From: Olga Kornievskaia To: Trond Myklebust Cc: Anna Schumaker , 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 Mon, Mar 2, 2015 at 5:29 PM, Trond Myklebust wrote: > On Mon, Mar 2, 2015 at 5:01 PM, Olga Kornievskaia wrote: >> On Mon, Mar 2, 2015 at 3:53 PM, Olga Kornievskaia wrote: >>> On Mon, Mar 2, 2015 at 3:47 PM, Trond Myklebust >>> wrote: >>>> Hi Olga, >>>> >>>> On Mon, Mar 2, 2015 at 3:15 PM, Olga Kornievskaia wrote: >>>>> Hi folks, >>>>> >>>>> I'm experiencing that CLOSE uses a delegation stateid instead of the >>>>> open stateid which I think is what the spec says. Server replies with >>>>> BAD_STATEID. >>>>> >>>>> Is this a bug or did I misread the spec? Thanks. >>>>> >>>> >>>> That would be a client bug. Do you have a reproducer? >>> >>> Yep. Just cat a (2nd) file after mount (i.e.., a file needs to have a >>> delegation). A CLOSE will use a delegation stateid. Problem is seenl >>> on the network trace. It will also leads to failure on unmount with >>> CLIENTID_BUSY because there is still an open state that client never >>> released. Please note that both "cat" and "unmount" will "succeed" >>> from the user's perspective. Thus, unless testing also looks at the >>> network trace, this failure will never be caught. >> >> Anna pointed me at the commit 566fcec60. It seems to be that's what >> broke it as it removed the use of openstateid for the stateid arg. But >> I really don't understand the necessity of the patch. CLOSE must >> always use the openstateid. Therefore it doesn't need to worry that >> stateid is changed from openstateid to delegation stateid or locking >> stateid. It should just use the openstateid as it did before. >> > > Doh! Yes, the change to ->stateid is wrong. I must have been on borken > autopilot... > > The reason for the patch itself is to ensure the seqid hasn't changed. > It has nothing to do with delegation stateids or locking. > Can either one of you please send a patch to fix up 566fcec60? Please > note that we also need to change the comparison in nfs4_close_done to > match the copy in nfs4_close_prepare. > Something like works (btw this is on top of nfs-for-next which also has that problem): After 566fcec60 the client uses the "current stateid" from the nfs4_state structure to close a file. This could potentially contain a delegation stateid, which is disallowed by the protocol and causes servers to return NFS4ERR_BAD_STATEID. This patch restores the (correct) behavior of sending the open stateid to close a file. Reported-by: Olga Kornievskaia Fixes: 566fcec60 (NFSv4: Fix an atomicity problem in CLOSE) Signed-off-by: Anna Schumaker > -- > 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/nfs4proc.c b/fs/nfs/nfs4proc.c index a211daf..732526e 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -2655,7 +2655,7 @@ static void nfs4_close_done(struct rpc_task *task, void *d case -NFS4ERR_BAD_STATEID: case -NFS4ERR_EXPIRED: if (!nfs4_stateid_match(&calldata->arg.stateid, - &state->stateid)) { + &state->open_stateid)) { rpc_restart_call_prepare(task); goto out_release; } @@ -2691,7 +2691,7 @@ static void nfs4_close_prepare(struct rpc_task *task, void is_rdwr = test_bit(NFS_O_RDWR_STATE, &state->flags); is_rdonly = test_bit(NFS_O_RDONLY_STATE, &state->flags); is_wronly = test_bit(NFS_O_WRONLY_STATE, &state->flags); - nfs4_stateid_copy(&calldata->arg.stateid, &state->stateid); + nfs4_stateid_copy(&calldata->arg.stateid, &state->open_stateid); /* Calculate the change in open mode */ calldata->arg.fmode = 0; if (state->n_rdwr == 0) {