[PATCH]QNX6 filesystem (RO) driver

[PATCH]QNX6 filesystem (RO) driver

[Kai Bankett]

Hi,

Onhttp://a6.ontika.net/patches/patch-3.2.2-qnx6.gz  you find a patch for
adding QNX6 FS readonly support (against 3.2.2) to the linux kernel.
(well, at least I would like to open the discussion about it ... ;))

It's the first linux kernel driver I wrote, so please be kind to me in
case some of my design decisions do violate some "common practice" well
known to any regular reader of this list.
I am very happy to work on the source in the "right" direction, if you
suggest me the direction.
So, suggestions, feedback, input, testing is highly welcome!

I reverse engineered - I would say - whole QNX6 Filesystem from ground
up. (hexedit, from superblock identification to adressing scheme to node
kinds etc.)
Successfully completed writing a FS resizing application before starting
work on this kernel driver.
(so you can expect more than just readonly to come ...)

Some sidenotes worth mentioning:
- Audi MMI FS support
I've included support for the Audi MMI HDD filesystem (who's driving a
recent Audi with 3G or 3G+ HDD multimedia system?)
So I've added a "mmi_fs" mount option to tell the driver to use the
right superblock layout and -offset for mounting that "modified" or "old
development state" QNX6 derivate filesystem.
Not sure if a mount option is the usual approach, but it workes well -
at least for me.
I took the decision to make that specific driver support configurable
via kernel config.

- Endianness support
QNX6 offers to create filesystems either little- or big endian. That's
an advantage if the created filesystem later is used on an architecture
that got a different endianness.
I introduced some TO_CPUxx() macros and a superblock detection mechanism
for detecting the FS endianness.
Could not test it on a different architecture than x86 and therefore I'm
unsure if it works on a different endian linux system, but from (my)
theory it should do.
At least on x86 it's no problem to mount a different endian QNX6 FS and
read from it - that's what I've tested.
If the macros are the right approach ... well, happy to receive feedback ;)

I also made the endianness support configurable - just to be on the safe
side.

- Checksums
Superblock checksums are validated.
Longfilenames in a qnx6fs got a checksum in the directory inode.
That checksum algorithm is also build in, but just logs to the kernel
log if it detects a wrong checksum.
(so it's not a hard check)
mmi_fs does not have that longfilename protection, so it's switched off
there.

So much for now ...

Kai

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Wed, Feb 01, 2012 at 09:41:35PM +0100, Kai Bankett wrote:

> - Endianness support
> QNX6 offers to create filesystems either little- or big endian. That's
> an advantage if the created filesystem later is used on an architecture
> that got a different endianness.
> I introduced some TO_CPUxx() macros and a superblock detection mechanism
> for detecting the FS endianness.
> Could not test it on a different architecture than x86 and therefore I'm
> unsure if it works on a different endian linux system, but from (my)
> theory it should do.
> At least on x86 it's no problem to mount a different endian QNX6 FS and
> read from it - that's what I've tested.
> If the macros are the right approach ... well, happy to receive feedback ;)

Nope, that's not the right way to do it.  Don't use __leNN for that; define
your own types.  Take a look at e.g. fs/sysv/sysv.h - __fs{16,32} in there
and functions for work with those.  And duplicate that.  What's more, you
really want the direction of conversion spelled out; sure, it's an involution
and you'll get the same code in both directions.  But it's much easier to
verify that conversions are right that way.  sparse is useful for that, BTW.
And don't use the same variable for host- and fs-endian values - it's not
worth the PITA.

Don't bother with struct qnx6_ptr - sparse _will_ complain about all places
where you do arithmetics on __bitwise types.

While we are at it, please use explicitly-sized type when you are dealing
with disk block numbers.  And pick unsigned one...

Note that unsigned long is target-dependent; on some it's 64bit, on some
32bit.  For some things that's exactly what we want, for some...

qnx6_find_bits(): what's wrong with ilog2()?  Or order_base_2(), if you
care about the case when argument itself may be not a power of 2.  Both
are in linux/log2.h...

sb_set_blocksize() may fail; check return value...

WTH are writepage and friends doing in read-only fs driver, especially
seeing that you don't seem to have anything resembling a block allocator?

qnx6_iget() - what, they really don't allow named pipes/sockets/device
nodes on the filesystem?  If not, you can just use init_special_inode()
instead of that printk+iget_failed().

_Never_ do strlen() on unsanitized on-disk data; you can't guarantee a
terminating '\0' in there.

+       /* "" means "." ---> so paths like "/usr/lib//libc.a" work */
+       if (!len && (lf->lf_fname[0] == '.') && (lf->lf_fname[1] == '\0'))
+               return TO_CPU32(dir->i_sb, de->de_inode);

Huh?  That thing is not going to get called with len == 0 and paths like
that are handled in fs/namei.c just fine without forcing every fs driver
to do that kind of contortions.  Moreover, . and .. are _also_ handled
there - ->lookup() never has to deal with those.

Incidentally, don't you have the name length right in the directory
entry?  Or is it just an upper limit?  If it's exact, it would be faster
to reject mismatches without looking at the name contents (and do memcmp()
in case of match).

Re: [PATCH]QNX6 filesystem (RO) driver

[Kai Bankett]

Updated patch: http://a6.ontika.net/patches/patch-3.2.5-qnx6.gz
Signed-off-by: Kai Bankett <chaosman@ontika.net>

