[BUG] lib: zram lz4 compression/decompression still broken on big endian

[BUG] lib: zram lz4 compression/decompression still broken on big endian

[Rui Salvaterra]

Hi,


I apologise in advance if I've cc'ed too many/the wrong people/lists.

Whenever I try to use zram with lz4, on my Power Mac G5 (tested with
kernel 4.4.0-16-powerpc64-smp from Ubuntu 16.04 LTS), I get the
following on my dmesg:

[13150.675820] zram: Added device: zram0
[13150.704133] zram0: detected capacity change from 0 to 5131976704
[13150.715960] zram: Decompression failed! err=-1, page=0
[13150.716008] zram: Decompression failed! err=-1, page=0
[13150.716027] zram: Decompression failed! err=-1, page=0
[13150.716032] Buffer I/O error on dev zram0, logical block 0, async page read

I believe Eunbong Song wrote a patch [1] to fix this (or a very
identical) bug on MIPS, but it never got merged (maybe
incorrect/incomplete?). Is there any hope of seeing this bug fixed?


Thanks,

Rui Salvaterra


[1] http://comments.gmane.org/gmane.linux.kernel/1752745

Re: [BUG] lib: zram lz4 compression/decompression still broken on big endian

[Greg KH]

On Tue, Apr 05, 2016 at 03:07:48PM +0100, Rui Salvaterra wrote:
> Hi,
> 
> 
> I apologise in advance if I've cc'ed too many/the wrong people/lists.
> 
> Whenever I try to use zram with lz4, on my Power Mac G5 (tested with
> kernel 4.4.0-16-powerpc64-smp from Ubuntu 16.04 LTS), I get the
> following on my dmesg:
> 
> [13150.675820] zram: Added device: zram0
> [13150.704133] zram0: detected capacity change from 0 to 5131976704
> [13150.715960] zram: Decompression failed! err=-1, page=0
> [13150.716008] zram: Decompression failed! err=-1, page=0
> [13150.716027] zram: Decompression failed! err=-1, page=0
> [13150.716032] Buffer I/O error on dev zram0, logical block 0, async page read
> 
> I believe Eunbong Song wrote a patch [1] to fix this (or a very
> identical) bug on MIPS, but it never got merged (maybe
> incorrect/incomplete?). Is there any hope of seeing this bug fixed?
> 
> 
> Thanks,
> 
> Rui Salvaterra
> 
> 
> [1] http://comments.gmane.org/gmane.linux.kernel/1752745

For some reason it never got merged, sorry, I don't remember why.

Have you tested this patch?  If so, can you resend it with your
tested-by: line added to it?

thanks,

greg k-h

Re: [BUG] lib: zram lz4 compression/decompression still broken on big endian

[Rui Salvaterra]

2016-04-05 16:34 GMT+01:00 Greg KH <gregkh@linuxfoundation.org>:
> On Tue, Apr 05, 2016 at 03:07:48PM +0100, Rui Salvaterra wrote:
>> Hi,
>>
>>
>> I apologise in advance if I've cc'ed too many/the wrong people/lists.
>>
>> Whenever I try to use zram with lz4, on my Power Mac G5 (tested with
>> kernel 4.4.0-16-powerpc64-smp from Ubuntu 16.04 LTS), I get the
>> following on my dmesg:
>>
>> [13150.675820] zram: Added device: zram0
>> [13150.704133] zram0: detected capacity change from 0 to 5131976704
>> [13150.715960] zram: Decompression failed! err=-1, page=0
>> [13150.716008] zram: Decompression failed! err=-1, page=0
>> [13150.716027] zram: Decompression failed! err=-1, page=0
>> [13150.716032] Buffer I/O error on dev zram0, logical block 0, async page read
>>
>> I believe Eunbong Song wrote a patch [1] to fix this (or a very
>> identical) bug on MIPS, but it never got merged (maybe
>> incorrect/incomplete?). Is there any hope of seeing this bug fixed?
>>
>>
>> Thanks,
>>
>> Rui Salvaterra
>>
>>
>> [1] http://comments.gmane.org/gmane.linux.kernel/1752745
>
> For some reason it never got merged, sorry, I don't remember why.
>
> Have you tested this patch?  If so, can you resend it with your
> tested-by: line added to it?
>
> thanks,
>
> greg k-h

