Re: [PATCH 0/9] btrfs-progs: btrfstune: accept multiple devices and cleanup

[Anand Jain <anand.jain@oracle.com>]

>>   As of now btrfstune works with only one regular file. Not possible
>>   to use multiple regular files. Unless loop device is used. This
>>   set fixes this limitation.
> 
> Here I want to make the point clear, is the patchset intended to handle
> ONE multi-device btrfs?
> 
> If that's the case, then my initial concerns on the multiple different
> fses case is still a concern.


>> $ btrfstune -m --noscan ./td2 ./td1 ./td3
>> warning, device 1 is missing
>> ERROR: cannot read chunk root
>> ERROR: open ctree failed
>>
>> $ btrfstune -m --noscan ./td1 ./td2 ./td3
>> $ echo $?
>> 0
> 
> This is exactly my concern. > We're combining target fs and device assembly into the same argument 
list.

> Thus changing the order of argument would lead to different results.

  yeah. --device solves it.


>>   btrfstune -m --noscan --devices=./td2,./td3  ./td1
> 
> That much better, less confusion.
> 
> Furthermore, the --devices (Although my initial proposal looks more like
> "--device td2 --device td3", which makes parsing a little simpler) can
> be applied to all other btrfs-progs, allowing a global way to assemble
> the device list.
> 

  --device td2 --device td3.. that makes it same as mount.
  I am not yet sure if it can be a global option though.

>>
>>
>>> - What's the proper error handling if operation on one of the parameter
>>>    failed if we choose to do the tune for all involved devices?
>>>    Should we revert the operation on the succeeded ones?
>>>    Should we continue on the remaining ones?
>>
>>   Hm. That's a possible scenario even without this patch.!
>>   However, we use the CHANGING_FSID flag to handle split-brain scenarios
>>   with incomplete metadata_uuid changes. Currently, the kernel
>>   fixes this situation based on the flag and generation number.
>>   However, kernel should fail these split-brain scenarios and
>>   instead address them in the btrfs-progs, which is wip.
>>
>>> I understand it's better to add the ability to do manual scan, but it
>>> looks like the multi-device arguments can be a little more complex than
>>> what we thought.
>>
>>   Hmm How? The device list enumeration logic which handles the automatic
>>   scan also handle the command line provided device list. So both are
>>   same.
> 
> The "--device=" option you proposed is exactly the way to handle it.

   Ok.


> 
> Thanks,
> Qu
>>
>>> At least I think we should add a dedicate --scan/--device option, and
>>> allow multiple --scan/--device to be provided for device list assembly,
>>> then still keep the single argument to avoid possible confusion.
>>
>>   btrfs-progs scans all the block devices in the system, by default.
>>   so IMO,
>>   "--noscan" is reasonable, similar to 'btrfs in dump-tree --noscan'.
>>
>>   I am ok with with --device/--devices option.
>>   So we could scan only commnd line provided devices
>>   with --noscan:
>>
>>     btrfstune -m --noscan --devices=./td1,/dev/sda1 ./td3
>>
>>   And to scan both command line and the block devices
>>   without --noscan:
>>
>>     btrfstune -m --devices=./td1 ./td3
>>
>>
>> Thanks, Anand
>>
>>>
>>> This also solves the problem I mentioned above. If multiple filesystems
>>> are provided, they are just assembled into device list, won't have an
>>> impact on the tune target.
>>>
>>> And since we still have a single device to tune, there is no extra error
>>> handling, nor confusion.
>>>
>>> Thanks,
>>> Qu
>>>
>>>>
>>>> Patches 1 to 5 primarily consist of cleanups. Patches 6 and 8 serve as
>>>> preparatory changes. Patch 7 enables btrfstune to accept multiple
>>>> devices. Patch 9 ensures that btrfstune no longer automatically uses 
>>>> the
>>>> system block devices when --noscan option is specified.
>>>> Patches 10 and 11 are help and documentation part.
>>>>
>>>> Anand Jain (11):
>>>>    btrfs-progs: check_mounted_where declare is_btrfs as bool
>>>>    btrfs-progs: check_mounted_where pack varibles type by size
>>>>    btrfs-progs: rename struct open_ctree_flags to open_ctree_args
>>>>    btrfs-progs: optimize device_list_add
>>>>    btrfs-progs: simplify btrfs_scan_one_device()
>>>>    btrfs-progs: factor out btrfs_scan_stdin_devices
>>>>    btrfs-progs: tune: add stdin device list
>>>>    btrfs-progs: refactor check_where_mounted with noscan option
>>>>    btrfs-progs: tune: add noscan option
>>>>    btrfs-progs: tune: add help for multiple devices and noscan option
>>>>    btrfs-progs: Documentation: update btrfstune --noscan option
>>>>
>>>>   Documentation/btrfstune.rst |  4 ++++
>>>>   btrfs-find-root.c           |  2 +-
>>>>   check/main.c                |  2 +-
>>>>   cmds/filesystem.c           |  2 +-
>>>>   cmds/inspect-dump-tree.c    | 39 
>>>> ++++---------------------------------
>>>>   cmds/rescue.c               |  4 ++--
>>>>   cmds/restore.c              |  2 +-
>>>>   common/device-scan.c        | 39 
>>>> +++++++++++++++++++++++++++++++++++++
>>>>   common/device-scan.h        |  1 +
>>>>   common/open-utils.c         | 21 +++++++++++---------
>>>>   common/open-utils.h         |  3 ++-
>>>>   common/utils.c              |  3 ++-
>>>>   image/main.c                |  4 ++--
>>>>   kernel-shared/disk-io.c     |  8 ++++----
>>>>   kernel-shared/disk-io.h     |  4 ++--
>>>>   kernel-shared/volumes.c     | 14 +++++--------
>>>>   mkfs/main.c                 |  2 +-
>>>>   tune/main.c                 | 25 +++++++++++++++++++-----
>>>>   18 files changed, 104 insertions(+), 75 deletions(-)
>>>>
>>