[PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output

[PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output

[Su Yue]

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

Re: [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output

[David Sterba]

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.

Re: [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output

[Boris Burkov]

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
> 

Re: [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output

[David Sterba]

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.

Re: [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output

[Su Yue]

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