[v2,2/2] exfat: do not zeroed the extended part

Commit Message

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

Since the read operation beyond the ValidDataLength returns zero,
if we just extend the size of the file, we don't need to zero the
extended part, but only change the DataLength without changing
the ValidDataLength.

Signed-off-by: Yuezhang Mo <[e-mail]>
Reviewed-by: Andy Wu <[e-mail]>
Reviewed-by: Aoyama Wataru <[e-mail]>
---
 fs/exfat/file.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

Comments

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

[snip]
> +static int exfat_cont_expand(struct inode *inode, loff_t size)
> +{
> +	if (mapping_writably_mapped(inode->i_mapping))
Could you elaborate why mapping_writably_mapped is used here instead
of comparing new size and old size ?

Thanks.
> +		return exfat_expand_and_zero(inode, size);
> +
> +	return exfat_expand(inode, size);
> +}
> +
>  static bool exfat_allow_set_time(struct exfat_sb_info *sbi, struct inode
> *inode)
>  {
>  	mode_t allow_utime = sbi->options.allow_utime;
> --
> 2.25.1
>
>

[e-mail] Aug. 15, 2023, 5:45 a.m. UTC | #2

> -----Original Message-----
> From: Namjae Jeon <[e-mail]>
> Sent: Monday, August 14, 2023 12:39 PM
> To: Mo, Yuezhang <[e-mail]>
> Cc: [e-mail]; [e-mail]; Wu, Andy
> <[e-mail]>; Aoyama, Wataru (SGC) <[e-mail]>
> Subject: Re: [PATCH v2 2/2] exfat: do not zeroed the extended part
> 
> [snip]
> > +static int exfat_cont_expand(struct inode *inode, loff_t size) {
> > +	if (mapping_writably_mapped(inode->i_mapping))
> Could you elaborate why mapping_writably_mapped is used here instead of
> comparing new size and old size ?
> 
> Thanks.
> > +		return exfat_expand_and_zero(inode, size);
> > +
> > +	return exfat_expand(inode, size);
> > +}
> > +
> >  static bool exfat_allow_set_time(struct exfat_sb_info *sbi, struct
> > inode
> > *inode)
> >  {
> >  	mode_t allow_utime = sbi->options.allow_utime;
> > --
> > 2.25.1
> >
> >

In the following case, since exfat_file_write_iter() is not called. If without
mapping_writably_mapped() and remove exfat_expand_and_zero(),
->valid_size is not fixup to the written size.

+ rm -fr /mnt/test/testfile
+ xfs_io -t -f -c 'truncate 5121' -c 'mmap -rw 0 5121' -c 'truncate 2047' -c 'truncate 5121' -c 'mwrite -S 0x59 2047 3071' -c close /mnt/test/testfile
+ umount /mnt/test
+ fsck.exfat /dev/sdc1
exfatprogs version : 1.2.1
ERROR: /testfile: valid size 5632 greater than size 5121 at 0x528ba0. Fix (y/N)? n
/dev/sdc1: corrupted. directories 58, files 4261
/dev/sdc1: files corrupted 1, files fixed 0

If moving fixup ->valid_size into __exfat_write_inode(), maybe we can remove exfat_expand_and_zero().

--- a/fs/exfat/inode.c
+++ b/fs/exfat/inode.c
@@ -72,6 +72,9 @@ int __exfat_write_inode(struct inode *inode, int sync)
        if (ei->start_clu == EXFAT_EOF_CLUSTER)
                on_disk_size = 0;

+       if (on_disk_size < ei->valid_size)
+               ei->valid_size = i_size_read(inode);
+
        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) {

If fixup ->valid_size in __exfat_write_inode(), it is not only for above case.
Do you think fixup ->valid_size in __exfat_write_inode() is acceptable?

Patch

diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index 8cd14bc16857..62a21c45517d 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -13,7 +13,7 @@ 
 #include "exfat_raw.h"
 #include "exfat_fs.h"
 
-static int exfat_cont_expand(struct inode *inode, loff_t size)
+static int exfat_expand_and_zero(struct inode *inode, loff_t size)
 {
 	struct address_space *mapping = inode->i_mapping;
 	loff_t start = i_size_read(inode), count = size - i_size_read(inode);
@@ -43,6 +43,81 @@  static int exfat_cont_expand(struct inode *inode, loff_t size)
 	return filemap_fdatawait_range(mapping, start, start + count - 1);
 }
 
+static int exfat_expand(struct inode *inode, loff_t size)
+{
+	int ret;
+	unsigned int num_clusters, new_num_clusters, last_clu;
+	struct exfat_inode_info *ei = EXFAT_I(inode);
+	struct super_block *sb = inode->i_sb;
+	struct exfat_sb_info *sbi = EXFAT_SB(sb);
+	struct exfat_chain clu;
+
+	ret = inode_newsize_ok(inode, size);
+	if (ret)
+		return ret;
+
+	num_clusters = EXFAT_B_TO_CLU_ROUND_UP(i_size_read(inode), sbi);
+	new_num_clusters = EXFAT_B_TO_CLU_ROUND_UP(size, sbi);
+
+	if (new_num_clusters == num_clusters)
+		goto out;
+
+	exfat_chain_set(&clu, ei->start_clu, num_clusters, ei->flags);
+	ret = exfat_find_last_cluster(sb, &clu, &last_clu);
+	if (ret)
+		return ret;
+
+	clu.dir = (last_clu == EXFAT_EOF_CLUSTER) ?
+			EXFAT_EOF_CLUSTER : last_clu + 1;
+	clu.size = 0;
+	clu.flags = ei->flags;
+
+	ret = exfat_alloc_cluster(inode, new_num_clusters - num_clusters,
+			&clu, IS_DIRSYNC(inode));
+	if (ret)
+		return ret;
+
+	/* Append new clusters to chain */
+	if (clu.flags != ei->flags) {
+		exfat_chain_cont_cluster(sb, ei->start_clu, num_clusters);
+		ei->flags = ALLOC_FAT_CHAIN;
+	}
+	if (clu.flags == ALLOC_FAT_CHAIN)
+		if (exfat_ent_set(sb, last_clu, clu.dir))
+			goto free_clu;
+
+	if (num_clusters == 0)
+		ei->start_clu = clu.dir;
+
+out:
+	inode->i_ctime = inode->i_mtime = current_time(inode);
+	/* Expanded range not zeroed, do not update valid_size */
+	i_size_write(inode, size);
+
+	ei->i_size_aligned = round_up(size, sb->s_blocksize);
+	ei->i_size_ondisk = ei->i_size_aligned;
+	inode->i_blocks = round_up(size, sbi->cluster_size) >> 9;
+
+	if (IS_DIRSYNC(inode))
+		return write_inode_now(inode, 1);
+
+	mark_inode_dirty(inode);
+
+	return 0;
+
+free_clu:
+	exfat_free_cluster(inode, &clu);
+	return -EIO;
+}
+
+static int exfat_cont_expand(struct inode *inode, loff_t size)
+{
+	if (mapping_writably_mapped(inode->i_mapping))
+		return exfat_expand_and_zero(inode, size);
+
+	return exfat_expand(inode, size);
+}
+
 static bool exfat_allow_set_time(struct exfat_sb_info *sbi, struct inode *inode)
 {
 	mode_t allow_utime = sbi->options.allow_utime;