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.
Signed-off-by: Tetsuhiro Kohada <[email protected]>
---
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 (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);
- 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
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 <[email protected]>
> ---
> 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
Thanks for your reply.
> > Also, rename ERR_MEDIUM to MED_FAILURE.
> I think that MEDIA_FAILURE looks better.
I think so too.
If so, should I change VOL_DIRTY to VOLUME_DIRTY?
> > -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 ?
That's right.
Neither 'set VOL_DIRTY' nor 'set VOL_CLEAN' makes a change to volume flags.
> > + 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);
In this way, the initial VOL_DIRTY cannot be retained.
The purpose of this patch is to avoid losing the initial VOL_DIRTY and MED_FAILURE.
Furthermore, I'm also thinking of setting MED_FAILURE.
However, the formula for 'new_flags' may be a bit complicated.
Is it better to change to the following?
if (new_flags == VOL_CLEAN)
new_flags = sbi->vol_flags & ~VOL_DIRTY
else
new_flags |= sbi->vol_flags;
new_flags |= sbi->vol_flags_noclear;
one more point,
Is there a better name than 'vol_flags_noclear'?
(I can't come up with a good name anymore)
BR
---
Kohada Tetsuhiro <[email protected]>
> Thanks for your reply.
>
> > > Also, rename ERR_MEDIUM to MED_FAILURE.
> > I think that MEDIA_FAILURE looks better.
>
> I think so too.
> If so, should I change VOL_DIRTY to VOLUME_DIRTY?
Yes, maybe.
>
> > > -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 ?
>
> That's right.
> Neither 'set VOL_DIRTY' nor 'set VOL_CLEAN' makes a change to volume flags.
>
>
> > > + 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);
>
> In this way, the initial VOL_DIRTY cannot be retained.
> The purpose of this patch is to avoid losing the initial VOL_DIRTY and MED_FAILURE.
> Furthermore, I'm also thinking of setting MED_FAILURE.
I know what you do. I mean this function does not need to be called if volume dirty
Is already set on on-disk volume flags as I said earlier.
>
> However, the formula for 'new_flags' may be a bit complicated.
> Is it better to change to the following?
>
> if (new_flags == VOL_CLEAN)
> new_flags = sbi->vol_flags & ~VOL_DIRTY
> else
> new_flags |= sbi->vol_flags;
>
> new_flags |= sbi->vol_flags_noclear;
>
>
> one more point,
> Is there a better name than 'vol_flags_noclear'?
> (I can't come up with a good name anymore)
It looks complicated. It would be better to simply set/clear VOLUME DIRTY bit.
Thanks!
>
> BR
> ---
> Kohada Tetsuhiro <[email protected]>
>>>> Also, rename ERR_MEDIUM to MED_FAILURE.
>>> I think that MEDIA_FAILURE looks better.
>>
>> I think so too.
>> If so, should I change VOL_DIRTY to VOLUME_DIRTY?
> Yes, maybe.
OK.
I'll rename both in v2.
>>>> - 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);
Please let me know about this code.
Does this code assume that 'sbi->vol_flags'(last vol_flag value) is not used?
If you use sbi->vol_flags, I think the original idea is fine.
sbi-> vol_flags = new_flag;
p_boot->vol_flags = cpu_to_le16(new_flag);
>> In this way, the initial VOL_DIRTY cannot be retained.
>> The purpose of this patch is to avoid losing the initial VOL_DIRTY and MED_FAILURE.
>> Furthermore, I'm also thinking of setting MED_FAILURE.
> I know what you do. I mean this function does not need to be called if volume dirty
> Is already set on on-disk volume flags as I said earlier.
Hmm?
Does it mean the caller check that volume was dirty at mount, and caller determine
whether to call exfat_set_vol_flags() or not?
If so, MEDIA_FAILUR needs to be set independently of the volume-dirty state.
>> However, the formula for 'new_flags' may be a bit complicated.
>> Is it better to change to the following?
>>
>> if (new_flags == VOL_CLEAN)
>> new_flags = sbi->vol_flags & ~VOL_DIRTY
>> else
>> new_flags |= sbi->vol_flags;
>>
>> new_flags |= sbi->vol_flags_noclear;
>>
>>
>> one more point,
>> Is there a better name than 'vol_flags_noclear'?
>> (I can't come up with a good name anymore)
> It looks complicated. It would be better to simply set/clear VOLUME DIRTY bit.
I think exfat_set_vol_flags() gets a little complicated,
because it needs the followings (with bit operation)
a) Set/Clear VOLUME_DIRTY.
b) Set MEDIA_FAILUR.
c) Do not change other flags.
d) Retain VOLUME_DIRTY/MEDIA_FAILUR as it is when mounted.
'vol_flags_noclear' is used for d).
Bit-by-bit operation makes the code redundant.
I think it's not a bad way to handle multiple bits.
What do you think?
BR
---
Tetsuhiro Kohada <[email protected]>
Ping..
On 2020/07/15 19:06, Tetsuhiro Kohada wrote:
>> It looks complicated. It would be better to simply set/clear VOLUME DIRTY bit.
>
> I think exfat_set_vol_flags() gets a little complicated,
> because it needs the followings (with bit operation)
> a) Set/Clear VOLUME_DIRTY.
> b) Set MEDIA_FAILUR.
How about splitting these into separate functions as below?
exfat_set_volume_dirty()
exfat_set_vol_flags(sb, sbi->vol_flag | VOLUME_DIRTY);
exfat_clear_volume_dirty()
exfat_set_vol_flags(sb, sbi->vol_flag & ~VOLUME_DIRTY);
exfat_set_media_failure()
exfat_set_vol_flags(sb, sbi->vol_flag | MEDIA_FAILURE);
The implementation is essentially the same for exfat_set_vol_flags(),
but I think the intention of the operation will be easier to understand.
BR
---
Tetsuhiro Kohada <[email protected]>
> Ping..
Hi Tetsuhiro,
>
> On 2020/07/15 19:06, Tetsuhiro Kohada wrote:
> >> It looks complicated. It would be better to simply set/clear VOLUME DIRTY bit.
> >
> > I think exfat_set_vol_flags() gets a little complicated, because it
> > needs the followings (with bit operation)
> > a) Set/Clear VOLUME_DIRTY.
> > b) Set MEDIA_FAILUR.
>
> How about splitting these into separate functions as below?
>
>
> exfat_set_volume_dirty()
> exfat_set_vol_flags(sb, sbi->vol_flag | VOLUME_DIRTY);
>
> exfat_clear_volume_dirty()
> exfat_set_vol_flags(sb, sbi->vol_flag & ~VOLUME_DIRTY);
Looks good.
>
> exfat_set_media_failure()
> exfat_set_vol_flags(sb, sbi->vol_flag | MEDIA_FAILURE);
Where will this function be called? We don't need to create unused functions in advance...
>
>
> The implementation is essentially the same for exfat_set_vol_flags(), but I think the intention of the
> operation will be easier to understand.
Yes.
Thanks!
>
>
> BR
> ---
> Tetsuhiro Kohada <[email protected]>
On 2020/07/30 15:59, Namjae Jeon wrote:
>> Ping..
> Hi Tetsuhiro,
Thank you for your reply.
>> On 2020/07/15 19:06, Tetsuhiro Kohada wrote:
>>>> It looks complicated. It would be better to simply set/clear VOLUME DIRTY bit.
>>>
>>> I think exfat_set_vol_flags() gets a little complicated, because it
>>> needs the followings (with bit operation)
>>> a) Set/Clear VOLUME_DIRTY.
>>> b) Set MEDIA_FAILUR.
>>
>> How about splitting these into separate functions as below?
>>
>>
>> exfat_set_volume_dirty()
>> exfat_set_vol_flags(sb, sbi->vol_flag | VOLUME_DIRTY);
>>
>> exfat_clear_volume_dirty()
>> exfat_set_vol_flags(sb, sbi->vol_flag & ~VOLUME_DIRTY);
> Looks good.
>
>>
>> exfat_set_media_failure()
>> exfat_set_vol_flags(sb, sbi->vol_flag | MEDIA_FAILURE);
> Where will this function be called? We don't need to create unused functions in advance...
Sorry. I ran ahead without explaining.
I would like to set MEDIA_FAILURE when a format error is detected so that fsck will be run on the next mount.
This patch is the basis for setting MEDIA_FAILURE.
But, I have no reason to implement this right now, as you say.
I'll add it in a patch that sets MEDIA_FAILURE.
BTW
I would like your opinion on the timing of clearing VOLUME_DIRTY.
https://lore.kernel.org/linux-fsdevel/[email protected]/#t
BR
---
Tetsuhiro Kohada <[email protected]>
>