Message-ID: <2056123.1701193802@warthog.procyon.org.uk>
Date:   Tue, 28 Nov 2023 17:50:02 +0000
From:   David Howells <dhowells@...hat.com>
To:     Steve French <sfrench@...ba.org>
cc:     dhowells@...hat.com, Shyam Prasad N <nspmangalore@...il.com>,
        Rohith Surabattula <rohiths.msft@...il.com>,
        Jeff Layton <jlayton@...nel.org>,
        Matthew Wilcox <willy@...radead.org>,
        linux-cifs@...r.kernel.org, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH] cifs: Fix issues with copy_file_range and FALLOC_FL_INSERT/ZERO_RANGE

    
Fix a number of issues in the cifs filesystem implementations of
copy_file_range(), FALLOC_FL_INSERT_RANGE and FALLOC_FL_ZERO_RANGE:

 (1) In cifs_file_copychunk_range(), 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.

 (2) In smb3_zero_range(), set i_size after extending the file on the
     server.

 (3) In smb3_insert_range(), 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).

 (4) Add a new MM function, discard_inode_pages_range(), which is
     equivalent to truncate_inode_pages_range(), but rounds out the range
     to include the entire folio at each end.

     [!] This might be better done by adding a flag to
         truncate_inode_pages_range().

 (5) In cifs_file_copychunk_range(), fix the invalidation of the
     destination range.

     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() as
     that will simply partially clear 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.

     Further, we shouldn't simply round out the range to PAGE_SIZE at each
     end as cifs should move to support multipage folios.

     So change the invalidation to flush the folio at each end of the range
     (which we can do simply by asking to flush a byte in it), then use
     discard_inode_pages_range() to fully invalidate the entire range of
     folios.

Fixes: 620d8745b35d ("Introduce cifs_copy_file_range()")
Fixes: 72c419d9b073 ("cifs: fix smb3_zero_range so it can expand the file-size when required")
Fixes: 7fe6fe95b936 ("cifs: add FALLOC_FL_INSERT_RANGE support")
Signed-off-by: David Howells <dhowells@...hat.com>
cc: Steve French <sfrench@...ba.org>
cc: Paulo Alcantara <pc@...guebit.com>
cc: Shyam Prasad N <nspmangalore@...il.com>
cc: Rohith Surabattula <rohiths.msft@...il.com>
cc: Jeff Layton <jlayton@...nel.org>
cc: Matthew Wilcox <willy@...radead.org>
cc: linux-cifs@...r.kernel.org
cc: linux-mm@...ck.org
---
 fs/smb/client/cifsfs.c  |   24 +++++++++++++++++++++---
 fs/smb/client/smb2ops.c |   13 +++++++++++--
 include/linux/mm.h      |    2 ++
 mm/truncate.c           |   37 +++++++++++++++++++++++++++++++++----
 4 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
index ea3a7a668b45..9b6e3cfd5a59 100644
--- a/fs/smb/client/cifsfs.c
+++ b/fs/smb/client/cifsfs.c
@@ -1267,6 +1267,7 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 	struct cifsFileInfo *smb_file_target;
 	struct cifs_tcon *src_tcon;
 	struct cifs_tcon *target_tcon;
+	unsigned long long destend;
 	ssize_t rc;
 
 	cifs_dbg(FYI, "copychunk range\n");
@@ -1306,13 +1307,30 @@ 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);
+	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.
+	 */
+	rc = filemap_write_and_wait_range(target_inode->i_mapping, destoff, destoff);
+	if (rc)
+		goto unlock;
+	if (destend > destoff) {
+		rc = filemap_write_and_wait_range(target_inode->i_mapping, destend, destend);
+		if (rc)
+			goto unlock;
+	}
+
+	/* Discard all the folios that overlap the destination region. */
+	discard_inode_pages_range(&target_inode->i_data, destoff, destend);
 
 	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);
 
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index a959ed2c9b22..65a00c8b8494 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:
@@ -3735,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;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 64cd1ee4aacc..e930c930e3f5 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3416,6 +3416,8 @@ extern unsigned long vm_unmapped_area(struct vm_unmapped_area_info *info);
 extern void truncate_inode_pages(struct address_space *, loff_t);
 extern void truncate_inode_pages_range(struct address_space *,
 				       loff_t lstart, loff_t lend);
