| Message ID | cover.1686131669.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs-progs: btrfstune: accept multiple devices and cleanup | expand |
Now I notice that for many patches, the Git changelog is empty, even though I had it at one time. I am not sure where they went. I am recreating and resending the whole set again. Thanks, Anand On 07/06/2023 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. > > 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(-) >
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? 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? - 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? 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. 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. 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(-) >
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(-) >>
On 2023/6/8 08:20, Anand Jain wrote: > 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. 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. > > >> 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 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. > > 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 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. > > >> - 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. 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(-) >>> >
>> 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(-) >>>> >>