2020-10-02 06:10:01

by Tetsuhiro Kohada

[permalink] [raw]
Subject: [PATCH v3 2/2] exfat: aggregate dir-entry updates into __exfat_write_inode().

The following function writes the updated inode information as dir-entry
by themselves.
- __exfat_truncate()
- exfat_map_cluster()
- exfat_find_empty_entry()
Aggregate these writes into __exfat_write_inode().

In exfat_map_cluster(), the value obtained from i_size_read() is set to
stream.valid_size and stream.size.
However, in the context of get_block(), inode->i_size has not been set yet,
so the same value as current will be set, which is a meaningless update.
Furthermore, if it is called with previous size=0, the newly allocated
cluster number will be set to stream.start_clu, and stream.valid_size/size
will be 0, which is illegal.
Update stream.valid_size/size and stream.start_clu when __exfat_write_inode
is called after i_size is set, to prevent meaningless/illegal updates.

Others:
- Remove double inode-update in __exfat_truncate() and exfat_truncate().
- In __exfat_write_inode(), rename 'on_disk_size' to 'filesize' and
add adjustment when filesize is 0.

Signed-off-by: Tetsuhiro Kohada <[email protected]>
---
Changes in v3
- Remove update_inode() in exfat_map_cluster()/exfat_truncate()
- Update commit-message
Changes in v2
- Fix endian issue

fs/exfat/file.c | 52 +++++-------------------------------------------
fs/exfat/inode.c | 47 +++++++++++--------------------------------
fs/exfat/namei.c | 26 +++++-------------------
3 files changed, 22 insertions(+), 103 deletions(-)

diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index e510b95dbf77..211fb947747a 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -100,7 +100,7 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
struct super_block *sb = inode->i_sb;
struct exfat_sb_info *sbi = EXFAT_SB(sb);
struct exfat_inode_info *ei = EXFAT_I(inode);
- int evict = (ei->dir.dir == DIR_DELETED) ? 1 : 0;
+ int ret;

/* check if the given file ID is opened */
if (ei->type != TYPE_FILE && ei->type != TYPE_DIR)
@@ -150,49 +150,10 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
ei->attr |= ATTR_ARCHIVE;

/* update the directory entry */
- if (!evict) {
- struct timespec64 ts;
- struct exfat_dentry *ep, *ep2;
- struct exfat_entry_set_cache *es;
- int err;
-
- es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry,
- ES_ALL_ENTRIES);
- if (!es)
- return -EIO;
- ep = exfat_get_dentry_cached(es, 0);
- ep2 = exfat_get_dentry_cached(es, 1);
-
- ts = current_time(inode);
- exfat_set_entry_time(sbi, &ts,
- &ep->dentry.file.modify_tz,
- &ep->dentry.file.modify_time,
- &ep->dentry.file.modify_date,
- &ep->dentry.file.modify_time_cs);
- ep->dentry.file.attr = cpu_to_le16(ei->attr);
-
- /* File size should be zero if there is no cluster allocated */
- if (ei->start_clu == EXFAT_EOF_CLUSTER) {
- ep2->dentry.stream.valid_size = 0;
- ep2->dentry.stream.size = 0;
- } else {
- ep2->dentry.stream.valid_size = cpu_to_le64(new_size);
- ep2->dentry.stream.size = ep2->dentry.stream.valid_size;
- }
-
- if (new_size == 0) {
- /* Any directory can not be truncated to zero */
- WARN_ON(ei->type != TYPE_FILE);
-
- ep2->dentry.stream.flags = ALLOC_FAT_CHAIN;
- ep2->dentry.stream.start_clu = EXFAT_FREE_CLUSTER;
- }
-
- exfat_update_dir_chksum_with_entry_set(es);
- err = exfat_free_dentry_set(es, inode_needs_sync(inode));
- if (err)
- return err;
- }
+ inode->i_ctime = inode->i_mtime = current_time(inode);
+ ret = exfat_update_inode(inode);
+ if (ret)
+ return ret;

/* cut off from the FAT chain */
if (ei->flags == ALLOC_FAT_CHAIN && last_clu != EXFAT_FREE_CLUSTER &&
@@ -244,9 +205,6 @@ void exfat_truncate(struct inode *inode, loff_t size)
if (err)
goto write_size;

- inode->i_ctime = inode->i_mtime = current_time(inode);
- exfat_update_inode(inode);
-
inode->i_blocks = ((i_size_read(inode) + (sbi->cluster_size - 1)) &
~(sbi->cluster_size - 1)) >> inode->i_blkbits;
write_size:
diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
index 5a55303e1f65..cf29b14ce7f9 100644
--- a/fs/exfat/inode.c
+++ b/fs/exfat/inode.c
@@ -19,7 +19,7 @@

static int __exfat_write_inode(struct inode *inode, int sync)
{
- unsigned long long on_disk_size;
+ unsigned long long filesize;
struct exfat_dentry *ep, *ep2;
struct exfat_entry_set_cache *es = NULL;
struct super_block *sb = inode->i_sb;
@@ -68,13 +68,14 @@ static int __exfat_write_inode(struct inode *inode, int sync)
NULL);

/* File size should be zero if there is no cluster allocated */
- on_disk_size = i_size_read(inode);
-
+ filesize = i_size_read(inode);
if (ei->start_clu == EXFAT_EOF_CLUSTER)
- on_disk_size = 0;
+ filesize = 0;

- ep2->dentry.stream.valid_size = cpu_to_le64(on_disk_size);
+ ep2->dentry.stream.valid_size = cpu_to_le64(filesize);
ep2->dentry.stream.size = ep2->dentry.stream.valid_size;
+ ep2->dentry.stream.flags = filesize ? ei->flags : ALLOC_FAT_CHAIN;
+ ep2->dentry.stream.start_clu = cpu_to_le32(filesize ? ei->start_clu : EXFAT_FREE_CLUSTER);

exfat_update_dir_chksum_with_entry_set(es);
return exfat_free_dentry_set(es, sync);
@@ -110,7 +111,7 @@ int exfat_update_inode(struct inode *inode)
static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
unsigned int *clu, int create)
{
- int ret, modified = false;
+ int ret;
unsigned int last_clu;
struct exfat_chain new_clu;
struct super_block *sb = inode->i_sb;
@@ -184,6 +185,11 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
return -EIO;
}

+ exfat_warn(sb, "alloc[%lu]@map: %lld (%d - %08x)",
+ inode->i_ino, i_size_read(inode),
+ (clu_offset << sbi->sect_per_clus_bits) * 512,
+ last_clu);
+
ret = exfat_alloc_cluster(inode, num_to_be_allocated, &new_clu);
if (ret)
return ret;
@@ -201,7 +207,6 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
if (new_clu.flags == ALLOC_FAT_CHAIN)
ei->flags = ALLOC_FAT_CHAIN;
ei->start_clu = new_clu.dir;
- modified = true;
} else {
if (new_clu.flags != ei->flags) {
/* no-fat-chain bit is disabled,
@@ -211,7 +216,6 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
exfat_chain_cont_cluster(sb, ei->start_clu,
num_clusters);
ei->flags = ALLOC_FAT_CHAIN;
- modified = true;
}
if (new_clu.flags == ALLOC_FAT_CHAIN)
if (exfat_ent_set(sb, last_clu, new_clu.dir))
@@ -221,33 +225,6 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
num_clusters += num_to_be_allocated;
*clu = new_clu.dir;

- if (ei->dir.dir != DIR_DELETED && modified) {
- struct exfat_dentry *ep;
- struct exfat_entry_set_cache *es;
- int err;
-
- es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry,
- ES_ALL_ENTRIES);
- if (!es)
- return -EIO;
- /* get stream entry */
- ep = exfat_get_dentry_cached(es, 1);
-
- /* update directory entry */
- ep->dentry.stream.flags = ei->flags;
- ep->dentry.stream.start_clu =
- cpu_to_le32(ei->start_clu);
- ep->dentry.stream.valid_size =
- cpu_to_le64(i_size_read(inode));
- ep->dentry.stream.size =
- ep->dentry.stream.valid_size;
-
- exfat_update_dir_chksum_with_entry_set(es);
- err = exfat_free_dentry_set(es, inode_needs_sync(inode));
- if (err)
- return err;
- } /* end of if != DIR_DELETED */
-
inode->i_blocks +=
num_to_be_allocated << sbi->sect_per_clus_bits;

diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index 4eb7cb528e97..ef0ea031ed9e 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -306,10 +306,8 @@ static int exfat_find_empty_entry(struct inode *inode,
{
int dentry;
unsigned int ret, last_clu;
- sector_t sector;
loff_t size = 0;
struct exfat_chain clu;
- struct exfat_dentry *ep = NULL;
struct super_block *sb = inode->i_sb;
struct exfat_sb_info *sbi = EXFAT_SB(sb);
struct exfat_inode_info *ei = EXFAT_I(inode);
@@ -374,31 +372,17 @@ static int exfat_find_empty_entry(struct inode *inode,
p_dir->size++;
size = EXFAT_CLU_TO_B(p_dir->size, sbi);

- /* update the directory entry */
- if (p_dir->dir != sbi->root_dir) {
- struct buffer_head *bh;
-
- ep = exfat_get_dentry(sb,
- &(ei->dir), ei->entry + 1, &bh, &sector);
- if (!ep)
- return -EIO;
-
- ep->dentry.stream.valid_size = cpu_to_le64(size);
- ep->dentry.stream.size = ep->dentry.stream.valid_size;
- ep->dentry.stream.flags = p_dir->flags;
- exfat_update_bh(bh, IS_DIRSYNC(inode));
- brelse(bh);
- if (exfat_update_dir_chksum(inode, &(ei->dir),
- ei->entry))
- return -EIO;
- }
-
/* directory inode should be updated in here */
i_size_write(inode, size);
EXFAT_I(inode)->i_size_ondisk += sbi->cluster_size;
EXFAT_I(inode)->i_size_aligned += sbi->cluster_size;
EXFAT_I(inode)->flags = p_dir->flags;
inode->i_blocks += 1 << sbi->sect_per_clus_bits;
+
+ /* update the directory entry */
+ ret = exfat_update_inode(inode);
+ if (ret)
+ return ret;
}

return dentry;
--
2.25.1


2020-10-16 08:00:21

by Namjae Jeon

[permalink] [raw]
Subject: RE: [PATCH v3 2/2] exfat: aggregate dir-entry updates into __exfat_write_inode().

> *inode) static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
> unsigned int *clu, int create)
> {
> - int ret, modified = false;
> + int ret;
> unsigned int last_clu;
> struct exfat_chain new_clu;
> struct super_block *sb = inode->i_sb;
> @@ -184,6 +185,11 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
> return -EIO;
> }
>
> + exfat_warn(sb, "alloc[%lu]@map: %lld (%d - %08x)",
> + inode->i_ino, i_size_read(inode),
> + (clu_offset << sbi->sect_per_clus_bits) * 512,
> + last_clu);
Is this leftover print from debugging?

2020-10-19 09:54:00

by Tetsuhiro Kohada

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] exfat: aggregate dir-entry updates into __exfat_write_inode().

>> @@ -184,6 +185,11 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
>> return -EIO;
>> }
>>
>> + exfat_warn(sb, "alloc[%lu]@map: %lld (%d - %08x)",
>> + inode->i_ino, i_size_read(inode),
>> + (clu_offset << sbi->sect_per_clus_bits) * 512,
>> + last_clu);
> Is this leftover print from debugging?
>
Oops!
Yes, just as you said.
I will post V4 soon.
Is there any other problem?


BR
---
Tetsuhiro Kohada <[email protected]>