+extern void discard_inode_pages_range(struct address_space *mapping,
+				      loff_t lstart, loff_t lend);
 extern void truncate_inode_pages_final(struct address_space *);
 
 /* generic vm_area_ops exported for stackable file systems */
diff --git a/mm/truncate.c b/mm/truncate.c
index 52e3a703e7b2..b91d67de449c 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -295,10 +295,11 @@ long mapping_evict_folio(struct address_space *mapping, struct folio *folio)
 }
 
 /**
- * truncate_inode_pages_range - truncate range of pages specified by start & end byte offsets
+ * __truncate_inode_pages_range - truncate range of pages specified by start & end byte offsets
  * @mapping: mapping to truncate
  * @lstart: offset from which to truncate
  * @lend: offset to which to truncate (inclusive)
+ * @round_out: Discard all overlapping folios
  *
  * Truncate the page cache, removing the pages that are between
  * specified offsets (and zeroing out partial pages
@@ -318,8 +319,9 @@ long mapping_evict_folio(struct address_space *mapping, struct folio *folio)
  * truncate_inode_pages_range is able to handle cases where lend + 1 is not
  * page aligned properly.
  */
-void truncate_inode_pages_range(struct address_space *mapping,
-				loff_t lstart, loff_t lend)
+static void __truncate_inode_pages_range(struct address_space *mapping,
+					 loff_t lstart, loff_t lend,
+					 bool round_out)
 {
 	pgoff_t		start;		/* inclusive */
 	pgoff_t		end;		/* exclusive */
@@ -367,7 +369,15 @@ void truncate_inode_pages_range(struct address_space *mapping,
 	same_folio = (lstart >> PAGE_SHIFT) == (lend >> PAGE_SHIFT);
 	folio = __filemap_get_folio(mapping, lstart >> PAGE_SHIFT, FGP_LOCK, 0);
 	if (!IS_ERR(folio)) {
-		same_folio = lend < folio_pos(folio) + folio_size(folio);
+		loff_t fend = folio_pos(folio) + folio_size(folio) - 1;
+
+		if (unlikely(round_out)) {
+			if (folio_pos(folio) < lstart)
+				lstart = folio_pos(folio);
+			if (lend < fend)
+				lend = fend;
+		}
+		same_folio = lend <= fend;
 		if (!truncate_inode_partial_folio(folio, lstart, lend)) {
 			start = folio_next_index(folio);
 			if (same_folio)
@@ -382,6 +392,12 @@ void truncate_inode_pages_range(struct address_space *mapping,
 		folio = __filemap_get_folio(mapping, lend >> PAGE_SHIFT,
 						FGP_LOCK, 0);
 		if (!IS_ERR(folio)) {
+			if (unlikely(round_out)) {
+				loff_t fend = folio_pos(folio) + folio_size(folio) - 1;
+
+				if (lend < fend)
+					lend = fend;
+			}
 			if (!truncate_inode_partial_folio(folio, lstart, lend))
 				end = folio->index;
 			folio_unlock(folio);
@@ -420,8 +436,21 @@ void truncate_inode_pages_range(struct address_space *mapping,
 		folio_batch_release(&fbatch);
 	}
 }
+
+void truncate_inode_pages_range(struct address_space *mapping,
+				loff_t lstart, loff_t lend)
+{
+	__truncate_inode_pages_range(mapping, lstart, lend, false);
+}
 EXPORT_SYMBOL(truncate_inode_pages_range);
 
+void discard_inode_pages_range(struct address_space *mapping,
+			       loff_t lstart, loff_t lend)
+{
+	__truncate_inode_pages_range(mapping, lstart, lend, true);
+}
+EXPORT_SYMBOL(discard_inode_pages_range);
+
 /**
  * truncate_inode_pages - truncate *all* the pages from an offset
  * @mapping: mapping to truncate

Powered by blists - more mailing lists