[v2,1/2] exfat: change to get file size from DataLength

Commit Message

[e-mail] June 28, 2023, 1:52 a.m. UTC

In stream extension directory entry, the ValidDataLength
field describes how far into the data stream user data has
been written, and the DataLength field describes the file
size.

Signed-off-by: Yuezhang Mo <[e-mail]>
Reviewed-by: Andy Wu <[e-mail]>
Reviewed-by: Aoyama Wataru <[e-mail]>
---
 fs/exfat/exfat_fs.h |   2 +
 fs/exfat/file.c     | 133 +++++++++++++++++++++++++++++++++++++++++++-
 fs/exfat/inode.c    |  92 +++++++++++++++++++++++++-----
 fs/exfat/namei.c    |   7 ++-
 4 files changed, 217 insertions(+), 17 deletions(-)

Comments

Namjae Jeon Aug. 14, 2023, 4:36 a.m. UTC | #1

[snip]
> +static ssize_t exfat_file_write_iter(struct kiocb *iocb, struct iov_iter
> *iter)
> +{
> +	ssize_t ret;
> +	struct file *file = iocb->ki_filp;
> +	struct inode *inode = file_inode(file);
> +	struct exfat_inode_info *ei = EXFAT_I(inode);
> +	loff_t pos = iocb->ki_pos;
> +	loff_t valid_size;
> +
> +	inode_lock(inode);
> +
> +	valid_size = ei->valid_size;
> +
> +	ret = generic_write_checks(iocb, iter);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	if (pos > valid_size) {
> +		ret = exfat_file_zeroed_range(file, valid_size, pos);
Can we use block_write_begin instead of cont_write_begin in exfat_write_begin?

> +		if (ret < 0 && ret != -ENOSPC) {
> +			exfat_err(inode->i_sb,
> +				"write: fail to zero from %llu to %llu(%ld)",
> +				valid_size, pos, ret);
> +		}
> +		if (ret < 0)
> +			goto unlock;
> +	}
> +
> +	ret = __generic_file_write_iter(iocb, iter);
> +	if (ret < 0)
Probably It should be if (ret <= 0)...

> +		goto unlock;
> +
> +	if (pos + ret > i_size_read(inode))
> +		i_size_write(inode, pos + ret);
> +
> +	if (pos + ret > ei->valid_size)
> +		ei->valid_size = pos + ret;
> +
> +	/*
> +	 * If valid_size is extended with sector-aligned length in
> +	 * exfat_get_block(), set to the writren length.
> +	 */
> +	if (i_size_read(inode) < ei->valid_size)
> +		ei->valid_size = i_size_read(inode);
> +
> +	mark_inode_dirty(inode);
> +	inode_unlock(inode);
> +
> +	if (pos > valid_size && iocb_is_dsync(iocb)) {
> +		ssize_t err = vfs_fsync_range(file, valid_size, pos - 1,
> +				iocb->ki_flags & IOCB_SYNC);
It should be moved to exfat_file_zeroed_range() ?

Thanks.

[e-mail] Aug. 14, 2023, 10:31 a.m. UTC | #2

> -----Original Message-----
> From: Namjae Jeon <[e-mail]>
> Sent: Monday, August 14, 2023 12:36 PM
> To: Mo, Yuezhang <[e-mail]>
> Cc: [e-mail]; [e-mail]; Wu, Andy
> <[e-mail]>; Aoyama, Wataru (SGC) <[e-mail]>
> Subject: Re: [PATCH v2 1/2] exfat: change to get file size from DataLength
> 
> [snip]
> > +static ssize_t exfat_file_write_iter(struct kiocb *iocb, struct iov_iter
> > *iter)
> > +{
> > +	ssize_t ret;
> > +	struct file *file = iocb->ki_filp;
> > +	struct inode *inode = file_inode(file);
> > +	struct exfat_inode_info *ei = EXFAT_I(inode);
> > +	loff_t pos = iocb->ki_pos;
> > +	loff_t valid_size;
> > +
> > +	inode_lock(inode);
> > +
> > +	valid_size = ei->valid_size;
> > +
> > +	ret = generic_write_checks(iocb, iter);
> > +	if (ret < 0)
> > +		goto unlock;
> > +
> > +	if (pos > valid_size) {
> > +		ret = exfat_file_zeroed_range(file, valid_size, pos);
> Can we use block_write_begin instead of cont_write_begin in
> exfat_write_begin?

Yes.

exfat_get_block() should be exported if block_write_begin() is used.

> 
> > +		if (ret < 0 && ret != -ENOSPC) {
> > +			exfat_err(inode->i_sb,
> > +				"write: fail to zero from %llu to %llu(%ld)",
> > +				valid_size, pos, ret);
> > +		}
> > +		if (ret < 0)
> > +			goto unlock;
> > +	}
> > +
> > +	ret = __generic_file_write_iter(iocb, iter);
> > +	if (ret < 0)
> Probably It should be if (ret <= 0)...

In the case of ret==0, exfat_file_zeroed_range() may have written zeros to page cache, and the
following vfs_fsync_range() is needed to synchronize these data.

> 
> > +		goto unlock;
> > +
> > +	if (pos + ret > i_size_read(inode))
> > +		i_size_write(inode, pos + ret);
> > +
> > +	if (pos + ret > ei->valid_size)
> > +		ei->valid_size = pos + ret;
> > +
> > +	/*
> > +	 * If valid_size is extended with sector-aligned length in
> > +	 * exfat_get_block(), set to the writren length.
> > +	 */
> > +	if (i_size_read(inode) < ei->valid_size)
> > +		ei->valid_size = i_size_read(inode);
> > +
> > +	mark_inode_dirty(inode);
> > +	inode_unlock(inode);
> > +
> > +	if (pos > valid_size && iocb_is_dsync(iocb)) {
> > +		ssize_t err = vfs_fsync_range(file, valid_size, pos - 1,
> > +				iocb->ki_flags & IOCB_SYNC);
> It should be moved to exfat_file_zeroed_range() ?

The inode lock is locked when exfat_file_zeroed_range() is called, and vfs_fsync_range() may
need to synchronize a lot of data. If vfs_fsync_range() is called in exfat_file_zeroed_range(), 
the inode lock will be locked for a long time.

Patch

diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index 729ada9e26e8..8d6248367433 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -208,6 +208,7 @@  struct exfat_dir_entry {
 	unsigned char flags;
 	unsigned short attr;
 	loff_t size;
+	loff_t valid_size;
 	unsigned int num_subdirs;
 	struct timespec64 atime;
 	struct timespec64 mtime;
@@ -317,6 +318,7 @@  struct exfat_inode_info {
 	loff_t i_size_aligned;
 	/* on-disk position of directory entry or 0 */
 	loff_t i_pos;
+	loff_t valid_size;
 	/* hash by i_location */
 	struct hlist_node i_hash_fat;
 	/* protect bmap against truncate */
diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index e99183a74611..8cd14bc16857 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -8,6 +8,7 @@ 
 #include <linux/cred.h>
 #include <linux/buffer_head.h>
 #include <linux/blkdev.h>
+#include <linux/writeback.h>
 
 #include "exfat_raw.h"
 #include "exfat_fs.h"
@@ -23,6 +24,7 @@  static int exfat_cont_expand(struct inode *inode, loff_t size)
 		return err;
 
 	inode->i_ctime = inode->i_mtime = current_time(inode);
+	EXFAT_I(inode)->valid_size = size;
 	mark_inode_dirty(inode);
 
 	if (!IS_SYNC(inode))
@@ -143,6 +145,9 @@  int __exfat_truncate(struct inode *inode)
 		ei->start_clu = EXFAT_EOF_CLUSTER;
 	}
 
+	if (i_size_read(inode) < ei->valid_size)
+		ei->valid_size = i_size_read(inode);
+
 	if (ei->type == TYPE_FILE)
 		ei->attr |= ATTR_ARCHIVE;
 
@@ -379,15 +384,139 @@  int exfat_file_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
 	return blkdev_issue_flush(inode->i_sb->s_bdev);
 }
 
+static int exfat_file_zeroed_range(struct file *file, loff_t start, loff_t end)
+{
+	int err;
+	struct inode *inode = file_inode(file);
+	struct exfat_inode_info *ei = EXFAT_I(inode);
+	struct address_space *mapping = inode->i_mapping;
+	const struct address_space_operations *ops = mapping->a_ops;
+
+	while (start < end) {
+		u32 zerofrom, len;
+		struct page *page;
+
+		zerofrom = start & (PAGE_SIZE - 1);
+		len = PAGE_SIZE - zerofrom;
+		if (start + len > end)
+			len = end - start;
+
+		err = ops->write_begin(file, mapping, start, len, &page, NULL);
+		if (err)
+			goto out;
+
+		zero_user_segment(page, zerofrom, zerofrom + len);
+
+		err = ops->write_end(file, mapping, start, len, len, page, NULL);
+		if (err < 0)
+			goto out;
+		start += len;
+
+		balance_dirty_pages_ratelimited(mapping);
+		cond_resched();
+	}
+
+	ei->valid_size = end;
+	mark_inode_dirty(inode);
+
+out:
+	return err;
+}
+
+static ssize_t exfat_file_write_iter(struct kiocb *iocb, struct iov_iter *iter)
+{
+	ssize_t ret;
+	struct file *file = iocb->ki_filp;
+	struct inode *inode = file_inode(file);
+	struct exfat_inode_info *ei = EXFAT_I(inode);
+	loff_t pos = iocb->ki_pos;
+	loff_t valid_size;
+
+	inode_lock(inode);
+
+	valid_size = ei->valid_size;
+
+	ret = generic_write_checks(iocb, iter);
+	if (ret < 0)
+		goto unlock;
+
+	if (pos > valid_size) {
+		ret = exfat_file_zeroed_range(file, valid_size, pos);
+		if (ret < 0 && ret != -ENOSPC) {
+			exfat_err(inode->i_sb,
+				"write: fail to zero from %llu to %llu(%ld)",
+				valid_size, pos, ret);
+		}
+		if (ret < 0)
+			goto unlock;
+	}
+
+	ret = __generic_file_write_iter(iocb, iter);
+	if (ret < 0)
+		goto unlock;
+
+	if (pos + ret > i_size_read(inode))
+		i_size_write(inode, pos + ret);
+
+	if (pos + ret > ei->valid_size)
+		ei->valid_size = pos + ret;
+
+	/*
+	 * If valid_size is extended with sector-aligned length in
+	 * exfat_get_block(), set to the writren length.
+	 */
+	if (i_size_read(inode) < ei->valid_size)
+		ei->valid_size = i_size_read(inode);
+
+	mark_inode_dirty(inode);
+	inode_unlock(inode);
+
+	if (pos > valid_size && iocb_is_dsync(iocb)) {
+		ssize_t err = vfs_fsync_range(file, valid_size, pos - 1,
+				iocb->ki_flags & IOCB_SYNC);
+		if (err < 0)
+			return err;
+	}
+
+	return generic_write_sync(iocb, ret);
+
+unlock:
+	inode_unlock(inode);
+
+	return ret;
+}
+
+static int exfat_file_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	int ret;
+	struct inode *inode = file_inode(file);
+	struct exfat_inode_info *ei = EXFAT_I(inode);
+	loff_t start = ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
+	loff_t end = min_t(loff_t, i_size_read(inode),
+			start + vma->vm_end - vma->vm_start);
+
+	if ((vma->vm_flags & VM_WRITE) && ei->valid_size < end) {
+		ret = exfat_file_zeroed_range(file, ei->valid_size, end);
+		if (ret < 0) {
+			exfat_err(inode->i_sb,
+				  "mmap: fail to zero from %llu to %llu(%d)",
+				  start, end, ret);
+			return ret;
+		}
+	}
+
+	return generic_file_mmap(file, vma);
+}
+
 const struct file_operations exfat_file_operations = {
 	.llseek		= generic_file_llseek,
 	.read_iter	= generic_file_read_iter,
-	.write_iter	= generic_file_write_iter,
+	.write_iter	= exfat_file_write_iter,
 	.unlocked_ioctl = exfat_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = exfat_compat_ioctl,
 #endif
-	.mmap		= generic_file_mmap,
+	.mmap		= exfat_file_mmap,
 	.fsync		= exfat_file_fsync,
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
index 481dd338f2b8..0c6b5897518c 100644
--- a/fs/exfat/inode.c
+++ b/fs/exfat/inode.c
@@ -72,8 +72,8 @@  int __exfat_write_inode(struct inode *inode, int sync)
 	if (ei->start_clu == EXFAT_EOF_CLUSTER)
 		on_disk_size = 0;
 
-	ep2->dentry.stream.valid_size = cpu_to_le64(on_disk_size);
-	ep2->dentry.stream.size = ep2->dentry.stream.valid_size;
+	ep2->dentry.stream.valid_size = cpu_to_le64(ei->valid_size);
+	ep2->dentry.stream.size = cpu_to_le64(on_disk_size);
 	if (on_disk_size) {
 		ep2->dentry.stream.flags = ei->flags;
 		ep2->dentry.stream.start_clu = cpu_to_le32(ei->start_clu);
@@ -276,6 +276,7 @@  static int exfat_get_block(struct inode *inode, sector_t iblock,
 	sector_t last_block;
 	sector_t phys = 0;
 	loff_t pos;
+	size_t b_size = bh_result->b_size;
 
 	mutex_lock(&sbi->s_lock);
 	last_block = EXFAT_B_TO_BLK_ROUND_UP(i_size_read(inode), sb);
@@ -303,17 +304,25 @@  static int exfat_get_block(struct inode *inode, sector_t iblock,
 	mapped_blocks = sbi->sect_per_clus - sec_offset;
 	max_blocks = min(mapped_blocks, max_blocks);
 
-	/* Treat newly added block / cluster */
-	if (iblock < last_block)
-		create = 0;
-
-	if (create || buffer_delay(bh_result)) {
-		pos = EXFAT_BLK_TO_B((iblock + 1), sb);
+	pos = EXFAT_BLK_TO_B((iblock + 1), sb);
+	if ((create && iblock >= last_block) || buffer_delay(bh_result)) {
 		if (ei->i_size_ondisk < pos)
 			ei->i_size_ondisk = pos;
 	}
 
+	map_bh(bh_result, sb, phys);
+	if (buffer_delay(bh_result))
+		clear_buffer_delay(bh_result);
+
 	if (create) {
+		sector_t valid_blks;
+
+		valid_blks = EXFAT_B_TO_BLK_ROUND_UP(ei->valid_size, sb);
+		if (iblock < valid_blks && iblock + max_blocks >= valid_blks) {
+			max_blocks = valid_blks - iblock;
+			goto done;
+		}
+
 		err = exfat_map_new_buffer(ei, bh_result, pos);
 		if (err) {
 			exfat_fs_error(sb,
@@ -321,11 +330,40 @@  static int exfat_get_block(struct inode *inode, sector_t iblock,
 					pos, ei->i_size_aligned);
 			goto unlock_ret;
 		}
-	}
 
-	if (buffer_delay(bh_result))
-		clear_buffer_delay(bh_result);
-	map_bh(bh_result, sb, phys);
+		if (pos - sb->s_blocksize + b_size > ei->valid_size) {
+			ei->valid_size = pos - sb->s_blocksize + b_size;
+			mark_inode_dirty(inode);
+		}
+	} else {
+		size_t b_size = EXFAT_BLK_TO_B(max_blocks, sb);
+
+		pos = EXFAT_BLK_TO_B(iblock, sb);
+		if (pos >= ei->valid_size) {
+			/* Read out of valid data */
+			clear_buffer_mapped(bh_result);
+		} else if (pos + b_size <= ei->valid_size) {
+			/* Normal read */
+		} else if (pos + sb->s_blocksize <= ei->valid_size) {
+			/* Normal short read */
+			max_blocks = 1;
+		} else {
+			/* Read across valid size */
+			if (bh_result->b_page) {
+				loff_t size = ei->valid_size - pos;
+				loff_t off = pos & (PAGE_SIZE - 1);
+
+				set_bh_page(bh_result, bh_result->b_page, off);
+				err = bh_read(bh_result, 0);
+				if (err < 0)
+					goto unlock_ret;
+
+				zero_user_segment(bh_result->b_page, off + size,
+						off + sb->s_blocksize);
+			}
+			max_blocks = 1;
+		}
+	}
 done:
 	bh_result->b_size = EXFAT_BLK_TO_B(max_blocks, sb);
 unlock_ret:
@@ -340,6 +378,17 @@  static int exfat_read_folio(struct file *file, struct folio *folio)
 
 static void exfat_readahead(struct readahead_control *rac)
 {
+	struct address_space *mapping = rac->mapping;
+	struct inode *inode = mapping->host;
+	struct exfat_inode_info *ei = EXFAT_I(inode);
+	loff_t pos = readahead_pos(rac);
+
+	/* Range cross valid_size, read it page by page. */
+	if (ei->valid_size < i_size_read(inode) &&
+	    pos <= ei->valid_size &&
+	    ei->valid_size < pos + readahead_length(rac))
+		return;
+
 	mpage_readahead(rac, exfat_get_block);
 }
 
@@ -410,6 +459,8 @@  static ssize_t exfat_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct address_space *mapping = iocb->ki_filp->f_mapping;
 	struct inode *inode = mapping->host;
+	struct exfat_inode_info *ei = EXFAT_I(inode);
+	loff_t pos = iocb->ki_pos;
 	loff_t size = iocb->ki_pos + iov_iter_count(iter);
 	int rw = iov_iter_rw(iter);
 	ssize_t ret;
@@ -433,8 +484,20 @@  static ssize_t exfat_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 	 * condition of exfat_get_block() and ->truncate().
 	 */
 	ret = blockdev_direct_IO(iocb, inode, iter, exfat_get_block);
-	if (ret < 0 && (rw & WRITE))
-		exfat_write_failed(mapping, size);
+	if (ret < 0) {
+		if (rw & WRITE)
+			exfat_write_failed(mapping, size);
+
+		if (ret != -EIOCBQUEUED)
+			return ret;
+	} else
+		size = pos + ret;
+
+	if ((rw & READ) && pos < ei->valid_size && ei->valid_size < size) {
+		iov_iter_revert(iter, size - ei->valid_size);
+		iov_iter_zero(size - ei->valid_size, iter);
+	}
+
 	return ret;
 }
 
@@ -534,6 +597,7 @@  static int exfat_fill_inode(struct inode *inode, struct exfat_dir_entry *info)
 	ei->start_clu = info->start_clu;
 	ei->flags = info->flags;
 	ei->type = info->type;
+	ei->valid_size = info->valid_size;
 
 	ei->version = 0;
 	ei->hint_stat.eidx = 0;
diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index e0ff9d156f6f..ceaa720804b9 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -395,6 +395,7 @@  static int exfat_find_empty_entry(struct inode *inode,
 		i_size_write(inode, size);
 		ei->i_size_ondisk += sbi->cluster_size;
 		ei->i_size_aligned += sbi->cluster_size;
+		ei->valid_size += sbi->cluster_size;
 		ei->flags = p_dir->flags;
 		inode->i_blocks += sbi->cluster_size >> 9;
 	}
@@ -544,6 +545,8 @@  static int exfat_add_entry(struct inode *inode, const char *path,
 		info->size = clu_size;
 		info->num_subdirs = EXFAT_MIN_SUBDIR;
 	}
+	info->valid_size = info->size;
+
 	memset(&info->crtime, 0, sizeof(info->crtime));
 	memset(&info->mtime, 0, sizeof(info->mtime));
 	memset(&info->atime, 0, sizeof(info->atime));
@@ -645,7 +648,8 @@  static int exfat_find(struct inode *dir, struct qstr *qname,
 
 	info->type = exfat_get_entry_type(ep);
 	info->attr = le16_to_cpu(ep->dentry.file.attr);
-	info->size = le64_to_cpu(ep2->dentry.stream.valid_size);
+	info->valid_size = le64_to_cpu(ep2->dentry.stream.valid_size);
+	info->size = le64_to_cpu(ep2->dentry.stream.size);
 	if ((info->type == TYPE_FILE) && (info->size == 0)) {
 		info->flags = ALLOC_NO_FAT_CHAIN;
 		info->start_clu = EXFAT_EOF_CLUSTER;
@@ -1271,6 +1275,7 @@  static int __exfat_rename(struct inode *old_parent_inode,
 			}
 
 			i_size_write(new_inode, 0);
+			new_ei->valid_size = 0;
 			new_ei->start_clu = EXFAT_EOF_CLUSTER;
 			new_ei->flags = ALLOC_NO_FAT_CHAIN;
 		}