Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
From: Trond Myklebust <trondmy@hammerspace.com>
To: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	"jencce.kernel@gmail.com" <jencce.kernel@gmail.com>
Subject: Re: [PATCH] NFSv4: fix stateid refreshing when CLOSE racing with OPEN
Date: Thu, 10 Oct 2019 14:46:40 +0000
Message-ID: <f81d80f09c59d78c32fddd535b5604bc05c2a2b5.camel@hammerspace.com> (raw)
In-Reply-To: <20191010074020.o2uwtuyegtmfdlze@XZHOUW.usersys.redhat.com>

On Thu, 2019-10-10 at 15:40 +0800, Murphy Zhou wrote:
> Since commit:
>   [0e0cb35] NFSv4: Handle NFS4ERR_OLD_STATEID in CLOSE/OPEN_DOWNGRADE
> 
> xfstests generic/168 on v4.2 starts to fail because reflink call
> gets:
>   +XFS_IOC_CLONE_RANGE: Resource temporarily unavailable
> 
> In tshark output, NFS4ERR_OLD_STATEID stands out when comparing with
> good ones:
> 
>  5210   NFS 406 V4 Reply (Call In 5209) OPEN StateID: 0xadb5
>  5211   NFS 314 V4 Call GETATTR FH: 0x8d44a6b1
>  5212   NFS 250 V4 Reply (Call In 5211) GETATTR
>  5213   NFS 314 V4 Call GETATTR FH: 0x8d44a6b1
>  5214   NFS 250 V4 Reply (Call In 5213) GETATTR
>  5216   NFS 422 V4 Call WRITE StateID: 0xa818 Offset: 851968 Len:
> 65536
>  5218   NFS 266 V4 Reply (Call In 5216) WRITE
>  5219   NFS 382 V4 Call OPEN DH: 0x8d44a6b1/
>  5220   NFS 338 V4 Call CLOSE StateID: 0xadb5
>  5222   NFS 406 V4 Reply (Call In 5219) OPEN StateID: 0xa342
>  5223   NFS 250 V4 Reply (Call In 5220) CLOSE Status:
> NFS4ERR_OLD_STATEID
>  5225   NFS 338 V4 Call CLOSE StateID: 0xa342
>  5226   NFS 314 V4 Call GETATTR FH: 0x8d44a6b1
>  5227   NFS 266 V4 Reply (Call In 5225) CLOSE
>  5228   NFS 250 V4 Reply (Call In 5226) GETATTR
> 
> It's easy to reproduce. By printing some logs, found that we are
> making
> CLOSE seqid larger then OPEN seqid when racing.
> 
> Fix this by not bumping seqid when it's equal to OPEN seqid. Also
> put the whole changing process into seqlock read protection in case
> really bad luck, and this is the same locking behavior with the
> old deleted function.
> 
> Fixes: 0e0cb35b417f ("NFSv4: Handle NFS4ERR_OLD_STATEID in
> CLOSE/OPEN_DOWNGRADE")
> Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
> ---
>  fs/nfs/nfs4proc.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 11eafcf..6db5a09 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -3334,12 +3334,13 @@ static void
> nfs4_sync_open_stateid(nfs4_stateid *dst,
>  			break;
>  		}
>  		seqid_open = state->open_stateid.seqid;
> -		if (read_seqretry(&state->seqlock, seq))
> -			continue;
>  
>  		dst_seqid = be32_to_cpu(dst->seqid);
>  		if ((s32)(dst_seqid - be32_to_cpu(seqid_open)) < 0)
>  			dst->seqid = seqid_open;
> +
> +		if (read_seqretry(&state->seqlock, seq))
> +			continue;

What's the intention of this change? Neither dst_seqid nor dst->seqid
are protected by the state->seqlock so why move this code under the
lock.

>  		break;
>  	}
>  }
> @@ -3367,14 +3368,16 @@ static bool
> nfs4_refresh_open_old_stateid(nfs4_stateid *dst,
>  			break;
>  		}
>  		seqid_open = state->open_stateid.seqid;
> -		if (read_seqretry(&state->seqlock, seq))
> -			continue;
>  
>  		dst_seqid = be32_to_cpu(dst->seqid);
> -		if ((s32)(dst_seqid - be32_to_cpu(seqid_open)) >= 0)
> +		if ((s32)(dst_seqid - be32_to_cpu(seqid_open)) > 0)
>  			dst->seqid = cpu_to_be32(dst_seqid + 1);

This negates the whole intention of the patch you reference in the
'Fixes:', which was to allow us to CLOSE files even if seqid bumps have
been lost due to interrupted RPC calls e.g. when using 'soft' or
'softerr' mounts.
With the above change, the check could just be tossed out altogether,
because dst_seqid will never become larger than seqid_open.

>  		else
>  			dst->seqid = seqid_open;
> +
> +		if (read_seqretry(&state->seqlock, seq))
> +			continue;
> +

Again, why this change to code that doesn't appear to need protection?
If the bump does succeed above, then looping back will actually cause
unpredictable behaviour.

>  		ret = true;
>  		break;
>  	}
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com



  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-10  7:40 Murphy Zhou
2019-10-10 14:46 ` Trond Myklebust [this message]
2019-10-11  8:49   ` Murphy Zhou
2019-10-11 14:14     ` Trond Myklebust
2019-10-10 17:32 ` Olga Kornievskaia
2019-10-11  9:42   ` Murphy Zhou
2019-10-11 14:18   ` Trond Myklebust
2019-10-11 18:50     ` Olga Kornievskaia
2019-10-19  0:34       ` Olga Kornievskaia

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f81d80f09c59d78c32fddd535b5604bc05c2a2b5.camel@hammerspace.com \
    --to=trondmy@hammerspace.com \
    --cc=jencce.kernel@gmail.com \
    --cc=linux-nfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org linux-nfs@archiver.kernel.org
	public-inbox-index linux-nfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox