[PATCH] btrfs: fix balance convert to single on 32-bit host CPUs

[PATCH] btrfs: fix balance convert to single on 32-bit host CPUs

[Zygo Blaxell]

Currently, the command:

	btrfs balance start -dconvert=single,soft .

on a Raspberry Pi produces the following kernel message:

	BTRFS error (device mmcblk0p2): balance: invalid convert data profile single

This fails because we use is_power_of_2(unsigned long) to validate
the new data profile, the constant for 'single' profile uses bit 48,
and there are only 32 bits in a long on ARM.

Fix by open-coding the check using u64 variables.

Tested by completing the original balance command on several Raspberry
Pis.

Signed-off-by: Zygo Blaxell <ce3g8jdj@umail.furryterror.org>
---
 fs/btrfs/volumes.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 88a323a453d8..252c6049c6b7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3906,7 +3906,11 @@ static int alloc_profile_is_valid(u64 flags, int extended)
 		return !extended; /* "0" is valid for usual profiles */
 
 	/* true if exactly one bit set */
-	return is_power_of_2(flags);
+	/*
+	 * Don't use is_power_of_2(unsigned long) because it won't work
+	 * for the single profile (1ULL << 48) on 32-bit CPUs.
+	 */
+	return flags != 0 && (flags & (flags - 1)) == 0;
 }
 
 static inline int balance_need_close(struct btrfs_fs_info *fs_info)
-- 
2.20.1

Re: [PATCH] btrfs: fix balance convert to single on 32-bit host CPUs

[Johannes Thumshirn]

On 13/09/2019 01:55, Zygo Blaxell wrote:
> Currently, the command:
> 
> 	btrfs balance start -dconvert=single,soft .
> 
> on a Raspberry Pi produces the following kernel message:
> 
> 	BTRFS error (device mmcblk0p2): balance: invalid convert data profile single
> 
> This fails because we use is_power_of_2(unsigned long) to validate
> the new data profile, the constant for 'single' profile uses bit 48,
> and there are only 32 bits in a long on ARM.
> 
> Fix by open-coding the check using u64 variables.
> 
> Tested by completing the original balance command on several Raspberry
> Pis.
> 
> Signed-off-by: Zygo Blaxell <ce3g8jdj@umail.furryterror.org>
> ---
>  fs/btrfs/volumes.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 88a323a453d8..252c6049c6b7 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3906,7 +3906,11 @@ static int alloc_profile_is_valid(u64 flags, int extended)
>  		return !extended; /* "0" is valid for usual profiles */
>  
>  	/* true if exactly one bit set */
> -	return is_power_of_2(flags);
> +	/*
> +	 * Don't use is_power_of_2(unsigned long) because it won't work
> +	 * for the single profile (1ULL << 48) on 32-bit CPUs.
> +	 */
> +	return flags != 0 && (flags & (flags - 1)) == 0;

Would
flags && IS_ALIGNED(flags)

Work as well? IS_ALIGNED() should be type-save due to the typeof():
#define IS_ALIGNED(x, a) (((x) & ((typeof(x))(a) - 1)) == 0)

Or maybe I'm missing something subtle?

Thanks,
	Johannes
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 247165, AG München)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] btrfs: fix balance convert to single on 32-bit host CPUs

[Zygo Blaxell]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

On Fri, Sep 13, 2019 at 10:24:08AM +0200, Johannes Thumshirn wrote:
> On 13/09/2019 01:55, Zygo Blaxell wrote:
> > Currently, the command:
> > 
> > 	btrfs balance start -dconvert=single,soft .
> > 
> > on a Raspberry Pi produces the following kernel message:
> > 
> > 	BTRFS error (device mmcblk0p2): balance: invalid convert data profile single
> > 
> > This fails because we use is_power_of_2(unsigned long) to validate
> > the new data profile, the constant for 'single' profile uses bit 48,
> > and there are only 32 bits in a long on ARM.
> > 
> > Fix by open-coding the check using u64 variables.
> > 
> > Tested by completing the original balance command on several Raspberry
> > Pis.
> > 
> > Signed-off-by: Zygo Blaxell <ce3g8jdj@umail.furryterror.org>
> > ---
> >  fs/btrfs/volumes.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 88a323a453d8..252c6049c6b7 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -3906,7 +3906,11 @@ static int alloc_profile_is_valid(u64 flags, int extended)
> >  		return !extended; /* "0" is valid for usual profiles */
> >  
> >  	/* true if exactly one bit set */
> > -	return is_power_of_2(flags);
> > +	/*
> > +	 * Don't use is_power_of_2(unsigned long) because it won't work
> > +	 * for the single profile (1ULL << 48) on 32-bit CPUs.
> > +	 */
> > +	return flags != 0 && (flags & (flags - 1)) == 0;
> 
> Would
> flags && IS_ALIGNED(flags)
> 
> Work as well? IS_ALIGNED() should be type-save due to the typeof():
> #define IS_ALIGNED(x, a) (((x) & ((typeof(x))(a) - 1)) == 0)
> 
> Or maybe I'm missing something subtle?

