Re: [PATCH] exfat: retain 'VolumeFlags' properly

From: Tetsuhiro Kohada <kohada.t2@gmail.com>
To: Namjae Jeon <namjae.jeon@samsung.com>,
	Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp
Cc: 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
Subject: Re: [PATCH] exfat: retain 'VolumeFlags' properly
Date: Wed, 15 Jul 2020 19:06:24 +0900	[thread overview]
Message-ID: <ad0beeab-48ba-ee6d-f4cf-de19ec35a405@gmail.com> (raw)
In-Reply-To: <015801d65a4a$ebedd380$c3c97a80$@samsung.com>

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