Prev: usbtouchscreen: support bigger iNexio touchscreens
Next: locking issue in vhost_net_set_backend
From: Phillip Lougher on 24 Mar 2010 01:30
Ferenc Wagner wrote:
> Now with the patch series, sorry.
>
> Ferenc Wagner <wferi(a)niif.hu> writes:
>
>> I've got one more patch, which I forgot to export, to pull out the
>> common logic from the backend init functions back into squashfs_read_data().
>> With the bdev backend, that entails reading the first block twice in a
>> row most of the time. This again could be worked around by extending
>> the backend interface, but I'm not sure if it's worth it.
>
> Here it is. I also corrected the name of SQUASHFS_METADATA_SIZE, so it
> may as well compile now.
>


Thanks for your patches.

First things first. Patch sending etiquette :-)

1. Please don't send separate git commits in one big email, it makes them
hard to cross-reference (I lost count of the times I had to repeatedly
scroll though the email to check an earlier commit).

2. Please don't send a patch series consisting of your development process.
Reviewing takes effort, having reviewed one commit to find it reworked
in a second commit, and then reworked again, as the code evolves from
a -> b -> c-> d is irritating, it makes the final solution harder to
evaluate (because you have to keep all the changes in your head), and
wastes my time.

3. Incremental patches are valid if they break up a patch series into easily
digestible changes which can be separately understood, but not if they're
just reworking your code as development progresses...

OK, now for the specific comments.

Frameworks are supposed to make the code cleaner, more understandable,
and generally better. Unfortunately, I see no evidence of that in your
patch. Sorry to be blunt, but there it is.

The backend has seven functions!

+ void *(*init)(struct squashfs_sb_info *, u64, size_t);
+ void (*free)(struct squashfs_sb_info *);
+ ssize_t (*read)(struct squashfs_sb_info *, void **, size_t);
+ int (*probe)(struct file_system_type *, int, const char *,
+ void*, struct vfsmount *);
+ void (*kill)(struct squashfs_sb_info *);
+ loff_t (*size)(const struct super_block *);
+ const char *(*devname)(const struct super_block *, char *);

We move from a single read() function to requiring seven functions to do
the same work, just to add mtd support...

Reading the superblock changes into a 6 function deep nightmare
squashfs_find_backend -> probe -> get_sb_dev -> fill_bdev_super ->
squashfs_fill_super -> add_backend

I find the current 3 function deep situation forced by VFS already a mess,
squashfs_get_sb -> get_sb_bdev -> squashfs_fill_super

Reading a block now takes 3 function calls, init, read, and free. Plus in
your last commit you make the huge mistake of preferring code elegance over
performance, and resort to calling init, read, and free *twice*, the first
just to read *two* bytes (yes, 2 bytes). Why do you think the code was
coded that way in the first place? A fit of absent mindedness perhaps? No, for
performance reasons.

Secondly you make the classic mistake of concentrating on what you want to do
(add MTD), and not giving a damn about anything else. You've totally broken all
the read optimisations in zlib decompression for block devices. Buffer_heads
are deliberately passed directly to the zlib decompressor to allow it to
optimise performance (pass the buffer_head directly to zlib etc.). Buffer_heads
are exposed to the decompressors without crappy go-between wrappers deliberately.

Unfortunately there are lots of other instances where you've eliminated carefully
optimised code.

That's another of my major issues, the patch seems hugely gratuitous, it
touches a huge amount of files/code it shouldn't do, much of this slicing up
optimisied heavily tested and fragile code into other files/functions hither
and thither. Unfortunately much of this code took a long time to get
right, and suffered numerous unanticipated edge conditions/fsfuzzer
triggered bugs. I only touch this code with caution and nothing like to
the extent you've subjected it to.

In short this doesn't pass my quality threshold or the make the minimum changes
necessary threshold. I wouldn't consider asking Linus to merge this.

BTW as a comparison, I have added MTD support to Squashfs twice in the
past, *with* bad block support. The latest has a diff patch of ~500 lines,
with far less complexity and code changes necessary.

BTW2 as evidenced by your FIXME comments against my code in your patch,
you really have to get over the notion that if you don't understand it,
the code must be wrong.

Cheers

Phillip

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Korsgaard on 24 Mar 2010 02:40
>>>>> "Phillip" == Phillip Lougher <phillip(a)lougher.demon.co.uk> writes:

Hi,

Phillip> BTW as a comparison, I have added MTD support to Squashfs
Phillip> twice in the past, *with* bad block support. The latest has a
Phillip> diff patch of ~500 lines, with far less complexity and code
Phillip> changes necessary.

Could we merge that then please?

Again, what kind of size reduction can we expect from getting rid of the
block layer?

--
Bye, Peter Korsgaard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Ferenc Wagner on 24 Mar 2010 07:30
Peter Korsgaard <jacmet(a)sunsite.dk> writes:

>>>>>> "Phillip" == Phillip Lougher <phillip(a)lougher.demon.co.uk> writes:
>
> Phillip> BTW as a comparison, I have added MTD support to Squashfs
> Phillip> twice in the past, *with* bad block support. The latest has a
> Phillip> diff patch of ~500 lines, with far less complexity and code
> Phillip> changes necessary.
>
> Again, what kind of size reduction can we expect from getting rid of
> the block layer?

My size-optimized OpenWRT vmlinux shrank ~170 kB after disabling
CONFIG_BLOCK. Similarly for the memory consumption.
--
Regards,
Feri.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Korsgaard on 24 Mar 2010 07:40
>>>>> "Ferenc" == Ferenc Wagner <wferi(a)niif.hu> writes:

