> 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 <[email protected]> > >> --- > >> 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 <[email protected]>
