| Message ID | 1658070503-25238-1-git-send-email-zhanglikernel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: resize: automatically add devid if device is not specifically | expand |
On Sun, Jul 17, 2022 at 11:08:23PM +0800, Li Zhang wrote: > related issues: > https://github.com/kdave/btrfs-progs/issues/470 > > [BUG] > If there is no devid=1, when the user uses the btrfs file system tool, the following error will be reported, > > $ sudo btrfs filesystem show /mnt/1 > Label: none uuid: 64dc0f68-9afa-4465-9ea1-2bbebfdb6cec > Total devices 2 FS bytes used 704.00KiB > devid 2 size 15.00GiB used 1.16GiB path /dev/loop2 > devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 > $ sudo btrfs filesystem resize -1G /mnt/1 > ERROR: cannot find devid: 1 > ERROR: unable to resize '/mnt/1': No such device > > [CAUSE] > If the user does not specify the devid id explicitly, btrfs will use the default devid 1, so it will report an error when dev 1 is missing. > > [FIX] > If there is no special devid, the first devid is added automatically and check the maximum length of args passed to kernel space. > After patch, when resize filesystem without specified, it would resize the first device, the result is list as following. > > $ sudo btrfs filesystem show /mnt/1/ > Label: none uuid: 7b4827da-bc6e-42aa-b03d-52c2533dfe94 > Total devices 2 FS bytes used 144.00KiB > devid 2 size 15.00GiB used 1.16GiB path /dev/loop2 > devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 > > $ sudo btrfs filesystem resize -1G /mnt/1 > Resize device id 2 (/dev/loop2) from 15.00GiB to 14.00GiB > $ sudo btrfs filesystem show /mnt/1/ > Label: none uuid: 7b4827da-bc6e-42aa-b03d-52c2533dfe94 > Total devices 2 FS bytes used 144.00KiB > devid 2 size 14.00GiB used 1.16GiB path /dev/loop2 > devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 Is that desirable behavior? I'd expect that if there are multiple devices present, and I haven't specified which one to resize, that the command would fail with an error, requiring me to specify which device I want resized. Under that expectation, the current behavior of resizing devid 1 by default is also incorrect. If there's only one device, then 'btrfs fi resize -1G' should resize that device, since no ambiguity is possible. > Signed-off-by: Li Zhang <zhanglikernel@gmail.com> > --- > cmds/filesystem.c | 49 ++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 38 insertions(+), 11 deletions(-) > > diff --git a/cmds/filesystem.c b/cmds/filesystem.c > index 7cd08fc..2e2414d 100644 > --- a/cmds/filesystem.c > +++ b/cmds/filesystem.c > @@ -1087,7 +1087,8 @@ static const char * const cmd_filesystem_resize_usage[] = { > NULL > }; > > -static int check_resize_args(const char *amount, const char *path) { > +static int check_resize_args(char * const amount, const char *path) > +{ > struct btrfs_ioctl_fs_info_args fi_args; > struct btrfs_ioctl_dev_info_args *di_args = NULL; > int ret, i, dev_idx = -1; > @@ -1102,7 +1103,8 @@ static int check_resize_args(const char *amount, const char *path) { > > if (ret) { > error("unable to retrieve fs info"); > - return 1; > + ret = 1; > + goto out; > } > > if (!fi_args.num_devices) { > @@ -1112,11 +1114,14 @@ static int check_resize_args(const char *amount, const char *path) { > } > > ret = snprintf(amount_dup, BTRFS_VOL_NAME_MAX, "%s", amount); > +check: > if (strlen(amount) != ret) { > error("newsize argument is too long"); > ret = 1; > goto out; > } > + if (strcmp(amount, amount_dup) != 0) > + strcpy(amount, amount_dup); > > sizestr = amount_dup; > devstr = strchr(sizestr, ':'); > @@ -1137,6 +1142,13 @@ static int check_resize_args(const char *amount, const char *path) { > > dev_idx = -1; > for(i = 0; i < fi_args.num_devices; i++) { > + if (!devstr) { > + memset(amount_dup, 0, BTRFS_VOL_NAME_MAX); > + ret = snprintf(amount_dup, BTRFS_VOL_NAME_MAX, "%llu:", di_args[i].devid); > + ret = snprintf(amount_dup + strlen(amount_dup), > + BTRFS_VOL_NAME_MAX - strlen(amount_dup), "%s", amount); > + goto check; > + } > if (di_args[i].devid == devid) { > dev_idx = i; > break; > @@ -1235,8 +1247,10 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, > } > } > > - if (check_argc_exact(argc - optind, 2)) > - return 1; > + if (check_argc_exact(argc - optind, 2)) { > + ret = 1; > + goto out; > + } > > amount = argv[optind]; > path = argv[optind + 1]; > @@ -1244,7 +1258,8 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, > len = strlen(amount); > if (len == 0 || len >= BTRFS_VOL_NAME_MAX) { > error("resize value too long (%s)", amount); > - return 1; > + ret = 1; > + goto out; > } > > cancel = (strcmp("cancel", amount) == 0); > @@ -1258,7 +1273,8 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, > "directories as argument. Passing file containing a btrfs image\n" > "would resize the underlying filesystem instead of the image.\n"); > } > - return 1; > + ret = 1; > + goto out; > } > > /* > @@ -1273,14 +1289,22 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, > error( > "unable to check status of exclusive operation: %m"); > close_file_or_dir(fd, dirstream); > - return 1; > + goto out; > } > } > > + amount = (char *)malloc(BTRFS_VOL_NAME_MAX); > + if (!amount) { > + ret = -ENOMEM; > + goto out; > + } > + strcpy(amount, argv[optind]); > + > ret = check_resize_args(amount, path); > if (ret != 0) { > close_file_or_dir(fd, dirstream); > - return 1; > + ret = 1; > + goto free_amount; > } > > memset(&args, 0, sizeof(args)); > @@ -1298,7 +1322,7 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, > error("unable to resize '%s': %m", path); > break; > } > - return 1; > + ret = 1; > } else if (res > 0) { > const char *err_str = btrfs_err_str(res); > > @@ -1308,9 +1332,12 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, > error("resizing of '%s' failed: unknown error %d", > path, res); > } > - return 1; > + ret = 1; > } > - return 0; > +free_amount: > + free(amount); > +out: > + return ret; > } > static DEFINE_SIMPLE_COMMAND(filesystem_resize, "resize"); > > -- > 1.8.3.1 >
On 2022/7/18 01:39, Zygo Blaxell wrote: > On Sun, Jul 17, 2022 at 11:08:23PM +0800, Li Zhang wrote: >> related issues: >> https://github.com/kdave/btrfs-progs/issues/470 >> >> [BUG] >> If there is no devid=1, when the user uses the btrfs file system tool, the following error will be reported, >> >> $ sudo btrfs filesystem show /mnt/1 >> Label: none uuid: 64dc0f68-9afa-4465-9ea1-2bbebfdb6cec >> Total devices 2 FS bytes used 704.00KiB >> devid 2 size 15.00GiB used 1.16GiB path /dev/loop2 >> devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 >> $ sudo btrfs filesystem resize -1G /mnt/1 >> ERROR: cannot find devid: 1 >> ERROR: unable to resize '/mnt/1': No such device >> >> [CAUSE] >> If the user does not specify the devid id explicitly, btrfs will use the default devid 1, so it will report an error when dev 1 is missing. >> >> [FIX] >> If there is no special devid, the first devid is added automatically and check the maximum length of args passed to kernel space. >> After patch, when resize filesystem without specified, it would resize the first device, the result is list as following. >> >> $ sudo btrfs filesystem show /mnt/1/ >> Label: none uuid: 7b4827da-bc6e-42aa-b03d-52c2533dfe94 >> Total devices 2 FS bytes used 144.00KiB >> devid 2 size 15.00GiB used 1.16GiB path /dev/loop2 >> devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 >> >> $ sudo btrfs filesystem resize -1G /mnt/1 >> Resize device id 2 (/dev/loop2) from 15.00GiB to 14.00GiB >> $ sudo btrfs filesystem show /mnt/1/ >> Label: none uuid: 7b4827da-bc6e-42aa-b03d-52c2533dfe94 >> Total devices 2 FS bytes used 144.00KiB >> devid 2 size 14.00GiB used 1.16GiB path /dev/loop2 >> devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 > > Is that desirable behavior? I'd expect that if there are multiple > devices present, and I haven't specified which one to resize, that the > command would fail with an error, requiring me to specify which device I > want resized. Under that expectation, the current behavior of resizing > devid 1 by default is also incorrect. I agree with Zygo. If there is only one device, then we can definitely use the first device we find. If there are multiple devices, then it's better to output an error message, provide candidate devids, and error out. Thanks, Qu > > If there's only one device, then 'btrfs fi resize -1G' should resize > that device, since no ambiguity is possible. > >> Signed-off-by: Li Zhang <zhanglikernel@gmail.com> >> --- >> cmds/filesystem.c | 49 ++++++++++++++++++++++++++++++++++++++----------- >> 1 file changed, 38 insertions(+), 11 deletions(-) >> >> diff --git a/cmds/filesystem.c b/cmds/filesystem.c >> index 7cd08fc..2e2414d 100644 >> --- a/cmds/filesystem.c >> +++ b/cmds/filesystem.c >> @@ -1087,7 +1087,8 @@ static const char * const cmd_filesystem_resize_usage[] = { >> NULL >> }; >> >> -static int check_resize_args(const char *amount, const char *path) { >> +static int check_resize_args(char * const amount, const char *path) >> +{ >> struct btrfs_ioctl_fs_info_args fi_args; >> struct btrfs_ioctl_dev_info_args *di_args = NULL; >> int ret, i, dev_idx = -1; >> @@ -1102,7 +1103,8 @@ static int check_resize_args(const char *amount, const char *path) { >> >> if (ret) { >> error("unable to retrieve fs info"); >> - return 1; >> + ret = 1; >> + goto out; >> } >> >> if (!fi_args.num_devices) { >> @@ -1112,11 +1114,14 @@ static int check_resize_args(const char *amount, const char *path) { >> } >> >> ret = snprintf(amount_dup, BTRFS_VOL_NAME_MAX, "%s", amount); >> +check: >> if (strlen(amount) != ret) { >> error("newsize argument is too long"); >> ret = 1; >> goto out; >> } >> + if (strcmp(amount, amount_dup) != 0) >> + strcpy(amount, amount_dup); >> >> sizestr = amount_dup; >> devstr = strchr(sizestr, ':'); >> @@ -1137,6 +1142,13 @@ static int check_resize_args(const char *amount, const char *path) { >> >> dev_idx = -1; >> for(i = 0; i < fi_args.num_devices; i++) { >> + if (!devstr) { >> + memset(amount_dup, 0, BTRFS_VOL_NAME_MAX); >> + ret = snprintf(amount_dup, BTRFS_VOL_NAME_MAX, "%llu:", di_args[i].devid); >> + ret = snprintf(amount_dup + strlen(amount_dup), >> + BTRFS_VOL_NAME_MAX - strlen(amount_dup), "%s", amount); >> + goto check; >> + } >> if (di_args[i].devid == devid) { >> dev_idx = i; >> break; >> @@ -1235,8 +1247,10 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, >> } >> } >> >> - if (check_argc_exact(argc - optind, 2)) >> - return 1; >> + if (check_argc_exact(argc - optind, 2)) { >> + ret = 1; >> + goto out; >> + } >> >> amount = argv[optind]; >> path = argv[optind + 1]; >> @@ -1244,7 +1258,8 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, >> len = strlen(amount); >> if (len == 0 || len >= BTRFS_VOL_NAME_MAX) { >> error("resize value too long (%s)", amount); >> - return 1; >> + ret = 1; >> + goto out; >> } >> >> cancel = (strcmp("cancel", amount) == 0); >> @@ -1258,7 +1273,8 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, >> "directories as argument. Passing file containing a btrfs image\n" >> "would resize the underlying filesystem instead of the image.\n"); >> } >> - return 1; >> + ret = 1; >> + goto out; >> } >> >> /* >> @@ -1273,14 +1289,22 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, >> error( >> "unable to check status of exclusive operation: %m"); >> close_file_or_dir(fd, dirstream); >> - return 1; >> + goto out; >> } >> } >> >> + amount = (char *)malloc(BTRFS_VOL_NAME_MAX); >> + if (!amount) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + strcpy(amount, argv[optind]); >> + >> ret = check_resize_args(amount, path); >> if (ret != 0) { >> close_file_or_dir(fd, dirstream); >> - return 1; >> + ret = 1; >> + goto free_amount; >> } >> >> memset(&args, 0, sizeof(args)); >> @@ -1298,7 +1322,7 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, >> error("unable to resize '%s': %m", path); >> break; >> } >> - return 1; >> + ret = 1; >> } else if (res > 0) { >> const char *err_str = btrfs_err_str(res); >> >> @@ -1308,9 +1332,12 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, >> error("resizing of '%s' failed: unknown error %d", >> path, res); >> } >> - return 1; >> + ret = 1; >> } >> - return 0; >> +free_amount: >> + free(amount); >> +out: >> + return ret; >> } >> static DEFINE_SIMPLE_COMMAND(filesystem_resize, "resize"); >> >> -- >> 1.8.3.1 >>
On 2022/7/18 09:16, Qu Wenruo wrote: > > > On 2022/7/18 01:39, Zygo Blaxell wrote: >> On Sun, Jul 17, 2022 at 11:08:23PM +0800, Li Zhang wrote: >>> related issues: >>> https://github.com/kdave/btrfs-progs/issues/470 >>> >>> [BUG] >>> If there is no devid=1, when the user uses the btrfs file system >>> tool, the following error will be reported, >>> >>> $ sudo btrfs filesystem show /mnt/1 >>> Label: none uuid: 64dc0f68-9afa-4465-9ea1-2bbebfdb6cec >>> Total devices 2 FS bytes used 704.00KiB >>> devid 2 size 15.00GiB used 1.16GiB path /dev/loop2 >>> devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 >>> $ sudo btrfs filesystem resize -1G /mnt/1 >>> ERROR: cannot find devid: 1 >>> ERROR: unable to resize '/mnt/1': No such device >>> >>> [CAUSE] >>> If the user does not specify the devid id explicitly, btrfs will use >>> the default devid 1, so it will report an error when dev 1 is missing. >>> >>> [FIX] >>> If there is no special devid, the first devid is added automatically >>> and check the maximum length of args passed to kernel space. >>> After patch, when resize filesystem without specified, it would >>> resize the first device, the result is list as following. >>> >>> $ sudo btrfs filesystem show /mnt/1/ >>> Label: none uuid: 7b4827da-bc6e-42aa-b03d-52c2533dfe94 >>> Total devices 2 FS bytes used 144.00KiB >>> devid 2 size 15.00GiB used 1.16GiB path /dev/loop2 >>> devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 >>> >>> $ sudo btrfs filesystem resize -1G /mnt/1 >>> Resize device id 2 (/dev/loop2) from 15.00GiB to 14.00GiB >>> $ sudo btrfs filesystem show /mnt/1/ >>> Label: none uuid: 7b4827da-bc6e-42aa-b03d-52c2533dfe94 >>> Total devices 2 FS bytes used 144.00KiB >>> devid 2 size 14.00GiB used 1.16GiB path /dev/loop2 >>> devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 >> >> Is that desirable behavior? I'd expect that if there are multiple >> devices present, and I haven't specified which one to resize, that the >> command would fail with an error, requiring me to specify which device I >> want resized. Under that expectation, the current behavior of resizing >> devid 1 by default is also incorrect. > > I agree with Zygo. > > If there is only one device, then we can definitely use the first device > we find. > > If there are multiple devices, then it's better to output an error > message, provide candidate devids, and error out. And forgot to mention, after all these changes, we also need to update the man page of "btrfs-filesystem" subcommand. Thanks, Qu > > Thanks, > Qu > >> >> If there's only one device, then 'btrfs fi resize -1G' should resize >> that device, since no ambiguity is possible. >> >>> Signed-off-by: Li Zhang <zhanglikernel@gmail.com> >>> --- >>> cmds/filesystem.c | 49 >>> ++++++++++++++++++++++++++++++++++++++----------- >>> 1 file changed, 38 insertions(+), 11 deletions(-) >>> >>> diff --git a/cmds/filesystem.c b/cmds/filesystem.c >>> index 7cd08fc..2e2414d 100644 >>> --- a/cmds/filesystem.c >>> +++ b/cmds/filesystem.c >>> @@ -1087,7 +1087,8 @@ static const char * const >>> cmd_filesystem_resize_usage[] = { >>> NULL >>> }; >>> >>> -static int check_resize_args(const char *amount, const char *path) { >>> +static int check_resize_args(char * const amount, const char *path) >>> +{ >>> struct btrfs_ioctl_fs_info_args fi_args; >>> struct btrfs_ioctl_dev_info_args *di_args = NULL; >>> int ret, i, dev_idx = -1; >>> @@ -1102,7 +1103,8 @@ static int check_resize_args(const char >>> *amount, const char *path) { >>> >>> if (ret) { >>> error("unable to retrieve fs info"); >>> - return 1; >>> + ret = 1; >>> + goto out; >>> } >>> >>> if (!fi_args.num_devices) { >>> @@ -1112,11 +1114,14 @@ static int check_resize_args(const char >>> *amount, const char *path) { >>> } >>> >>> ret = snprintf(amount_dup, BTRFS_VOL_NAME_MAX, "%s", amount); >>> +check: >>> if (strlen(amount) != ret) { >>> error("newsize argument is too long"); >>> ret = 1; >>> goto out; >>> } >>> + if (strcmp(amount, amount_dup) != 0) >>> + strcpy(amount, amount_dup); >>> >>> sizestr = amount_dup; >>> devstr = strchr(sizestr, ':'); >>> @@ -1137,6 +1142,13 @@ static int check_resize_args(const char >>> *amount, const char *path) { >>> >>> dev_idx = -1; >>> for(i = 0; i < fi_args.num_devices; i++) { >>> + if (!devstr) { >>> + memset(amount_dup, 0, BTRFS_VOL_NAME_MAX); >>> + ret = snprintf(amount_dup, BTRFS_VOL_NAME_MAX, "%llu:", >>> di_args[i].devid); >>> + ret = snprintf(amount_dup + strlen(amount_dup), >>> + BTRFS_VOL_NAME_MAX - strlen(amount_dup), "%s", amount); >>> + goto check; >>> + } >>> if (di_args[i].devid == devid) { >>> dev_idx = i; >>> break; >>> @@ -1235,8 +1247,10 @@ static int cmd_filesystem_resize(const struct >>> cmd_struct *cmd, >>> } >>> } >>> >>> - if (check_argc_exact(argc - optind, 2)) >>> - return 1; >>> + if (check_argc_exact(argc - optind, 2)) { >>> + ret = 1; >>> + goto out; >>> + } >>> >>> amount = argv[optind]; >>> path = argv[optind + 1]; >>> @@ -1244,7 +1258,8 @@ static int cmd_filesystem_resize(const struct >>> cmd_struct *cmd, >>> len = strlen(amount); >>> if (len == 0 || len >= BTRFS_VOL_NAME_MAX) { >>> error("resize value too long (%s)", amount); >>> - return 1; >>> + ret = 1; >>> + goto out; >>> } >>> >>> cancel = (strcmp("cancel", amount) == 0); >>> @@ -1258,7 +1273,8 @@ static int cmd_filesystem_resize(const struct >>> cmd_struct *cmd, >>> "directories as argument. Passing file containing a btrfs >>> image\n" >>> "would resize the underlying filesystem instead of the >>> image.\n"); >>> } >>> - return 1; >>> + ret = 1; >>> + goto out; >>> } >>> >>> /* >>> @@ -1273,14 +1289,22 @@ static int cmd_filesystem_resize(const struct >>> cmd_struct *cmd, >>> error( >>> "unable to check status of exclusive operation: %m"); >>> close_file_or_dir(fd, dirstream); >>> - return 1; >>> + goto out; >>> } >>> } >>> >>> + amount = (char *)malloc(BTRFS_VOL_NAME_MAX); >>> + if (!amount) { >>> + ret = -ENOMEM; >>> + goto out; >>> + } >>> + strcpy(amount, argv[optind]); >>> + >>> ret = check_resize_args(amount, path); >>> if (ret != 0) { >>> close_file_or_dir(fd, dirstream); >>> - return 1; >>> + ret = 1; >>> + goto free_amount; >>> } >>> >>> memset(&args, 0, sizeof(args)); >>> @@ -1298,7 +1322,7 @@ static int cmd_filesystem_resize(const struct >>> cmd_struct *cmd, >>> error("unable to resize '%s': %m", path); >>> break; >>> } >>> - return 1; >>> + ret = 1; >>> } else if (res > 0) { >>> const char *err_str = btrfs_err_str(res); >>> >>> @@ -1308,9 +1332,12 @@ static int cmd_filesystem_resize(const struct >>> cmd_struct *cmd, >>> error("resizing of '%s' failed: unknown error %d", >>> path, res); >>> } >>> - return 1; >>> + ret = 1; >>> } >>> - return 0; >>> +free_amount: >>> + free(amount); >>> +out: >>> + return ret; >>> } >>> static DEFINE_SIMPLE_COMMAND(filesystem_resize, "resize"); >>> >>> -- >>> 1.8.3.1 >>>
Yes, that makes sense, I'll correct this patch later. I'll also try to fix this: https://github.com/kdave/btrfs-progs/issues/471, add parameter all to resize all devices. li zhang <zhanglikernel@gmail.com> 于2022年7月19日周二 00:34写道： > > Yes, that makes sense, I'll correct this patch later. > I'll also try to fix this: > https://github.com/kdave/btrfs-progs/issues/471, add parameter all to > resize all devices. > > Qu Wenruo <quwenruo.btrfs@gmx.com> 于2022年7月18日周一 09:44写道： > > > > > > > > On 2022/7/18 09:16, Qu Wenruo wrote: > > > > > > > > > On 2022/7/18 01:39, Zygo Blaxell wrote: > > >> On Sun, Jul 17, 2022 at 11:08:23PM +0800, Li Zhang wrote: > > >>> related issues: > > >>> https://github.com/kdave/btrfs-progs/issues/470 > > >>> > > >>> [BUG] > > >>> If there is no devid=1, when the user uses the btrfs file system > > >>> tool, the following error will be reported, > > >>> > > >>> $ sudo btrfs filesystem show /mnt/1 > > >>> Label: none uuid: 64dc0f68-9afa-4465-9ea1-2bbebfdb6cec > > >>> Total devices 2 FS bytes used 704.00KiB > > >>> devid 2 size 15.00GiB used 1.16GiB path /dev/loop2 > > >>> devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 > > >>> $ sudo btrfs filesystem resize -1G /mnt/1 > > >>> ERROR: cannot find devid: 1 > > >>> ERROR: unable to resize '/mnt/1': No such device > > >>> > > >>> [CAUSE] > > >>> If the user does not specify the devid id explicitly, btrfs will use > > >>> the default devid 1, so it will report an error when dev 1 is missing. > > >>> > > >>> [FIX] > > >>> If there is no special devid, the first devid is added automatically > > >>> and check the maximum length of args passed to kernel space. > > >>> After patch, when resize filesystem without specified, it would > > >>> resize the first device, the result is list as following. > > >>> > > >>> $ sudo btrfs filesystem show /mnt/1/ > > >>> Label: none uuid: 7b4827da-bc6e-42aa-b03d-52c2533dfe94 > > >>> Total devices 2 FS bytes used 144.00KiB > > >>> devid 2 size 15.00GiB used 1.16GiB path /dev/loop2 > > >>> devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 > > >>> > > >>> $ sudo btrfs filesystem resize -1G /mnt/1 > > >>> Resize device id 2 (/dev/loop2) from 15.00GiB to 14.00GiB > > >>> $ sudo btrfs filesystem show /mnt/1/ > > >>> Label: none uuid: 7b4827da-bc6e-42aa-b03d-52c2533dfe94 > > >>> Total devices 2 FS bytes used 144.00KiB > > >>> devid 2 size 14.00GiB used 1.16GiB path /dev/loop2 > > >>> devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 > > >> > > >> Is that desirable behavior? I'd expect that if there are multiple > > >> devices present, and I haven't specified which one to resize, that the > > >> command would fail with an error, requiring me to specify which device I > > >> want resized. Under that expectation, the current behavior of resizing > > >> devid 1 by default is also incorrect. > > > > > > I agree with Zygo. > > > > > > If there is only one device, then we can definitely use the first device > > > we find. > > > > > > If there are multiple devices, then it's better to output an error > > > message, provide candidate devids, and error out. > > > > And forgot to mention, after all these changes, we also need to update > > the man page of "btrfs-filesystem" subcommand. > > > > Thanks, > > Qu > > > > > > > > Thanks, > > > Qu > > > > > >> > > >> If there's only one device, then 'btrfs fi resize -1G' should resize > > >> that device, since no ambiguity is possible. > > >> > > >>> Signed-off-by: Li Zhang <zhanglikernel@gmail.com> > > >>> --- > > >>> cmds/filesystem.c | 49 > > >>> ++++++++++++++++++++++++++++++++++++++----------- > > >>> 1 file changed, 38 insertions(+), 11 deletions(-) > > >>> > > >>> diff --git a/cmds/filesystem.c b/cmds/filesystem.c > > >>> index 7cd08fc..2e2414d 100644 > > >>> --- a/cmds/filesystem.c > > >>> +++ b/cmds/filesystem.c > > >>> @@ -1087,7 +1087,8 @@ static const char * const > > >>> cmd_filesystem_resize_usage[] = { > > >>> NULL > > >>> }; > > >>> > > >>> -static int check_resize_args(const char *amount, const char *path) { > > >>> +static int check_resize_args(char * const amount, const char *path) > > >>> +{ > > >>> struct btrfs_ioctl_fs_info_args fi_args; > > >>> struct btrfs_ioctl_dev_info_args *di_args = NULL; > > >>> int ret, i, dev_idx = -1; > > >>> @@ -1102,7 +1103,8 @@ static int check_resize_args(const char > > >>> *amount, const char *path) { > > >>> > > >>> if (ret) { > > >>> error("unable to retrieve fs info"); > > >>> - return 1; > > >>> + ret = 1; > > >>> + goto out; > > >>> } > > >>> > > >>> if (!fi_args.num_devices) { > > >>> @@ -1112,11 +1114,14 @@ static int check_resize_args(const char > > >>> *amount, const char *path) { > > >>> } > > >>> > > >>> ret = snprintf(amount_dup, BTRFS_VOL_NAME_MAX, "%s", amount); > > >>> +check: > > >>> if (strlen(amount) != ret) { > > >>> error("newsize argument is too long"); > > >>> ret = 1; > > >>> goto out; > > >>> } > > >>> + if (strcmp(amount, amount_dup) != 0) > > >>> + strcpy(amount, amount_dup); > > >>> > > >>> sizestr = amount_dup; > > >>> devstr = strchr(sizestr, ':'); > > >>> @@ -1137,6 +1142,13 @@ static int check_resize_args(const char > > >>> *amount, const char *path) { > > >>> > > >>> dev_idx = -1; > > >>> for(i = 0; i < fi_args.num_devices; i++) { > > >>> + if (!devstr) { > > >>> + memset(amount_dup, 0, BTRFS_VOL_NAME_MAX); > > >>> + ret = snprintf(amount_dup, BTRFS_VOL_NAME_MAX, "%llu:", > > >>> di_args[i].devid); > > >>> + ret = snprintf(amount_dup + strlen(amount_dup), > > >>> + BTRFS_VOL_NAME_MAX - strlen(amount_dup), "%s", amount); > > >>> + goto check; > > >>> + } > > >>> if (di_args[i].devid == devid) { > > >>> dev_idx = i; > > >>> break; > > >>> @@ -1235,8 +1247,10 @@ static int cmd_filesystem_resize(const struct > > >>> cmd_struct *cmd, > > >>> } > > >>> } > > >>> > > >>> - if (check_argc_exact(argc - optind, 2)) > > >>> - return 1; > > >>> + if (check_argc_exact(argc - optind, 2)) { > > >>> + ret = 1; > > >>> + goto out; > > >>> + } > > >>> > > >>> amount = argv[optind]; > > >>> path = argv[optind + 1]; > > >>> @@ -1244,7 +1258,8 @@ static int cmd_filesystem_resize(const struct > > >>> cmd_struct *cmd, > > >>> len = strlen(amount); > > >>> if (len == 0 || len >= BTRFS_VOL_NAME_MAX) { > > >>> error("resize value too long (%s)", amount); > > >>> - return 1; > > >>> + ret = 1; > > >>> + goto out; > > >>> } > > >>> > > >>> cancel = (strcmp("cancel", amount) == 0); > > >>> @@ -1258,7 +1273,8 @@ static int cmd_filesystem_resize(const struct > > >>> cmd_struct *cmd, > > >>> "directories as argument. Passing file containing a btrfs > > >>> image\n" > > >>> "would resize the underlying filesystem instead of the > > >>> image.\n"); > > >>> } > > >>> - return 1; > > >>> + ret = 1; > > >>> + goto out; > > >>> } > > >>> > > >>> /* > > >>> @@ -1273,14 +1289,22 @@ static int cmd_filesystem_resize(const struct > > >>> cmd_struct *cmd, > > >>> error( > > >>> "unable to check status of exclusive operation: %m"); > > >>> close_file_or_dir(fd, dirstream); > > >>> - return 1; > > >>> + goto out; > > >>> } > > >>> } > > >>> > > >>> + amount = (char *)malloc(BTRFS_VOL_NAME_MAX); > > >>> + if (!amount) { > > >>> + ret = -ENOMEM; > > >>> + goto out; > > >>> + } > > >>> + strcpy(amount, argv[optind]); > > >>> + > > >>> ret = check_resize_args(amount, path); > > >>> if (ret != 0) { > > >>> close_file_or_dir(fd, dirstream); > > >>> - return 1; > > >>> + ret = 1; > > >>> + goto free_amount; > > >>> } > > >>> > > >>> memset(&args, 0, sizeof(args)); > > >>> @@ -1298,7 +1322,7 @@ static int cmd_filesystem_resize(const struct > > >>> cmd_struct *cmd, > > >>> error("unable to resize '%s': %m", path); > > >>> break; > > >>> } > > >>> - return 1; > > >>> + ret = 1; > > >>> } else if (res > 0) { > > >>> const char *err_str = btrfs_err_str(res); > > >>> > > >>> @@ -1308,9 +1332,12 @@ static int cmd_filesystem_resize(const struct > > >>> cmd_struct *cmd, > > >>> error("resizing of '%s' failed: unknown error %d", > > >>> path, res); > > >>> } > > >>> - return 1; > > >>> + ret = 1; > > >>> } > > >>> - return 0; > > >>> +free_amount: > > >>> + free(amount); > > >>> +out: > > >>> + return ret; > > >>> } > > >>> static DEFINE_SIMPLE_COMMAND(filesystem_resize, "resize"); > > >>> > > >>> -- > > >>> 1.8.3.1 > > >>>
diff --git a/cmds/filesystem.c b/cmds/filesystem.c index 7cd08fc..2e2414d 100644 --- a/cmds/filesystem.c +++ b/cmds/filesystem.c @@ -1087,7 +1087,8 @@ static const char * const cmd_filesystem_resize_usage[] = { NULL }; -static int check_resize_args(const char *amount, const char *path) { +static int check_resize_args(char * const amount, const char *path) +{ struct btrfs_ioctl_fs_info_args fi_args; struct btrfs_ioctl_dev_info_args *di_args = NULL; int ret, i, dev_idx = -1; @@ -1102,7 +1103,8 @@ static int check_resize_args(const char *amount, const char *path) { if (ret) { error("unable to retrieve fs info"); - return 1; + ret = 1; + goto out; } if (!fi_args.num_devices) { @@ -1112,11 +1114,14 @@ static int check_resize_args(const char *amount, const char *path) { } ret = snprintf(amount_dup, BTRFS_VOL_NAME_MAX, "%s", amount); +check: if (strlen(amount) != ret) { error("newsize argument is too long"); ret = 1; goto out; } + if (strcmp(amount, amount_dup) != 0) + strcpy(amount, amount_dup); sizestr = amount_dup; devstr = strchr(sizestr, ':'); @@ -1137,6 +1142,13 @@ static int check_resize_args(const char *amount, const char *path) { dev_idx = -1; for(i = 0; i < fi_args.num_devices; i++) { + if (!devstr) { + memset(amount_dup, 0, BTRFS_VOL_NAME_MAX); + ret = snprintf(amount_dup, BTRFS_VOL_NAME_MAX, "%llu:", di_args[i].devid); + ret = snprintf(amount_dup + strlen(amount_dup), + BTRFS_VOL_NAME_MAX - strlen(amount_dup), "%s", amount); + goto check; + } if (di_args[i].devid == devid) { dev_idx = i; break; @@ -1235,8 +1247,10 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, } } - if (check_argc_exact(argc - optind, 2)) - return 1; + if (check_argc_exact(argc - optind, 2)) { + ret = 1; + goto out; + } amount = argv[optind]; path = argv[optind + 1]; @@ -1244,7 +1258,8 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, len = strlen(amount); if (len == 0 || len >= BTRFS_VOL_NAME_MAX) { error("resize value too long (%s)", amount); - return 1; + ret = 1; + goto out; } cancel = (strcmp("cancel", amount) == 0); @@ -1258,7 +1273,8 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, "directories as argument. Passing file containing a btrfs image\n" "would resize the underlying filesystem instead of the image.\n"); } - return 1; + ret = 1; + goto out; } /* @@ -1273,14 +1289,22 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, error( "unable to check status of exclusive operation: %m"); close_file_or_dir(fd, dirstream); - return 1; + goto out; } } + amount = (char *)malloc(BTRFS_VOL_NAME_MAX); + if (!amount) { + ret = -ENOMEM; + goto out; + } + strcpy(amount, argv[optind]); + ret = check_resize_args(amount, path); if (ret != 0) { close_file_or_dir(fd, dirstream); - return 1; + ret = 1; + goto free_amount; } memset(&args, 0, sizeof(args)); @@ -1298,7 +1322,7 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, error("unable to resize '%s': %m", path); break; } - return 1; + ret = 1; } else if (res > 0) { const char *err_str = btrfs_err_str(res); @@ -1308,9 +1332,12 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd, error("resizing of '%s' failed: unknown error %d", path, res); } - return 1; + ret = 1; } - return 0; +free_amount: + free(amount); +out: + return ret; } static DEFINE_SIMPLE_COMMAND(filesystem_resize, "resize");
related issues: https://github.com/kdave/btrfs-progs/issues/470 [BUG] If there is no devid=1, when the user uses the btrfs file system tool, the following error will be reported, $ sudo btrfs filesystem show /mnt/1 Label: none uuid: 64dc0f68-9afa-4465-9ea1-2bbebfdb6cec Total devices 2 FS bytes used 704.00KiB devid 2 size 15.00GiB used 1.16GiB path /dev/loop2 devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 $ sudo btrfs filesystem resize -1G /mnt/1 ERROR: cannot find devid: 1 ERROR: unable to resize '/mnt/1': No such device [CAUSE] If the user does not specify the devid id explicitly, btrfs will use the default devid 1, so it will report an error when dev 1 is missing. [FIX] If there is no special devid, the first devid is added automatically and check the maximum length of args passed to kernel space. After patch, when resize filesystem without specified, it would resize the first device, the result is list as following. $ sudo btrfs filesystem show /mnt/1/ Label: none uuid: 7b4827da-bc6e-42aa-b03d-52c2533dfe94 Total devices 2 FS bytes used 144.00KiB devid 2 size 15.00GiB used 1.16GiB path /dev/loop2 devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 $ sudo btrfs filesystem resize -1G /mnt/1 Resize device id 2 (/dev/loop2) from 15.00GiB to 14.00GiB $ sudo btrfs filesystem show /mnt/1/ Label: none uuid: 7b4827da-bc6e-42aa-b03d-52c2533dfe94 Total devices 2 FS bytes used 144.00KiB devid 2 size 14.00GiB used 1.16GiB path /dev/loop2 devid 3 size 15.00GiB used 1.16GiB path /dev/loop3 Signed-off-by: Li Zhang <zhanglikernel@gmail.com> --- cmds/filesystem.c | 49 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 11 deletions(-)