On 02/12/2012 05:56 AM, Al Viro wrote:
> On Wed, Feb 01, 2012 at 09:41:35PM +0100, Kai Bankett wrote:
>
>> - Endianness support
>> QNX6 offers to create filesystems either little- or big endian. That's
>> an advantage if the created filesystem later is used on an architecture
>> that got a different endianness.
>> I introduced some TO_CPUxx() macros and a superblock detection mechanism
>> for detecting the FS endianness.
>> Could not test it on a different architecture than x86 and therefore I'm
>> unsure if it works on a different endian linux system, but from (my)
>> theory it should do.
>> At least on x86 it's no problem to mount a different endian QNX6 FS and
>> read from it - that's what I've tested.
>> If the macros are the right approach ... well, happy to receive feedback ;)
> Nope, that's not the right way to do it.  Don't use __leNN for that; define
> your own types.  Take a look at e.g. fs/sysv/sysv.h - __fs{16,32} in there
> and functions for work with those.  And duplicate that.  What's more, you
> really want the direction of conversion spelled out; sure, it's an involution
> and you'll get the same code in both directions.  But it's much easier to
> verify that conversions are right that way.  sparse is useful for that, BTW.
> And don't use the same variable for host- and fs-endian values - it's not
> worth the PITA.

Thanks alot. That was exactly what I was looking for when implementing 
that TO_CPU stuff.
Looked around in some other fs drivers, but did not have a look in the sysv.
You are right, using the alignment functions asymetric makes 
understanding the code far easier as one can directly see in which 
direction things are moved.
At least sparse seems to be fine with my work ...

>
> Don't bother with struct qnx6_ptr - sparse _will_ complain about all places
> where you do arithmetics on __bitwise types.
>
> While we are at it, please use explicitly-sized type when you are dealing
> with disk block numbers.  And pick unsigned one...
I decided to use unsigned int. All adressing is using 32bit pointers. So 
this seems to make most sense to me.
>
> Note that unsigned long is target-dependent; on some it's 64bit, on some
> 32bit.  For some things that's exactly what we want, for some...
Fixed. I decided to stay with unsigned int. Just a couple of blocks (12 
blocks of 32bit).
So, in case someone is really creating a full 32bit * blocksize fs, 
there could be a problem.
I currently can not test how QNX would deal with that special situation 
- sorry.
I hope that's ok ...
>
> qnx6_find_bits(): what's wrong with ilog2()?  Or order_base_2(), if you
> care about the case when argument itself may be not a power of 2.  Both
> are in linux/log2.h...
Great. Exactly what I was looking for!
Fixed.
>
> sb_set_blocksize() may fail; check return value...
Fixed
> WTH are writepage and friends doing in read-only fs driver, especially
> seeing that you don't seem to have anything resembling a block allocator?
Fixed
Those were leftovers from the qnx4 fs driver.
Maybe it would be a good idea if I create a patch for the qnx4 driver 
for removing those?

