* [PATCH] exfat: remove EXFAT_SB_DIRTY flag @ 2020-06-12 1:28 ` Tetsuhiro Kohada 2020-06-12 8:34 ` Sungjong Seo 0 siblings, 1 reply; 7+ messages in thread From: Tetsuhiro Kohada @ 2020-06-12 1:28 UTC (permalink / raw) To: kohada.t2 Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, Namjae Jeon, Sungjong Seo, linux-fsdevel, linux-kernel 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. Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> --- 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 | 11 +++-------- 7 files changed, 23 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, &ep->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, &bh, §or); @@ -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, &bh, §or); @@ -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); + exfat_update_bh(es->bh[i], sync); brelse(es->bh[i]); } kfree(es); diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index 595f3117f492..84664024e51e 100644 --- a/fs/exfat/exfat_fs.h +++ b/fs/exfat/exfat_fs.h @@ -13,8 +13,6 @@ #define EXFAT_SUPER_MAGIC 0x2011BAB0UL #define EXFAT_ROOT_INO 1 -#define EXFAT_SB_DIRTY 0 - #define EXFAT_CLUSTERS_UNTRACKED (~0u) /* @@ -238,7 +236,6 @@ struct exfat_sb_info { unsigned int clu_srch_ptr; /* cluster search pointer */ unsigned int used_clusters; /* number of used clusters */ - unsigned long s_state; struct mutex s_lock; /* superblock lock */ struct exfat_mount_options options; struct nls_table *nls_io; /* Charset used for input and display */ @@ -514,7 +511,7 @@ void exfat_set_entry_time(struct exfat_sb_info *sbi, struct timespec64 *ts, u8 *tz, __le16 *time, __le16 *date, u8 *time_cs); u16 exfat_calc_chksum16(void *data, int len, u16 chksum, int type); u32 exfat_calc_chksum32(void *data, int len, u32 chksum, int type); -void exfat_update_bh(struct super_block *sb, struct buffer_head *bh, int sync); +void exfat_update_bh(struct buffer_head *bh, int sync); void exfat_chain_set(struct exfat_chain *ec, unsigned int dir, unsigned int size, unsigned char flags); void exfat_chain_dup(struct exfat_chain *dup, struct exfat_chain *ec); diff --git a/fs/exfat/fatent.c b/fs/exfat/fatent.c index 4e5c5c9c0f2d..82ee8246c080 100644 --- a/fs/exfat/fatent.c +++ b/fs/exfat/fatent.c @@ -75,7 +75,7 @@ int exfat_ent_set(struct super_block *sb, unsigned int loc, fat_entry = (__le32 *)&(bh->b_data[off]); *fat_entry = cpu_to_le32(content); - exfat_update_bh(sb, bh, sb->s_flags & SB_SYNCHRONOUS); + exfat_update_bh(bh, sb->s_flags & SB_SYNCHRONOUS); exfat_mirror_bh(sb, sec, bh); brelse(bh); return 0; @@ -174,7 +174,6 @@ int exfat_free_cluster(struct inode *inode, struct exfat_chain *p_chain) return -EIO; } - set_bit(EXFAT_SB_DIRTY, &sbi->s_state); clu = p_chain->dir; if (p_chain->flags == ALLOC_NO_FAT_CHAIN) { @@ -274,7 +273,7 @@ int exfat_zeroed_cluster(struct inode *dir, unsigned int clu) goto release_bhs; } memset(bhs[n]->b_data, 0, sb->s_blocksize); - exfat_update_bh(sb, bhs[n], 0); + exfat_update_bh(bhs[n], 0); n++; blknr++; @@ -358,8 +357,6 @@ int exfat_alloc_cluster(struct inode *inode, unsigned int num_alloc, } } - set_bit(EXFAT_SB_DIRTY, &sbi->s_state); - p_chain->dir = EXFAT_EOF_CLUSTER; while ((new_clu = exfat_find_free_bitmap(sb, hint_clu)) != diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c index 17d41f3d3709..8a3dde59052b 100644 --- a/fs/exfat/misc.c +++ b/fs/exfat/misc.c @@ -163,9 +163,8 @@ u32 exfat_calc_chksum32(void *data, int len, u32 chksum, int type) return chksum; } -void exfat_update_bh(struct super_block *sb, struct buffer_head *bh, int sync) +void exfat_update_bh(struct buffer_head *bh, int sync) { - set_bit(EXFAT_SB_DIRTY, &EXFAT_SB(sb)->s_state); set_buffer_uptodate(bh); mark_buffer_dirty(bh); diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c index edd8023865a0..5eef2217fcf2 100644 --- a/fs/exfat/namei.c +++ b/fs/exfat/namei.c @@ -387,7 +387,7 @@ static int exfat_find_empty_entry(struct inode *inode, 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(sb, bh, IS_DIRSYNC(inode)); + exfat_update_bh(bh, IS_DIRSYNC(inode)); brelse(bh); if (exfat_update_dir_chksum(inode, &(ei->dir), ei->entry)) @@ -1071,7 +1071,7 @@ static int exfat_rename_file(struct inode *inode, struct exfat_chain *p_dir, epnew->dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE); ei->attr |= ATTR_ARCHIVE; } - exfat_update_bh(sb, new_bh, sync); + exfat_update_bh(new_bh, sync); brelse(old_bh); brelse(new_bh); @@ -1087,7 +1087,7 @@ static int exfat_rename_file(struct inode *inode, struct exfat_chain *p_dir, } memcpy(epnew, epold, DENTRY_SIZE); - exfat_update_bh(sb, new_bh, sync); + exfat_update_bh(new_bh, sync); brelse(old_bh); brelse(new_bh); @@ -1104,7 +1104,7 @@ static int exfat_rename_file(struct inode *inode, struct exfat_chain *p_dir, epold->dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE); ei->attr |= ATTR_ARCHIVE; } - exfat_update_bh(sb, old_bh, sync); + exfat_update_bh(old_bh, sync); brelse(old_bh); ret = exfat_init_ext_entry(inode, p_dir, oldentry, num_new_entries, p_uniname); @@ -1159,7 +1159,7 @@ static int exfat_move_file(struct inode *inode, struct exfat_chain *p_olddir, epnew->dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE); ei->attr |= ATTR_ARCHIVE; } - exfat_update_bh(sb, new_bh, IS_DIRSYNC(inode)); + exfat_update_bh(new_bh, IS_DIRSYNC(inode)); brelse(mov_bh); brelse(new_bh); @@ -1175,7 +1175,7 @@ static int exfat_move_file(struct inode *inode, struct exfat_chain *p_olddir, } memcpy(epnew, epmov, DENTRY_SIZE); - exfat_update_bh(sb, new_bh, IS_DIRSYNC(inode)); + exfat_update_bh(new_bh, IS_DIRSYNC(inode)); brelse(mov_bh); brelse(new_bh); diff --git a/fs/exfat/super.c b/fs/exfat/super.c index e650e65536f8..b2611050b671 100644 --- a/fs/exfat/super.c +++ b/fs/exfat/super.c @@ -45,9 +45,6 @@ static void exfat_put_super(struct super_block *sb) struct exfat_sb_info *sbi = EXFAT_SB(sb); mutex_lock(&sbi->s_lock); - if (test_and_clear_bit(EXFAT_SB_DIRTY, &sbi->s_state)) - sync_blockdev(sb->s_bdev); - exfat_set_vol_flags(sb, VOL_CLEAN); exfat_free_bitmap(sbi); brelse(sbi->boot_bh); mutex_unlock(&sbi->s_lock); @@ -62,11 +59,9 @@ static int exfat_sync_fs(struct super_block *sb, int wait) /* If there are some dirty buffers in the bdev inode */ mutex_lock(&sbi->s_lock); - if (test_and_clear_bit(EXFAT_SB_DIRTY, &sbi->s_state)) { - sync_blockdev(sb->s_bdev); - if (exfat_set_vol_flags(sb, VOL_CLEAN)) - err = -EIO; - } + sync_blockdev(sb->s_bdev); + if (exfat_set_vol_flags(sb, VOL_CLEAN)) + err = -EIO; mutex_unlock(&sbi->s_lock); return err; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH] exfat: remove EXFAT_SB_DIRTY flag 2020-06-12 1:28 ` [PATCH] exfat: remove EXFAT_SB_DIRTY flag Tetsuhiro Kohada @ 2020-06-12 8:34 ` Sungjong Seo 2020-06-12 10:22 ` Tetsuhiro Kohada 0 siblings, 1 reply; 7+ messages in thread From: Sungjong Seo @ 2020-06-12 8:34 UTC (permalink / raw) To: 'Tetsuhiro Kohada' Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, 'Namjae Jeon', linux-fsdevel, linux-kernel > 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. > > Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> > --- > 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 | 11 +++-------- > 7 files changed, 23 insertions(+), 35 deletions(-) > [snip] > > @@ -62,11 +59,9 @@ static int exfat_sync_fs(struct super_block *sb, int > wait) > > /* If there are some dirty buffers in the bdev inode */ > mutex_lock(&sbi->s_lock); > - if (test_and_clear_bit(EXFAT_SB_DIRTY, &sbi->s_state)) { > - sync_blockdev(sb->s_bdev); > - if (exfat_set_vol_flags(sb, VOL_CLEAN)) > - err = -EIO; > - } I looked through most codes related to EXFAT_SB_DIRTY and VOL_DIRTY. And your approach looks good because all of them seem to be protected by s_lock. BTW, as you know, sync_filesystem() calls sync_fs() with 'nowait' first, and then calls it again with 'wait' twice. No need to sync with lock twice. If so, isn't it okay to do nothing when wait is 0? > + sync_blockdev(sb->s_bdev); > + if (exfat_set_vol_flags(sb, VOL_CLEAN)) > + err = -EIO; > mutex_unlock(&sbi->s_lock); > return err; > } > -- > 2.25.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] exfat: remove EXFAT_SB_DIRTY flag 2020-06-12 8:34 ` Sungjong Seo @ 2020-06-12 10:22 ` Tetsuhiro Kohada 2020-06-15 2:59 ` Sungjong Seo 0 siblings, 1 reply; 7+ messages in thread From: Tetsuhiro Kohada @ 2020-06-12 10:22 UTC (permalink / raw) To: Sungjong Seo Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, 'Namjae Jeon', linux-fsdevel, linux-kernel On 2020/06/12 17:34, 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. >> >> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> >> --- >> 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 | 11 +++-------- >> 7 files changed, 23 insertions(+), 35 deletions(-) >> > [snip] >> >> @@ -62,11 +59,9 @@ static int exfat_sync_fs(struct super_block *sb, int >> wait) >> >> /* If there are some dirty buffers in the bdev inode */ >> mutex_lock(&sbi->s_lock); >> - if (test_and_clear_bit(EXFAT_SB_DIRTY, &sbi->s_state)) { >> - sync_blockdev(sb->s_bdev); >> - if (exfat_set_vol_flags(sb, VOL_CLEAN)) >> - err = -EIO; >> - } > > I looked through most codes related to EXFAT_SB_DIRTY and VOL_DIRTY. > And your approach looks good because all of them seem to be protected by > s_lock. > > BTW, as you know, sync_filesystem() calls sync_fs() with 'nowait' first, > and then calls it again with 'wait' twice. No need to sync with lock twice. > If so, isn't it okay to do nothing when wait is 0? I also think ‘do nothing when wait is 0’ as you say, but I'm still not sure. Some other Filesystems do nothing with nowait and just return. However, a few Filesystems always perform sync. sync_blockdev() waits for completion, so it may be inappropriate to call with nowait. (But it was called in the original code) I'm still not sure, so I excluded it in this patch. Is it okay to include it? >> + sync_blockdev(sb->s_bdev); >> + if (exfat_set_vol_flags(sb, VOL_CLEAN)) >> + err = -EIO; >> mutex_unlock(&sbi->s_lock); >> return err; >> } >> -- >> 2.25.1 > > BR --- Tetsuhiro Kohada <kohada.t2@gmail.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] exfat: remove EXFAT_SB_DIRTY flag 2020-06-12 10:22 ` Tetsuhiro Kohada @ 2020-06-15 2:59 ` Sungjong Seo 2020-06-15 8:31 ` Kohada.Tetsuhiro 0 siblings, 1 reply; 7+ messages in thread From: Sungjong Seo @ 2020-06-15 2:59 UTC (permalink / raw) To: 'Tetsuhiro Kohada' Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, 'Namjae Jeon', linux-fsdevel, linux-kernel > On 2020/06/12 17:34, 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. > >> > >> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> > >> --- > >> 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 | 11 +++-------- > >> 7 files changed, 23 insertions(+), 35 deletions(-) > >> > > [snip] > >> > >> @@ -62,11 +59,9 @@ static int exfat_sync_fs(struct super_block *sb, > >> int > >> wait) > >> > >> /* If there are some dirty buffers in the bdev inode */ > >> mutex_lock(&sbi->s_lock); > >> - if (test_and_clear_bit(EXFAT_SB_DIRTY, &sbi->s_state)) { > >> - sync_blockdev(sb->s_bdev); > >> - if (exfat_set_vol_flags(sb, VOL_CLEAN)) > >> - err = -EIO; > >> - } > > > > I looked through most codes related to EXFAT_SB_DIRTY and VOL_DIRTY. > > And your approach looks good because all of them seem to be protected > > by s_lock. > > > > BTW, as you know, sync_filesystem() calls sync_fs() with 'nowait' > > first, and then calls it again with 'wait' twice. No need to sync with > lock twice. > > If so, isn't it okay to do nothing when wait is 0? > > I also think ‘do nothing when wait is 0’ as you say, but I'm still not > sure. > > Some other Filesystems do nothing with nowait and just return. > However, a few Filesystems always perform sync. > > sync_blockdev() waits for completion, so it may be inappropriate to call > with nowait. (But it was called in the original code) > > I'm still not sure, so I excluded it in this patch. > Is it okay to include it? > Yes, I think so. sync_filesystem() will call __sync_blockdev() without 'wait' first. So, it's enough to call sync_blockdev() with s_lock just one time. > > >> + sync_blockdev(sb->s_bdev); > >> + if (exfat_set_vol_flags(sb, VOL_CLEAN)) > >> + err = -EIO; > >> mutex_unlock(&sbi->s_lock); > >> return err; > >> } > >> -- > >> 2.25.1 > > > > > > BR > --- > Tetsuhiro Kohada <kohada.t2@gmail.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] exfat: remove EXFAT_SB_DIRTY flag 2020-06-15 2:59 ` Sungjong Seo @ 2020-06-15 8:31 ` Kohada.Tetsuhiro 0 siblings, 0 replies; 7+ messages in thread From: Kohada.Tetsuhiro @ 2020-06-15 8:31 UTC (permalink / raw) To: 'sj1557.seo@samsung.com' Cc: Mori.Takahiro, Motai.Hirotaka, 'Namjae Jeon', 'linux-fsdevel@vger.kernel.org', 'linux-kernel@vger.kernel.org' > > On 2020/06/12 17:34, 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. > > >> > > >> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> > > >> --- > > >> 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 | 11 +++-------- > > >> 7 files changed, 23 insertions(+), 35 deletions(-) > > >> > > > [snip] > > >> > > >> @@ -62,11 +59,9 @@ static int exfat_sync_fs(struct super_block *sb, > > >> int > > >> wait) > > >> > > >> /* If there are some dirty buffers in the bdev inode */ > > >> mutex_lock(&sbi->s_lock); > > >> - if (test_and_clear_bit(EXFAT_SB_DIRTY, &sbi->s_state)) { > > >> - sync_blockdev(sb->s_bdev); > > >> - if (exfat_set_vol_flags(sb, VOL_CLEAN)) > > >> - err = -EIO; > > >> - } > > > > > > I looked through most codes related to EXFAT_SB_DIRTY and VOL_DIRTY. > > > And your approach looks good because all of them seem to be > > > protected by s_lock. > > > > > > BTW, as you know, sync_filesystem() calls sync_fs() with 'nowait' > > > first, and then calls it again with 'wait' twice. No need to sync > > > with > > lock twice. > > > If so, isn't it okay to do nothing when wait is 0? > > > > I also think ‘do nothing when wait is 0’ as you say, but I'm still > > not sure. > > > > Some other Filesystems do nothing with nowait and just return. > > However, a few Filesystems always perform sync. > > > > sync_blockdev() waits for completion, so it may be inappropriate to > > call with nowait. (But it was called in the original code) > > > > I'm still not sure, so I excluded it in this patch. > > Is it okay to include it? > > > > Yes, I think so. sync_filesystem() will call __sync_blockdev() without 'wait' first. > So, it's enough to call sync_blockdev() with s_lock just one time. OK. I will repost v2-patch with the 'wait' check added. Thanks for your comment. > > >> + sync_blockdev(sb->s_bdev); > > >> + if (exfat_set_vol_flags(sb, VOL_CLEAN)) > > >> + err = -EIO; > > >> mutex_unlock(&sbi->s_lock); > > >> return err; > > >> } > > >> -- > > >> 2.25.1 BR --- Kohada Tetsuhiro <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] exfat: remove EXFAT_SB_DIRTY flag @ 2020-06-12 8:48 Markus Elfring 2020-06-12 10:01 ` Greg KH 0 siblings, 1 reply; 7+ messages in thread From: Markus Elfring @ 2020-06-12 8:48 UTC (permalink / raw) To: Tetsuhiro Kohada, linux-fsdevel Cc: Tetsuhiro Kohada, Takahiro Mori, Hirotaka Motai, Namjae Jeon, Sungjong Seo, linux-kernel > remove EXFAT_SB_DIRTY flag and related codes. I suggest to omit this sentence because a similar information is provided a bit later again for this change description. > If performe 'sync' in this state, VOL_DIRTY will not be cleared. Please improve this wording. Would you like to add the tag “Fixes” to the commit message? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=b791d1bdf9212d944d749a5c7ff6febdba241771#n183 Regards, Markus ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] exfat: remove EXFAT_SB_DIRTY flag 2020-06-12 8:48 Markus Elfring @ 2020-06-12 10:01 ` Greg KH 0 siblings, 0 replies; 7+ messages in thread From: Greg KH @ 2020-06-12 10:01 UTC (permalink / raw) To: Markus Elfring Cc: Tetsuhiro Kohada, linux-fsdevel, Tetsuhiro Kohada, Takahiro Mori, Hirotaka Motai, Namjae Jeon, Sungjong Seo, linux-kernel On Fri, Jun 12, 2020 at 10:48:20AM +0200, Markus Elfring wrote: > > remove EXFAT_SB_DIRTY flag and related codes. > > I suggest to omit this sentence because a similar information > is provided a bit later again for this change description. > > > > If performe 'sync' in this state, VOL_DIRTY will not be cleared. > > Please improve this wording. > > > Would you like to add the tag “Fixes” to the commit message? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=b791d1bdf9212d944d749a5c7ff6febdba241771#n183 > > Regards, > Markus Hi, This is the semi-friendly patch-bot of Greg Kroah-Hartman. Markus, you seem to have sent a nonsensical or otherwise pointless review comment to a patch submission on a Linux kernel developer mailing list. I strongly suggest that you not do this anymore. Please do not bother developers who are actively working to produce patches and features with comments that, in the end, are a waste of time. Patch submitter, please ignore Markus's suggestion; you do not need to follow it at all. The person/bot/AI that sent it is being ignored by almost all Linux kernel maintainers for having a persistent pattern of behavior of producing distracting and pointless commentary, and inability to adapt to feedback. Please feel free to also ignore emails from them. thanks, greg k-h's patch email bot ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-06-15 8:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20200612012902epcas1p4194d6fa3b3f7c46a8becb9bb6ce23d56@epcas1p4.samsung.com>
2020-06-12 1:28 ` [PATCH] exfat: remove EXFAT_SB_DIRTY flag Tetsuhiro Kohada
2020-06-12 8:34 ` Sungjong Seo
2020-06-12 10:22 ` Tetsuhiro Kohada
2020-06-15 2:59 ` Sungjong Seo
2020-06-15 8:31 ` Kohada.Tetsuhiro
2020-06-12 8:48 Markus Elfring
2020-06-12 10:01 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).