From:   "Benjamin Coddington" <bcodding@redhat.com>
To:     "Trond Myklebust" <trondmy@hammerspace.com>
Cc:     anna.schumaker@netapp.com, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 1/3] NFSv42: Fix pagecache invalidation after COPY/CLONE
Date:   Tue, 16 Nov 2021 09:06:32 -0500
Message-ID: <01F85A5B-483C-41B4-8D3E-0CD6209232C6@redhat.com>
In-Reply-To: <EE06FD52-D67E-4792-8512-AEF97E8D8570@redhat.com>
References: <cover.1637069577.git.bcodding@redhat.com>
 <8b8ccdb69af2473eef4a36968894e7aee34d5851.1637069577.git.bcodding@redhat.com>
 <6dbcb2f39dcee7314bf772aaabf4923104ec7aee.camel@hammerspace.com>
 <EE06FD52-D67E-4792-8512-AEF97E8D8570@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 16 Nov 2021, at 9:01, Benjamin Coddington wrote:

> On 16 Nov 2021, at 8:57, Trond Myklebust wrote:
>
>> On Tue, 2021-11-16 at 08:49 -0500, Benjamin Coddington wrote:
>>> The mechanism in use to allow the client to see the results of
>>> COPY/CLONE
>>> is to drop those pages from the pagecache.  This forces the client 
>>> to
>>> read
>>> those pages once more from the server.  However,
>>> truncate_pagecache_range()
>>> zeros out partial pages instead of dropping them.  Let us instead 
>>> use
>>> invalidate_inode_pages2_range() with full-page offsets to ensure the
>>> client
>>> properly sees the results of COPY/CLONE operations.
>>>
>>> Cc: <stable@vger.kernel.org> # v4.7+
>>> Fixes: 2e72448b07dc ("NFS: Add COPY nfs operation")
>>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>>> ---
>>>  fs/nfs/nfs42proc.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
>>> index a24349512ffe..bbcd4c80c5a6 100644
>>> --- a/fs/nfs/nfs42proc.c
>>> +++ b/fs/nfs/nfs42proc.c
>>> @@ -285,7 +285,10 @@ static void nfs42_copy_dest_done(struct inode
>>> *inode, loff_t pos, loff_t len)
>>>         loff_t newsize = pos + len;
>>>         loff_t end = newsize - 1;
>>>  
>>> -       truncate_pagecache_range(inode, pos, end);
>>> +       int error = 
>>> invalidate_inode_pages2_range(inode->i_mapping,
>>> +                               pos 
>>> >> PAGE_SHIFT, end >>
>>> PAGE_SHIFT);
>>
>> Shouldn't that be "(end + PAGE_SIZE - 1) >> PAGE_SHIFT" in order to
>> align to the set of pages that fully contains the byte range from pos
>> to end?
>
> It's embarrassing that I've messed that up, I will resend it.

I've had it sitting around a bit too long -- on a second look no, it 
should
be right because invalidate_inode_pages2_range()'s index arguments are
inclusive.

Ben