* [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output
@ 2021-04-19 13:05 Su Yue
2021-04-19 17:03 ` David Sterba
2021-04-19 17:08 ` Boris Burkov
0 siblings, 2 replies; 5+ messages in thread
From: Su Yue @ 2021-04-19 13:05 UTC (permalink / raw)
To: linux-btrfs; +Cc: l, Chris Murphy
Resize to nums without sign prefix makes false output:
btrfs fi resize 1:150g /srv/extra
Resize device id 1 (/dev/sdb1) from 298.09GiB to 0.00B
The resize operation would take effect though.
Fix it by handling the case if mod is 0 in check_resize_args().
Issue: #307
Reported-by: Chris Murphy <lists@colorremedies.com>
Signed-off-by: Su Yue <l@damenly.su>
---
cmds/filesystem.c | 10 ++++++++++
1 file changed, 10 insertions(+)
---
Changelog:
v2:
Calculate u64 diff using max() and min().
Calculate mod by comparing new and old size.
diff --git a/cmds/filesystem.c b/cmds/filesystem.c
index 9e3cce687d6e..54d46470c53f 100644
--- a/cmds/filesystem.c
+++ b/cmds/filesystem.c
@@ -1158,6 +1158,16 @@ static int check_resize_args(const char *amount, const char *path) {
}
old_size = di_args[dev_idx].total_bytes;
+ /* For target sizes without '+'/'-' sign prefix(e.g. 1:150g) */
+ if (mod == 0) {
+ new_size = diff;
+ diff = max(old_size, new_size) - min(old_size, new_size);
+ if (new_size > old_size)
+ mod = 1;
+ else if (new_size < old_size)
+ mod = -1;
+ }
+
if (mod < 0) {
if (diff > old_size) {
error("current size is %s which is smaller than %s",
--
2.30.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output
2021-04-19 13:05 [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output Su Yue
@ 2021-04-19 17:03 ` David Sterba
2021-04-19 17:08 ` Boris Burkov
1 sibling, 0 replies; 5+ messages in thread
From: David Sterba @ 2021-04-19 17:03 UTC (permalink / raw)
To: 20210419124541.148269-1-l; +Cc: linux-btrfs, l, Chris Murphy
On Mon, Apr 19, 2021 at 09:05:49PM +0800, Su Yue wrote:
> Resize to nums without sign prefix makes false output:
> btrfs fi resize 1:150g /srv/extra
> Resize device id 1 (/dev/sdb1) from 298.09GiB to 0.00B
>
> The resize operation would take effect though.
>
> Fix it by handling the case if mod is 0 in check_resize_args().
>
> Issue: #307
> Reported-by: Chris Murphy <lists@colorremedies.com>
> Signed-off-by: Su Yue <l@damenly.su>
Thanks, added to devel.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output
2021-04-19 13:05 [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output Su Yue
2021-04-19 17:03 ` David Sterba
@ 2021-04-19 17:08 ` Boris Burkov
2021-04-19 17:28 ` David Sterba
2021-04-20 4:20 ` Su Yue
1 sibling, 2 replies; 5+ messages in thread
From: Boris Burkov @ 2021-04-19 17:08 UTC (permalink / raw)
To: 20210419124541.148269-1-l; +Cc: linux-btrfs, l, Chris Murphy
On Mon, Apr 19, 2021 at 09:05:49PM +0800, Su Yue wrote:
> Resize to nums without sign prefix makes false output:
> btrfs fi resize 1:150g /srv/extra
> Resize device id 1 (/dev/sdb1) from 298.09GiB to 0.00B
>
> The resize operation would take effect though.
>
> Fix it by handling the case if mod is 0 in check_resize_args().
>
> Issue: #307
> Reported-by: Chris Murphy <lists@colorremedies.com>
> Signed-off-by: Su Yue <l@damenly.su>
> ---
> cmds/filesystem.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> ---
> Changelog:
> v2:
> Calculate u64 diff using max() and min().
> Calculate mod by comparing new and old size.
> diff --git a/cmds/filesystem.c b/cmds/filesystem.c
> index 9e3cce687d6e..54d46470c53f 100644
> --- a/cmds/filesystem.c
> +++ b/cmds/filesystem.c
> @@ -1158,6 +1158,16 @@ static int check_resize_args(const char *amount, const char *path) {
> }
> old_size = di_args[dev_idx].total_bytes;
>
> + /* For target sizes without '+'/'-' sign prefix(e.g. 1:150g) */
> + if (mod == 0) {
> + new_size = diff;
> + diff = max(old_size, new_size) - min(old_size, new_size);
> + if (new_size > old_size)
> + mod = 1;
> + else if (new_size < old_size)
> + mod = -1;
> + }
> +
> if (mod < 0) {
> if (diff > old_size) {
> error("current size is %s which is smaller than %s",
This fix seems correct to me, but it feels a tiny bit over-complicated.
Personally, I think it would be cleaner to do something like:
if (mod == 0) {
new_size = diff;
} else if (mod < 0) {
// >0 check
new_size = old_size - diff
} else {
// overflow check
new_size = old_size + diff
}
At this point, new_size is set correctly in all cases and we can do the
print. I tested this version on some simple cases, and it seems to work
ok as well.
Thanks for the fix,
Boris
> --
> 2.30.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output
2021-04-19 17:08 ` Boris Burkov
@ 2021-04-19 17:28 ` David Sterba
2021-04-20 4:20 ` Su Yue
1 sibling, 0 replies; 5+ messages in thread
From: David Sterba @ 2021-04-19 17:28 UTC (permalink / raw)
To: Boris Burkov; +Cc: 20210419124541.148269-1-l, linux-btrfs, l, Chris Murphy
On Mon, Apr 19, 2021 at 10:08:50AM -0700, Boris Burkov wrote:
> On Mon, Apr 19, 2021 at 09:05:49PM +0800, Su Yue wrote:
> > @@ -1158,6 +1158,16 @@ static int check_resize_args(const char *amount, const char *path) {
> > }
> > old_size = di_args[dev_idx].total_bytes;
> >
> > + /* For target sizes without '+'/'-' sign prefix(e.g. 1:150g) */
> > + if (mod == 0) {
> > + new_size = diff;
> > + diff = max(old_size, new_size) - min(old_size, new_size);
> > + if (new_size > old_size)
> > + mod = 1;
> > + else if (new_size < old_size)
> > + mod = -1;
> > + }
> > +
> > if (mod < 0) {
> > if (diff > old_size) {
> > error("current size is %s which is smaller than %s",
>
> This fix seems correct to me, but it feels a tiny bit over-complicated.
> Personally, I think it would be cleaner to do something like:
>
> if (mod == 0) {
> new_size = diff;
> } else if (mod < 0) {
> // >0 check
> new_size = old_size - diff
> } else {
> // overflow check
> new_size = old_size + diff
> }
Right, this looks much better and shares a lot of with the code that
follows the original fix.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output
2021-04-19 17:08 ` Boris Burkov
2021-04-19 17:28 ` David Sterba
@ 2021-04-20 4:20 ` Su Yue
1 sibling, 0 replies; 5+ messages in thread
From: Su Yue @ 2021-04-20 4:20 UTC (permalink / raw)
To: Boris Burkov; +Cc: 20210419124541.148269-1-l, linux-btrfs, Chris Murphy
On Tue 20 Apr 2021 at 01:08, Boris Burkov <boris@bur.io> wrote:
> On Mon, Apr 19, 2021 at 09:05:49PM +0800, Su Yue wrote:
>> Resize to nums without sign prefix makes false output:
>> btrfs fi resize 1:150g /srv/extra
>> Resize device id 1 (/dev/sdb1) from 298.09GiB to 0.00B
>>
>> The resize operation would take effect though.
>>
>> Fix it by handling the case if mod is 0 in check_resize_args().
>>
>> Issue: #307
>> Reported-by: Chris Murphy <lists@colorremedies.com>
>> Signed-off-by: Su Yue <l@damenly.su>
>> ---
>> cmds/filesystem.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> ---
>> Changelog:
>> v2:
>> Calculate u64 diff using max() and min().
>> Calculate mod by comparing new and old size.
>> diff --git a/cmds/filesystem.c b/cmds/filesystem.c
>> index 9e3cce687d6e..54d46470c53f 100644
>> --- a/cmds/filesystem.c
>> +++ b/cmds/filesystem.c
>> @@ -1158,6 +1158,16 @@ static int check_resize_args(const char
>> *amount, const char *path) {
>> }
>> old_size = di_args[dev_idx].total_bytes;
>>
>> + /* For target sizes without '+'/'-' sign prefix(e.g.
>> 1:150g) */
>> + if (mod == 0) {
>> + new_size = diff;
>> + diff = max(old_size, new_size) - min(old_size,
>> new_size);
>> + if (new_size > old_size)
>> + mod = 1;
>> + else if (new_size < old_size)
>> + mod = -1;
>> + }
>> +
>> if (mod < 0) {
>> if (diff > old_size) {
>> error("current size is %s which is smaller
>> than %s",
>
> This fix seems correct to me, but it feels a tiny bit
> over-complicated.
> Personally, I think it would be cleaner to do something like:
>
> if (mod == 0) {
> new_size = diff;
> } else if (mod < 0) {
> // >0 check
> new_size = old_size - diff
> } else {
> // overflow check
> new_size = old_size + diff
> }
>
> At this point, new_size is set correctly in all cases and we can
> do the
> print. I tested this version on some simple cases, and it seems
> to work
> ok as well.
>
Right. The first fix when I saw the code was same with yours.
Now I relaized that I was over thinking about boundary checks and
falling through checks are meaningless if mod is 0.
Just to clarify that my fix is wrong:
if (mod == 0) {
new_size = diff;
diff = max(old_size, new_size) - min(old_size,
new_size);
if (new_size > old_size)
mod = 1;
else if (new_size < old_size)
mod = -1;
} /* falls through to check diff */
if (mod < 0) { // new_size < old_size, diff = old_size -
new_size
// so diff < new_size
if (diff > old_size) // can't happen, if mod is 0
before.
...
} else if (mod > 0) {
// new_size > old_size so diff = new_size(u64) - old_size,
if (diff > ULLONG_MAX - old_size) // can't happen even
new_size is
// ULLONG_MAX if mod
is 0 before
...
}
Thanks for your suggestion, will send v3.
--
Su
> Thanks for the fix,
> Boris
>
>> --
>> 2.30.1
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-04-20 4:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-19 13:05 [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output Su Yue
2021-04-19 17:03 ` David Sterba
2021-04-19 17:08 ` Boris Burkov
2021-04-19 17:28 ` David Sterba
2021-04-20 4:20 ` Su Yue
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.