From: "Sungjong Seo" <sj1557.seo@samsung.com>
To: "'Tetsuhiro Kohada'" <kohada.t2@gmail.com>
Cc: <kohada.tetsuhiro@dc.mitsubishielectric.co.jp>,
<mori.takahiro@ab.mitsubishielectric.co.jp>,
<motai.hirotaka@aj.mitsubishielectric.co.jp>,
"'Namjae Jeon'" <namjae.jeon@samsung.com>,
<linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v3] exfat: remove EXFAT_SB_DIRTY flag
Date: Wed, 17 Jun 2020 16:20:37 +0900 [thread overview]
Message-ID: <414101d64477$ccb661f0$662325d0$@samsung.com> (raw)
In-Reply-To: <20200616021808.5222-1-kohada.t2@gmail.com>
> 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.
>
Since this patch does not resolve 'VOL_DIRTY in ENOTEMPTY' problem you
mentioned,
it would be better to remove the description above for that and to make new
patch.
> 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>
Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
> ---
> Changes in v2:
> - exfat_sync_fs() avoids synchronous processing when wait=0 Changes in
v3:
> - fix return value in exfat_sync_fs()
>
> 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,
> &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..8cb146376d6b 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);
> @@ -60,13 +57,14 @@ static int exfat_sync_fs(struct super_block *sb, int
> wait)
> struct exfat_sb_info *sbi = EXFAT_SB(sb);
> int err = 0;
>
> + if (!wait)
> + return 0;
> +
> /* 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
next prev parent reply other threads:[~2020-06-17 7:20 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20200616021816epcas1p44b0833f14bbad0e25cc0efb27fb2ebd3@epcas1p4.samsung.com>
2020-06-16 2:18 ` [PATCH v3] exfat: remove EXFAT_SB_DIRTY flag Tetsuhiro Kohada
2020-06-16 23:55 ` Namjae Jeon
2020-06-17 7:20 ` Sungjong Seo [this message]
2020-06-17 8:41 ` Namjae Jeon
2020-06-18 8:36 ` Tetsuhiro Kohada
2020-06-18 13:11 ` Sungjong Seo
2020-06-19 4:22 ` Tetsuhiro Kohada
2020-07-10 7:36 ` Tetsuhiro Kohada
2020-08-08 17:47 ` Sungjong Seo
2020-08-12 9:19 ` Tetsuhiro Kohada
2020-08-13 4:03 ` Namjae Jeon
2020-08-18 1:20 ` Tetsuhiro Kohada
2020-06-16 6:16 Markus Elfring
2020-06-16 14:45 ` Matthew Wilcox
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='414101d64477$ccb661f0$662325d0$@samsung.com' \
--to=sj1557.seo@samsung.com \
--cc=kohada.t2@gmail.com \
--cc=kohada.tetsuhiro@dc.mitsubishielectric.co.jp \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mori.takahiro@ab.mitsubishielectric.co.jp \
--cc=motai.hirotaka@aj.mitsubishielectric.co.jp \
--cc=namjae.jeon@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.