diff mbox

btrfs: scrub: use do_div() for 64-by-32 division

Message ID 20170408210737.5456-1-kilobyte@angband.pl (mailing list archive)
State New, archived
Headers show

Commit Message

Adam Borowski April 8, 2017, 9:07 p.m. UTC
Unbreaks ARM and possibly other 32-bit architectures.

Fixes: 7d0ef8b4d: Btrfs: update scrub_parity to use u64 stripe_len
Reported-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
You'd probably want to squash this with Liu's commit, to be nice to future
bisects.

Tested on amd64 where all is fine, and on arm (Odroid-U2) where scrub
sometimes works, but, like most operations, randomly dies with some badness
that doesn't look related: io_schedule, kunmap_high.  That badness wasn't
there in 4.11-rc5, needs investigating, but since it's not connected to our
issue at hand, I consider this patch sort-of tested.

 fs/btrfs/scrub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Adam Borowski April 9, 2017, 3:58 a.m. UTC | #1
On Sat, Apr 08, 2017 at 11:07:37PM +0200, Adam Borowski wrote:
> Unbreaks ARM and possibly other 32-bit architectures.

Turns out those "other 32-bit architectures" happen to include i386.

A modular build:

ERROR: "__udivdi3" [fs/btrfs/btrfs.ko] undefined!

With the patch, i386 builds fine.

> Tested on amd64 where all is fine, and on arm (Odroid-U2) where scrub
> sometimes works, but, like most operations, randomly dies with some badness
> that doesn't look related: io_schedule, kunmap_high.  That badness wasn't
> there in 4.11-rc5, needs investigating, but since it's not connected to our
> issue at hand, I consider this patch sort-of tested.

Looks like current -next is pretty broken: while amd64 is ok, on an i386 box
(non-NX Pentium 4) it hangs very early during boot, way before filesystem
modules would be loaded.  Qemu boots but has random hangs.

So it looks like it's compile only for now...
Adam Borowski April 10, 2017, 1:13 a.m. UTC | #2
On Sun, Apr 09, 2017 at 05:58:54AM +0200, Adam Borowski wrote:
> On Sat, Apr 08, 2017 at 11:07:37PM +0200, Adam Borowski wrote:
> > Unbreaks ARM and possibly other 32-bit architectures.
> 
> Turns out those "other 32-bit architectures" happen to include i386.
> 
> A modular build:
> ERROR: "__udivdi3" [fs/btrfs/btrfs.ko] undefined!
> With the patch, i386 builds fine.
> 
> > Tested on amd64 where all is fine, and on arm (Odroid-U2) where scrub
> > sometimes works, but, like most operations, randomly dies with some badness
> > that doesn't look related: io_schedule, kunmap_high.  That badness wasn't
> > there in 4.11-rc5, needs investigating, but since it's not connected to our
> > issue at hand, I consider this patch sort-of tested.
> 
> Looks like current -next is pretty broken: while amd64 is ok, on an i386 box
> (non-NX Pentium 4) it hangs very early during boot, way before filesystem
> modules would be loaded.  Qemu boots but has random hangs.

A non-modular i386_defconfig + btrfs of -next is ok; whatever the problem
is, it's not relevant to our division build failure in scrub.

But, it looks like parity scrub is ${EXPLETIVE}ed on 32-bit.  Not just on
-next, also on 4.11-rc5 and 4.9.  Test script I used is attached, although
it's enough to just scrub a kosher filesystem without even damaging it.
On 64-bit it mostly works, but still warns about bogus unrecoverable errors
when in fact it succeeded.

Thus, I'd recommend:
* applying this patch to at least make it compile
* taking steps to warn outside people about RAID5/6

Let's discuss the rest in another thread, it's no longer interesting to ARM
people, they just want no build failures.
David Sterba April 10, 2017, 11:13 a.m. UTC | #3
On Sat, Apr 08, 2017 at 11:07:37PM +0200, Adam Borowski wrote:
> Unbreaks ARM and possibly other 32-bit architectures.
> 
> Fixes: 7d0ef8b4d: Btrfs: update scrub_parity to use u64 stripe_len
> Reported-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> ---
> You'd probably want to squash this with Liu's commit, to be nice to future
> bisects.

Thanks for finding it!

