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:01:50 -0500
Message-ID: <EE06FD52-D67E-4792-8512-AEF97E8D8570@redhat.com>
In-Reply-To: <6dbcb2f39dcee7314bf772aaabf4923104ec7aee.camel@hammerspace.com>
References: <cover.1637069577.git.bcodding@redhat.com>
 <8b8ccdb69af2473eef4a36968894e7aee34d5851.1637069577.git.bcodding@redhat.com>
 <6dbcb2f39dcee7314bf772aaabf4923104ec7aee.camel@hammerspace.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 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.

Ben