| Message ID | 20190912235507.3DE794232AF@james.kirk.hungrycats.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix balance convert to single on 32-bit host CPUs | expand |
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
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
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
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
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
On 14/09/2019 06:59, Zygo Blaxell wrote: > > Wouldn't that be > > flags && IS_ALIGNED(flags, flags) > Of cause, I'm stupid, sorry.
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.
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.
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)
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(-)