[PATCH 0/3] cifs: Fixes for copy_file_range() and FALLOC_FL_INSERT/ZERO_RANGE

[PATCH 0/3] cifs: Fixes for copy_file_range() and FALLOC_FL_INSERT/ZERO_RANGE

[David Howells]

Hi Steve,

Here are three patches for cifs:

 (1) Fix FALLOC_FL_ZERO_RANGE support to change i_size if the file is
     extended.

 (2) Fix FALLOC_FL_INSERT_RANGE support to change i_size after it moves the
     EOF on the server.

 (3) Fix copy_file_range() support to handle invalidation and flushing of
     overlapping dirty data correctly, to move the EOF on the server to
     deal with lazy flushing of locally dirty data and to set the i_size
     afterwards if the copy extended the file.

I've pushed the patches here also:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=cifs-fixes

David

David Howells (3):
  cifs: Fix FALLOC_FL_ZERO_RANGE by setting i_size if EOF moved
  cifs: Fix FALLOC_FL_INSERT_RANGE by setting i_size after EOF moved
  cifs: Fix flushing, invalidation and file size with copy_file_range()

 fs/smb/client/cifsfs.c  | 80 +++++++++++++++++++++++++++++++++++++++--
 fs/smb/client/smb2ops.c | 13 +++++--
 2 files changed, 88 insertions(+), 5 deletions(-)

[PATCH 1/3] cifs: Fix FALLOC_FL_ZERO_RANGE by setting i_size if EOF moved

[David Howells]

Fix the cifs filesystem implementations of FALLOC_FL_ZERO_RANGE, in
smb3_zero_range(), to set i_size after extending the file on the server.

Fixes: 72c419d9b073 ("cifs: fix smb3_zero_range so it can expand the file-size when required")
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: Jeff Layton <jlayton@kernel.org>
cc: linux-cifs@vger.kernel.org
cc: linux-mm@kvack.org
---
 fs/smb/client/smb2ops.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index a959ed2c9b22..572d2c22a703 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -3307,6 +3307,7 @@ static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
 	struct inode *inode = file_inode(file);
 	struct cifsInodeInfo *cifsi = CIFS_I(inode);
 	struct cifsFileInfo *cfile = file->private_data;
+	unsigned long long new_size;
 	long rc;
 	unsigned int xid;
 	__le64 eof;
@@ -3337,10 +3338,15 @@ static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
 	/*
 	 * do we also need to change the size of the file?
 	 */
-	if (keep_size == false && i_size_read(inode) < offset + len) {
-		eof = cpu_to_le64(offset + len);
+	new_size = offset + len;
+	if (keep_size == false && (unsigned long long)i_size_read(inode) < new_size) {
+		eof = cpu_to_le64(new_size);
 		rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
 				  cfile->fid.volatile_fid, cfile->pid, &eof);
+		if (rc >= 0) {
+			truncate_setsize(inode, new_size);
+			fscache_resize_cookie(cifs_inode_cookie(inode), new_size);
+		}
 	}
 
  zero_range_exit:

[PATCH 2/3] cifs: Fix FALLOC_FL_INSERT_RANGE by setting i_size after EOF moved

[David Howells]

Fix the cifs filesystem implementations of FALLOC_FL_INSERT_RANGE, in
smb3_insert_range(), to set i_size after extending the file on the server
and before we do the copy to open the gap (as we don't clean up the EOF
marker if the copy fails).