> Tested on amd64 where all is fine, and on arm (Odroid-U2) where scrub
> sometimes works, but, like most operations, randomly dies with some badness
> that doesn't look related: io_schedule, kunmap_high.  That badness wasn't
> there in 4.11-rc5, needs investigating, but since it's not connected to our
> issue at hand, I consider this patch sort-of tested.
> 
>  fs/btrfs/scrub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index b6fe1cd08048..95372e3679f3 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -2407,7 +2407,7 @@ static inline void __scrub_mark_bitmap(struct scrub_parity *sparity,
>  
>  	start -= sparity->logic_start;
>  	start = div64_u64_rem(start, sparity->stripe_len, &offset);
> -	offset /= sectorsize;
> +	do_div(offset, sectorsize);

I'll use the div_u64 helper instead, I don't want to reintroduce do_div
to fs/btrfs , for-next will be updated in a minute.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Austin S. Hemmelgarn April 10, 2017, 12:50 p.m. UTC | #4
On 2017-04-08 17:07, Adam Borowski wrote:
> Unbreaks ARM and possibly other 32-bit architectures.
>
> Fixes: 7d0ef8b4d: Btrfs: update scrub_parity to use u64 stripe_len
> Reported-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> ---
> You'd probably want to squash this with Liu's commit, to be nice to future
> bisects.
>
> Tested on amd64 where all is fine, and on arm (Odroid-U2) where scrub
> sometimes works, but, like most operations, randomly dies with some badness
> that doesn't look related: io_schedule, kunmap_high.  That badness wasn't
> there in 4.11-rc5, needs investigating, but since it's not connected to our
> issue at hand, I consider this patch sort-of tested.
>
>  fs/btrfs/scrub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index b6fe1cd08048..95372e3679f3 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -2407,7 +2407,7 @@ static inline void __scrub_mark_bitmap(struct scrub_parity *sparity,
>
>  	start -= sparity->logic_start;
>  	start = div64_u64_rem(start, sparity->stripe_len, &offset);
> -	offset /= sectorsize;
> +	do_div(offset, sectorsize);
>  	nsectors = (int)len / sectorsize;
>
>  	if (offset + nsectors <= sparity->nsectors) {
>
Also fixes things on:
32-bit MIPS (eb and el variants)
32-bit SPARC
32-bit PPC

You can add my Tested-by if you want.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Liu Bo April 10, 2017, 6:41 p.m. UTC | #5
On Sat, Apr 08, 2017 at 11:07:37PM +0200, Adam Borowski wrote:
> Unbreaks ARM and possibly other 32-bit architectures.
>

Thanks a lot for the fix.

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Thanks,

-liubo

> Fixes: 7d0ef8b4d: Btrfs: update scrub_parity to use u64 stripe_len
> Reported-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> ---
> You'd probably want to squash this with Liu's commit, to be nice to future
> bisects.
> 
> Tested on amd64 where all is fine, and on arm (Odroid-U2) where scrub
> sometimes works, but, like most operations, randomly dies with some badness
> that doesn't look related: io_schedule, kunmap_high.  That badness wasn't
> there in 4.11-rc5, needs investigating, but since it's not connected to our
> issue at hand, I consider this patch sort-of tested.
> 
>  fs/btrfs/scrub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index b6fe1cd08048..95372e3679f3 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -2407,7 +2407,7 @@ static inline void __scrub_mark_bitmap(struct scrub_parity *sparity,
>  
>  	start -= sparity->logic_start;
>  	start = div64_u64_rem(start, sparity->stripe_len, &offset);
> -	offset /= sectorsize;
> +	do_div(offset, sectorsize);
>  	nsectors = (int)len / sectorsize;
>  
>  	if (offset + nsectors <= sparity->nsectors) {
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index b6fe1cd08048..95372e3679f3 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -2407,7 +2407,7 @@  static inline void __scrub_mark_bitmap(struct scrub_parity *sparity,
 
 	start -= sparity->logic_start;
 	start = div64_u64_rem(start, sparity->stripe_len, &offset);
-	offset /= sectorsize;
+	do_div(offset, sectorsize);
 	nsectors = (int)len / sectorsize;
 
 	if (offset + nsectors <= sparity->nsectors) {