> qnx6_iget() - what, they really don't allow named pipes/sockets/device
> nodes on the filesystem?  If not, you can just use init_special_inode()
> instead of that printk+iget_failed().
At least so far I could not find any additional extras.
I followed your advise and used init_special_inode() to get rid of that 
printk+stuff.
>
> _Never_ do strlen() on unsanitized on-disk data; you can't guarantee a
> terminating '\0' in there.
Fixed.
Full ack. One more leftover from the qnx4 driver.
However, it seems that the qnx4 driver cannot live without. (at least 
from a quick check, I'm not deep enough in the qnx4 fs)
> +       /* "" means "." --->  so paths like "/usr/lib//libc.a" work */
> +       if (!len&&  (lf->lf_fname[0] == '.')&&  (lf->lf_fname[1] == '\0'))
> +               return TO_CPU32(dir->i_sb, de->de_inode);
>
> Huh?  That thing is not going to get called with len == 0 and paths like
> that are handled in fs/namei.c just fine without forcing every fs driver
> to do that kind of contortions.  Moreover, . and .. are _also_ handled
> there - ->lookup() never has to deal with those.
Removed.
Leftover from qnx4 driver.
In my eyes that one could well be also removed from there.
> Incidentally, don't you have the name length right in the directory
> entry?  Or is it just an upper limit?  If it's exact, it would be faster
> to reject mismatches without looking at the name contents (and do memcmp()
> in case of match).
>
Yes, I have the name length in the directory entry, but only for short 
filenames.
For long filenames the length field in the directory entry is always 0xff.
I just moved the length check up by one if-clause.
Personally I don't think that moving it around further makes that much 
sense, as the length for short filenames is also checked before copying 
the filename.
The way it currently is it's at least symmetrical between short und long 
filename handling code.

I hope I got all the points and fixes right?

Additionally I removed the old bitmap.c. Currently it was not used anyways.
I also cleaned up the previously (at least in my eyes) ugly 
qnx6_super_block and mmi superblock structure.
As the superblock root nodes (inode, bitmap and longfilename) all got 
the same structure, I've added a qnx6_root_inode structure for each root 
node in the superblock.

Thank you very much for the extremely helpful feedback!

Kai

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Sun, Feb 12, 2012 at 11:14:38PM +0100, Kai Bankett wrote:

re junk removal from qnx4 - sure, just make is a separate patch.

> >qnx6_iget() - what, they really don't allow named pipes/sockets/device
> >nodes on the filesystem?  If not, you can just use init_special_inode()
> >instead of that printk+iget_failed().
> At least so far I could not find any additional extras.
> I followed your advise and used init_special_inode() to get rid of
> that printk+stuff.

Umm...  How does qnx encode device numbers for block/character devices?

> Yes, I have the name length in the directory entry, but only for
> short filenames.
> For long filenames the length field in the directory entry is always 0xff.
> I just moved the length check up by one if-clause.
> Personally I don't think that moving it around further makes that
> much sense, as the length for short filenames is also checked before
> copying the filename.
> The way it currently is it's at least symmetrical between short und
> long filename handling code.
> 
> I hope I got all the points and fixes right?

A few more things:
	* duplicating di_block_ptr[] array seems to be pointless
	* qnx6_get_inode_loc(), qnx6_dir_lfile_block() and qnx6_block_map()
seem to have a lot of duplicated code, at the very least.  Looks like
a missing helper function...  Incidentally, I'd switched qnx6_get_devblock()
and qnx6_check_blockptr() to __fs32 argument, along with making QNX6_PTR_UNSET
(~(__fs32)0)
	* what is actually stored in ->de_size for long entries in case
of big-endian fs?  4 bytes of de_inode - fine, as as for short ones, but
then what?  If it's really 32bit big-endian 0xff, it will have the first byte
equal to 0, which would confuse the living hell out of your code.  Or is
it actually __u8 + 3 bytes of padding?  Then it would be compatible with
short dir entries, but conversions would be the wrong thing to do there...
	* I seriously suspect that you want to cook yourself a couple of
unhashed in-core struct inode for Longfile and Inode ones; then get_inode_loc(),
dir_lfile_block() and block_map() would become identical *and* you just might
be able to use page cache instead of all that messing with buffer_heads in
readdir et.al.  Just do new_inode() and fill di_block_ptr/levels/i_mode/i_size
and ->a_ops.  iput() both in ->put_super() and that's it...  For directories
in pagecache see how e.g. ext2 is doing it - or minixfs, for that matter.
	* mmu_private is never used.  Just lose it...

Re: [PATCH]QNX6 filesystem (RO) driver

[Kai Bankett]

On 02/12/2012 11:43 PM, Al Viro wrote:
> On Sun, Feb 12, 2012 at 11:14:38PM +0100, Kai Bankett wrote:
>
> re junk removal from qnx4 - sure, just make is a separate patch.
>
>>> qnx6_iget() - what, they really don't allow named pipes/sockets/device
>>> nodes on the filesystem?  If not, you can just use init_special_inode()
>>> instead of that printk+iget_failed().
>> At least so far I could not find any additional extras.
>> I followed your advise and used init_special_inode() to get rid of
>> that printk+stuff.
> Umm...  How does qnx encode device numbers for block/character devices?
Actually I have to check again. My statement was not meant to say "there 
are none", but so far I have not seen any.
I did my reverse engineering on test qnx6 filesystems created with QNX. 
So far I have not analyzed these special cases.
I will have to try to generate some. Will come back on that one after 
some analysis.
Maybe I will have time tomorrow to dig a bit deeper into that ...
Anyways, the init_special_inode() at the final end seems to be right 
there, so at least one point less on the list.
> A few more things:
> 	* duplicating di_block_ptr[] array seems to be pointless
Again, well spotted. Just removed the "raw" thing.
To me just taking the required stuff into qnx6_inode_info gives better 
control and more speed.
> 	* qnx6_get_inode_loc(), qnx6_dir_lfile_block() and qnx6_block_map()
> seem to have a lot of duplicated code, at the very least.  Looks like
> a missing helper function...
Yep. With the write code it will become even more.
Solution found. Just block_map() survived.
> Incidentally, I'd switched qnx6_get_devblock()
> and qnx6_check_blockptr() to __fs32 argument, along with making QNX6_PTR_UNSET
> (~(__fs32)0)
That was my exactly first approach.
However, I finally decided to move away from that.
My thinking was that maybe qnx6_get_devblock() in the future may be 
called from with a non-__fs32 value holding function.
Also, If I see it right, it won't save any cpu cycles + getting to a 
"uniformed" endianess directly at the fs address pointer reading 
functions makes the endianess border very clear when reading the source.
Feedback very welcome. Easy to change, if you think __fs32 arguments 
make more sense.
> 	* what is actually stored in ->de_size for long entries in case
> of big-endian fs?  4 bytes of de_inode - fine, as as for short ones, but
> then what?  If it's really 32bit big-endian 0xff, it will have the first byte
> equal to 0, which would confuse the living hell out of your code.  Or is
> it actually __u8 + 3 bytes of padding?  Then it would be compatible with
> short dir entries, but conversions would be the wrong thing to do there...
Checked against a hexdump. The de_size for long filename direntries is 
just a single byte.
Corrected - thanks.
> 	* I seriously suspect that you want to cook yourself a couple of
> unhashed in-core struct inode for Longfile and Inode ones; then get_inode_loc(),
> dir_lfile_block() and block_map() would become identical *and* you just might
> be able to use page cache instead of all that messing with buffer_heads in
> readdir et.al.  Just do new_inode() and fill di_block_ptr/levels/i_mode/i_size
> and ->a_ops.  iput() both in ->put_super() and that's it...  For directories
> in pagecache see how e.g. ext2 is doing it - or minixfs, for that matter.
I had a look at that point.
After successfully switching find_enty() to pagemap I spend quite some 
time on switching long_match() to pagemap cache.
At the end I gave up. Things just became too complicated.
Far more complicated than bh stuff.
At least if I get no further clue on how to manage the different 
pagesizes efficient - compared to the very efficient bh code - I cannot 
see light at the end of the tunnel.
For long Filenames, the Filename always is stored in a seperate block. 
Whatever I tried, I did not get the pagemap code as efficient, as it 
abstracts from blocksize. (that's where it got it's stength - IHMO)
Ok, I could save the block adressing steps, but I will add so many other 
steps on the way ... I am really unsure if that pays off at the end. At 
least code complexity rises extremely.
Extremley happy if you could give me some additional guidance an that point.
> 	* mmu_private is never used.  Just lose it...
>
It's history.

Updated Patch: http://a6.ontika.net/patches/patch-3.2.5-qnx6-v2.gz
Signed-off-by: Kai Bankett <chaosman@ontika.net>

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Wed, Feb 15, 2012 at 03:52:01AM +0100, Kai Bankett wrote:
> After successfully switching find_enty() to pagemap I spend quite
> some time on switching long_match() to pagemap cache.
> At the end I gave up. Things just became too complicated.

Not if you have a separate struct inode for /.longfile - then it's
simply read_cache_page() + kmap().

> Far more complicated than bh stuff.
> At least if I get no further clue on how to manage the different
> pagesizes efficient - compared to the very efficient bh code - I
> cannot see light at the end of the tunnel.
> For long Filenames, the Filename always is stored in a seperate
> block. Whatever I tried, I did not get the pagemap code as
> efficient, as it abstracts from blocksize. (that's where it got it's
> stength - IHMO)
> Ok, I could save the block adressing steps, but I will add so many
> other steps on the way ... I am really unsure if that pays off at
> the end. At least code complexity rises extremely.

You do realize that buffer cache does _not_ exist separately from
pagecache, right?  It's just the pagecache of underlying block
device; see __find_get_block_slow() for what really happens behind
the scene.

The difference between page and buffer cache is not in abstracting the
size away; that's trivial.  The real difference is that one is that
between virtually-indexed and physically-indexed caches.  Essentially,
qnx6_get_block() is address translation for that sucker.  And buffer_head
is just a TLB entry.

Think of struct address_space as MMU context; we have one for identity
mapping of block device and what you are doing is manually asking for
address translation and feeding the resulting physical address into
that identity mapped context.  You could use the address spaces of
directory and /.longfiles resp. instead and let the normal logics to
take care of things.  Moreover, you get address translation cached
that way - no need to recalculate physical block addresses every
time.  That map_bh() in qnx6_get_block() is there exactly for that
reason...

Block size is not an issue, really - readdir() should just map pages
one by one and loop over entries in those; when it hits longname,
it would just do read_cache_page() on longfiles inode and look into
it; that's all.  I.e. you need something like

struct qnx6_long_filename *qnx6_longname(sbi, de, p)
{
	u32 s = fs32_to_cpu(sbi, de->de_long_inode); /* in 512-byte units */
	u32 n = s >> (PAGE_CACHE_SHIFT - 9);	/* in pages */
	u32 offs = (s << 9) & ~PAGE_CACHE_MASK;	/* within page */
	struct qnx6_long_filename *lf;
	struct address_space *mapping = sbi->longfiles->i_mapping;
	struct page *page = read_mapping_page(mapping, n, NULL);
	if (IS_ERR(page))
		return ERR_CAST(page);
	kmap(*p = page);
	return (struct qnx6_long_filename *)(page_address(page) + offs);
}

and have readdir do
	lf = qnx6_longname(sbi, de, &page);
	if (IS_ERR(lf)) {
		err = PTR_ERR(lf);
		goto out;
	}
	if (filldir(....) < 0) {
		put_page(page);
		err = 0;
		goto out;
	}
	put_page(page);

where put_page() is exactly what we do in e.g. ext2_put_page() - kunmap()
followed by page_cache_release().  And the same sucker would be used by
qnx6_find_entry(), of course.

BTW, after looking at your readdir() - you have a serious bug there.
It bloody ignores ->f_pos - it *always* starts reading from the beginning
of directory.  Try to do ls on really big directory and you'll have
trouble - beginning of directory returned several times.  It will stop
eventually, since you do increment ->f_pos, but...

Another bug there: you leak lf_bh on "too long longname" failure exit...

Looking at namei.c: you know, (void*)((char*)de + QNX6_DIR_ENTRY_SIZE) is a
really weird way to spell de + 1, seeing that actual argument is always
qnx6_dirent *...

I think you are overdesigning things; unlike ext2, your entries are
fixed-sized and that kills most of the complexity in there.  So you
don't need to bother with last_byte() - just do an analog that would
return the index of _entry_ within page; PAGE_CACHE_SIZE / <entry size>
for all pages but the last one and (i_size & ~PAGE_CACHE_MASK) / <entry size>
for the last page.

And just have something like
	for ( ; n < npages; n++, start = 0) {
		int limit = last_entry(inode, n);
		<get and map page>
		de = page_address(page);
		for (i = start, de += start; i < limit; i++, de++) {
			/* handle this entry */
		}
	}
for your loops over directory contents, both in readdir and find_entry.

One more thing: why do you bother passing inodebits separately
to block_map()?  You are guaranteed that
	block_map(..., n, ..., inodebits)
is equal to
	block_map(..., n >> inodebits, ..., 0)
It's really not hard to prove - introduce __bitdelta and __levelsub
and maintain them so that
        levelsub == __levelsub << inodebits
        bitdelta == __bitdelta + inodebits
all the time.  Then observe that (no - levelsub) >> bitdelta is equal to
(no - (__levelsub << inodebits)) >> (__bitdelta + inodebits), i.e. to
((no >> inodebits) - __levelsub) >> __bitdelta.  Now you can lose levelsub -
it's not used anymore.  And after _that_ neither is __bitdelta.  And after
that you can rename __levelsub and __bitdelta to levelsub and bitdelta resp.
and see that you have reproduced the original function with original
uses of inodebits replaced with 0 and all instances of no with no >> inodebits.
The only exception is that debugging printk ("no too large" case) actually
prints what it claims to print ;-)