Hi, Greg


No, I haven't tested the patch at all. I want to do so, and fix if if
necessary, but I still need to learn how to (meaning, I need to watch
your "first kernel patch" presentation again). I'd love to get
involved in kernel development, and this seems to be a good
opportunity, if none of the kernel gods beat me to it (I may need a
month, but then again nobody complained about this bug in almost two
years).


Thanks,

Rui

Re: [BUG] lib: zram lz4 compression/decompression still broken on big endian

[Sergey Senozhatsky]

On (04/05/16 17:02), Rui Salvaterra wrote:
[..]
> > For some reason it never got merged, sorry, I don't remember why.
> >
> > Have you tested this patch?  If so, can you resend it with your
> > tested-by: line added to it?
> >
> > thanks,
> >
> > greg k-h
> 
> Hi, Greg
> 
> 
> No, I haven't tested the patch at all. I want to do so, and fix if if
> necessary, but I still need to learn how to (meaning, I need to watch
> your "first kernel patch" presentation again). I'd love to get
> involved in kernel development, and this seems to be a good
> opportunity, if none of the kernel gods beat me to it (I may need a
> month, but then again nobody complained about this bug in almost two
> years).

Hello Rui,

may we please ask you to test the patch first? quite possible there
is nothing to fix there; I've no access to mips h/w but the patch
seems correct to me.

LZ4_READ_LITTLEENDIAN_16 does get_unaligned_le16(), so
LZ4_WRITE_LITTLEENDIAN_16 must do put_unaligned_le16() /* not put_unaligned() */

	-ss

Re: [BUG] lib: zram lz4 compression/decompression still broken on big endian

[Rui Salvaterra]

2016-04-06 6:33 GMT+01:00 Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com>:
> On (04/05/16 17:02), Rui Salvaterra wrote:
> [..]
>> > For some reason it never got merged, sorry, I don't remember why.
>> >
>> > Have you tested this patch?  If so, can you resend it with your
>> > tested-by: line added to it?
>> >
>> > thanks,
>> >
>> > greg k-h
>>
>> Hi, Greg
>>
>>
>> No, I haven't tested the patch at all. I want to do so, and fix if if
>> necessary, but I still need to learn how to (meaning, I need to watch
>> your "first kernel patch" presentation again). I'd love to get
>> involved in kernel development, and this seems to be a good
>> opportunity, if none of the kernel gods beat me to it (I may need a
>> month, but then again nobody complained about this bug in almost two
>> years).
>
> Hello Rui,
>
> may we please ask you to test the patch first? quite possible there
> is nothing to fix there; I've no access to mips h/w but the patch
> seems correct to me.
>
> LZ4_READ_LITTLEENDIAN_16 does get_unaligned_le16(), so
> LZ4_WRITE_LITTLEENDIAN_16 must do put_unaligned_le16() /* not put_unaligned() */
>
>         -ss

Hi, Sergey


Besides ppc64, I have ppc32, x86 and x86_64 hardware readily
available. The only mips (74kc, also big endian) hardware I have
access to is my router, running OpenWrt, I can try to test it there
too, but it will be more complicated. Still, after reading the
existing code [1] more thoroughly, I can't see how Eunbong Song's
patch [2] would fix the ppc case (please correct me if I'm wrong,
which is highly likely, since my C preprocessor knowledge varies
between nonexistent to very superficial).

Now, LZ4_READ_LITTLEENDIAN_16 is unconditionally defined as:

#define LZ4_READ_LITTLEENDIAN_16(d, s, p)
                (d = s - get_unaligned_le16(p))

As far as I can tell, and unlike ppc, mips doesn't define
HAVE_EFFICIENT_UNALIGNED_ACCESS, which means for mips case,
LZ4_WRITE_LITTLEENDIAN_16 will be defined as:

#define LZ4_WRITE_LITTLEENDIAN_16(p, v)
                do {
                                put_unaligned(v, (u16 *)(p));
                                p += 2;
                } while (0)

