Re: freevxfs: hp-ux support. patchset 1-7/7 rev 2

[Christoph Hellwig <hch@infradead.org>]

Hi Krzysztof,

> I can check but here are my remarks:
> the dip2vip_cpy() does partial byte-swap from file system endian to cpu
> endian, so the union uses fs native endianess and this raises difficulty
> of using struct vxfs_inode_info to yet another level.
> Is this a good practice ? (apart from a benefit like some minor speed
> gain)

It seems easier this way around.  If you think byte swapping makes
life easier for you I'm happy to take an incremental patch.  Howerver
that means we'll need separate structure defintions for the union
members.

> Are there any real benefits from marking anything "bitwise" except that
> it is just another type ?

bitwise is an annotation for sparse so that you can't directly assign
to the variable, and need to use accessors instead.  In this case it's
used so that we are forced to use the byte swapping helpers and get a
warning from sparse if that's not done properly.

> 
> because for some strange reason the patch uses "PAGE_SHIFT" while
> original lookup.c from 3.16 or 3.18 kernels uses PAGE_CACHE_SHIFT but
> 4.6 kernel. so the patch below does not apply cleanly to source from 3.x
> kernels. I use 3.16, double checked latest 3.18.

Yes, PAGE_CACHE_SIZE, PAGE_CACHE_SHIFT and co were removed in kernel
4.6, as they have always been identical to the versions without _CACHE.

> Anyway because readdir() is not aware of fs endianess (because 0004
> patch is not in there) the hp-ux tests will fail with some general
> protection fault very likely.

I think all readdir structures are fixed up now, please check.

> I can't see also patches which fix these obvious bugs like freeing
> kmem_cache_alloc() objects with kfree.
> Even worse is releasing inode->i_private from the "evict_inode"
> callback.
> This leads to dangling objects in vxfs's kmem_cache when the fs is
> unmounted. Destroying cache on module unload causes kmem_cache_destroy
> to complain about it and after a few tries ((module load, mount, some
> ops on mountpoint, unmount, unload) x few times) hard lockup is
> inevitable.

Yeah, this was just getting started.  I've spent some more time on the
whole inode issues and have implemented this a bit different from what
you did, although the end result is very similar.  Can you take a look
at the tree here:

	http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/freevxfs

> Do you think other patches should be updated with regard to the patch
> you sent ?

Please take a look at the branch above.  The only major thing that
should be missing is your directory code refactoring.