Allocation of private inodes is easy -
static struct inode *qnx6_private_inode(struct super_block *s,
                                        struct qnx6_root_node *p)
{
        struct inode *inode = new_inode(s);
        if (inode) {
                struct qnx6_inode_info *ei = QNX6_I(inode);
                struct qnx6_sb_info *sbi = QNX6_SB(s);
                inode->i_size = fs64_to_cpu(sbi, p->size);
                memcpy(ei->di_block_ptr, p->ptr, sizeof(p->ptr));
                ei->di_filelevels = p->levels;
                inode->i_mode = S_IFREG | S_IRUSR; /* that will do */
                inode->i_mapping->a_ops = &qnx6_aops;
        }
        return inode;
}
and that's it.  With that (and inodebits killed) you can turn block_map
into qnx6_block_map(inode, block_nr) - superblock will be inode->i_sb...

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Wed, Feb 15, 2012 at 06:10:12AM +0000, Al Viro wrote:
> ((no >> inodebits) - __levelsub) >> __bitdelta.  Now you can lose levelsub -
> it's not used anymore.  And after _that_ neither is __bitdelta.  And after
                                                      ^^^^^^^^^^ 
Sorry about the braino; that would be "neither is bitdelta", of course...

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Wed, Feb 15, 2012 at 06:10:12AM +0000, Al Viro wrote:

