Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Namjae Jeon" <namjae.jeon@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>,
        "'Sungjong Seo'" <sj1557.seo@samsung.com>,
        <linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>
In-Reply-To: <20200708095746.4179-1-kohada.t2@gmail.com>
Subject: RE: [PATCH] exfat: retain 'VolumeFlags' properly
Date:   Mon, 13 Jul 2020 13:52:43 +0900
Message-ID: <005101d658d1$7202e5d0$5608b170$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Thread-Index: AQJOchBlupSQwdEhi4rggGDYoy3yLwE5agcKqAruaRA=
Content-Language: ko
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <CGME20200708095813epcas1p2277cdf7de6a8bb20c27bcd030eec431f@epcas1p2.samsung.com>
        <20200708095746.4179-1-kohada.t2@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Tetsuhiro,

> Retain ActiveFat, MediaFailure and ClearToZero fields.
> And, never clear VolumeDirty, if it is dirty at mount.
> 
> In '3.1.13.3 Media Failure Field' of exfat specification says ...
>  If, upon mounting a volume, the value of this field is 1, implementations  which scan the entire
> volume for media failures and record all failures as  "bad" clusters in the FAT (or otherwise resolve
> media failures) may clear  the value of this field to 0.
> Therefore, should not clear MediaFailure without scanning volume.
> 
> In '8.1 Recommended Write Ordering' of exfat specification says ...
>  Clear the value of the VolumeDirty field to 0, if its value prior to the  first step was 0 Therefore,
> should not clear VolumeDirty when mounted.
> 
> Also, rename ERR_MEDIUM to MED_FAILURE.
I think that MEDIA_FAILURE looks better.
> 
> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
> ---
>  fs/exfat/exfat_fs.h  |  5 +++--
>  fs/exfat/exfat_raw.h |  2 +-
>  fs/exfat/super.c     | 22 ++++++++++++++--------
>  3 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index cb51d6e83199..3f8dc4ca8109 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -224,7 +224,8 @@ struct exfat_sb_info {
>  	unsigned int num_FAT_sectors; /* num of FAT sectors */
>  	unsigned int root_dir; /* root dir cluster */
>  	unsigned int dentries_per_clu; /* num of dentries per cluster */
> -	unsigned int vol_flag; /* volume dirty flag */
> +	unsigned int vol_flags; /* volume flags */
> +	unsigned int vol_flags_noclear; /* volume flags to retain */
>  	struct buffer_head *boot_bh; /* buffer_head of BOOT sector */
> 
>  	unsigned int map_clu; /* allocation bitmap start cluster */ @@ -380,7 +381,7 @@ static inline
> int exfat_sector_to_cluster(struct exfat_sb_info *sbi,  }
> 
>  /* super.c */
> -int exfat_set_vol_flags(struct super_block *sb, unsigned short new_flag);
> +int exfat_set_vol_flags(struct super_block *sb, unsigned short
> +new_flags);
> 
>  /* fatent.c */
>  #define exfat_get_next_cluster(sb, pclu) exfat_ent_get(sb, *(pclu), pclu) diff --git
> a/fs/exfat/exfat_raw.h b/fs/exfat/exfat_raw.h index 350ce59cc324..d86a8a6b0601 100644
> --- a/fs/exfat/exfat_raw.h
> +++ b/fs/exfat/exfat_raw.h
> @@ -16,7 +16,7 @@
> 
>  #define VOL_CLEAN		0x0000
>  #define VOL_DIRTY		0x0002
> -#define ERR_MEDIUM		0x0004
> +#define MED_FAILURE		0x0004
> 
>  #define EXFAT_EOF_CLUSTER	0xFFFFFFFFu
>  #define EXFAT_BAD_CLUSTER	0xFFFFFFF7u
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c index b5bf6dedbe11..c26b0f5a0875 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -96,17 +96,22 @@ static int exfat_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	return 0;
>  }
> 
> -int exfat_set_vol_flags(struct super_block *sb, unsigned short new_flag)
> +int exfat_set_vol_flags(struct super_block *sb, unsigned short
> +new_flags)
>  {
>  	struct exfat_sb_info *sbi = EXFAT_SB(sb);
>  	struct boot_sector *p_boot = (struct boot_sector *)sbi->boot_bh->b_data;
>  	bool sync;
If dirty bit is set in on-disk volume flags, we can just return 0 at the beginning
of this function ?

> 
> +	if (new_flags == VOL_CLEAN)
> +		new_flags = (sbi->vol_flags & ~VOL_DIRTY) | sbi->vol_flags_noclear;
> +	else
> +		new_flags |= sbi->vol_flags;
> +
>  	/* flags are not changed */
> -	if (sbi->vol_flag == new_flag)
> +	if (sbi->vol_flags == new_flags)
>  		return 0;
> 
> -	sbi->vol_flag = new_flag;
> +	sbi->vol_flags = new_flags;
> 
>  	/* skip updating volume dirty flag,
>  	 * if this volume has been mounted with read-only @@ -114,9 +119,9 @@ int
> exfat_set_vol_flags(struct super_block *sb, unsigned short new_flag)
>  	if (sb_rdonly(sb))
>  		return 0;
> 
> -	p_boot->vol_flags = cpu_to_le16(new_flag);
> +	p_boot->vol_flags = cpu_to_le16(new_flags);
How about set or clear only dirty bit to on-disk volume flags instead of using
sbi->vol_flags_noclear ?
       if (set)
               p_boot->vol_flags |= cpu_to_le16(VOL_DIRTY);
       else
               p_boot->vol_flags &= cpu_to_le16(~VOL_DIRTY);

> 
> -	if (new_flag == VOL_DIRTY && !buffer_dirty(sbi->boot_bh))
> +	if ((new_flags & VOL_DIRTY) && !buffer_dirty(sbi->boot_bh))
>  		sync = true;
>  	else
>  		sync = false;
> @@ -457,7 +462,8 @@ static int exfat_read_boot_sector(struct super_block *sb)
>  	sbi->dentries_per_clu = 1 <<
>  		(sbi->cluster_size_bits - DENTRY_SIZE_BITS);
> 
> -	sbi->vol_flag = le16_to_cpu(p_boot->vol_flags);
> +	sbi->vol_flags = le16_to_cpu(p_boot->vol_flags);
> +	sbi->vol_flags_noclear = sbi->vol_flags & (VOL_DIRTY | MED_FAILURE);
>  	sbi->clu_srch_ptr = EXFAT_FIRST_CLUSTER;
>  	sbi->used_clusters = EXFAT_CLUSTERS_UNTRACKED;
> 
> @@ -472,9 +478,9 @@ static int exfat_read_boot_sector(struct super_block *sb)
>  		exfat_err(sb, "bogus data start sector");
>  		return -EINVAL;
>  	}
> -	if (sbi->vol_flag & VOL_DIRTY)
> +	if (sbi->vol_flags & VOL_DIRTY)
>  		exfat_warn(sb, "Volume was not properly unmounted. Some data may be corrupt. Please run
> fsck.");
> -	if (sbi->vol_flag & ERR_MEDIUM)
> +	if (sbi->vol_flags & MED_FAILURE)
>  		exfat_warn(sb, "Medium has reported failures. Some data may be lost.");
> 
>  	/* exFAT file size is limited by a disk volume size */
> --
> 2.25.1