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

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

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



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.

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