Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] exfat: retain 'VolumeFlags' properly
@ 2020-07-08  9:57 ` Tetsuhiro Kohada
  2020-07-13  4:52   ` Namjae Jeon
  0 siblings, 1 reply; 8+ messages in thread
From: Tetsuhiro Kohada @ 2020-07-08  9:57 UTC (permalink / raw)
  To: kohada.t2
  Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, Namjae Jeon,
	Sungjong Seo, linux-fsdevel, linux-kernel

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 <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 (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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] exfat: retain 'VolumeFlags' properly
  2020-07-08  9:57 ` [PATCH] exfat: retain 'VolumeFlags' properly Tetsuhiro Kohada
@ 2020-07-13  4:52   ` Namjae Jeon
  2020-07-14  1:56     ` Kohada.Tetsuhiro
  0 siblings, 1 reply; 8+ messages in thread
From: Namjae Jeon @ 2020-07-13  4:52 UTC (permalink / raw)
  To: 'Tetsuhiro Kohada'
  Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka,
	'Sungjong Seo',
	linux-fsdevel, linux-kernel

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] exfat: retain 'VolumeFlags' properly
  2020-07-13  4:52   ` Namjae Jeon
@ 2020-07-14  1:56     ` Kohada.Tetsuhiro
  2020-07-15  1:54       ` Namjae Jeon
  0 siblings, 1 reply; 8+ messages in thread
From: Kohada.Tetsuhiro @ 2020-07-14  1:56 UTC (permalink / raw)
  To: 'Namjae Jeon'
  Cc: Mori.Takahiro, Motai.Hirotaka, 'Sungjong Seo',
	linux-fsdevel, linux-kernel, 'Tetsuhiro Kohada''

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 <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] exfat: retain 'VolumeFlags' properly
  2020-07-14  1:56     ` Kohada.Tetsuhiro
@ 2020-07-15  1:54       ` Namjae Jeon
  2020-07-15 10:06         ` Tetsuhiro Kohada
  0 siblings, 1 reply; 8+ messages in thread
From: Namjae Jeon @ 2020-07-15  1:54 UTC (permalink / raw)
  To: Kohada.Tetsuhiro
  Cc: Mori.Takahiro, Motai.Hirotaka, 'Sungjong Seo',
	linux-fsdevel, linux-kernel, 'Tetsuhiro Kohada''

> 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 <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] exfat: retain 'VolumeFlags' properly
  2020-07-15  1:54       ` Namjae Jeon
@ 2020-07-15 10:06         ` Tetsuhiro Kohada
  2020-07-30  6:24           ` Tetsuhiro Kohada
  0 siblings, 1 reply; 8+ messages in thread
From: Tetsuhiro Kohada @ 2020-07-15 10:06 UTC (permalink / raw)
  To: Namjae Jeon, Kohada.Tetsuhiro
  Cc: Mori.Takahiro, Motai.Hirotaka, 'Sungjong Seo',
	linux-fsdevel, linux-kernel


>>>> 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 <kohada.t2@gmail.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] exfat: retain 'VolumeFlags' properly
  2020-07-15 10:06         ` Tetsuhiro Kohada
@ 2020-07-30  6:24           ` Tetsuhiro Kohada
  2020-07-30  6:59             ` Namjae Jeon
  0 siblings, 1 reply; 8+ messages in thread
From: Tetsuhiro Kohada @ 2020-07-30  6:24 UTC (permalink / raw)
  To: Namjae Jeon, Kohada.Tetsuhiro
  Cc: Mori.Takahiro, Motai.Hirotaka, 'Sungjong Seo',
	linux-fsdevel, linux-kernel

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 <kohada.t2@gmail.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] exfat: retain 'VolumeFlags' properly
  2020-07-30  6:24           ` Tetsuhiro Kohada
@ 2020-07-30  6:59             ` Namjae Jeon
  2020-07-31  1:29               ` Tetsuhiro Kohada
  0 siblings, 1 reply; 8+ messages in thread
From: Namjae Jeon @ 2020-07-30  6:59 UTC (permalink / raw)
  To: 'Tetsuhiro Kohada', Kohada.Tetsuhiro
  Cc: Mori.Takahiro, Motai.Hirotaka, 'Sungjong Seo',
	linux-fsdevel, linux-kernel

> 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 <kohada.t2@gmail.com>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] exfat: retain 'VolumeFlags' properly
  2020-07-30  6:59             ` Namjae Jeon
@ 2020-07-31  1:29               ` Tetsuhiro Kohada
  0 siblings, 0 replies; 8+ messages in thread
From: Tetsuhiro Kohada @ 2020-07-31  1:29 UTC (permalink / raw)
  To: Namjae Jeon, Kohada.Tetsuhiro
  Cc: Mori.Takahiro, Motai.Hirotaka, 'Sungjong Seo',
	linux-fsdevel, linux-kernel

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/c635e965-6b78-436a-3959-e4777e1732c1@gmail.com/#t

BR
---
Tetsuhiro Kohada <kohada.t2@gmail.com>
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20200708095813epcas1p2277cdf7de6a8bb20c27bcd030eec431f@epcas1p2.samsung.com>
2020-07-08  9:57 ` [PATCH] exfat: retain 'VolumeFlags' properly Tetsuhiro Kohada
2020-07-13  4:52   ` Namjae Jeon
2020-07-14  1:56     ` Kohada.Tetsuhiro
2020-07-15  1:54       ` Namjae Jeon
2020-07-15 10:06         ` Tetsuhiro Kohada
2020-07-30  6:24           ` Tetsuhiro Kohada
2020-07-30  6:59             ` Namjae Jeon
2020-07-31  1:29               ` Tetsuhiro Kohada

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git