Whereas for ppc, which defines HAVE_EFFICIENT_UNALIGNED_ACCESS,
LZ4_WRITE_LITTLEENDIAN_16 will be defined as:

#define LZ4_WRITE_LITTLEENDIAN_16(p, v)
                do {
                                A16(p) = v;
                                p += 2;
                } while (0)

Consequentially, while I believe the patch will fix the mips case, I'm
not so sure about ppc (or any other big endian architecture with
efficient unaligned accesses).


Thanks,

Rui

[1] https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/lib/lz4/lz4defs.h?h=v4.4.6
[2] http://permalink.gmane.org/gmane.linux.kernel/1752745

Re: [BUG] lib: zram lz4 compression/decompression still broken on big endian

[Sergey Senozhatsky]

Cc Chanho Min, Kyungsik Lee


Hello,

On (04/06/16 10:39), Rui Salvaterra wrote:
> > may we please ask you to test the patch first? quite possible there
> > is nothing to fix there; I've no access to mips h/w but the patch
> > seems correct to me.
> >
> > LZ4_READ_LITTLEENDIAN_16 does get_unaligned_le16(), so
> > LZ4_WRITE_LITTLEENDIAN_16 must do put_unaligned_le16() /* not put_unaligned() */
> >
[..]
> Consequentially, while I believe the patch will fix the mips case, I'm
> not so sure about ppc (or any other big endian architecture with
> efficient unaligned accesses).

frankly, yes, I took a quick look today (after I sent my initial
message, tho) ... and it is fishy, I agree. was going to followup
on my email but somehow got interrupted, sorry.

so we have, write:
	((U16_S *)(p)) = v    OR    put_unaligned(v, (u16 *)(p))

and only one read:
	get_unaligned_le16(p))

I guess it's either read part also must depend on
HAVE_EFFICIENT_UNALIGNED_ACCESS, or write path
should stop doing so.

I ended up with two patches, NONE was tested (!!!). like at all.

1) provide CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS-dependent
   LZ4_READ_LITTLEENDIAN_16

2) provide common LZ4_WRITE_LITTLEENDIAN_16 and LZ4_READ_LITTLEENDIAN_16
   regardless CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS.


assuming that common LZ4_WRITE_LITTLEENDIAN_16 will somehow hit the
performance, I'd probably prefer option #1.

the patch is below. would be great if you can help testing it.

---

 lib/lz4/lz4defs.h | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/lib/lz4/lz4defs.h b/lib/lz4/lz4defs.h
index abcecdc..a23e6c2 100644
--- a/lib/lz4/lz4defs.h
+++ b/lib/lz4/lz4defs.h
@@ -36,10 +36,14 @@ typedef struct _U64_S { u64 v; } U64_S;
 #define PUT4(s, d) (A32(d) = A32(s))
 #define PUT8(s, d) (A64(d) = A64(s))
 #define LZ4_WRITE_LITTLEENDIAN_16(p, v)	\