[snip]

OK, I've dumped something that compiles in #qnx6 in vfs.git; I don't promise
anything beyond that - consider that as slightly more elaborate variant of
the previous mail.  At that stage I would definitely prefer to see fixes,
etc. *folded* into the patch before it gets merged, so do not treat that
branch as anything other than convenient (for me) way to post a series of
incrementals.

Oh, and the final result will obviously need s-o-b by Kai + sane commit
message, etc.

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Wed, Feb 15, 2012 at 06:47:28AM +0000, Al Viro wrote:
> On Wed, Feb 15, 2012 at 06:10:12AM +0000, Al Viro wrote:
> 
> [snip]
> 
> OK, I've dumped something that compiles in #qnx6 in vfs.git; I don't promise
> anything beyond that - consider that as slightly more elaborate variant of
> the previous mail.  At that stage I would definitely prefer to see fixes,
> etc. *folded* into the patch before it gets merged, so do not treat that
> branch as anything other than convenient (for me) way to post a series of
> incrementals.
> 
> Oh, and the final result will obviously need s-o-b by Kai + sane commit
> message, etc.

If you prefer to test on top of -stable, just copy d_make_root() from
current mainline into fs/dcache.c and export it - that's the only dependency
on the stuff from the last merge window.  Or just open-code the sucker
into your fill_super...

One more thing - unless I'm misreading the code it looks like your ->sb becomes
a dangling pointer once fill_super finishes and ->statfs() is dereferencing
it...

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Wed, Feb 15, 2012 at 07:11:19AM +0000, Al Viro wrote:

> One more thing - unless I'm misreading the code it looks like your ->sb becomes
> a dangling pointer once fill_super finishes and ->statfs() is dereferencing
> it...

BTW, what happens if root directory grows to more than one level of
pointers?  AFAICS, qnx6_checkroot() assumes that it won't happen...
And why not do that after getting the root inode, anyway?

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Wed, Feb 15, 2012 at 07:57:50AM +0000, Al Viro wrote:
> On Wed, Feb 15, 2012 at 07:11:19AM +0000, Al Viro wrote:
> 
> > One more thing - unless I'm misreading the code it looks like your ->sb becomes
> > a dangling pointer once fill_super finishes and ->statfs() is dereferencing
> > it...
> 
> BTW, what happens if root directory grows to more than one level of
> pointers?  AFAICS, qnx6_checkroot() assumes that it won't happen...
> And why not do that after getting the root inode, anyway?

Ho-hum...  I've added conversion of qnx6_iget() to pagecache and
checkroot move past getting root; result:
	* qnx6_bread/qnx6_getblk are gone
	* qnx6_block_map() has only one caller left
	* s_inodebits/s_dirent_blk/s_inodes_blk gone
s_ptrbits remains, of course, but that one has to survive - it determines
the way indirect blocks' tree is branched.

Remaining issues I see:
	* sbi->sb dangling pointer (those brelse() in success case of
fill_super()).  Probably only one of them needs to be dropped at that
point, leaving the other in sbi->sb_bh to be dropped by put_super
	* sb_set_blocksize() can and will invalidate buffer_heads of the
wrong size.  qnx6_fill_super() probably needs to redo sb_bread() on the
primary superblock after it has done the second sb_set_blocksize().
	* readdir handling of errors (especially in longname case) needs
to be done better.

Other than that... this stuff could use some tidying up, but that's not
a big deal.  I really wonder if these config options are worthwhile -
might be better to stick endianness and mmi to y unconditionally.  And
of course all those incrementals are completely untested and might very
well br broken.

Re: [PATCH]QNX6 filesystem (RO) driver

[Kai Bankett]

