Messages in this thread | | | Subject | Re: [PATCH] exfat: retain 'VolumeFlags' properly | From | Tetsuhiro Kohada <> | Date | Wed, 15 Jul 2020 19:06:24 +0900 |
| |
>>>> 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>
| |