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;
Subject: Re: [PATCH] exfat: retain 'VolumeFlags' properly
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
References: <CGME20200708095813epcas1p2277cdf7de6a8bb20c27bcd030eec431f@epcas1p2.samsung.com>
 <20200708095746.4179-1-kohada.t2@gmail.com>
 <005101d658d1$7202e5d0$5608b170$@samsung.com>
 <TY2PR01MB2875C88DD10CC13D0C70DE1690610@TY2PR01MB2875.jpnprd01.prod.outlook.com>
 <015801d65a4a$ebedd380$c3c97a80$@samsung.com>
From:   Tetsuhiro Kohada <kohada.t2@gmail.com>
Message-ID: <ad0beeab-48ba-ee6d-f4cf-de19ec35a405@gmail.com>
Date:   Wed, 15 Jul 2020 19:06:24 +0900
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <015801d65a4a$ebedd380$c3c97a80$@samsung.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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