Fixes: 7fe6fe95b936 ("cifs: add FALLOC_FL_INSERT_RANGE support")
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: Jeff Layton <jlayton@kernel.org>
cc: linux-cifs@vger.kernel.org
cc: linux-mm@kvack.org
---
 fs/smb/client/smb2ops.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 572d2c22a703..65a00c8b8494 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -3741,6 +3741,9 @@ static long smb3_insert_range(struct file *file, struct cifs_tcon *tcon,
 	if (rc < 0)
 		goto out_2;
 
+	truncate_setsize(inode, old_eof + len);
+	fscache_resize_cookie(cifs_inode_cookie(inode), i_size_read(inode));
+
 	rc = smb2_copychunk_range(xid, cfile, cfile, off, count, off + len);
 	if (rc < 0)
 		goto out_2;

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

[David Howells]

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(-)

diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
index ea3a7a668b45..6db88422f314 100644
--- a/fs/smb/client/cifsfs.c
+++ b/fs/smb/client/cifsfs.c
@@ -1256,6 +1256,45 @@ static loff_t cifs_remap_file_range(struct file *src_file, loff_t off,
 	return rc < 0 ? rc : len;
 }
 
+/*
+ * Flush out either the folio that overlaps the beginning of a range in which
+ * pos resides (if _fstart is given) or the folio that overlaps the end of a
+ * range (if _fstart is NULL) unless that folio is entirely within the range
+ * we're going to invalidate.
+ */
+static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t *_fstart, loff_t *_fend)
+{
+	struct folio *folio;
+	unsigned long long fpos, fend;
+	pgoff_t index = pos / PAGE_SIZE;
+	size_t size;
+	int rc = 0;
+
+	folio = filemap_get_folio(inode->i_mapping, index);
+	if (IS_ERR(folio)) {
+		if (_fstart)
+			*_fstart = pos;
+		*_fend = pos;
+		return 0;
+	}
+
+	size = folio_size(folio);
+	fpos = folio_pos(folio);
+	fend = fpos + size - 1;
+	if (_fstart)
+		*_fstart = fpos;
+	*_fend = fend;
+	if (_fstart && pos == fpos)
+		goto out;
+	if (!_fstart && pos == fend)
+		goto out;
+	
+	rc = filemap_write_and_wait_range(inode->i_mapping, fpos, fend);
+out:
+	folio_put(folio);
+	return rc;
+}
+
 ssize_t cifs_file_copychunk_range(unsigned int xid,
 				struct file *src_file, loff_t off,
 				struct file *dst_file, loff_t destoff,
@@ -1263,10 +1302,12 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 {
 	struct inode *src_inode = file_inode(src_file);
 	struct inode *target_inode = file_inode(dst_file);
+	struct cifsInodeInfo *src_cifsi = CIFS_I(src_inode);
 	struct cifsFileInfo *smb_file_src;
 	struct cifsFileInfo *smb_file_target;
 	struct cifs_tcon *src_tcon;
 	struct cifs_tcon *target_tcon;
+	unsigned long long destend, fstart, fend;
 	ssize_t rc;
 
 	cifs_dbg(FYI, "copychunk range\n");
@@ -1306,13 +1347,46 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 	if (rc)
 		goto unlock;
 
-	/* should we flush first and last page first */
-	truncate_inode_pages(&target_inode->i_data, 0);
+	/* The server-side copy will fail if the source crosses the EOF marker.
+	 * Advance the EOF marker after the flush above to the end of the range
+	 * if it's short of that.
+	 */
+	if (src_cifsi->server_eof < off + len) {
+		rc = src_tcon->ses->server->ops->set_file_size(
+			xid, src_tcon, smb_file_src, off + len, false);
+		if (rc < 0)
+			goto unlock;
+
+		fscache_resize_cookie(cifs_inode_cookie(src_inode),
+				      i_size_read(src_inode));
+	}
+
+	destend = destoff + len - 1;
+
+	/* Flush the folios at either end of the destination range to prevent
+	 * accidental loss of dirty data outside of the range.
+	 */
+	fstart = destoff;
+
+	rc = cifs_flush_folio(target_inode, destoff, &fstart, &fend);
+	if (rc)
+		goto unlock;
+	if (destend > fend) {
+		rc = cifs_flush_folio(target_inode, destend, NULL, &fend);
+		if (rc)
+			goto unlock;
+	}
+
+	/* Discard all the folios that overlap the destination region. */
+	truncate_inode_pages_range(&target_inode->i_data, fstart, fend);
 
 	rc = file_modified(dst_file);
-	if (!rc)
+	if (!rc) {
 		rc = target_tcon->ses->server->ops->copychunk_range(xid,
 			smb_file_src, smb_file_target, off, len, destoff);
+		if (rc > 0 && destoff + rc > i_size_read(target_inode))
+			truncate_setsize(target_inode, destoff + rc);
+	}
 
 	file_accessed(src_file);
 

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

[Steve French]

Fixed a minor whitespace issue, and tentatively added to cifs-2.6.git
for-next (all three) pending additional testing

On Wed, Nov 29, 2023 at 10:56 AM David Howells <dhowells@redhat.com> wrote:
>
> 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(-)
>
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index ea3a7a668b45..6db88422f314 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -1256,6 +1256,45 @@ static loff_t cifs_remap_file_range(struct file *src_file, loff_t off,
>         return rc < 0 ? rc : len;
>  }
>
> +/*
> + * Flush out either the folio that overlaps the beginning of a range in which
> + * pos resides (if _fstart is given) or the folio that overlaps the end of a
> + * range (if _fstart is NULL) unless that folio is entirely within the range
> + * we're going to invalidate.
> + */
> +static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t *_fstart, loff_t *_fend)
> +{
> +       struct folio *folio;
> +       unsigned long long fpos, fend;
> +       pgoff_t index = pos / PAGE_SIZE;
> +       size_t size;
> +       int rc = 0;
> +
> +       folio = filemap_get_folio(inode->i_mapping, index);
> +       if (IS_ERR(folio)) {
> +               if (_fstart)
> +                       *_fstart = pos;
> +               *_fend = pos;
> +               return 0;
> +       }
> +
> +       size = folio_size(folio);
> +       fpos = folio_pos(folio);
> +       fend = fpos + size - 1;
> +       if (_fstart)
> +               *_fstart = fpos;
> +       *_fend = fend;
> +       if (_fstart && pos == fpos)
> +               goto out;
> +       if (!_fstart && pos == fend)
> +               goto out;
> +
> +       rc = filemap_write_and_wait_range(inode->i_mapping, fpos, fend);
> +out:
> +       folio_put(folio);
> +       return rc;
> +}
> +
>  ssize_t cifs_file_copychunk_range(unsigned int xid,
>                                 struct file *src_file, loff_t off,
>                                 struct file *dst_file, loff_t destoff,
> @@ -1263,10 +1302,12 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
>  {
>         struct inode *src_inode = file_inode(src_file);
>         struct inode *target_inode = file_inode(dst_file);
> +       struct cifsInodeInfo *src_cifsi = CIFS_I(src_inode);
>         struct cifsFileInfo *smb_file_src;
>         struct cifsFileInfo *smb_file_target;
>         struct cifs_tcon *src_tcon;
>         struct cifs_tcon *target_tcon;
> +       unsigned long long destend, fstart, fend;
>         ssize_t rc;
>
>         cifs_dbg(FYI, "copychunk range\n");
> @@ -1306,13 +1347,46 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
>         if (rc)
>                 goto unlock;
>
> -       /* should we flush first and last page first */
> -       truncate_inode_pages(&target_inode->i_data, 0);
> +       /* The server-side copy will fail if the source crosses the EOF marker.
> +        * Advance the EOF marker after the flush above to the end of the range
> +        * if it's short of that.
> +        */
> +       if (src_cifsi->server_eof < off + len) {
> +               rc = src_tcon->ses->server->ops->set_file_size(
> +                       xid, src_tcon, smb_file_src, off + len, false);
> +               if (rc < 0)
> +                       goto unlock;
> +
> +               fscache_resize_cookie(cifs_inode_cookie(src_inode),
> +                                     i_size_read(src_inode));
> +       }
> +
> +       destend = destoff + len - 1;
> +
> +       /* Flush the folios at either end of the destination range to prevent
> +        * accidental loss of dirty data outside of the range.
> +        */
> +       fstart = destoff;
> +
> +       rc = cifs_flush_folio(target_inode, destoff, &fstart, &fend);
> +       if (rc)
> +               goto unlock;
> +       if (destend > fend) {
> +               rc = cifs_flush_folio(target_inode, destend, NULL, &fend);
> +               if (rc)
> +                       goto unlock;
> +       }
> +
> +       /* Discard all the folios that overlap the destination region. */
> +       truncate_inode_pages_range(&target_inode->i_data, fstart, fend);
>
>         rc = file_modified(dst_file);
> -       if (!rc)
> +       if (!rc) {
>                 rc = target_tcon->ses->server->ops->copychunk_range(xid,
>                         smb_file_src, smb_file_target, off, len, destoff);
> +               if (rc > 0 && destoff + rc > i_size_read(target_inode))
> +                       truncate_setsize(target_inode, destoff + rc);
> +       }
>
>         file_accessed(src_file);
>
>
>


-- 
Thanks,

Steve

Re: [PATCH 1/3] cifs: Fix FALLOC_FL_ZERO_RANGE by setting i_size if EOF moved

[Paulo Alcantara]

David Howells <dhowells@redhat.com> writes:

> @@ -3307,6 +3307,7 @@ static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
>  	struct inode *inode = file_inode(file);
>  	struct cifsInodeInfo *cifsi = CIFS_I(inode);
>  	struct cifsFileInfo *cfile = file->private_data;
> +	unsigned long long new_size;
>  	long rc;
>  	unsigned int xid;
>  	__le64 eof;
> @@ -3337,10 +3338,15 @@ static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
>  	/*
>  	 * do we also need to change the size of the file?
>  	 */

Perhaps remove the comment above as we're now updating the inode size?

> -	if (keep_size == false && i_size_read(inode) < offset + len) {
> -		eof = cpu_to_le64(offset + len);
> +	new_size = offset + len;
> +	if (keep_size == false && (unsigned long long)i_size_read(inode) < new_size) {
> +		eof = cpu_to_le64(new_size);
>  		rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
>  				  cfile->fid.volatile_fid, cfile->pid, &eof);
> +		if (rc >= 0) {
> +			truncate_setsize(inode, new_size);
> +			fscache_resize_cookie(cifs_inode_cookie(inode), new_size);
> +		}
>  	}
>  
>   zero_range_exit:

Looks good,

Acked-by: Paulo Alcantara (SUSE) <pc@manguebit.com>

Re: [PATCH 2/3] cifs: Fix FALLOC_FL_INSERT_RANGE by setting i_size after EOF moved

[Paulo Alcantara]

David Howells <dhowells@redhat.com> writes:

> Fix the cifs filesystem implementations of FALLOC_FL_INSERT_RANGE, in
> smb3_insert_range(), to set i_size after extending the file on the server
> and before we do the copy to open the gap (as we don't clean up the EOF
> marker if the copy fails).
>
> Fixes: 7fe6fe95b936 ("cifs: add FALLOC_FL_INSERT_RANGE support")
> 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: Jeff Layton <jlayton@kernel.org>
> cc: linux-cifs@vger.kernel.org
> cc: linux-mm@kvack.org
> ---
>  fs/smb/client/smb2ops.c | 3 +++
>  1 file changed, 3 insertions(+)

Looks good,

Acked-by: Paulo Alcantara (SUSE) <pc@manguebit.com>

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

[Paulo Alcantara]

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>

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

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 3279 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 4745 bytes --]

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

[Steve French]

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

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

[Steve French]

There is a minor problem with the patch in the change to
cifs_file_copychunk_range() in cifsfs.c.  With this change it can
attempt to set the file size using a file handle without write
permission  (in this path it is common for the source file to be
opened for read when doing a copy).  Fortunately I can't reproduce
that in any of my tests (because the file size is up to date and data
from source file was already flushed) but safer to fix it.

        /* The server-side copy will fail if the source crosses the EOF marker.
         * Advance the EOF marker after the flush above to the end of the range
         * if it's short of that.
         */
        if (src_cifsi->server_eof < off + len) {
                rc = src_tcon->ses->server->ops->set_file_size(
                        xid, src_tcon, smb_file_src, off + len, false);

This should be calling the path based equivalent to set the file size
so it can find a writeable file.

On Wed, Nov 29, 2023 at 3:37 PM Steve French <smfrench@gmail.com> wrote:
>
> Fixed a minor whitespace issue, and tentatively added to cifs-2.6.git
> for-next (all three) pending additional testing
>
> On Wed, Nov 29, 2023 at 10:56 AM David Howells <dhowells@redhat.com> wrote:
> >
> > 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(-)
> >
> > diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> > index ea3a7a668b45..6db88422f314 100644
> > --- a/fs/smb/client/cifsfs.c
> > +++ b/fs/smb/client/cifsfs.c
> > @@ -1256,6 +1256,45 @@ static loff_t cifs_remap_file_range(struct file *src_file, loff_t off,
> >         return rc < 0 ? rc : len;
> >  }
> >
> > +/*
> > + * Flush out either the folio that overlaps the beginning of a range in which
> > + * pos resides (if _fstart is given) or the folio that overlaps the end of a
> > + * range (if _fstart is NULL) unless that folio is entirely within the range
> > + * we're going to invalidate.
> > + */
> > +static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t *_fstart, loff_t *_fend)
> > +{
> > +       struct folio *folio;
> > +       unsigned long long fpos, fend;
> > +       pgoff_t index = pos / PAGE_SIZE;
> > +       size_t size;
> > +       int rc = 0;
> > +
> > +       folio = filemap_get_folio(inode->i_mapping, index);
> > +       if (IS_ERR(folio)) {
> > +               if (_fstart)
> > +                       *_fstart = pos;
> > +               *_fend = pos;
> > +               return 0;
> > +       }
> > +
> > +       size = folio_size(folio);
> > +       fpos = folio_pos(folio);
> > +       fend = fpos + size - 1;
> > +       if (_fstart)
> > +               *_fstart = fpos;
> > +       *_fend = fend;
> > +       if (_fstart && pos == fpos)
> > +               goto out;
> > +       if (!_fstart && pos == fend)
> > +               goto out;
> > +
> > +       rc = filemap_write_and_wait_range(inode->i_mapping, fpos, fend);
> > +out:
> > +       folio_put(folio);
> > +       return rc;
> > +}
> > +
> >  ssize_t cifs_file_copychunk_range(unsigned int xid,
> >                                 struct file *src_file, loff_t off,
> >                                 struct file *dst_file, loff_t destoff,
> > @@ -1263,10 +1302,12 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
> >  {
> >         struct inode *src_inode = file_inode(src_file);
> >         struct inode *target_inode = file_inode(dst_file);
> > +       struct cifsInodeInfo *src_cifsi = CIFS_I(src_inode);
> >         struct cifsFileInfo *smb_file_src;
> >         struct cifsFileInfo *smb_file_target;
> >         struct cifs_tcon *src_tcon;
> >         struct cifs_tcon *target_tcon;
> > +       unsigned long long destend, fstart, fend;
> >         ssize_t rc;
> >
> >         cifs_dbg(FYI, "copychunk range\n");
> > @@ -1306,13 +1347,46 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
> >         if (rc)
> >                 goto unlock;
> >
> > -       /* should we flush first and last page first */
> > -       truncate_inode_pages(&target_inode->i_data, 0);
> > +       /* The server-side copy will fail if the source crosses the EOF marker.
> > +        * Advance the EOF marker after the flush above to the end of the range
> > +        * if it's short of that.
> > +        */
> > +       if (src_cifsi->server_eof < off + len) {
> > +               rc = src_tcon->ses->server->ops->set_file_size(
> > +                       xid, src_tcon, smb_file_src, off + len, false);
> > +               if (rc < 0)
> > +                       goto unlock;
> > +
> > +               fscache_resize_cookie(cifs_inode_cookie(src_inode),
> > +                                     i_size_read(src_inode));
> > +       }
> > +
> > +       destend = destoff + len - 1;
> > +
> > +       /* Flush the folios at either end of the destination range to prevent
> > +        * accidental loss of dirty data outside of the range.
> > +        */
> > +       fstart = destoff;
> > +
> > +       rc = cifs_flush_folio(target_inode, destoff, &fstart, &fend);
> > +       if (rc)
> > +               goto unlock;
> > +       if (destend > fend) {
> > +               rc = cifs_flush_folio(target_inode, destend, NULL, &fend);
> > +               if (rc)
> > +                       goto unlock;
> > +       }
> > +
> > +       /* Discard all the folios that overlap the destination region. */
> > +       truncate_inode_pages_range(&target_inode->i_data, fstart, fend);
> >
> >         rc = file_modified(dst_file);
> > -       if (!rc)
> > +       if (!rc) {
> >                 rc = target_tcon->ses->server->ops->copychunk_range(xid,
> >                         smb_file_src, smb_file_target, off, len, destoff);
> > +               if (rc > 0 && destoff + rc > i_size_read(target_inode))
> > +                       truncate_setsize(target_inode, destoff + rc);
> > +       }
> >
> >         file_accessed(src_file);
> >
> >
> >
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve