Re: [PATCH v2] exfat: remove EXFAT_SB_DIRTY flag

2020-06-15 Thread Tetsuhiro Kohada 

On 2020/06/16 11:03, Sungjong Seo wrote:

remove EXFAT_SB_DIRTY flag and related codes.

This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid
sync_blockdev().
However ...
- exfat_put_super():
Before calling this, the VFS has already called sync_filesystem(), so sync
is never performed here.
- exfat_sync_fs():
After calling this, the VFS calls sync_blockdev(), so, it is meaningless
to check EXFAT_SB_DIRTY or to bypass sync_blockdev() here.
Not only that, but in some cases can't clear VOL_DIRTY.
ex:
VOL_DIRTY is set when rmdir starts, but when non-empty-dir is detected,
return error without setting EXFAT_SB_DIRTY.
If performe 'sync' in this state, VOL_DIRTY will not be cleared.

Remove the EXFAT_SB_DIRTY check to ensure synchronization.
And, remove the code related to the flag.

Suggested-by: Sungjong Seo 
Signed-off-by: Tetsuhiro Kohada 
---
Changes in v2:
  - exfat_sync_fs() avoids synchronous processing when wait=0

  fs/exfat/balloc.c   |  4 ++--
  fs/exfat/dir.c  | 16 
  fs/exfat/exfat_fs.h |  5 +
  fs/exfat/fatent.c   |  7 ++-
  fs/exfat/misc.c |  3 +--
  fs/exfat/namei.c| 12 ++--
  fs/exfat/super.c| 14 ++
  7 files changed, 26 insertions(+), 35 deletions(-)

diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c index
4055eb00ea9b..a987919686c0 100644
--- a/fs/exfat/balloc.c
+++ b/fs/exfat/balloc.c
@@ -158,7 +158,7 @@ int exfat_set_bitmap(struct inode *inode, unsigned int
clu)
b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx);

set_bit_le(b, sbi->vol_amap[i]->b_data);
-   exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode));
+   exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode));
return 0;
  }

@@ -180,7 +180,7 @@ void exfat_clear_bitmap(struct inode *inode, unsigned
int clu)
b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx);

clear_bit_le(b, sbi->vol_amap[i]->b_data);
-   exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode));
+   exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode));

if (opts->discard) {
int ret_discard;
diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
8e775bd5d523..02acbb6ddf02 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -470,7 +470,7 @@ int exfat_init_dir_entry(struct inode *inode, struct
exfat_chain *p_dir,
>dentry.file.access_date,
NULL);

-   exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
+   exfat_update_bh(bh, IS_DIRSYNC(inode));
brelse(bh);

ep = exfat_get_dentry(sb, p_dir, entry + 1, , ); @@ -
480,7 +480,7 @@ int exfat_init_dir_entry(struct inode *inode, struct
exfat_chain *p_dir,
exfat_init_stream_entry(ep,
(type == TYPE_FILE) ? ALLOC_FAT_CHAIN : ALLOC_NO_FAT_CHAIN,
start_clu, size);
-   exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
+   exfat_update_bh(bh, IS_DIRSYNC(inode));
brelse(bh);

return 0;
@@ -516,7 +516,7 @@ int exfat_update_dir_chksum(struct inode *inode,
struct exfat_chain *p_dir,
}

fep->dentry.file.checksum = cpu_to_le16(chksum);
-   exfat_update_bh(sb, fbh, IS_DIRSYNC(inode));
+   exfat_update_bh(fbh, IS_DIRSYNC(inode));
  release_fbh:
brelse(fbh);
return ret;
@@ -538,7 +538,7 @@ int exfat_init_ext_entry(struct inode *inode, struct
exfat_chain *p_dir,
return -EIO;

ep->dentry.file.num_ext = (unsigned char)(num_entries - 1);
-   exfat_update_bh(sb, bh, sync);
+   exfat_update_bh(bh, sync);
brelse(bh);

ep = exfat_get_dentry(sb, p_dir, entry + 1, , ); @@ -
547,7 +547,7 @@ int exfat_init_ext_entry(struct inode *inode, struct
exfat_chain *p_dir,

ep->dentry.stream.name_len = p_uniname->name_len;
ep->dentry.stream.name_hash = cpu_to_le16(p_uniname->name_hash);
-   exfat_update_bh(sb, bh, sync);
+   exfat_update_bh(bh, sync);
brelse(bh);

for (i = EXFAT_FIRST_CLUSTER; i < num_entries; i++) { @@ -556,7
+556,7 @@ int exfat_init_ext_entry(struct inode *inode, struct exfat_chain
*p_dir,
return -EIO;

exfat_init_name_entry(ep, uniname);
-   exfat_update_bh(sb, bh, sync);
+   exfat_update_bh(bh, sync);
brelse(bh);
uniname += EXFAT_FILE_NAME_LEN;
}
@@ -580,7 +580,7 @@ int exfat_remove_entries(struct inode *inode, struct
exfat_chain *p_dir,
return -EIO;

exfat_set_entry_type(ep, TYPE_DELETED);
-   exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
+   exfat_update_bh(bh, IS_DIRSYNC(inode));
brelse(bh);
}

@@ -610,7 +610,7 @@ void exfat_free_dentry_set(struct
exfat_entry_set_cache *es, int sync)

for (i = 0; i < es->num_bh; i++) {
if (es->modified)
-   exfat_update_bh(es->sb, es->bh[i], sync);
+

RE: [PATCH v2] exfat: remove EXFAT_SB_DIRTY flag

2020-06-15 Thread Sungjong Seo 
> remove EXFAT_SB_DIRTY flag and related codes.
> 
> This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid
> sync_blockdev().
> However ...
> - exfat_put_super():
> Before calling this, the VFS has already called sync_filesystem(), so sync
> is never performed here.
> - exfat_sync_fs():
> After calling this, the VFS calls sync_blockdev(), so, it is meaningless
> to check EXFAT_SB_DIRTY or to bypass sync_blockdev() here.
> Not only that, but in some cases can't clear VOL_DIRTY.
> ex:
> VOL_DIRTY is set when rmdir starts, but when non-empty-dir is detected,
> return error without setting EXFAT_SB_DIRTY.
> If performe 'sync' in this state, VOL_DIRTY will not be cleared.
> 
> Remove the EXFAT_SB_DIRTY check to ensure synchronization.
> And, remove the code related to the flag.
> 
> Suggested-by: Sungjong Seo 
> Signed-off-by: Tetsuhiro Kohada 
> ---
> Changes in v2:
>  - exfat_sync_fs() avoids synchronous processing when wait=0
> 
>  fs/exfat/balloc.c   |  4 ++--
>  fs/exfat/dir.c  | 16 
>  fs/exfat/exfat_fs.h |  5 +
>  fs/exfat/fatent.c   |  7 ++-
>  fs/exfat/misc.c |  3 +--
>  fs/exfat/namei.c| 12 ++--
>  fs/exfat/super.c| 14 ++
>  7 files changed, 26 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c index
> 4055eb00ea9b..a987919686c0 100644
> --- a/fs/exfat/balloc.c
> +++ b/fs/exfat/balloc.c
> @@ -158,7 +158,7 @@ int exfat_set_bitmap(struct inode *inode, unsigned int
> clu)
>   b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx);
> 
>   set_bit_le(b, sbi->vol_amap[i]->b_data);
> - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode));
> + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode));
>   return 0;
>  }
> 
> @@ -180,7 +180,7 @@ void exfat_clear_bitmap(struct inode *inode, unsigned
> int clu)
>   b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx);
> 
>   clear_bit_le(b, sbi->vol_amap[i]->b_data);
> - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode));
> + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode));
> 
>   if (opts->discard) {
>   int ret_discard;
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> 8e775bd5d523..02acbb6ddf02 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -470,7 +470,7 @@ int exfat_init_dir_entry(struct inode *inode, struct
> exfat_chain *p_dir,
>   >dentry.file.access_date,
>   NULL);
> 
> - exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
> + exfat_update_bh(bh, IS_DIRSYNC(inode));
>   brelse(bh);
> 
>   ep = exfat_get_dentry(sb, p_dir, entry + 1, , ); @@ -
> 480,7 +480,7 @@ int exfat_init_dir_entry(struct inode *inode, struct
> exfat_chain *p_dir,
>   exfat_init_stream_entry(ep,
>   (type == TYPE_FILE) ? ALLOC_FAT_CHAIN : ALLOC_NO_FAT_CHAIN,
>   start_clu, size);
> - exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
> + exfat_update_bh(bh, IS_DIRSYNC(inode));
>   brelse(bh);
> 
>   return 0;
> @@ -516,7 +516,7 @@ int exfat_update_dir_chksum(struct inode *inode,
> struct exfat_chain *p_dir,
>   }
> 
>   fep->dentry.file.checksum = cpu_to_le16(chksum);
> - exfat_update_bh(sb, fbh, IS_DIRSYNC(inode));
> + exfat_update_bh(fbh, IS_DIRSYNC(inode));
>  release_fbh:
>   brelse(fbh);
>   return ret;
> @@ -538,7 +538,7 @@ int exfat_init_ext_entry(struct inode *inode, struct
> exfat_chain *p_dir,
>   return -EIO;
> 
>   ep->dentry.file.num_ext = (unsigned char)(num_entries - 1);
> - exfat_update_bh(sb, bh, sync);
> + exfat_update_bh(bh, sync);
>   brelse(bh);
> 
>   ep = exfat_get_dentry(sb, p_dir, entry + 1, , ); @@ -
> 547,7 +547,7 @@ int exfat_init_ext_entry(struct inode *inode, struct
> exfat_chain *p_dir,
> 
>   ep->dentry.stream.name_len = p_uniname->name_len;
>   ep->dentry.stream.name_hash = cpu_to_le16(p_uniname->name_hash);
> - exfat_update_bh(sb, bh, sync);
> + exfat_update_bh(bh, sync);
>   brelse(bh);
> 
>   for (i = EXFAT_FIRST_CLUSTER; i < num_entries; i++) { @@ -556,7
> +556,7 @@ int exfat_init_ext_entry(struct inode *inode, struct exfat_chain
> *p_dir,
>   return -EIO;
> 
>   exfat_init_name_entry(ep, uniname);
> - exfat_update_bh(sb, bh, sync);
> + exfat_update_bh(bh, sync);
>   brelse(bh);
>   uniname += EXFAT_FILE_NAME_LEN;
>   }
> @@ -580,7 +580,7 @@ int exfat_remove_entries(struct inode *inode, struct
> exfat_chain *p_dir,
>   return -EIO;
> 
>   exfat_set_entry_type(ep, TYPE_DELETED);
> - exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
> + exfat_update_bh(bh, IS_DIRSYNC(inode));
>   brelse(bh);
>   }
> 
> @@ -610,7 +610,7 @@ void exfat_free_dentry_set(struct
> exfat_entry_set_cache *es, int sync)
> 
>   for (i = 0; i < es->num_bh; i++) {
>   if (es->modified)