Wouldn't that be
	
	flags && IS_ALIGNED(flags, flags)

?

> Thanks,
> 	Johannes
> -- 
> Johannes Thumshirn                            SUSE Labs Filesystems
> jthumshirn@suse.de                                +49 911 74053 689
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5
> 90409 Nürnberg
> Germany
> (HRB 247165, AG München)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] btrfs: fix balance convert to single on 32-bit host CPUs

[Johannes Thumshirn]

On 14/09/2019 06:59, Zygo Blaxell wrote:
> 
> Wouldn't that be
> 	
> 	flags && IS_ALIGNED(flags, flags)
> 
Of cause, I'm stupid, sorry.

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 247165, AG München)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] btrfs: fix balance convert to single on 32-bit host CPUs

[Josef Bacik]

On Thu, Sep 12, 2019 at 07:55:01PM -0400, Zygo Blaxell wrote:
> Currently, the command:
> 
> 	btrfs balance start -dconvert=single,soft .
> 
> on a Raspberry Pi produces the following kernel message:
> 
> 	BTRFS error (device mmcblk0p2): balance: invalid convert data profile single
> 
> This fails because we use is_power_of_2(unsigned long) to validate
> the new data profile, the constant for 'single' profile uses bit 48,
> and there are only 32 bits in a long on ARM.
> 
> Fix by open-coding the check using u64 variables.
> 
> Tested by completing the original balance command on several Raspberry
> Pis.
> 
> Signed-off-by: Zygo Blaxell <ce3g8jdj@umail.furryterror.org>

Honestly I'd rather we fixed is_power_of_2 to work on 32bit, that way any other
users don't get bitten by the same problem.  Thanks,

Josef

Re: [PATCH] btrfs: fix balance convert to single on 32-bit host CPUs

[Zygo Blaxell]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

On Fri, Sep 13, 2019 at 09:14:08AM -0700, Josef Bacik wrote:
> On Thu, Sep 12, 2019 at 07:55:01PM -0400, Zygo Blaxell wrote:
> > Currently, the command:
> > 
> > 	btrfs balance start -dconvert=single,soft .
> > 
> > on a Raspberry Pi produces the following kernel message:
> > 
> > 	BTRFS error (device mmcblk0p2): balance: invalid convert data profile single
> > 
> > This fails because we use is_power_of_2(unsigned long) to validate
> > the new data profile, the constant for 'single' profile uses bit 48,
> > and there are only 32 bits in a long on ARM.
> > 
> > Fix by open-coding the check using u64 variables.
> > 
> > Tested by completing the original balance command on several Raspberry
> > Pis.
> > 
> > Signed-off-by: Zygo Blaxell <ce3g8jdj@umail.furryterror.org>
> 
> Honestly I'd rather we fixed is_power_of_2 to work on 32bit, that way any other
> users don't get bitten by the same problem.  Thanks,

is_power_of_2 doesn't live in the btrfs tree.  I considered modifying it,
but worried about side-effects elsewhere (i.e. breaking some other 32-bit
device I've never heard of).

What do you think about using the IS_ALIGNED macro?

> Josef

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] btrfs: fix balance convert to single on 32-bit host CPUs

[Nikolay Borisov]

