Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On 05/03/14 16:24, Lasse Collin wrote:
On 2014-03-05 Phillip Lougher wrote:
(BTW Kyle you should have CC'd me on the patch as a courtesy).
I could have done that too but somehow I didn't, sorry.
np
But speaking as the Squashfs author, the lack of BCJ support for
an architecture creates a subtle failure mode in Squashfs, this is
because not all blocks in a Squashfs filesystem get compressed
with a BCJ filter. At compression time each block is compressed
without any BCJ filter, and then with the BCJ filter(s) selected on
the command line, and the best compression for *that* block is
chosen. What this means is kernels without a particular
BCJ filter can still read the Squashfs metadata (mount, ls etc.) and
read many of the files, it is only some files that mysteriously
fail with decompression error. As such this will be (and has been)
invariably treated as a bug in Squashfs.
There is an easy way to make Squashfs give an error message in the
kernel log. xz_dec_run() gives XZ_OPTIONS_ERROR when valid-looking but
unsupported input is detected. Currently Squashfs treats all error
codes from xz_dec_run() the same way so the reason for the
decompression error is silently lost.
Yes, that is deliberate. It's to give a generic easy to understand
error message for potentially novice users that may be running
Linux from LiveCDs.
When I wrote the original zlib support for Squashfs, I put in a lot
of debug information in the zlib error messages, e.g.
ERROR("zlib_inflate returned unexpected result"
" 0x%x, srclength %d, avail_in %d,"
" avail_out %d\n", zlib_err, srclength,
msblk->stream.avail_in,
msblk->stream.avail_out);
But after mainlining Squashfs in 2009, I started to get increasing
complaints that the error messages were too technical (full of
hex numbers) and confusing to new users - the kind of people who
maybe burn a corrupt liveCD and then get screenfulls of these
errors full of different numbers coming up on the screen. They
would do a websearch and discover that the errors meant
"corrupted disk", and then ask why didn't it just say that, and
not give all those numbers. Or worse they'd just silently give up
and go back to Windows.
So in March 2009 I changed it to the error message
ERROR("zlib_inflate error, data probably corrupt\n")
With *no* numbers ... and I copied the same approach for xz.
In kernel 3.13 (released earlier this year) I went further and
pulled out the error message printing from the compression
wrappers, and made it a single generic message, because I
realised there was no longer any decompressor specific error
handling (just the same message in each wrapper).
ERROR("%s decompression failed, data probably corrupt\n",
msblk->decompressor->name);
Putting back separate error messages into each decompressor wrapper,
and then putting back different error messages based on the
error code return seems like a retrograde step because distros don't
like them.
Below is an *untested* fix. I'm not sure about the exact wording of the
error message, so feel free to improve it.
diff -Narup linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c
linux-3.14-rc5/fs/squashfs/xz_wrapper.c
--- linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c2014-03-03
04:56:16.0 +0200
+++ linux-3.14-rc5/fs/squashfs/xz_wrapper.c 2014-03-05 18:08:58.729643127
+0200
@@ -170,8 +170,13 @@ static int squashfs_xz_uncompress(struct
squashfs_finish_page(output);
- if (xz_err != XZ_STREAM_END || k < b)
+ if (xz_err != XZ_STREAM_END || k < b) {
+ if (xz_err == XZ_OPTIONS_ERROR)
+ ERROR("Unsupported XZ-compressed data; check the XZ "
+ "options in the kernel config\n");
+
goto out;
+ }
return total + stream->buf.out_pos;
Moreover, without expert knowledge of Squashfs, and the config
options, most people will not have a clue how to fix the issue.
This is why I prefer the first option, which is to reinstate
the enabling of all filters by default, and then to allow people
to remove the filters they don't want.
I will submit the first option. In the other email Florian Fainelli
seemed to be OK with that too.
BTW there is a potential additional fix for Squashfs that will
make its handling of (lack of) BCJ filters more intelligent
at mount time, but this of course only addresses Squashfs,
and it relies on an additional call into XZ being added. The
BCJ filters specified at filesystem creation are stored in the
compression options part of the superblock, and are known at
mount time. Squashfs should check that these filters are
supported by the kernel and refuse to mount it otherwise. This
has not been done because AFAIK there is no way to query XZ to
determine which BCJ filters are supported (beyond
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On 05/03/14 16:24, Lasse Collin wrote:
On 2014-03-05 Phillip Lougher wrote:
(BTW Kyle you should have CC'd me on the patch as a courtesy).
I could have done that too but somehow I didn't, sorry.
np
But speaking as the Squashfs author, the lack of BCJ support for
an architecture creates a subtle failure mode in Squashfs, this is
because not all blocks in a Squashfs filesystem get compressed
with a BCJ filter. At compression time each block is compressed
without any BCJ filter, and then with the BCJ filter(s) selected on
the command line, and the best compression for *that* block is
chosen. What this means is kernels without a particular
BCJ filter can still read the Squashfs metadata (mount, ls etc.) and
read many of the files, it is only some files that mysteriously
fail with decompression error. As such this will be (and has been)
invariably treated as a bug in Squashfs.
There is an easy way to make Squashfs give an error message in the
kernel log. xz_dec_run() gives XZ_OPTIONS_ERROR when valid-looking but
unsupported input is detected. Currently Squashfs treats all error
codes from xz_dec_run() the same way so the reason for the
decompression error is silently lost.
Yes, that is deliberate. It's to give a generic easy to understand
error message for potentially novice users that may be running
Linux from LiveCDs.
When I wrote the original zlib support for Squashfs, I put in a lot
of debug information in the zlib error messages, e.g.
ERROR(zlib_inflate returned unexpected result
0x%x, srclength %d, avail_in %d,
avail_out %d\n, zlib_err, srclength,
msblk-stream.avail_in,
msblk-stream.avail_out);
But after mainlining Squashfs in 2009, I started to get increasing
complaints that the error messages were too technical (full of
hex numbers) and confusing to new users - the kind of people who
maybe burn a corrupt liveCD and then get screenfulls of these
errors full of different numbers coming up on the screen. They
would do a websearch and discover that the errors meant
corrupted disk, and then ask why didn't it just say that, and
not give all those numbers. Or worse they'd just silently give up
and go back to Windows.
So in March 2009 I changed it to the error message
ERROR(zlib_inflate error, data probably corrupt\n)
With *no* numbers ... and I copied the same approach for xz.
In kernel 3.13 (released earlier this year) I went further and
pulled out the error message printing from the compression
wrappers, and made it a single generic message, because I
realised there was no longer any decompressor specific error
handling (just the same message in each wrapper).
ERROR(%s decompression failed, data probably corrupt\n,
msblk-decompressor-name);
Putting back separate error messages into each decompressor wrapper,
and then putting back different error messages based on the
error code return seems like a retrograde step because distros don't
like them.
Below is an *untested* fix. I'm not sure about the exact wording of the
error message, so feel free to improve it.
diff -Narup linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c
linux-3.14-rc5/fs/squashfs/xz_wrapper.c
--- linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c2014-03-03
04:56:16.0 +0200
+++ linux-3.14-rc5/fs/squashfs/xz_wrapper.c 2014-03-05 18:08:58.729643127
+0200
@@ -170,8 +170,13 @@ static int squashfs_xz_uncompress(struct
squashfs_finish_page(output);
- if (xz_err != XZ_STREAM_END || k b)
+ if (xz_err != XZ_STREAM_END || k b) {
+ if (xz_err == XZ_OPTIONS_ERROR)
+ ERROR(Unsupported XZ-compressed data; check the XZ
+ options in the kernel config\n);
+
goto out;
+ }
return total + stream-buf.out_pos;
Moreover, without expert knowledge of Squashfs, and the config
options, most people will not have a clue how to fix the issue.
This is why I prefer the first option, which is to reinstate
the enabling of all filters by default, and then to allow people
to remove the filters they don't want.
I will submit the first option. In the other email Florian Fainelli
seemed to be OK with that too.
BTW there is a potential additional fix for Squashfs that will
make its handling of (lack of) BCJ filters more intelligent
at mount time, but this of course only addresses Squashfs,
and it relies on an additional call into XZ being added. The
BCJ filters specified at filesystem creation are stored in the
compression options part of the superblock, and are known at
mount time. Squashfs should check that these filters are
supported by the kernel and refuse to mount it otherwise. This
has not been done because AFAIK there is no way to query XZ to
determine which BCJ filters are supported (beyond passing it a
test stream
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On Sat, Mar 8, 2014 at 11:11 AM, Lasse Collin wrote:
> On 2014-03-06 Geert Uytterhoeven wrote:
>> I once tried xz with an initrd on ARM. The kernel complained it
>> couldn't decompress the initrd, oops. I didn't investigate it at that
>> time, but probably I didn't have the x86 BCJ filter enabled, while I
>> compressed the initrd on\ amd64.
>
> With so little information I can only guess, but it sounds unlikely
I understand. Will see if I find time to retry...
> that you would have enabled the x86 BCJ filter without knowing it
> unless you used some wrapper script that does it. It's more likely that
> the ARM kernel didn't support XZ at all, you forgot --check=crc32,
I definitely didn't pass --check=crc32, just plain xz (like I'm used to plain
gzip ;-) So that's the most likely culprit.
> or it ran out of RAM due to too big LZMA2 dictionary (if you used -9,
> the decompressor allocates 64 MiB of memory, but I cannot guess how
> much RAM the target system had).
2 GiB, so that shouldn't be an issue.
> In Documentation/xz.txt under "Notes on compression options" there are
> some tips about compressing files for the in-kernel XZ decompressor.
Thanks, the --check=crc32 is indeed mentioned there.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On 2014-03-06 Geert Uytterhoeven wrote: > I once tried xz with an initrd on ARM. The kernel complained it > couldn't decompress the initrd, oops. I didn't investigate it at that > time, but probably I didn't have the x86 BCJ filter enabled, while I > compressed the initrd on\ amd64. With so little information I can only guess, but it sounds unlikely that you would have enabled the x86 BCJ filter without knowing it unless you used some wrapper script that does it. It's more likely that the ARM kernel didn't support XZ at all, you forgot --check=crc32, or it ran out of RAM due to too big LZMA2 dictionary (if you used -9, the decompressor allocates 64 MiB of memory, but I cannot guess how much RAM the target system had). In Documentation/xz.txt under "Notes on compression options" there are some tips about compressing files for the in-kernel XZ decompressor. -- Lasse Collin | IRC: Larhzu @ IRCnet & Freenode -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On 2014-03-06 Geert Uytterhoeven wrote: I once tried xz with an initrd on ARM. The kernel complained it couldn't decompress the initrd, oops. I didn't investigate it at that time, but probably I didn't have the x86 BCJ filter enabled, while I compressed the initrd on\ amd64. With so little information I can only guess, but it sounds unlikely that you would have enabled the x86 BCJ filter without knowing it unless you used some wrapper script that does it. It's more likely that the ARM kernel didn't support XZ at all, you forgot --check=crc32, or it ran out of RAM due to too big LZMA2 dictionary (if you used -9, the decompressor allocates 64 MiB of memory, but I cannot guess how much RAM the target system had). In Documentation/xz.txt under Notes on compression options there are some tips about compressing files for the in-kernel XZ decompressor. -- Lasse Collin | IRC: Larhzu @ IRCnet Freenode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On Sat, Mar 8, 2014 at 11:11 AM, Lasse Collin lasse.col...@tukaani.org wrote:
On 2014-03-06 Geert Uytterhoeven wrote:
I once tried xz with an initrd on ARM. The kernel complained it
couldn't decompress the initrd, oops. I didn't investigate it at that
time, but probably I didn't have the x86 BCJ filter enabled, while I
compressed the initrd on\ amd64.
With so little information I can only guess, but it sounds unlikely
I understand. Will see if I find time to retry...
that you would have enabled the x86 BCJ filter without knowing it
unless you used some wrapper script that does it. It's more likely that
the ARM kernel didn't support XZ at all, you forgot --check=crc32,
I definitely didn't pass --check=crc32, just plain xz (like I'm used to plain
gzip ;-) So that's the most likely culprit.
or it ran out of RAM due to too big LZMA2 dictionary (if you used -9,
the decompressor allocates 64 MiB of memory, but I cannot guess how
much RAM the target system had).
2 GiB, so that shouldn't be an issue.
In Documentation/xz.txt under Notes on compression options there are
some tips about compressing files for the in-kernel XZ decompressor.
Thanks, the --check=crc32 is indeed mentioned there.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On Wed, Mar 5, 2014 at 4:50 AM, Florian Fainelli wrote:
> I am okay with enabling all filters by default as long as there is a
> knob to allow turning achitecture specific BCJ filters off, as it
> barely makes sense for embedded systems to have a BCJ filter
> implementation for anything but the architecture you are running on.
And for x86? (oh no, I started promoting x86 ;-)
I once tried xz with an initrd on ARM. The kernel complained it couldn't
decompress the initrd, oops. I didn't investigate it at that time, but probably
I didn't have the x86 BCJ filter enabled, while I compressed the initrd on\
amd64.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On Wed, Mar 5, 2014 at 4:50 AM, Florian Fainelli f.faine...@gmail.com wrote:
I am okay with enabling all filters by default as long as there is a
knob to allow turning achitecture specific BCJ filters off, as it
barely makes sense for embedded systems to have a BCJ filter
implementation for anything but the architecture you are running on.
And for x86? (oh no, I started promoting x86 ;-)
I once tried xz with an initrd on ARM. The kernel complained it couldn't
decompress the initrd, oops. I didn't investigate it at that time, but probably
I didn't have the x86 BCJ filter enabled, while I compressed the initrd on\
amd64.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On 2014-03-05 Phillip Lougher wrote:
> (BTW Kyle you should have CC'd me on the patch as a courtesy).
I could have done that too but somehow I didn't, sorry.
> But speaking as the Squashfs author, the lack of BCJ support for
> an architecture creates a subtle failure mode in Squashfs, this is
> because not all blocks in a Squashfs filesystem get compressed
> with a BCJ filter. At compression time each block is compressed
> without any BCJ filter, and then with the BCJ filter(s) selected on
> the command line, and the best compression for *that* block is
> chosen. What this means is kernels without a particular
> BCJ filter can still read the Squashfs metadata (mount, ls etc.) and
> read many of the files, it is only some files that mysteriously
> fail with decompression error. As such this will be (and has been)
> invariably treated as a bug in Squashfs.
There is an easy way to make Squashfs give an error message in the
kernel log. xz_dec_run() gives XZ_OPTIONS_ERROR when valid-looking but
unsupported input is detected. Currently Squashfs treats all error
codes from xz_dec_run() the same way so the reason for the
decompression error is silently lost.
Below is an *untested* fix. I'm not sure about the exact wording of the
error message, so feel free to improve it.
diff -Narup linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c
linux-3.14-rc5/fs/squashfs/xz_wrapper.c
--- linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c2014-03-03
04:56:16.0 +0200
+++ linux-3.14-rc5/fs/squashfs/xz_wrapper.c 2014-03-05 18:08:58.729643127
+0200
@@ -170,8 +170,13 @@ static int squashfs_xz_uncompress(struct
squashfs_finish_page(output);
- if (xz_err != XZ_STREAM_END || k < b)
+ if (xz_err != XZ_STREAM_END || k < b) {
+ if (xz_err == XZ_OPTIONS_ERROR)
+ ERROR("Unsupported XZ-compressed data; check the XZ "
+ "options in the kernel config\n");
+
goto out;
+ }
return total + stream->buf.out_pos;
> Moreover, without expert knowledge of Squashfs, and the config
> options, most people will not have a clue how to fix the issue.
>
> This is why I prefer the first option, which is to reinstate
> the enabling of all filters by default, and then to allow people
> to remove the filters they don't want.
I will submit the first option. In the other email Florian Fainelli
seemed to be OK with that too.
> BTW there is a potential additional fix for Squashfs that will
> make its handling of (lack of) BCJ filters more intelligent
> at mount time, but this of course only addresses Squashfs,
> and it relies on an additional call into XZ being added. The
> BCJ filters specified at filesystem creation are stored in the
> compression options part of the superblock, and are known at
> mount time. Squashfs should check that these filters are
> supported by the kernel and refuse to mount it otherwise. This
> has not been done because AFAIK there is no way to query XZ to
> determine which BCJ filters are supported (beyond passing it a
> test stream which is too messy).
You can use #ifdef CONFIG_XZ_DEC_X86 and such, although maybe that's
not convenient enough.
Adding a function to xz_dec module could be sort of OK. It's important
to keep in mind that the ability to disable filters is there to reduce
code size *slightly*. If you or we add something extra to figure out
what is supported at runtime, the size benefit may get lost.
If all filters are enabled by default, a clear error message on
XZ_OPTIONS_ERROR should be enough, I think.
--
Lasse Collin | IRC: Larhzu @ IRCnet & Freenode
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On 2014-03-05 Phillip Lougher wrote:
(BTW Kyle you should have CC'd me on the patch as a courtesy).
I could have done that too but somehow I didn't, sorry.
But speaking as the Squashfs author, the lack of BCJ support for
an architecture creates a subtle failure mode in Squashfs, this is
because not all blocks in a Squashfs filesystem get compressed
with a BCJ filter. At compression time each block is compressed
without any BCJ filter, and then with the BCJ filter(s) selected on
the command line, and the best compression for *that* block is
chosen. What this means is kernels without a particular
BCJ filter can still read the Squashfs metadata (mount, ls etc.) and
read many of the files, it is only some files that mysteriously
fail with decompression error. As such this will be (and has been)
invariably treated as a bug in Squashfs.
There is an easy way to make Squashfs give an error message in the
kernel log. xz_dec_run() gives XZ_OPTIONS_ERROR when valid-looking but
unsupported input is detected. Currently Squashfs treats all error
codes from xz_dec_run() the same way so the reason for the
decompression error is silently lost.
Below is an *untested* fix. I'm not sure about the exact wording of the
error message, so feel free to improve it.
diff -Narup linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c
linux-3.14-rc5/fs/squashfs/xz_wrapper.c
--- linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c2014-03-03
04:56:16.0 +0200
+++ linux-3.14-rc5/fs/squashfs/xz_wrapper.c 2014-03-05 18:08:58.729643127
+0200
@@ -170,8 +170,13 @@ static int squashfs_xz_uncompress(struct
squashfs_finish_page(output);
- if (xz_err != XZ_STREAM_END || k b)
+ if (xz_err != XZ_STREAM_END || k b) {
+ if (xz_err == XZ_OPTIONS_ERROR)
+ ERROR(Unsupported XZ-compressed data; check the XZ
+ options in the kernel config\n);
+
goto out;
+ }
return total + stream-buf.out_pos;
Moreover, without expert knowledge of Squashfs, and the config
options, most people will not have a clue how to fix the issue.
This is why I prefer the first option, which is to reinstate
the enabling of all filters by default, and then to allow people
to remove the filters they don't want.
I will submit the first option. In the other email Florian Fainelli
seemed to be OK with that too.
BTW there is a potential additional fix for Squashfs that will
make its handling of (lack of) BCJ filters more intelligent
at mount time, but this of course only addresses Squashfs,
and it relies on an additional call into XZ being added. The
BCJ filters specified at filesystem creation are stored in the
compression options part of the superblock, and are known at
mount time. Squashfs should check that these filters are
supported by the kernel and refuse to mount it otherwise. This
has not been done because AFAIK there is no way to query XZ to
determine which BCJ filters are supported (beyond passing it a
test stream which is too messy).
You can use #ifdef CONFIG_XZ_DEC_X86 and such, although maybe that's
not convenient enough.
Adding a function to xz_dec module could be sort of OK. It's important
to keep in mind that the ability to disable filters is there to reduce
code size *slightly*. If you or we add something extra to figure out
what is supported at runtime, the size benefit may get lost.
If all filters are enabled by default, a clear error message on
XZ_OPTIONS_ERROR should be enough, I think.
--
Lasse Collin | IRC: Larhzu @ IRCnet Freenode
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
2014-03-04 18:51 GMT-08:00 Phillip Lougher : > On 04/03/14 18:20, Lasse Collin wrote: >> On 2014-03-03 Florian Fainelli wrote: >>> 2014-03-03 4:51 GMT-08:00 Lasse Collin : The second version enables the BCJ filter only for the current arch by default if CONFIG_EXPERT; without CONFIG_EXPERT all filters are forced on: >>> >>> I like this version better because it still provides you with accurate >>> defaults (i.e: enable only th X86 BCJ filter decoder where it makes >>> sense by default) and still provides enough flexibility. >> >> I understand your viewpoint, but different people probably consider >> different options as "accurate defaults". The current behavior certainly >> isn't very accurate; otherwise Kyle McMartin wouldn't have had a reason >> to send a patch. The patch you prefer would give both of you the >> defaults you want *if* embedded devs always use CONFIG_EXPERT and >> non-embedded people never do. If a non-embedded dev uses CONFIG_EXPERT, >> then Kyle's problem can reappear. >> > > CONFIG_EMBEDDED was renamed CONFIG_EXPERT a couple of years > ago because it was being used by more than embedded people. As > such I'd be wary adding an option which relied on the original > meaning. > > http://lwn.net/Articles/421304/ > > >> I still think it is clearest to enable all by default and allow >> deselecting filters if CONFIG_EXPERT. That way the default selection >> doesn't change behind anyone's back when CONFIG_EXPERT is selected. >> I guess embedded devs go through the config carefully looking for >> things to deselect anyway, but maybe it was easy to miss these options >> just like some desktop users have missed them now. >> >> I'd like to make the defaults such that minimum number of people get >> annoyed in the long term. There will always be people who will need to >> change the options (otherwise options wouldn't be needed). At the same >> time this is so very tiny issue that I don't expect many people to care >> at all. >> >> I'll submit some patch this week but I'll wait for more opinions for a >> day or two. Thanks. >> > > As Kyle should know Redhat hit this problem with Squashfs a while back, > and it was I who identified the config changes as the culprit > (BTW Kyle you should have CC'd me on the patch as a courtesy). > The bug is only viewable by Redhat employees (disclaimer, I'm > also a Redhat employee), and so I'll say no more. > > But speaking as the Squashfs author, the lack of BCJ support for > an architecture creates a subtle failure mode in Squashfs, this is > because not all blocks in a Squashfs filesystem get compressed > with a BCJ filter. At compression time each block is compressed > without any BCJ filter, and then with the BCJ filter(s) selected on > the command line, and the best compression for *that* block is > chosen. What this means is kernels without a particular > BCJ filter can still read the Squashfs metadata (mount, ls etc.) and > read many of the files, it is only some files that mysteriously > fail with decompression error. As such this will be (and has been) > invariably treated as a bug in Squashfs. > > Moreover, without expert knowledge of Squashfs, and the config > options, most people will not have a clue how to fix the issue. > > This is why I prefer the first option, which is to reinstate > the enabling of all filters by default, and then to allow people > to remove the filters they don't want. > > BTW there is a potential additional fix for Squashfs that will > make its handling of (lack of) BCJ filters more intelligent > at mount time, but this of course only addresses Squashfs, > and it relies on an additional call into XZ being added. The > BCJ filters specified at filesystem creation are stored in the > compression options part of the superblock, and are known at > mount time. Squashfs should check that these filters are > supported by the kernel and refuse to mount it otherwise. This > has not been done because AFAIK there is no way to query XZ to > determine which BCJ filters are supported (beyond passing it a > test stream which is too messy). > > If this call was added, I would be happy to add a patch to Squashfs > to do this. I am okay with enabling all filters by default as long as there is a knob to allow turning achitecture specific BCJ filters off, as it barely makes sense for embedded systems to have a BCJ filter implementation for anything but the architecture you are running on. It does make sense for a desktop use-case like what Kyle describes in his initial patch. What you describes sounds like there is room for improvements though instead of silently failing... Thanks -- Florian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On 2014-03-03 Florian Fainelli wrote: > 2014-03-03 4:51 GMT-08:00 Lasse Collin : > > The second version enables the BCJ filter only for the current arch > > by default if CONFIG_EXPERT; without CONFIG_EXPERT all filters are > > forced on: > > I like this version better because it still provides you with accurate > defaults (i.e: enable only th X86 BCJ filter decoder where it makes > sense by default) and still provides enough flexibility. I understand your viewpoint, but different people probably consider different options as "accurate defaults". The current behavior certainly isn't very accurate; otherwise Kyle McMartin wouldn't have had a reason to send a patch. The patch you prefer would give both of you the defaults you want *if* embedded devs always use CONFIG_EXPERT and non-embedded people never do. If a non-embedded dev uses CONFIG_EXPERT, then Kyle's problem can reappear. I still think it is clearest to enable all by default and allow deselecting filters if CONFIG_EXPERT. That way the default selection doesn't change behind anyone's back when CONFIG_EXPERT is selected. I guess embedded devs go through the config carefully looking for things to deselect anyway, but maybe it was easy to miss these options just like some desktop users have missed them now. I'd like to make the defaults such that minimum number of people get annoyed in the long term. There will always be people who will need to change the options (otherwise options wouldn't be needed). At the same time this is so very tiny issue that I don't expect many people to care at all. I'll submit some patch this week but I'll wait for more opinions for a day or two. Thanks. -- Lasse Collin | IRC: Larhzu @ IRCnet & Freenode -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On 2014-03-03 Florian Fainelli wrote: 2014-03-03 4:51 GMT-08:00 Lasse Collin lasse.col...@tukaani.org: The second version enables the BCJ filter only for the current arch by default if CONFIG_EXPERT; without CONFIG_EXPERT all filters are forced on: I like this version better because it still provides you with accurate defaults (i.e: enable only th X86 BCJ filter decoder where it makes sense by default) and still provides enough flexibility. I understand your viewpoint, but different people probably consider different options as accurate defaults. The current behavior certainly isn't very accurate; otherwise Kyle McMartin wouldn't have had a reason to send a patch. The patch you prefer would give both of you the defaults you want *if* embedded devs always use CONFIG_EXPERT and non-embedded people never do. If a non-embedded dev uses CONFIG_EXPERT, then Kyle's problem can reappear. I still think it is clearest to enable all by default and allow deselecting filters if CONFIG_EXPERT. That way the default selection doesn't change behind anyone's back when CONFIG_EXPERT is selected. I guess embedded devs go through the config carefully looking for things to deselect anyway, but maybe it was easy to miss these options just like some desktop users have missed them now. I'd like to make the defaults such that minimum number of people get annoyed in the long term. There will always be people who will need to change the options (otherwise options wouldn't be needed). At the same time this is so very tiny issue that I don't expect many people to care at all. I'll submit some patch this week but I'll wait for more opinions for a day or two. Thanks. -- Lasse Collin | IRC: Larhzu @ IRCnet Freenode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
2014-03-04 18:51 GMT-08:00 Phillip Lougher phil...@lougher.demon.co.uk: On 04/03/14 18:20, Lasse Collin wrote: On 2014-03-03 Florian Fainelli wrote: 2014-03-03 4:51 GMT-08:00 Lasse Collin lasse.col...@tukaani.org: The second version enables the BCJ filter only for the current arch by default if CONFIG_EXPERT; without CONFIG_EXPERT all filters are forced on: I like this version better because it still provides you with accurate defaults (i.e: enable only th X86 BCJ filter decoder where it makes sense by default) and still provides enough flexibility. I understand your viewpoint, but different people probably consider different options as accurate defaults. The current behavior certainly isn't very accurate; otherwise Kyle McMartin wouldn't have had a reason to send a patch. The patch you prefer would give both of you the defaults you want *if* embedded devs always use CONFIG_EXPERT and non-embedded people never do. If a non-embedded dev uses CONFIG_EXPERT, then Kyle's problem can reappear. CONFIG_EMBEDDED was renamed CONFIG_EXPERT a couple of years ago because it was being used by more than embedded people. As such I'd be wary adding an option which relied on the original meaning. http://lwn.net/Articles/421304/ I still think it is clearest to enable all by default and allow deselecting filters if CONFIG_EXPERT. That way the default selection doesn't change behind anyone's back when CONFIG_EXPERT is selected. I guess embedded devs go through the config carefully looking for things to deselect anyway, but maybe it was easy to miss these options just like some desktop users have missed them now. I'd like to make the defaults such that minimum number of people get annoyed in the long term. There will always be people who will need to change the options (otherwise options wouldn't be needed). At the same time this is so very tiny issue that I don't expect many people to care at all. I'll submit some patch this week but I'll wait for more opinions for a day or two. Thanks. As Kyle should know Redhat hit this problem with Squashfs a while back, and it was I who identified the config changes as the culprit (BTW Kyle you should have CC'd me on the patch as a courtesy). The bug is only viewable by Redhat employees (disclaimer, I'm also a Redhat employee), and so I'll say no more. But speaking as the Squashfs author, the lack of BCJ support for an architecture creates a subtle failure mode in Squashfs, this is because not all blocks in a Squashfs filesystem get compressed with a BCJ filter. At compression time each block is compressed without any BCJ filter, and then with the BCJ filter(s) selected on the command line, and the best compression for *that* block is chosen. What this means is kernels without a particular BCJ filter can still read the Squashfs metadata (mount, ls etc.) and read many of the files, it is only some files that mysteriously fail with decompression error. As such this will be (and has been) invariably treated as a bug in Squashfs. Moreover, without expert knowledge of Squashfs, and the config options, most people will not have a clue how to fix the issue. This is why I prefer the first option, which is to reinstate the enabling of all filters by default, and then to allow people to remove the filters they don't want. BTW there is a potential additional fix for Squashfs that will make its handling of (lack of) BCJ filters more intelligent at mount time, but this of course only addresses Squashfs, and it relies on an additional call into XZ being added. The BCJ filters specified at filesystem creation are stored in the compression options part of the superblock, and are known at mount time. Squashfs should check that these filters are supported by the kernel and refuse to mount it otherwise. This has not been done because AFAIK there is no way to query XZ to determine which BCJ filters are supported (beyond passing it a test stream which is too messy). If this call was added, I would be happy to add a patch to Squashfs to do this. I am okay with enabling all filters by default as long as there is a knob to allow turning achitecture specific BCJ filters off, as it barely makes sense for embedded systems to have a BCJ filter implementation for anything but the architecture you are running on. It does make sense for a desktop use-case like what Kyle describes in his initial patch. What you describes sounds like there is room for improvements though instead of silently failing... Thanks -- Florian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
2014-03-03 4:51 GMT-08:00 Lasse Collin : > On 2014-02-28 Kyle McMartin wrote: >> Having these optional is more trouble than is justified by the >> negligible increase in code size to lib/xz/ if they're all compiled >> in. Their optional status ends up necessitating rebuilds of the kernel >> in order to be able to decompress XZ-compressed squashfs images which >> use non-native BCJ filters (ie: you need to enable XZ_DEC_POWERPC on >> x86_64 in order to loop-mount a ppc64 squashfs that uses it.) > > Originally all BCJ filters were enabled by default and CONFIG_EXPERT > allowed disabling them one by one. It was changed a year ago to the > current state because I wasn't against it when it was suggested: > > http://lkml.org/lkml/2013/1/15/449 > > It seems that I should have been against it. I suggest things are > rolled back like they were: all BCJ filters enabled by default and > CONFIG_EXPERT makes them deselectable. This time I have a somewhat > strong opinion that this is the best way to go, even if it means that > the embedded people must remember to deselect the unnneeded filters. > > An alternative could be that with CONFIG_EXPERT only the BCJ filter for > the current arch would be selected by default. Without CONFIG_EXPERT > all filters would be forced on. I guess this could be confusing since > the level of XZ support they get by default when they enable Squashfs' > XZ support would depend on CONFIG_EXPERT. So I think my first suggestion > is better. > >> So save ourselves the trouble and build them all into xz_dec_bcj.o to >> begin with. (I could conceivably see a case where CONFIG_EXPERT made >> these selectable, but again, even on embedded platforms, the .text >> increase we're talking about is noise...) >> >> text databss dec hex filename >> 1239 0 0 12394d7 xz_dec_bcj.o >> 2263 0 0 22638d7 xz_dec_bcj.o.2 > > No, let's not unconditionally build them all: > > - 1 KiB might be noise, but since on embedded systems that 1 KiB > really is completely useless code and it's very easy to omit it, > the option to omit such code should be kept. > > - If the kernel image is compressed with XZ, a separate copy of > the decompressor is built for the pre-boot environment. > Conditional compilation of the BCJ filters is also used for > pre-boot environments which your patch doesn't touch. The > 1 KiB increase would affect the copy in the pre-boot code too. > > - This XZ decompressor was written with the Linux kernel in mind, > but it's used elsewhere too. It would be nice to minimize the > differences between the code in Linux and the out-of-kernel tree. > Outside Linux I will keep the BCJ filters optional anyway. Absolutely, this was actually the primary concern for these patches. 1KiB might not sound like a lot, but when you take than into account in the bigger picture of saving kernel size, this ends up making a difference. > > Below are two alternative patches matching my two suggestions. I prefer > the first patch and I will submit it in a day or two unless people > have better ideas. > > diff -Nru linux-3.14-rc5.orig/lib/xz/Kconfig linux-3.14-rc5/lib/xz/Kconfig > --- linux-3.14-rc5.orig/lib/xz/Kconfig 2014-03-03 04:56:16.0 +0200 > +++ linux-3.14-rc5/lib/xz/Kconfig 2014-03-03 13:26:58.402872951 +0200 > @@ -9,33 +9,33 @@ > if XZ_DEC > > config XZ_DEC_X86 > - bool "x86 BCJ filter decoder" > - default y if X86 > + bool "x86 BCJ filter decoder" if EXPERT > + default y > select XZ_DEC_BCJ > > config XZ_DEC_POWERPC > - bool "PowerPC BCJ filter decoder" > - default y if PPC > + bool "PowerPC BCJ filter decoder" if EXPERT > + default y > select XZ_DEC_BCJ > > config XZ_DEC_IA64 > - bool "IA-64 BCJ filter decoder" > - default y if IA64 > + bool "IA-64 BCJ filter decoder" if EXPERT > + default y > select XZ_DEC_BCJ > > config XZ_DEC_ARM > - bool "ARM BCJ filter decoder" > - default y if ARM > + bool "ARM BCJ filter decoder" if EXPERT > + default y > select XZ_DEC_BCJ > > config XZ_DEC_ARMTHUMB > - bool "ARM-Thumb BCJ filter decoder" > - default y if (ARM && ARM_THUMB) > + bool "ARM-Thumb BCJ filter decoder" if EXPERT > + default y > select XZ_DEC_BCJ > > config XZ_DEC_SPARC > - bool "SPARC BCJ filter decoder" > - default y if SPARC > + bool "SPARC BCJ filter decoder" if EXPERT > + default y > select XZ_DEC_BCJ > > endif > > The second version enables the BCJ filter only for the current arch by > default if CONFIG_EXPERT; without CONFIG_EXPERT all filters are forced > on: I like this version better because it still provides you with accurate defaults (i.e: enable only th X86 BCJ filter decoder where it makes sense by default) and still provides enough flexibility. > > diff -Nru
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On 2014-02-28 Kyle McMartin wrote: > Having these optional is more trouble than is justified by the > negligible increase in code size to lib/xz/ if they're all compiled > in. Their optional status ends up necessitating rebuilds of the kernel > in order to be able to decompress XZ-compressed squashfs images which > use non-native BCJ filters (ie: you need to enable XZ_DEC_POWERPC on > x86_64 in order to loop-mount a ppc64 squashfs that uses it.) Originally all BCJ filters were enabled by default and CONFIG_EXPERT allowed disabling them one by one. It was changed a year ago to the current state because I wasn't against it when it was suggested: http://lkml.org/lkml/2013/1/15/449 It seems that I should have been against it. I suggest things are rolled back like they were: all BCJ filters enabled by default and CONFIG_EXPERT makes them deselectable. This time I have a somewhat strong opinion that this is the best way to go, even if it means that the embedded people must remember to deselect the unnneeded filters. An alternative could be that with CONFIG_EXPERT only the BCJ filter for the current arch would be selected by default. Without CONFIG_EXPERT all filters would be forced on. I guess this could be confusing since the level of XZ support they get by default when they enable Squashfs' XZ support would depend on CONFIG_EXPERT. So I think my first suggestion is better. > So save ourselves the trouble and build them all into xz_dec_bcj.o to > begin with. (I could conceivably see a case where CONFIG_EXPERT made > these selectable, but again, even on embedded platforms, the .text > increase we're talking about is noise...) > > text databss dec hex filename > 1239 0 0 12394d7 xz_dec_bcj.o > 2263 0 0 22638d7 xz_dec_bcj.o.2 No, let's not unconditionally build them all: - 1 KiB might be noise, but since on embedded systems that 1 KiB really is completely useless code and it's very easy to omit it, the option to omit such code should be kept. - If the kernel image is compressed with XZ, a separate copy of the decompressor is built for the pre-boot environment. Conditional compilation of the BCJ filters is also used for pre-boot environments which your patch doesn't touch. The 1 KiB increase would affect the copy in the pre-boot code too. - This XZ decompressor was written with the Linux kernel in mind, but it's used elsewhere too. It would be nice to minimize the differences between the code in Linux and the out-of-kernel tree. Outside Linux I will keep the BCJ filters optional anyway. Below are two alternative patches matching my two suggestions. I prefer the first patch and I will submit it in a day or two unless people have better ideas. diff -Nru linux-3.14-rc5.orig/lib/xz/Kconfig linux-3.14-rc5/lib/xz/Kconfig --- linux-3.14-rc5.orig/lib/xz/Kconfig 2014-03-03 04:56:16.0 +0200 +++ linux-3.14-rc5/lib/xz/Kconfig 2014-03-03 13:26:58.402872951 +0200 @@ -9,33 +9,33 @@ if XZ_DEC config XZ_DEC_X86 - bool "x86 BCJ filter decoder" - default y if X86 + bool "x86 BCJ filter decoder" if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_POWERPC - bool "PowerPC BCJ filter decoder" - default y if PPC + bool "PowerPC BCJ filter decoder" if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_IA64 - bool "IA-64 BCJ filter decoder" - default y if IA64 + bool "IA-64 BCJ filter decoder" if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_ARM - bool "ARM BCJ filter decoder" - default y if ARM + bool "ARM BCJ filter decoder" if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_ARMTHUMB - bool "ARM-Thumb BCJ filter decoder" - default y if (ARM && ARM_THUMB) + bool "ARM-Thumb BCJ filter decoder" if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_SPARC - bool "SPARC BCJ filter decoder" - default y if SPARC + bool "SPARC BCJ filter decoder" if EXPERT + default y select XZ_DEC_BCJ endif The second version enables the BCJ filter only for the current arch by default if CONFIG_EXPERT; without CONFIG_EXPERT all filters are forced on: diff -Nru linux-3.14-rc5.orig/lib/xz/Kconfig linux-3.14-rc5/lib/xz/Kconfig --- linux-3.14-rc5.orig/lib/xz/Kconfig 2014-03-03 04:56:16.0 +0200 +++ linux-3.14-rc5/lib/xz/Kconfig 2014-03-03 14:15:42.196209325 +0200 @@ -9,33 +9,33 @@ if XZ_DEC config XZ_DEC_X86 - bool "x86 BCJ filter decoder" - default y if X86 + bool "x86 BCJ filter decoder" if EXPERT + default y if !EXPERT || X86 select XZ_DEC_BCJ config XZ_DEC_POWERPC - bool "PowerPC BCJ filter decoder" - default y if PPC + bool "PowerPC BCJ filter decoder" if EXPERT + default y if !EXPERT || PPC select XZ_DEC_BCJ config
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
On 2014-02-28 Kyle McMartin wrote: Having these optional is more trouble than is justified by the negligible increase in code size to lib/xz/ if they're all compiled in. Their optional status ends up necessitating rebuilds of the kernel in order to be able to decompress XZ-compressed squashfs images which use non-native BCJ filters (ie: you need to enable XZ_DEC_POWERPC on x86_64 in order to loop-mount a ppc64 squashfs that uses it.) Originally all BCJ filters were enabled by default and CONFIG_EXPERT allowed disabling them one by one. It was changed a year ago to the current state because I wasn't against it when it was suggested: http://lkml.org/lkml/2013/1/15/449 It seems that I should have been against it. I suggest things are rolled back like they were: all BCJ filters enabled by default and CONFIG_EXPERT makes them deselectable. This time I have a somewhat strong opinion that this is the best way to go, even if it means that the embedded people must remember to deselect the unnneeded filters. An alternative could be that with CONFIG_EXPERT only the BCJ filter for the current arch would be selected by default. Without CONFIG_EXPERT all filters would be forced on. I guess this could be confusing since the level of XZ support they get by default when they enable Squashfs' XZ support would depend on CONFIG_EXPERT. So I think my first suggestion is better. So save ourselves the trouble and build them all into xz_dec_bcj.o to begin with. (I could conceivably see a case where CONFIG_EXPERT made these selectable, but again, even on embedded platforms, the .text increase we're talking about is noise...) text databss dec hex filename 1239 0 0 12394d7 xz_dec_bcj.o 2263 0 0 22638d7 xz_dec_bcj.o.2 No, let's not unconditionally build them all: - 1 KiB might be noise, but since on embedded systems that 1 KiB really is completely useless code and it's very easy to omit it, the option to omit such code should be kept. - If the kernel image is compressed with XZ, a separate copy of the decompressor is built for the pre-boot environment. Conditional compilation of the BCJ filters is also used for pre-boot environments which your patch doesn't touch. The 1 KiB increase would affect the copy in the pre-boot code too. - This XZ decompressor was written with the Linux kernel in mind, but it's used elsewhere too. It would be nice to minimize the differences between the code in Linux and the out-of-kernel tree. Outside Linux I will keep the BCJ filters optional anyway. Below are two alternative patches matching my two suggestions. I prefer the first patch and I will submit it in a day or two unless people have better ideas. diff -Nru linux-3.14-rc5.orig/lib/xz/Kconfig linux-3.14-rc5/lib/xz/Kconfig --- linux-3.14-rc5.orig/lib/xz/Kconfig 2014-03-03 04:56:16.0 +0200 +++ linux-3.14-rc5/lib/xz/Kconfig 2014-03-03 13:26:58.402872951 +0200 @@ -9,33 +9,33 @@ if XZ_DEC config XZ_DEC_X86 - bool x86 BCJ filter decoder - default y if X86 + bool x86 BCJ filter decoder if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_POWERPC - bool PowerPC BCJ filter decoder - default y if PPC + bool PowerPC BCJ filter decoder if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_IA64 - bool IA-64 BCJ filter decoder - default y if IA64 + bool IA-64 BCJ filter decoder if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_ARM - bool ARM BCJ filter decoder - default y if ARM + bool ARM BCJ filter decoder if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_ARMTHUMB - bool ARM-Thumb BCJ filter decoder - default y if (ARM ARM_THUMB) + bool ARM-Thumb BCJ filter decoder if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_SPARC - bool SPARC BCJ filter decoder - default y if SPARC + bool SPARC BCJ filter decoder if EXPERT + default y select XZ_DEC_BCJ endif The second version enables the BCJ filter only for the current arch by default if CONFIG_EXPERT; without CONFIG_EXPERT all filters are forced on: diff -Nru linux-3.14-rc5.orig/lib/xz/Kconfig linux-3.14-rc5/lib/xz/Kconfig --- linux-3.14-rc5.orig/lib/xz/Kconfig 2014-03-03 04:56:16.0 +0200 +++ linux-3.14-rc5/lib/xz/Kconfig 2014-03-03 14:15:42.196209325 +0200 @@ -9,33 +9,33 @@ if XZ_DEC config XZ_DEC_X86 - bool x86 BCJ filter decoder - default y if X86 + bool x86 BCJ filter decoder if EXPERT + default y if !EXPERT || X86 select XZ_DEC_BCJ config XZ_DEC_POWERPC - bool PowerPC BCJ filter decoder - default y if PPC + bool PowerPC BCJ filter decoder if EXPERT + default y if !EXPERT || PPC select XZ_DEC_BCJ config XZ_DEC_IA64 - bool IA-64 BCJ filter decoder -
Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional
2014-03-03 4:51 GMT-08:00 Lasse Collin lasse.col...@tukaani.org: On 2014-02-28 Kyle McMartin wrote: Having these optional is more trouble than is justified by the negligible increase in code size to lib/xz/ if they're all compiled in. Their optional status ends up necessitating rebuilds of the kernel in order to be able to decompress XZ-compressed squashfs images which use non-native BCJ filters (ie: you need to enable XZ_DEC_POWERPC on x86_64 in order to loop-mount a ppc64 squashfs that uses it.) Originally all BCJ filters were enabled by default and CONFIG_EXPERT allowed disabling them one by one. It was changed a year ago to the current state because I wasn't against it when it was suggested: http://lkml.org/lkml/2013/1/15/449 It seems that I should have been against it. I suggest things are rolled back like they were: all BCJ filters enabled by default and CONFIG_EXPERT makes them deselectable. This time I have a somewhat strong opinion that this is the best way to go, even if it means that the embedded people must remember to deselect the unnneeded filters. An alternative could be that with CONFIG_EXPERT only the BCJ filter for the current arch would be selected by default. Without CONFIG_EXPERT all filters would be forced on. I guess this could be confusing since the level of XZ support they get by default when they enable Squashfs' XZ support would depend on CONFIG_EXPERT. So I think my first suggestion is better. So save ourselves the trouble and build them all into xz_dec_bcj.o to begin with. (I could conceivably see a case where CONFIG_EXPERT made these selectable, but again, even on embedded platforms, the .text increase we're talking about is noise...) text databss dec hex filename 1239 0 0 12394d7 xz_dec_bcj.o 2263 0 0 22638d7 xz_dec_bcj.o.2 No, let's not unconditionally build them all: - 1 KiB might be noise, but since on embedded systems that 1 KiB really is completely useless code and it's very easy to omit it, the option to omit such code should be kept. - If the kernel image is compressed with XZ, a separate copy of the decompressor is built for the pre-boot environment. Conditional compilation of the BCJ filters is also used for pre-boot environments which your patch doesn't touch. The 1 KiB increase would affect the copy in the pre-boot code too. - This XZ decompressor was written with the Linux kernel in mind, but it's used elsewhere too. It would be nice to minimize the differences between the code in Linux and the out-of-kernel tree. Outside Linux I will keep the BCJ filters optional anyway. Absolutely, this was actually the primary concern for these patches. 1KiB might not sound like a lot, but when you take than into account in the bigger picture of saving kernel size, this ends up making a difference. Below are two alternative patches matching my two suggestions. I prefer the first patch and I will submit it in a day or two unless people have better ideas. diff -Nru linux-3.14-rc5.orig/lib/xz/Kconfig linux-3.14-rc5/lib/xz/Kconfig --- linux-3.14-rc5.orig/lib/xz/Kconfig 2014-03-03 04:56:16.0 +0200 +++ linux-3.14-rc5/lib/xz/Kconfig 2014-03-03 13:26:58.402872951 +0200 @@ -9,33 +9,33 @@ if XZ_DEC config XZ_DEC_X86 - bool x86 BCJ filter decoder - default y if X86 + bool x86 BCJ filter decoder if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_POWERPC - bool PowerPC BCJ filter decoder - default y if PPC + bool PowerPC BCJ filter decoder if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_IA64 - bool IA-64 BCJ filter decoder - default y if IA64 + bool IA-64 BCJ filter decoder if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_ARM - bool ARM BCJ filter decoder - default y if ARM + bool ARM BCJ filter decoder if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_ARMTHUMB - bool ARM-Thumb BCJ filter decoder - default y if (ARM ARM_THUMB) + bool ARM-Thumb BCJ filter decoder if EXPERT + default y select XZ_DEC_BCJ config XZ_DEC_SPARC - bool SPARC BCJ filter decoder - default y if SPARC + bool SPARC BCJ filter decoder if EXPERT + default y select XZ_DEC_BCJ endif The second version enables the BCJ filter only for the current arch by default if CONFIG_EXPERT; without CONFIG_EXPERT all filters are forced on: I like this version better because it still provides you with accurate defaults (i.e: enable only th X86 BCJ filter decoder where it makes sense by default) and still provides enough flexibility. diff -Nru linux-3.14-rc5.orig/lib/xz/Kconfig linux-3.14-rc5/lib/xz/Kconfig --- linux-3.14-rc5.orig/lib/xz/Kconfig 2014-03-03 04:56:16.0 +0200 +++
[PATCH] xz: make XZ_DEC_BCJ filters non-optional
From: Kyle McMartin
Having these optional is more trouble than is justified by the
negligible increase in code size to lib/xz/ if they're all compiled in.
Their optional status ends up necessitating rebuilds of the kernel
in order to be able to decompress XZ-compressed squashfs images which
use non-native BCJ filters (ie: you need to enable XZ_DEC_POWERPC on
x86_64 in order to loop-mount a ppc64 squashfs that uses it.)
So save ourselves the trouble and build them all into xz_dec_bcj.o to
begin with. (I could conceivably see a case where CONFIG_EXPERT made
these selectable, but again, even on embedded platforms, the .text
increase we're talking about is noise...)
textdatabss dec hex filename
12390 0 12394d7 xz_dec_bcj.o
22630 0 22638d7 xz_dec_bcj.o.2
regards, Kyle
Signed-off-by: Kyle McMartin
--- a/lib/xz/Kconfig
+++ b/lib/xz/Kconfig
@@ -6,44 +6,6 @@ config XZ_DEC
the .xz file format as the container. For integrity checking,
CRC32 is supported. See Documentation/xz.txt for more information.
-if XZ_DEC
-
-config XZ_DEC_X86
- bool "x86 BCJ filter decoder"
- default y if X86
- select XZ_DEC_BCJ
-
-config XZ_DEC_POWERPC
- bool "PowerPC BCJ filter decoder"
- default y if PPC
- select XZ_DEC_BCJ
-
-config XZ_DEC_IA64
- bool "IA-64 BCJ filter decoder"
- default y if IA64
- select XZ_DEC_BCJ
-
-config XZ_DEC_ARM
- bool "ARM BCJ filter decoder"
- default y if ARM
- select XZ_DEC_BCJ
-
-config XZ_DEC_ARMTHUMB
- bool "ARM-Thumb BCJ filter decoder"
- default y if (ARM && ARM_THUMB)
- select XZ_DEC_BCJ
-
-config XZ_DEC_SPARC
- bool "SPARC BCJ filter decoder"
- default y if SPARC
- select XZ_DEC_BCJ
-
-endif
-
-config XZ_DEC_BCJ
- bool
- default n
-
config XZ_DEC_TEST
tristate "XZ decompressor tester"
default n
diff --git a/lib/xz/Makefile b/lib/xz/Makefile
index a7fa769..4f209f7 100644
--- a/lib/xz/Makefile
+++ b/lib/xz/Makefile
@@ -1,5 +1,4 @@
obj-$(CONFIG_XZ_DEC) += xz_dec.o
-xz_dec-y := xz_dec_syms.o xz_dec_stream.o xz_dec_lzma2.o
-xz_dec-$(CONFIG_XZ_DEC_BCJ) += xz_dec_bcj.o
+xz_dec-y := xz_dec_syms.o xz_dec_stream.o xz_dec_lzma2.o xz_dec_bcj.o
obj-$(CONFIG_XZ_DEC_TEST) += xz_dec_test.o
diff --git a/lib/xz/xz_dec_bcj.c b/lib/xz/xz_dec_bcj.c
index a768e6d..e2ac55c 100644
--- a/lib/xz/xz_dec_bcj.c
+++ b/lib/xz/xz_dec_bcj.c
@@ -10,12 +10,6 @@
#include "xz_private.h"
-/*
- * The rest of the file is inside this ifdef. It makes things a little more
- * convenient when building without support for any BCJ filters.
- */
-#ifdef XZ_DEC_BCJ
-
struct xz_dec_bcj {
/* Type of the BCJ filter being used */
enum {
@@ -75,7 +69,6 @@ struct xz_dec_bcj {
} temp;
};
-#ifdef XZ_DEC_X86
/*
* This is used to test the most significant byte of a memory address
* in an x86 instruction.
@@ -154,9 +147,7 @@ static size_t bcj_x86(struct xz_dec_bcj *s, uint8_t *buf,
size_t size)
s->x86_prev_mask = prev_pos > 3 ? 0 : prev_mask << (prev_pos - 1);
return i;
}
-#endif
-#ifdef XZ_DEC_POWERPC
static size_t bcj_powerpc(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
{
size_t i;
@@ -175,9 +166,7 @@ static size_t bcj_powerpc(struct xz_dec_bcj *s, uint8_t
*buf, size_t size)
return i;
}
-#endif
-#ifdef XZ_DEC_IA64
static size_t bcj_ia64(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
{
static const uint8_t branch_table[32] = {
@@ -259,9 +248,7 @@ static size_t bcj_ia64(struct xz_dec_bcj *s, uint8_t *buf,
size_t size)
return i;
}
-#endif
-#ifdef XZ_DEC_ARM
static size_t bcj_arm(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
{
size_t i;
@@ -282,9 +269,7 @@ static size_t bcj_arm(struct xz_dec_bcj *s, uint8_t *buf,
size_t size)
return i;
}
-#endif
-#ifdef XZ_DEC_ARMTHUMB
static size_t bcj_armthumb(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
{
size_t i;
@@ -310,9 +295,7 @@ static size_t bcj_armthumb(struct xz_dec_bcj *s, uint8_t
*buf, size_t size)
return i;
}
-#endif
-#ifdef XZ_DEC_SPARC
static size_t bcj_sparc(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
{
size_t i;
@@ -332,7 +315,6 @@ static size_t bcj_sparc(struct xz_dec_bcj *s, uint8_t *buf,
size_t size)
return i;
}
-#endif
/*
* Apply the selected BCJ filter. Update *pos and s->pos to match the amount
@@ -351,36 +333,24 @@ static void bcj_apply(struct xz_dec_bcj *s,
size -= *pos;
switch (s->type) {
-#ifdef XZ_DEC_X86
case BCJ_X86:
filtered = bcj_x86(s, buf, size);
break;
-#endif
-#ifdef XZ_DEC_POWERPC
case BCJ_POWERPC:
filtered = bcj_powerpc(s, buf, size);
break;
-#endif
-#ifdef XZ_DEC_IA64
case BCJ_IA64:
filtered = bcj_ia64(s, buf, size);
break;
[PATCH] xz: make XZ_DEC_BCJ filters non-optional
From: Kyle McMartin k...@redhat.com
Having these optional is more trouble than is justified by the
negligible increase in code size to lib/xz/ if they're all compiled in.
Their optional status ends up necessitating rebuilds of the kernel
in order to be able to decompress XZ-compressed squashfs images which
use non-native BCJ filters (ie: you need to enable XZ_DEC_POWERPC on
x86_64 in order to loop-mount a ppc64 squashfs that uses it.)
So save ourselves the trouble and build them all into xz_dec_bcj.o to
begin with. (I could conceivably see a case where CONFIG_EXPERT made
these selectable, but again, even on embedded platforms, the .text
increase we're talking about is noise...)
textdatabss dec hex filename
12390 0 12394d7 xz_dec_bcj.o
22630 0 22638d7 xz_dec_bcj.o.2
regards, Kyle
Signed-off-by: Kyle McMartin k...@redhat.com
--- a/lib/xz/Kconfig
+++ b/lib/xz/Kconfig
@@ -6,44 +6,6 @@ config XZ_DEC
the .xz file format as the container. For integrity checking,
CRC32 is supported. See Documentation/xz.txt for more information.
-if XZ_DEC
-
-config XZ_DEC_X86
- bool x86 BCJ filter decoder
- default y if X86
- select XZ_DEC_BCJ
-
-config XZ_DEC_POWERPC
- bool PowerPC BCJ filter decoder
- default y if PPC
- select XZ_DEC_BCJ
-
-config XZ_DEC_IA64
- bool IA-64 BCJ filter decoder
- default y if IA64
- select XZ_DEC_BCJ
-
-config XZ_DEC_ARM
- bool ARM BCJ filter decoder
- default y if ARM
- select XZ_DEC_BCJ
-
-config XZ_DEC_ARMTHUMB
- bool ARM-Thumb BCJ filter decoder
- default y if (ARM ARM_THUMB)
- select XZ_DEC_BCJ
-
-config XZ_DEC_SPARC
- bool SPARC BCJ filter decoder
- default y if SPARC
- select XZ_DEC_BCJ
-
-endif
-
-config XZ_DEC_BCJ
- bool
- default n
-
config XZ_DEC_TEST
tristate XZ decompressor tester
default n
diff --git a/lib/xz/Makefile b/lib/xz/Makefile
index a7fa769..4f209f7 100644
--- a/lib/xz/Makefile
+++ b/lib/xz/Makefile
@@ -1,5 +1,4 @@
obj-$(CONFIG_XZ_DEC) += xz_dec.o
-xz_dec-y := xz_dec_syms.o xz_dec_stream.o xz_dec_lzma2.o
-xz_dec-$(CONFIG_XZ_DEC_BCJ) += xz_dec_bcj.o
+xz_dec-y := xz_dec_syms.o xz_dec_stream.o xz_dec_lzma2.o xz_dec_bcj.o
obj-$(CONFIG_XZ_DEC_TEST) += xz_dec_test.o
diff --git a/lib/xz/xz_dec_bcj.c b/lib/xz/xz_dec_bcj.c
index a768e6d..e2ac55c 100644
--- a/lib/xz/xz_dec_bcj.c
+++ b/lib/xz/xz_dec_bcj.c
@@ -10,12 +10,6 @@
#include xz_private.h
-/*
- * The rest of the file is inside this ifdef. It makes things a little more
- * convenient when building without support for any BCJ filters.
- */
-#ifdef XZ_DEC_BCJ
-
struct xz_dec_bcj {
/* Type of the BCJ filter being used */
enum {
@@ -75,7 +69,6 @@ struct xz_dec_bcj {
} temp;
};
-#ifdef XZ_DEC_X86
/*
* This is used to test the most significant byte of a memory address
* in an x86 instruction.
@@ -154,9 +147,7 @@ static size_t bcj_x86(struct xz_dec_bcj *s, uint8_t *buf,
size_t size)
s-x86_prev_mask = prev_pos 3 ? 0 : prev_mask (prev_pos - 1);
return i;
}
-#endif
-#ifdef XZ_DEC_POWERPC
static size_t bcj_powerpc(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
{
size_t i;
@@ -175,9 +166,7 @@ static size_t bcj_powerpc(struct xz_dec_bcj *s, uint8_t
*buf, size_t size)
return i;
}
-#endif
-#ifdef XZ_DEC_IA64
static size_t bcj_ia64(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
{
static const uint8_t branch_table[32] = {
@@ -259,9 +248,7 @@ static size_t bcj_ia64(struct xz_dec_bcj *s, uint8_t *buf,
size_t size)
return i;
}
-#endif
-#ifdef XZ_DEC_ARM
static size_t bcj_arm(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
{
size_t i;
@@ -282,9 +269,7 @@ static size_t bcj_arm(struct xz_dec_bcj *s, uint8_t *buf,
size_t size)
return i;
}
-#endif
-#ifdef XZ_DEC_ARMTHUMB
static size_t bcj_armthumb(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
{
size_t i;
@@ -310,9 +295,7 @@ static size_t bcj_armthumb(struct xz_dec_bcj *s, uint8_t
*buf, size_t size)
return i;
}
-#endif
-#ifdef XZ_DEC_SPARC
static size_t bcj_sparc(struct xz_dec_bcj *s, uint8_t *buf, size_t size)
{
size_t i;
@@ -332,7 +315,6 @@ static size_t bcj_sparc(struct xz_dec_bcj *s, uint8_t *buf,
size_t size)
return i;
}
-#endif
/*
* Apply the selected BCJ filter. Update *pos and s-pos to match the amount
@@ -351,36 +333,24 @@ static void bcj_apply(struct xz_dec_bcj *s,
size -= *pos;
switch (s-type) {
-#ifdef XZ_DEC_X86
case BCJ_X86:
filtered = bcj_x86(s, buf, size);
break;
-#endif
-#ifdef XZ_DEC_POWERPC
case BCJ_POWERPC:
filtered = bcj_powerpc(s, buf, size);
break;
-#endif
-#ifdef XZ_DEC_IA64
case BCJ_IA64:
filtered = bcj_ia64(s, buf, size);
