Re: [PATCH]QNX6 filesystem (RO) driver

[Kai Bankett <chaosman@ontika.net>]

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>