Re: [PATCH 3/3] cifs: Fix flushing, invalidation and file size with copy_file_range()

[Steve French <smfrench@gmail.com>]

updated all three patches with the acked-by and put in cifs-2.6.git for-next


On Wed, Nov 29, 2023 at 4:28 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> David Howells <dhowells@redhat.com> writes:
>
> > Fix a number of issues in the cifs filesystem implementation of the
> > copy_file_range() syscall in cifs_file_copychunk_range().
> >
> > Firstly, the invalidation of the destination range is handled incorrectly:
> > We shouldn't just invalidate the whole file as dirty data in the file may
> > get lost and we can't just call truncate_inode_pages_range() to invalidate
> > the destination range as that will erase parts of a partial folio at each
> > end whilst invalidating and discarding all the folios in the middle.  We
> > need to force all the folios covering the range to be reloaded, but we
> > mustn't lose dirty data in them that's not in the destination range.
> >
> > Further, we shouldn't simply round out the range to PAGE_SIZE at each end
> > as cifs should move to support multipage folios.
> >
> > Secondly, there's an issue whereby a write may have extended the file
> > locally, but not have been written back yet.  This can leaves the local
> > idea of the EOF at a later point than the server's EOF.  If a copy request
> > is issued, this will fail on the server with STATUS_INVALID_VIEW_SIZE
> > (which gets translated to -EIO locally) if the copy source extends past the
> > server's EOF.
> >
> > Fix this by:
> >
> >  (0) Flush the source region (already done).  The flush does nothing and
> >      the EOF isn't moved if the source region has no dirty data.
> >
> >  (1) Move the EOF to the end of the source region if it isn't already at
> >      least at this point.
> >
> >      [!] Rather than moving the EOF, it might be better to split the copy
> >      range into a part to be copied and a part to be cleared with
> >      FSCTL_SET_ZERO_DATA.
> >
> >  (2) Find the folio (if present) at each end of the range, flushing it and
> >      increasing the region-to-be-invalidated to cover those in their
> >      entirety.
> >
> >  (3) Fully discard all the folios covering the range as we want them to be
> >      reloaded.
> >
> >  (4) Then perform the copy.
> >
> > Thirdly, set i_size after doing the copychunk_range operation as this value
> > may be used by various things internally.  stat() hides the issue because
> > setting ->time to 0 causes cifs_getatr() to revalidate the attributes.
> >
> > These were causing the generic/075 xfstest to fail.
> >
> > Fixes: 620d8745b35d ("Introduce cifs_copy_file_range()")
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > cc: Steve French <sfrench@samba.org>
> > cc: Paulo Alcantara <pc@manguebit.com>
> > cc: Shyam Prasad N <nspmangalore@gmail.com>
> > cc: Rohith Surabattula <rohiths.msft@gmail.com>
> > cc: Matthew Wilcox <willy@infradead.org>
> > cc: Jeff Layton <jlayton@kernel.org>
> > cc: linux-cifs@vger.kernel.org
> > cc: linux-mm@kvack.org
> > ---
> >  fs/smb/client/cifsfs.c | 80 ++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 77 insertions(+), 3 deletions(-)
>
> Looks good,
>
> Acked-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
>


-- 
Thanks,

Steve