On 14.09.19 г. 7:59 ч., Zygo Blaxell wrote:
> On Fri, Sep 13, 2019 at 09:14:08AM -0700, Josef Bacik wrote:
>> On Thu, Sep 12, 2019 at 07:55:01PM -0400, Zygo Blaxell wrote:
>>> Currently, the command:
>>>
>>> 	btrfs balance start -dconvert=single,soft .
>>>
>>> on a Raspberry Pi produces the following kernel message:
>>>
>>> 	BTRFS error (device mmcblk0p2): balance: invalid convert data profile single
>>>
>>> This fails because we use is_power_of_2(unsigned long) to validate
>>> the new data profile, the constant for 'single' profile uses bit 48,
>>> and there are only 32 bits in a long on ARM.
>>>
>>> Fix by open-coding the check using u64 variables.
>>>
>>> Tested by completing the original balance command on several Raspberry
>>> Pis.
>>>
>>> Signed-off-by: Zygo Blaxell <ce3g8jdj@umail.furryterror.org>
>>
>> Honestly I'd rather we fixed is_power_of_2 to work on 32bit, that way any other
>> users don't get bitten by the same problem.  Thanks,
> 
> is_power_of_2 doesn't live in the btrfs tree.  I considered modifying it,
> but worried about side-effects elsewhere (i.e. breaking some other 32-bit
> device I've never heard of).
> 
> What do you think about using the IS_ALIGNED macro?

THe problem is that is_power_of_2 is defined as a function that takes an
unsigned long which is defined as able to hold at least 32 bits. One
possible change will be do change it to being a macro. However, as you
note, this might lead to subtle breakage elsewhere. Furthermore other
functions in log2.h are also defined with unsigned long as their input
params.
> 
>> Josef

Re: [PATCH] btrfs: fix balance convert to single on 32-bit host CPUs

[David Sterba]

On Thu, Sep 12, 2019 at 07:55:01PM -0400, Zygo Blaxell wrote:
> Currently, the command:
> 
> 	btrfs balance start -dconvert=single,soft .
> 
> on a Raspberry Pi produces the following kernel message:
> 
> 	BTRFS error (device mmcblk0p2): balance: invalid convert data profile single
> 
> This fails because we use is_power_of_2(unsigned long) to validate
> the new data profile, the constant for 'single' profile uses bit 48,
> and there are only 32 bits in a long on ARM.
> 
> Fix by open-coding the check using u64 variables.
> 
> Tested by completing the original balance command on several Raspberry
> Pis.
> 
> Signed-off-by: Zygo Blaxell <ce3g8jdj@umail.furryterror.org>
> ---
>  fs/btrfs/volumes.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 88a323a453d8..252c6049c6b7 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3906,7 +3906,11 @@ static int alloc_profile_is_valid(u64 flags, int extended)
>  		return !extended; /* "0" is valid for usual profiles */
>  
>  	/* true if exactly one bit set */
> -	return is_power_of_2(flags);
> +	/*
> +	 * Don't use is_power_of_2(unsigned long) because it won't work
> +	 * for the single profile (1ULL << 48) on 32-bit CPUs.
> +	 */
> +	return flags != 0 && (flags & (flags - 1)) == 0;

I'd rather not open code it again. Based on the discussion, we need a
separate helper that takes u64 and possibly has the "value has exactly
one bit set" semantics from the beginnin. We now have a file for such
helpers (fs/btrfs/misc.h).

There would a few more users of the new helper (now done using the
is_power_of_2 helper), that would improve readability.

Re: [PATCH] btrfs: fix balance convert to single on 32-bit host CPUs

[David Sterba]

On Mon, Sep 23, 2019 at 05:14:04PM +0200, David Sterba wrote:
> On Thu, Sep 12, 2019 at 07:55:01PM -0400, Zygo Blaxell wrote:
> > Currently, the command:
> > 
> > 	btrfs balance start -dconvert=single,soft .
> > 
> > on a Raspberry Pi produces the following kernel message:
> > 
> > 	BTRFS error (device mmcblk0p2): balance: invalid convert data profile single
> > 
> > This fails because we use is_power_of_2(unsigned long) to validate
> > the new data profile, the constant for 'single' profile uses bit 48,
> > and there are only 32 bits in a long on ARM.
> > 
> > Fix by open-coding the check using u64 variables.
> > 
> > Tested by completing the original balance command on several Raspberry
> > Pis.
> > 
> > Signed-off-by: Zygo Blaxell <ce3g8jdj@umail.furryterror.org>
> > ---
> >  fs/btrfs/volumes.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 88a323a453d8..252c6049c6b7 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -3906,7 +3906,11 @@ static int alloc_profile_is_valid(u64 flags, int extended)
> >  		return !extended; /* "0" is valid for usual profiles */
> >  
> >  	/* true if exactly one bit set */
> > -	return is_power_of_2(flags);
> > +	/*
> > +	 * Don't use is_power_of_2(unsigned long) because it won't work
> > +	 * for the single profile (1ULL << 48) on 32-bit CPUs.
> > +	 */
> > +	return flags != 0 && (flags & (flags - 1)) == 0;
> 
> I'd rather not open code it again. Based on the discussion, we need a
> separate helper that takes u64 and possibly has the "value has exactly
> one bit set" semantics from the beginnin. We now have a file for such
> helpers (fs/btrfs/misc.h).
> 
> There would a few more users of the new helper (now done using the
> is_power_of_2 helper), that would improve readability.

I'll apply the patch as-is so it can go to stable and do the helper and
other cleanups myself.