> On Wed, Feb 15, 2012 at 07:57:50AM +0000, Al Viro wrote:
>> On Wed, Feb 15, 2012 at 07:11:19AM +0000, Al Viro wrote:
>>
>> > One more thing - unless I'm misreading the code it looks like your
>> ->sb becomes
>> > a dangling pointer once fill_super finishes and ->statfs() is
>> dereferencing
>> > it...
>>
>> BTW, what happens if root directory grows to more than one level of
>> pointers?  AFAICS, qnx6_checkroot() assumes that it won't happen...
>> And why not do that after getting the root inode, anyway?
>
> Ho-hum...  I've added conversion of qnx6_iget() to pagecache and
> checkroot move past getting root; result:
> 	* qnx6_bread/qnx6_getblk are gone
> 	* qnx6_block_map() has only one caller left
> 	* s_inodebits/s_dirent_blk/s_inodes_blk gone
> s_ptrbits remains, of course, but that one has to survive - it determines
> the way indirect blocks' tree is branched.
>
> Remaining issues I see:
> 	* sbi->sb dangling pointer (those brelse() in success case of
> fill_super()).  Probably only one of them needs to be dropped at that
> point, leaving the other in sbi->sb_bh to be dropped by put_super
> 	* sb_set_blocksize() can and will invalidate buffer_heads of the
> wrong size.  qnx6_fill_super() probably needs to redo sb_bread() on the
> primary superblock after it has done the second sb_set_blocksize().
> 	* readdir handling of errors (especially in longname case) needs
> to be done better.
>
> Other than that... this stuff could use some tidying up, but that's not
> a big deal.  I really wonder if these config options are worthwhile -
> might be better to stick endianness and mmi to y unconditionally.  And
> of course all those incrementals are completely untested and might very
> well br broken.

I've fixed sbi->sb and sb_set_blocksize().
I also fixed inodebits thing. (stupid me ... ;))
Missing brelse()'s in dir_longfilename fixed. (together with error handling)

Where do I find your vfs.git tree? Cloned
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git, but could not
find it there.

If I got it right, you would like to always receive complete patches - right?

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Wed, Feb 15, 2012 at 05:27:42PM +0100, Kai Bankett wrote:

> Where do I find your vfs.git tree? Cloned
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git, but could not
> find it there.

Um?  It is there, the branch is called qnx6...  FWIW, its head is commit
aeee926a72fad9cc6d021c20900dd919769d2fe5; if something's fishy with
branches, check if that commit is there.

> If I got it right, you would like to always receive complete patches - right?

Until it's in the mainline - yes.

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Wed, Feb 15, 2012 at 02:40:37PM +0000, Al Viro wrote:

> Other than that... this stuff could use some tidying up, but that's not
> a big deal.  I really wonder if these config options are worthwhile -
> might be better to stick endianness and mmi to y unconditionally.  And
> of course all those incrementals are completely untested and might very
> well br broken.

FWIW, having looked at that code again, I'm definitely in favour of dumping
the ENDIANNESS and MMI config options; the latter is not time-critical at all
(of anything even remotely hot it only affects the checksum verification
for longnames on readdir *and* its only effect is possible short-circuiting
a relatively long calculation - I've just spotted and fixed a braino in
that ifdef from hell, BTW).  And the former... I would be _very_ surprised
if disabling it would be a visible win on any workloads.

BTW, reporting non-options in ->show_options() is probably a bad idea;
sure, you end up ignoring options in your ->mount(), so it won't confuse
any userland scripts too badly, but...

Re: [PATCH]QNX6 filesystem (RO) driver

[Kai Bankett]

> On Wed, Feb 15, 2012 at 02:40:37PM +0000, Al Viro wrote:
>
> FWIW, having looked at that code again, I'm definitely in favour of
> dumping
> the ENDIANNESS and MMI config options; the latter is not time-critical at
> all
>
- removed both options
The MMI option was more meant to make that specific extra functionality
more present to the kernel (compile) user.

- removed the endian_swap mount option (the reported option)
I agree it shall be just sufficient to silently mount the fs.
Different endianness is not of any big importance for using it.
Sounds to me more like something for any userland tools if someone desires
more information about low-level fs-structure stuff.

- fixed a page adressing bug in long_filename()
Long filename blocks are not always 512 bytes. Yes, the longfilename
structure itself is limited to 512bytes, but that's a result of the
smallest supported blocksize to be 512 bytes.
Longfile limit is 510 + 2 bytes length field = longfile record.
However, in a 1k blocksize filesystem, it will still eat up a whole block.

- fixed broken mmi superblock handling, broken by the switch to pagemap,
root_node etc. stuff ;)

- Documentation file added
That should give a quick overview on how the qnx6fs bits and pieces fit in
the whole context.
Feedback very welcome! (as always)
I hope that someone with coding skills after reading that introduction,
should be able to fiddle out the rest by reading the driver source.

- Lost sb fixed
Active sb-bh is now released in put_super() - not before.

- dir_longfilename() return codes cleaned up

- qnx6fs.h cleaned up

- switched from for-loops to memcpy()'s in super_mmi() and iput() to copy
data from one struct to the other

- became friend with git and created the attached patch with it (hope the
commit format is now proper)

+ wanted to say "Thank you!" for the absolut fantastic work you've done on
the pagemap stuff!

Patch here:
http://a6.ontika.net/patches/0001-fs-initial-qnx6fs-addition.patch.gz
It's against vfs.fs git tree.

Kai

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Fri, Feb 17, 2012 at 04:06:19PM +0100, Kai Bankett wrote:

> - fixed broken mmi superblock handling, broken by the switch to pagemap,
> root_node etc. stuff ;)

+       bh1 = sb_bread(s, s->s_blocksize);
in there looks rather fishy...

qnx6_lfile_checksum() doesn't use superblock argument anymore (used to
be used for conversion, but since you are passing length from the
caller...)

qnx6_longname() - since you are passing superblock anyway, sbi is
redundant...

