RE: [PATCH] exfat: remove EXFAT_SB_DIRTY flag
> > 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 > > >> --- > > >> 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(>s_lock); > > >> -if (test_and_clear_bit(EXFAT_SB_DIRTY, >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(>s_lock); > > >> return err; > > >> } > > >> -- > > >> 2.25.1 BR --- Kohada Tetsuhiro
RE: [PATCH] exfat: remove EXFAT_SB_DIRTY flag
> 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 > >> --- > >> 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(>s_lock); > >> - if (test_and_clear_bit(EXFAT_SB_DIRTY, >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(>s_lock); > >>return err; > >> } > >> -- > >> 2.25.1 > > > > > > BR > --- > Tetsuhiro Kohada
Re: [PATCH] exfat: remove EXFAT_SB_DIRTY flag
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 --- 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(>s_lock); - if (test_and_clear_bit(EXFAT_SB_DIRTY, >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(>s_lock); return err; } -- 2.25.1 BR --- Tetsuhiro Kohada
Re: [PATCH] exfat: remove EXFAT_SB_DIRTY flag
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
Re: [PATCH] exfat: remove EXFAT_SB_DIRTY flag
> 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
RE: [PATCH] exfat: remove EXFAT_SB_DIRTY flag
> 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 > --- > 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(>s_lock); > - if (test_and_clear_bit(EXFAT_SB_DIRTY, >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(>s_lock); > return err; > } > -- > 2.25.1