Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] nfs: Fix nfsi->nrequests count error on
 nfs_inode_remove_request
To:     Trond Myklebust <trondmy@hammerspace.com>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "anna.schumaker@netapp.com" <anna.schumaker@netapp.com>
References: <1569479378-128669-1-git-send-email-zhangxiaoxu5@huawei.com>
 <08ce0101-e8df-509a-f3e5-07063aa5492e@huawei.com>
 <5c50a4be3562877a5d96523e943b9976a3792e23.camel@hammerspace.com>
 <cb955e718bd3c62f9c23661e95db77efa65d7177.camel@hammerspace.com>
From:   "zhangxiaoxu (A)" <zhangxiaoxu5@huawei.com>
Message-ID: <4096d9e4-f6a2-b1e1-4f52-81cf44fc4a1f@huawei.com>
Date:   Tue, 8 Oct 2019 10:00:07 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.1.1
MIME-Version: 1.0
In-Reply-To: <cb955e718bd3c62f9c23661e95db77efa65d7177.camel@hammerspace.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Thanks for your review.

I think PG_REMOVE and PG_INODE_REF can't be both setted except in
'nfs_inode_remove_request' function.

nfs_inode_remove_request
	// maybe set PG_REMOVE here.
	nfs_page_group_sync_on_bit(req, PG_REMOVE)
		nfs_page_group_lock(req);
		nfs_page_group_sync_on_bit_locked
			WARN_ON_ONCE(test_and_set_bit(bit, &req->wb_flags));
		nfs_page_group_unlock(req);
	// But also clear the PG_INODE_REF flag.
	test_and_clear_bit(PG_INODE_REF, &req->wb_flags)

'nfs_lock_and_join_requests' also need the PG_HEADLOCK flag:

nfs_lock_and_join_requests
	nfs_page_group_lock(head);
		test_and_clear_bit(PG_REMOVE, &head->wb_flags)
	nfs_page_group_unlock(head);


在 2019/10/5 22:35, Trond Myklebust 写道:
> However nfs_lock_and_join_requests() looks like it does need to change
> to something like the following:
> 
>          /* Postpone destruction of this request */
> -       if (test_and_clear_bit(PG_REMOVE, &head->wb_flags)) {
> -               set_bit(PG_INODE_REF, &head->wb_flags);
> +       if (test_and_clear_bit(PG_REMOVE, &head->wb_flags) &&
> +           !test_and_set_bit(PG_INODE_REF, &head->wb_flags)) {
>                  kref_get(&head->wb_kref);
>                  atomic_long_inc(&NFS_I(inode)->nrequests);
>          }
> 
> 
> Do you agree?