+       inode->i_blocks = (inode->i_size >> sb->s_blocksize_bits)
+                       << (sb->s_blocksize_bits - 9);
looks too convoluted and if you really want to be pedantic, not 100%
correct - you end up rounding it down, not up.

FWIW, I would probably simply do (inode->i_size + 511) >> 9 and devil
take the rounding effects on big block sizes; it's an approximation
anyway, since you are not counting indirect blocks...

Other than that (and assuming it survives your testing), I'm fine with
that variant.

Re: [PATCH]QNX6 filesystem (RO) driver

[Kai Bankett]

> On Fri, Feb 17, 2012 at 04:06:19PM +0100, Kai Bankett wrote:
>
> +       bh1 = sb_bread(s, s->s_blocksize);
> in there looks rather fishy...

Fixed.

>
> qnx6_lfile_checksum() doesn't use superblock argument anymore (used to
> be used for conversion, but since you are passing length from the
> caller...)
>
> qnx6_longname() - since you are passing superblock anyway, sbi is
> redundant...

Removed.

>
> FWIW, I would probably simply do (inode->i_size + 511) >> 9 and devil
> take the rounding effects on big block sizes; it's an approximation
> anyway, since you are not counting indirect blocks...
>

Changed. Fair enough and safes cpu cycles.

> Other than that (and assuming it survives your testing), I'm fine with
> that variant.

At least no problems with all my test images. Just ran a complete test
(file reads, dir(s), filepermissions, symlink, hardlink, longfilenames,
longdirectories etc.) over (mmi/be/le/1k/2k blocksize images).

http://a6.ontika.net/patches/0001-fs-initial-qnx6fs-addition.patch.gz

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Fri, Feb 17, 2012 at 06:53:52PM +0100, Kai Bankett wrote:
> > Other than that (and assuming it survives your testing), I'm fine with
> > that variant.
> 
> At least no problems with all my test images. Just ran a complete test
> (file reads, dir(s), filepermissions, symlink, hardlink, longfilenames,
> longdirectories etc.) over (mmi/be/le/1k/2k blocksize images).
> 
> http://a6.ontika.net/patches/0001-fs-initial-qnx6fs-addition.patch.gz

Hmmm...

