Re: [PATCH v3 2/3] btrfs: Introduce "rescue=" mount option

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3021 bytes --]



On 2019/6/13 上午9:30, Qu Wenruo wrote:
> 
> 
> On 2019/6/12 下午11:09, David Sterba wrote:
>> On Wed, Jun 12, 2019 at 02:36:56PM +0800, Qu Wenruo wrote:
>>> This patch introduces a new "rescue=" mount option for all those mount
>>> options for data recovery.
>>>
>>> Different rescue sub options are seperated by ':'. E.g
>>> "ro,rescue=no_log_replay:use_backup_root".
>>> (The original plan is to use ';', but ';' needs to be escaped/quoted,
>>> or it will be interpreted by bash)
>>
>> ':' as separator is fine
>>
>>> The following mount options are converted to "rescue=", old mount
>>> options are deprecated but still available for compatibility purpose:
>>>
>>> - usebackuproot
>>>   Now it's "rescue=use_backup_root"
>>>
>>> - nologreplay
>>>   Now it's "rescue=no_log_replay"
>>>
>>> The new underscore is here to make the option more readable and make
>>> spell check happier.
>>
>> Who uses spell checker on mount options, really? I'd expect that the new
>> syntax would build on top of the old so the exact same names of the
>> options are now shifted to the value of 'rescue='.
>>
>> The usability is better with switching
>>
>>   -o usebackuproot
>>
>> to
>>
>>   -o rescue=usebackuproot
> 
> The problem is, every time I see things like usebackuproot or
> nologreplay, it's not that easy to understand them just by a quick glance.
> Further more, they are already rescue options, not something we would
> need to use in a daily basis.
> 
> A little longer but easier to read looks valid to me in such use case.

Gentle ping here.

Do we still need to follow the hard to read mount options?
If so, I can go ahead with old names, but I really want to know if
others are OK with the existing naming.

Thanks,
Qu
> 
> Thanks,
> Qu
> 
>>
>> ie. just prepending 'rescue='.
>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  fs/btrfs/super.c | 65 +++++++++++++++++++++++++++++++++++++++++++++---
>>>  1 file changed, 62 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>> index 64f20725615a..4512f25dcf69 100644
>>> --- a/fs/btrfs/super.c
>>> +++ b/fs/btrfs/super.c
>>> @@ -310,7 +310,6 @@ enum {
>>>  	Opt_datasum, Opt_nodatasum,
>>>  	Opt_defrag, Opt_nodefrag,
>>>  	Opt_discard, Opt_nodiscard,
>>> -	Opt_nologreplay,
>>>  	Opt_ratio,
>>>  	Opt_rescan_uuid_tree,
>>>  	Opt_skip_balance,
>>> @@ -323,7 +322,6 @@ enum {
>>>  	Opt_subvolid,
>>>  	Opt_thread_pool,
>>>  	Opt_treelog, Opt_notreelog,
>>> -	Opt_usebackuproot,
>>>  	Opt_user_subvol_rm_allowed,
>>>  
>>>  	/* Deprecated options */
>>> @@ -341,6 +339,8 @@ enum {
>>>  #ifdef CONFIG_BTRFS_FS_REF_VERIFY
>>>  	Opt_ref_verify,
>>>  #endif
>>> +	/* Rescue options */
>>> +	Opt_rescue, Opt_usebackuproot, Opt_nologreplay,
>>
>> The options have been sorted and grouped, don't mess it up again please.
>> Check the list and find a better place than after the deprecated and
>> debugging options.
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]