-	do {	\
-		A16(p) = v; \
-		p += 2; \
+	do {					\
+		A16(p) = v; 			\
+		p += 2; 			\
 	} while (0)
+
+#define LZ4_READ_LITTLEENDIAN_16(d, s, p)	\
+	(d = s - A16(p))
+
 #else /* CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS */
 
 #define A64(x) get_unaligned((u64 *)&(((U16_S *)(x))->v))
@@ -52,10 +56,13 @@ typedef struct _U64_S { u64 v; } U64_S;
 	put_unaligned(get_unaligned((const u64 *) s), (u64 *) d)
 
 #define LZ4_WRITE_LITTLEENDIAN_16(p, v)	\
-	do {	\
-		put_unaligned(v, (u16 *)(p)); \
-		p += 2; \
+	do {						\
+		put_unaligned_le16(v, (u16 *)(p));	\
+		p += 2; 				\
 	} while (0)
+
+#define LZ4_READ_LITTLEENDIAN_16(d, s, p) 		\
+	(d = s - get_unaligned_le16(p))
 #endif
 
 #define COPYLENGTH 8
@@ -140,9 +147,6 @@ typedef struct _U64_S { u64 v; } U64_S;
 
 #endif
 
-#define LZ4_READ_LITTLEENDIAN_16(d, s, p) \
-	(d = s - get_unaligned_le16(p))
-
 #define LZ4_WILDCOPY(s, d, e)		\
 	do {				\
 		LZ4_COPYPACKET(s, d);	\

Re: [BUG] lib: zram lz4 compression/decompression still broken on big endian

[Rui Salvaterra]

2016-04-06 14:09 GMT+01:00 Sergey Senozhatsky <sergey.senozhatsky@gmail.com>:
> Cc Chanho Min, Kyungsik Lee
>
>
> Hello,
>
> On (04/06/16 10:39), Rui Salvaterra wrote:
>> > may we please ask you to test the patch first? quite possible there
>> > is nothing to fix there; I've no access to mips h/w but the patch
>> > seems correct to me.
>> >
>> > LZ4_READ_LITTLEENDIAN_16 does get_unaligned_le16(), so
>> > LZ4_WRITE_LITTLEENDIAN_16 must do put_unaligned_le16() /* not put_unaligned() */
>> >
> [..]
>> Consequentially, while I believe the patch will fix the mips case, I'm
>> not so sure about ppc (or any other big endian architecture with
>> efficient unaligned accesses).
>
> frankly, yes, I took a quick look today (after I sent my initial
> message, tho) ... and it is fishy, I agree. was going to followup
> on my email but somehow got interrupted, sorry.
>
> so we have, write:
>         ((U16_S *)(p)) = v    OR    put_unaligned(v, (u16 *)(p))
>
> and only one read:
>         get_unaligned_le16(p))
>
> I guess it's either read part also must depend on
> HAVE_EFFICIENT_UNALIGNED_ACCESS, or write path
> should stop doing so.
>
> I ended up with two patches, NONE was tested (!!!). like at all.
>
> 1) provide CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS-dependent
>    LZ4_READ_LITTLEENDIAN_16
>
> 2) provide common LZ4_WRITE_LITTLEENDIAN_16 and LZ4_READ_LITTLEENDIAN_16
>    regardless CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS.
>
>
> assuming that common LZ4_WRITE_LITTLEENDIAN_16 will somehow hit the
> performance, I'd probably prefer option #1.
>
> the patch is below. would be great if you can help testing it.
>
> ---
>
>  lib/lz4/lz4defs.h | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/lib/lz4/lz4defs.h b/lib/lz4/lz4defs.h
> index abcecdc..a23e6c2 100644
> --- a/lib/lz4/lz4defs.h
> +++ b/lib/lz4/lz4defs.h
> @@ -36,10 +36,14 @@ typedef struct _U64_S { u64 v; } U64_S;
>  #define PUT4(s, d) (A32(d) = A32(s))
>  #define PUT8(s, d) (A64(d) = A64(s))
>  #define LZ4_WRITE_LITTLEENDIAN_16(p, v)        \
> -       do {    \
> -               A16(p) = v; \
> -               p += 2; \
> +       do {                                    \
> +               A16(p) = v;                     \
> +               p += 2;                         \
>         } while (0)
> +
> +#define LZ4_READ_LITTLEENDIAN_16(d, s, p)      \
> +       (d = s - A16(p))
> +
>  #else /* CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS */
>
>  #define A64(x) get_unaligned((u64 *)&(((U16_S *)(x))->v))
> @@ -52,10 +56,13 @@ typedef struct _U64_S { u64 v; } U64_S;
>         put_unaligned(get_unaligned((const u64 *) s), (u64 *) d)
>
>  #define LZ4_WRITE_LITTLEENDIAN_16(p, v)        \
> -       do {    \
> -               put_unaligned(v, (u16 *)(p)); \
> -               p += 2; \
> +       do {                                            \
> +               put_unaligned_le16(v, (u16 *)(p));      \
> +               p += 2;                                 \
>         } while (0)
> +
> +#define LZ4_READ_LITTLEENDIAN_16(d, s, p)              \
> +       (d = s - get_unaligned_le16(p))
>  #endif
>
>  #define COPYLENGTH 8
> @@ -140,9 +147,6 @@ typedef struct _U64_S { u64 v; } U64_S;
>
>  #endif
>
> -#define LZ4_READ_LITTLEENDIAN_16(d, s, p) \
> -       (d = s - get_unaligned_le16(p))
> -
>  #define LZ4_WILDCOPY(s, d, e)          \
>         do {                            \
>                 LZ4_COPYPACKET(s, d);   \
>


Hi again, Sergey


Thanks for the patch, I'll test it as soon as possible. I agree with
your second option, usually one selects lz4 when (especially
decompression) speed is paramount, so it needs all the help it can
get.

Speaking of fishy, the 64-bit detection code also looks suspiciously
bogus. Some of the identifiers don't even exist anywhere in the kernel
(__ppc64__, por example, after grepping all .c and .h files).
Shouldn't we instead check for CONFIG_64BIT or BITS_PER_LONG == 64?


Thanks,

Rui

Re: [BUG] lib: zram lz4 compression/decompression still broken on big endian

[Sergey Senozhatsky]

On (04/07/16 13:33), Rui Salvaterra wrote:
[..]
> Hi again, Sergey

Hello,

> Thanks for the patch, I'll test it as soon as possible. I agree with
> your second option, usually one selects lz4 when (especially
> decompression) speed is paramount, so it needs all the help it can
> get.

thanks!

> Speaking of fishy, the 64-bit detection code also looks suspiciously
> bogus. Some of the identifiers don't even exist anywhere in the kernel
> (__ppc64__, por example, after grepping all .c and .h files).
> Shouldn't we instead check for CONFIG_64BIT or BITS_PER_LONG == 64?

definitely a good question. personally, I'd prefer to test for
CONFIG_64BIT only, looking at this hairy

  /* Detects 64 bits mode */
  #if (defined(__x86_64__) || defined(__x86_64) || defined(__amd64__) \
         || defined(__ppc64__) || defined(__LP64__))

and remove/rewrite a bunch of other stuff. but the thing with cleanups
is that they don't fix anything, while potentially can introduce bugs.
it's more risky to touch the stable code. /* well, removing those 'ghost'
identifiers is sort of OK to me */. but that's just my opinion, I'll
leave it to you and Greg.

	-ss

Re: [BUG] lib: zram lz4 compression/decompression still broken on big endian

[Rui Salvaterra]

2016-04-07 15:07 GMT+01:00 Sergey Senozhatsky <sergey.senozhatsky@gmail.com>:
> On (04/07/16 13:33), Rui Salvaterra wrote:
> [..]
>> Hi again, Sergey
>
> Hello,
>
>> Thanks for the patch, I'll test it as soon as possible. I agree with
>> your second option, usually one selects lz4 when (especially
>> decompression) speed is paramount, so it needs all the help it can
>> get.
>
> thanks!
>
>> Speaking of fishy, the 64-bit detection code also looks suspiciously
>> bogus. Some of the identifiers don't even exist anywhere in the kernel
>> (__ppc64__, por example, after grepping all .c and .h files).
>> Shouldn't we instead check for CONFIG_64BIT or BITS_PER_LONG == 64?
>
> definitely a good question. personally, I'd prefer to test for
> CONFIG_64BIT only, looking at this hairy
>
>   /* Detects 64 bits mode */
>   #if (defined(__x86_64__) || defined(__x86_64) || defined(__amd64__) \
>          || defined(__ppc64__) || defined(__LP64__))
>
> and remove/rewrite a bunch of other stuff. but the thing with cleanups
> is that they don't fix anything, while potentially can introduce bugs.
> it's more risky to touch the stable code. /* well, removing those 'ghost'
> identifiers is sort of OK to me */. but that's just my opinion, I'll
> leave it to you and Greg.
>
>         -ss

Hi again, Sergey

I finally was able to test your patch but, as I suspected, it wasn't
enough. However, based on it, I was able to write a (hopefully)
correct one, which I'll send soon (tested on ppc64, with no
regressions on x86_64).

Thanks,

Rui