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

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

On 07/06/2023 19:06, Qu Wenruo wrote:
> 
> 
> On 2023/6/7 17:59, Anand Jain wrote:
>> In an attempt to enable btrfstune to accept multiple devices from the
>> command line, this patch includes some cleanup around the related code
>> and functions.
> 
> Mind to share the use case of the new ability?
> 

  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.


> My concern related to multi-device parameters are:
> 
> - What if the provided devices are belonging to different filesystems?
>    Should we still do the tune operation on all of them or just the
>    first/last device?
> 

  Hmm, the scan part remains same with/without this patchset.
  The device_list_add() function organizes the devices based on the fsid.
  Any tool within the btrfs-progs uses this list to obtain the partner5
  device list. This patch set still relies the same thing.

  btrfstune gets the fsid to work on from the first deivce in the list.

  Here is an example:

$ btrfs in dump-super ./td1 ./td2 ./td3 | egrep 
'device=|^fsid|^metadata_uuid'
superblock: bytenr=65536, device=./td1
fsid			c931379a-a119-4eda-a338-badb0a197512
metadata_uuid		f761b688-2642-4c94-be90-22f58e2a66d7
superblock: bytenr=65536, device=./td2
fsid			f9643d74-1d3d-4b0d-b56b-b05ada340f57
metadata_uuid		f761b688-2642-4c94-be90-22f58e2a66d7
superblock: bytenr=65536, device=./td3
fsid			c931379a-a119-4eda-a338-badb0a197512
metadata_uuid		f761b688-2642-4c94-be90-22f58e2a66d7


$ 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

  If you are concerned about the lack of explicit device's fsid to work
  on. How about,

  (proposal only, these options does not exists yet)

  btrfstune -m --noscan --devices=./td2,./td3  ./td1


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

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