RFC: direct MTD support for SquashFS
wferi at niif.hu
Wed Mar 24 09:48:09 EDT 2010
Phillip Lougher <phillip at lougher.demon.co.uk> writes:
> Ferenc Wagner wrote:
>> Now with the patch series, sorry.
>> Ferenc Wagner <wferi at 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.
> + 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
> 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
> 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
More information about the linux-mtd