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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