+                               if (len == de->de_size)
+                                       /* normal filename */
+                                       ino = qnx6_match(s, len, name, de);
+                                       if (ino)
+                                               goto found;
+                               else if ((de->de_size == 0xff) &&

Actually, gcc ought to have screamed at that.  Note that "else" here matches
the second "if", not the first one.  And you are not guaranteed that
ino has _ever_ been assigned anything; that comparison is deep in nasal
daemon country...

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Fri, Feb 17, 2012 at 06:35:49PM +0000, Al Viro wrote:
> On Fri, Feb 17, 2012 at 06:53:52PM +0100, Kai Bankett wrote:
> > > Other than that (and assuming it survives your testing), I'm fine with
> > > that variant.
> > 
> > At least no problems with all my test images. Just ran a complete test
> > (file reads, dir(s), filepermissions, symlink, hardlink, longfilenames,
> > longdirectories etc.) over (mmi/be/le/1k/2k blocksize images).
> > 
> > http://a6.ontika.net/patches/0001-fs-initial-qnx6fs-addition.patch.gz
> 
> Hmmm...
> 
> +                               if (len == de->de_size)
> +                                       /* normal filename */
> +                                       ino = qnx6_match(s, len, name, de);
> +                                       if (ino)
> +                                               goto found;
> +                               else if ((de->de_size == 0xff) &&
> 
> Actually, gcc ought to have screamed at that.  Note that "else" here matches
> the second "if", not the first one.  And you are not guaranteed that
> ino has _ever_ been assigned anything; that comparison is deep in nasal
> daemon country...

And in the same area, what happens if you have a long entry *and* ask for
lookup for name that is exactly 255 bytes long?  AFAICS, qnx6_match() will
proceed to do memcmp() on 255 bytes - note that you pass it "len", not
"thislen".  Might (at least in theory) run out of page before it stops...

Something like
	if (len <= QNX6_SHORT_NAME_MAX) {
		if (de->de_size != len)
			continue;
		<qnx6_match>
	} else {
		if (de->de_size != 0xff)
			continue;
		<qnx6_long_match>
	}
would probably make sense...  Wait.  You've mentioned something about
special characters in names forcing a longname entry even for short
ones.  Could you elaborate?

Re: [PATCH]QNX6 filesystem (RO) driver

[Kai Bankett]

Am 02/17/2012 07:53 PM, schrieb Al Viro:
>> Hmmm...
>>
>> +                               if (len == de->de_size)
>> +                                       /* normal filename */
>> +                                       ino = qnx6_match(s, len, name, de);
>> +                                       if (ino)
>> +                                               goto found;
>> +                               else if ((de->de_size == 0xff)&&
>>
>> Actually, gcc ought to have screamed at that.  Note that "else" here matches
>> the second "if", not the first one.  And you are not guaranteed that
>> ino has _ever_ been assigned anything; that comparison is deep in nasal
>> daemon country...
At least code was robust enough to deal with it ;)
I guess ino was set to 0 anyways by compiler? At least it does not seem 
to have been sort of random.
Otherwise testing would have shown strange effects - right?
However, fixed. I just re-added the braces. At some point in time there 
were some ... seem they got lost somewhere in mod space.

> And in the same area, what happens if you have a long entry *and* ask for
> lookup for name that is exactly 255 bytes long?  AFAICS, qnx6_match() will
> proceed to do memcmp() on 255 bytes - note that you pass it "len", not
> "thislen".  Might (at least in theory) run out of page before it stops...(
>
> Something like
> 	if (len<= QNX6_SHORT_NAME_MAX) {
> 		if (de->de_size != len)
> 			continue;
> 		<qnx6_match>
> 	} else {
> 		if (de->de_size != 0xff)
> 			continue;
> 		<qnx6_long_match>
> 	}
> would probably make sense...  Wait.  You've mentioned something about
> special characters in names forcing a longname entry even for short
> ones.  Could you elaborate?
I just checked, but could not get qnx to split a short name in a long 
filename node.
So we can bury that one ... also does not really make sense to design it 
that way.

To me the length field clearly is the master. For short filenames always 
de_size < 0xff and equal the filename length.
If de_size == 0xff, filename length has to be taken out of lf node.
The interesting question is: What is in between? To me, the answer is: 
File-system error land.

Just re-pull the patch and take a look ...
http://a6.ontika.net/patches/0001-fs-initial-qnx6fs-addition.patch.gz

BTW, I had a look at character and block device files. QNX drivers just 
dynamically add them to /dev if a driver offers any.
There seems to be no way to create or move any by hand + they are not 
written to the fs.

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Fri, Feb 17, 2012 at 10:38:12PM +0100, Kai Bankett wrote:
> At least code was robust enough to deal with it ;)
> I guess ino was set to 0 anyways by compiler? At least it does not
> seem to have been sort of random.

Nope.  Uninitialized is uninitialized - in reality it's "whatever happens
to be in that word on stack", in theory it's "compiler has every right
to do _anything_ to reads from it".  Nasal daemon country...

> Otherwise testing would have shown strange effects - right?

Depends.

> http://a6.ontika.net/patches/0001-fs-initial-qnx6fs-addition.patch.gz

Still a problem with memcmp() use in qnx6_long_match() - suppose you've
got lf_size (on corrupt fs) set to 3Kb.  With 2Kb blocks and odd
->de_long_inode.  You are doing a lookup for 3Kb name.  What we get is
qnx6_longname() returning a pointer 2Kb into 4Kb page.  And qnx6_long_match()
proceeding to do memcmp() on 3Kb starting from that point.  Oops - literally.

What we need to do is either
	* silently return 0 on thislen > QNX6_LONG_NAME_MAX, or
	* return 0 and yell about corrupt /longfiles, or
	* have qnx6_lookup() check that len <= QNX6_LONG_NAME_MAX and
return ERR_PTR(-ENAMETOOLONG) if it isn't.  And lose the thislen check
completely.

I'd probably go for the last variant; it's consistent with what other
filesystems are doing and it would avoid mess if we ever try to do
r/w variant.  BTW, thislen check in qnx6_match() is also pointless -
we'd already checked that in caller.  Otherwise it would be wrong in
the same way as for long ones...

Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro]

On Tue, Feb 21, 2012 at 10:04:55PM +0000, Al Viro wrote:
> What we need to do is either
> 	* silently return 0 on thislen > QNX6_LONG_NAME_MAX, or
> 	* return 0 and yell about corrupt /longfiles, or
> 	* have qnx6_lookup() check that len <= QNX6_LONG_NAME_MAX and
> return ERR_PTR(-ENAMETOOLONG) if it isn't.  And lose the thislen check
> completely.
> 
> I'd probably go for the last variant; it's consistent with what other
> filesystems are doing and it would avoid mess if we ever try to do
> r/w variant.

Oh, wait - you already are doing that in ->lookup()?  Then it's definitely
"let's lose thislen checks in qnx6_long_match() and qnx6_match()".  Dead
code is dead code...

Pushed with that correction.

Re: [PATCH]QNX6 filesystem (RO) driver

[Kai Bankett]

> On Tue, Feb 21, 2012 at 10:04:55PM +0000, Al Viro wrote:
>> What we need to do is either
>> 	* silently return 0 on thislen > QNX6_LONG_NAME_MAX, or
>> 	* return 0 and yell about corrupt /longfiles, or
>> 	* have qnx6_lookup() check that len <= QNX6_LONG_NAME_MAX and
>> return ERR_PTR(-ENAMETOOLONG) if it isn't.  And lose the thislen check
>> completely.
>>
>> I'd probably go for the last variant; it's consistent with what other
>> filesystems are doing and it would avoid mess if we ever try to do
>> r/w variant.
>
> Oh, wait - you already are doing that in ->lookup()?  Then it's definitely
> "let's lose thislen checks in qnx6_long_match() and qnx6_match()".  Dead
> code is dead code...
>
Correct - dead code.
Removed in the patch... (same location as before)

A r/o driver to me always looks like "giving up half the way". (ok, maybe
just one third the way)
Well, I'm sure I've collected enough fs information for impelemting r/w.
However, doing that still means a whole bunch of work.
Already started with some parts, but that was still completely based on
buffer_head stuff.

My suggestion would be to do it in smaller incremental steps. (e.g. start
with Bitmap handling stuff and then move on)
At least I spent a lot of time on analyzing and understanding how that
bitmap system area is used any calculated.
I'd prefer to dump it soon out of my brain to get freed up some space ;)

It would be very helpful to get some ideas on how to implement that
failsafe stuff properly. (QNX calls it CoW - Copy on Write)
I've described it's basic mechanics in qnx6.txt.
I'm just - let's carefully say - clueless on how to approach that beast
properly.
Maybe first just implement simple write support (just on the active
superblock) and then later - if still not tired - extend the failsafe
support.

Kai