Hi,

>> Again, what kind of size reduction can we expect from getting rid of
>> the block layer?

Ferenc> My size-optimized OpenWRT vmlinux shrank ~170 kB after disabling
Ferenc> CONFIG_BLOCK. Similarly for the memory consumption.

Neat, thanks - That sounds worthwhile.

--
Bye, Peter Korsgaard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Ferenc Wagner on 24 Mar 2010 09:50
Phillip Lougher <phillip(a)lougher.demon.co.uk> writes:

> Ferenc Wagner wrote:
>
>> Now with the patch series, sorry.
>>
>> Ferenc Wagner <wferi(a)niif.hu> writes:
>>
>>> I've got one more patch, which I forgot to export, to pull out the
>>> common logic from the backend init functions back into squashfs_read_data().
>>> With the bdev backend, that entails reading the first block twice in a
>>> row most of the time. This again could be worked around by extending
>>> the backend interface, but I'm not sure if it's worth it.
>>
>> Here it is. I also corrected the name of SQUASHFS_METADATA_SIZE, so it
>> may as well compile now.
>
> Thanks for your patches.
>
> First things first. Patch sending etiquette :-) [...]

Points taken. About the splits, they were the result of quite some
rebasing: I tried to give each patch a unique focus, but apparently
failed. Only the last one touches substantial changed code, for the
explicitly stated reason: it hurts performance.

> Frameworks are supposed to make the code cleaner, more understandable,
> and generally better.

This framework is about added flexibility. I agree it requires some
added complexity, but I hope we can manage it.

> Unfortunately, I see no evidence of that in your patch. Sorry to be
> blunt, but there it is.

No problem.

> + void *(*init)(struct squashfs_sb_info *, u64, size_t);
> + void (*free)(struct squashfs_sb_info *);
> + ssize_t (*read)(struct squashfs_sb_info *, void **, size_t);
> + int (*probe)(struct file_system_type *, int, const char *,
> + void*, struct vfsmount *);
> + void (*kill)(struct squashfs_sb_info *);
> + loff_t (*size)(const struct super_block *);
> + const char *(*devname)(const struct super_block *, char *);
>
> We move from a single read() function to requiring seven functions to do
> the same work, just to add mtd support...

Your single read() function also had separate init, read and free
phases, with something generic in between. I can try to invert the
structure, but see below.

> Reading the superblock changes into a 6 function deep nightmare
> squashfs_find_backend -> probe -> get_sb_dev -> fill_bdev_super ->
> squashfs_fill_super -> add_backend

It's convoluted, but only moves original code around. If you prefer, I
can simplify that by embedding squashfs_sb_info into backend-specific
structures and allocating those separately. This would also get rid of
some of the above 7 functions.

> Reading a block now takes 3 function calls, init, read, and free.

Actually, read must be called several times. Are you worried by the
cost of the indirect function calls?

> Plus in your last commit you make the huge mistake of preferring code
> elegance over performance, and resort to calling init, read, and free
> *twice*, the first just to read *two* bytes (yes, 2 bytes). Why do
> you think the code was coded that way in the first place? A fit of
> absent mindedness perhaps? No, for performance reasons.

I also pointed this out, and that's mosly why I created a separate last
patch. I don't think it's optimal, but replicating the metadata length
handling in the backends isn't either. There's RFC in the subject after
all...

> You've totally broken all the read optimisations in zlib decompression
> for block devices. Buffer_heads are deliberately passed directly to
> the zlib decompressor to allow it to optimise performance (pass the
> buffer_head directly to zlib etc.). Buffer_heads are exposed to the
> decompressors without crappy go-between wrappers deliberately.

At least this is something I noticed and expressed my concers about.
You probably missed that due to the lots of scrolling required by my
sub-par patch-posting technique. :)

> Unfortunately there are lots of other instances where you've
> eliminated carefully optimised code.

Could you please provide some examples?

> In short this doesn't pass my quality threshold or the make the
> minimum changes necessary threshold. I wouldn't consider asking Linus
> to merge this.

Please don't. It's a proof of concept, nothing more. Even a proof of a
wrong concept.

> BTW as a comparison, I have added MTD support to Squashfs twice in the
> past, *with* bad block support.

Unfortunately you aren't allowed to share those, if I read it right.

> The latest has a diff patch of ~500 lines, with far less complexity
> and code changes necessary.

Sure, have a look at my very first patch: 3 files changed, 210
insertions, 21 deletions. And that didn't remove bdev support.

> BTW2 as evidenced by your FIXME comments against my code in your patch,
> you really have to get over the notion that if you don't understand it,
> the code must be wrong.

Sometimes those are FIXMEs to make finding them easier, and are
questions really. If you feel like answering them:

/* FIXME: == s->s_blocksize(_bits)? */
Why do you use devblksize and devblksize_log2 in squashfs_sb_info?
Aren't they the same as sb->s_blocksize and sb->s_blocksize_bits, as
their names suggest?

/* FIXME: squashfs_put_super has meta_index instead */
The failed_mount: part of squashfs_fill_super() is very similar to
squashfs_put_super(), but frees meta_index instead of id_table. Why?

These are probably easy questions for people knowing the VFS layer well,
but I am not one of those. They just triggered my built-in pattern
matcher.
--
Regards,
Feri.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: usbtouchscreen: support bigger iNexio touchscreens
Next: locking issue in vhost_net_set_backend