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
[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(-)
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);
+ 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
