From patchwork Wed Mar 30 21:02:56 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Olga Kornievskaia X-Patchwork-Id: 8705221 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 998029F44D for ; Wed, 30 Mar 2016 21:03:02 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 90EE12035E for ; Wed, 30 Mar 2016 21:03:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 658BE20253 for ; Wed, 30 Mar 2016 21:02:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752701AbcC3VC6 (ORCPT ); Wed, 30 Mar 2016 17:02:58 -0400 Received: from mail-io0-f171.google.com ([209.85.223.171]:33486 "EHLO mail-io0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751156AbcC3VC5 (ORCPT ); Wed, 30 Mar 2016 17:02:57 -0400 Received: by mail-io0-f171.google.com with SMTP id a129so83311281ioe.0 for ; Wed, 30 Mar 2016 14:02:57 -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; bh=cZcYK7KUs56rT1hAfHlloDM0yNHnqsXG7IhBLKdPvzQ=; b=OZOv52EoetjmKyeN4kHjPuNEvHGBQo6YzMPZl1T29PN5IaGFHSLlxdmcmxLRb/pVpp l3oYvB13cD9xFUEojRhMh4a6k4qcvgCXf0OLcMKJtC0KvYmRs7QzVXTmj65X9wXYMORi QV3TEAmP9DzxMxa1ZNHi2wA8oqUktIlThydt97z96ms8upOEI0QAxTohY54knVGzwIx1 RxTcOaxcijVvF8poKOEFdW4U7AOEvaukYEOCB24+0XXsZ6khm2aT8nol6uw3AVA0Bbxm G7g27eCTCPrt1GWf+BJRGiLUt4pSsNGBM3JmHZ1K22HmBXDYV6sCMMmupGWctpYKISbd 9Vkw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=umich.edu; s=google; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to; bh=cZcYK7KUs56rT1hAfHlloDM0yNHnqsXG7IhBLKdPvzQ=; b=IitARYNco/GWBftPHk5iMqj/UyXxacOkKvOP5wrW3IRubo32eMkk1Qm6dBYtLsanAa Y0iIsiAHO/GFf5HTiMeirCSZlZ44HgnkqrCzf3L3Q82g55vEfiX6UlH9gtBJxjbbswc7 Hc0v1jxGAwzsd+j5go60fII/eADeAakRyQtRQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:date :message-id:subject:from:to; bh=cZcYK7KUs56rT1hAfHlloDM0yNHnqsXG7IhBLKdPvzQ=; b=Rg3wuMF4hAV7Vd3rCDyndQL7z0n+z60eyQqg2UI4rJkn9LE4b4OzEZ/bGvo7m6Ii83 QXYLAuikCcLLZP1ENUpywtO+te/BP75qrxh3ipFbIbRZa5z6KMQ8nWyQ7lwk5xBanrIP zPJIqyaDTfHKyVne5QndK7HCcWO+ZGzsbfh5DoPsH7+2pZDqBHpGGOWDam1ltIbqfIKV EdlkEJZLGNTfwuEpa/HtpaAn6c8h/FaQSYTphU76jpbW4yBZlyu+FFQBakw0O+RlJzAB KEcINJNh9JxyGCKJz3aAU7nOq5+ZyPxD0W0WyTTB0zC45zxUjqfjwSViUurXaGnzg6Yf 2Ziw== X-Gm-Message-State: AD7BkJJt2ITGWOWq/nZaocGiI+lUMImFdqdR0o40rXiEZJdq8sATylvr6XIqnUDthFLa4o9T40VuSQzsL2v6DA== MIME-Version: 1.0 X-Received: by 10.107.128.137 with SMTP id k9mr910653ioi.26.1459371776564; Wed, 30 Mar 2016 14:02:56 -0700 (PDT) Received: by 10.107.8.200 with HTTP; Wed, 30 Mar 2016 14:02:56 -0700 (PDT) In-Reply-To: References: Date: Wed, 30 Mar 2016 17:02:56 -0400 X-Google-Sender-Auth: _2zh_6hGqa60lX3_xmh5VjttQcY Message-ID: Subject: Re: commit 7c2dad99d6 "Don't let the ctime override attribute barriers" From: Olga Kornievskaia To: Trond Myklebust , 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=-7.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,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, Mar 29, 2016 at 3:47 PM, Olga Kornievskaia wrote: > I think that patch introduces a problem. Since the checking for the > change in ctime was removed by the commit it leads to (improper) cache > invalidation in NFSv3. > > Test is write 10240bytes to the server then read it. Expectation is > not to see read on the wire. In the test the write is spread over > 3rpcs. > > On the 1nd reply > fattr->gencount=33 nfsi->gencount=32 generation_counter=35 > On the 2nd reply > fattr->gencount=34 nfsi->gencount=36 generation_counter=36 > > In the code when processing 2nd reply, > nfs_post_op_update_inode_force_wcc_locked() calls into > nfs_inode_attrs_need_update() it determines that it doesn't need to > update them (even though the size and the time have changed). so it > doesn't call nfs_wcc_update_inode() so the inode->i_version doesn't > get set to the ctime that was received in the 2nd reply. > > On the 3rd reply > fattr->gencount=37 nfsi->gencount=36 generation_counter=37 > > It leads to nfs_inode_attrs_need_update() returns 1 and in the > nfs_update_inode() the difference in the ctimes leads to invalidation. > fattr->gencount was update from nfs_writeback_update_node() -> > nfs_post_op_update_inode_force_wcc() calling nfs_fattr_set_barrier(). > > I'm not sure what appropriate values for "gencount" should have been. > But if the check for nfs_ctime_need_update() was still there in > nfs_inode_attrs_need_update() then the 2nd reply would have > appropriately updated the i_version and not lead to invalidation. Would like to add that this problem is not seen against the Linux server because it doesn't send "before" attributes. So code doesn't set the "pre_change_attr" which later doesn't make what's stored in inode->i_version. The problem also not seen for v4 because pre_change_attr is not gotten from the "before" attributes but instead from the previous value in inode->i_version which is then compared to the itself. If reverting the problematic commit is not the solution, then how about ignoring the "before" ctime attributes sent by the server. This also helps with the out-of-order RPCs. out_overflow: --- 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/nfs3xdr.c b/fs/nfs/nfs3xdr.c index 267126d..feca07f 100644 --- a/fs/nfs/nfs3xdr.c +++ b/fs/nfs/nfs3xdr.c @@ -732,14 +732,12 @@ static int decode_wcc_attr(struct xdr_stream *xdr, struct nfs_fattr *fattr) goto out_overflow; fattr->valid |= NFS_ATTR_FATTR_PRESIZE - | NFS_ATTR_FATTR_PRECHANGE | NFS_ATTR_FATTR_PREMTIME | NFS_ATTR_FATTR_PRECTIME; p = xdr_decode_size3(p, &fattr->pre_size); p = xdr_decode_nfstime3(p, &fattr->pre_mtime); xdr_decode_nfstime3(p, &fattr->pre_ctime); - fattr->pre_change_attr = nfs_timespec_to_change_attr(&fattr->pre_ctime); return 0;