Hi Krzysztof, thanks for doing this work. I did the vxfs work for SCO Unixware file systems and never even looked at a HP-UX system. Good to know that it's useful for HP-UX as well with a few changes. I'd love to merged it, but we should go through it a bit and make it fit our normal kernel process and style. I'm happy to help you on the list or in personal mails with that. Initial comments below: > index c8a9265..9890a84 100644 > --- a/fs/freevxfs/vxfs.h > +++ b/fs/freevxfs/vxfs.h > @@ -2,6 +2,39 @@ > * Copyright (c) 2000-2001 Christoph Hellwig. > * All rights reserved. > * > + * > + * (c) 2016 Krzysztof Blaszkowski. Just add your copyrights on the top next to mine. > + * Many bug fixes, improvements & tests. > + * > + * These bugs and improvements were as follows: > + * - code not aware of cpu endianess and ondisk data is BE. > + * - misaligned structures read from block device > + * - wrong SB block number. default offset is 8kB > + * - kmem_cache_alloc() objectes released with kfree() > + * - inode.i_private released in evict_inode() callback. > + * - refactored vxfs_readdir() and vxfs_find_entry() > + * > + * Tests were performed with image of HP 9000/779 disk (/ lvol) > + * Example: */ > +// * cksum mnt/usr/share/man/man3.Z/* > +// * cksum mnt/usr/share/doc/10.20RelNotes > +// * cksum mnt/usr/local/doom/* > +// * cksum mnt/usr/sprockets/tools/instrument/16700/* > +// * cksum mnt/usr/share/doc/* > +// * cksum mnt/usr/sprockets/lib/* > +// * cksum mnt/usr/sprockets/bin/* > +/* > + * Needles to say that checksums of files match these evaluated by > + * HP-UX B.10.20 cksum. E.g.: > + * 3457951056 4196020 /usr/local/doom/doom1.wad > + * 2527157998 35344 /usr/local/doom/doomlaunch > + * 2974998129 413696 /usr/local/doom/hpdoom This just belongs into the git commit log and not into the file. > + * The hpux_mdsetup tool project which is aimed at making possible > + * accessing HP-UX logical volumes by device mapper is here: > + * https://sourceforge.net/projects/linux-vxfs/ > + * I think this should just go into the Kconfig help text, or a documentation text file if we want to add one. > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions > * are met: > @@ -38,6 +71,10 @@ > */ > #include <linux/types.h> > > +//#define DIAGNOSTIC > +#undef DIAGNOSTIC > +#undef DIAGNOSTIC_V2 > +#undef DIAGNOSTIC_V3 > > /* > * Data types for use with the VxFS ondisk format. > @@ -152,7 +189,7 @@ struct vxfs_sb { > /* > * Actually much more... > */ > -}; > +} __attribute__((packed)); Can you explain why we need the packed annoation? It should probably be a patch on it's own and use __packed. > +#ifdef DIAGNOSTIC > +#define F_ENTER(fmt, arg...) printk("%s:%d ENTER. " fmt "\n", __FUNCTION__, __LINE__, ##arg) > +#define F_EXIT(fmt, arg...) printk("%s:%d EXIT. " fmt "\n", __FUNCTION__, __LINE__, ##arg) > + > +#ifdef DIAGNOSTIC_V2 > +#define F_ENTER_V2(fmt, arg...) printk("%s:%d ENTER. " fmt "\n", __FUNCTION__, __LINE__, ##arg) > +#define F_EXIT_V2(fmt, arg...) printk("%s:%d EXIT. " fmt "\n", __FUNCTION__, __LINE__, ##arg) > +#else > +#define F_ENTER_V2(a, b...) > +#define F_EXIT_V2(a, b...) > +#endif > + > +#ifdef DIAGNOSTIC_V3 > +#define F_ENTER_V3(fmt, arg...) printk("%s:%d ENTER. " fmt "\n", __FUNCTION__, __LINE__, ##arg) > +#define F_EXIT_V3(fmt, arg...) printk("%s:%d EXIT. " fmt "\n", __FUNCTION__, __LINE__, ##arg) > +#else > +#define F_ENTER_V3(a, b...) > +#define F_EXIT_V3(a, b...) > +#endif Have you looked into ftrace for function tracing? I'd prefer not to add all these macros if we can avoid it. > for (i = 0; i < VXFS_NDADDR; i++) { > - struct direct *d = vip->vii_ext4.ve4_direct + i; > - if (bn >= 0 && bn < d->size) > - return (bn + d->extent); > + struct direct *d = vip->vii_ext4.ve4_direct + i; // cpu endian no // comments for Linux kernel code please. Also we should not need comments about endianess. Instead we should use sparse annotations (see Documentation/sparse.txt for details). We also have a few examples for code that supports both little and big endian in a single driver that way - take a look at the end of fs/sysv/sysv.h for an example. > - bno = indir[(bn/indsize) % (indsize*bn)] + (bn%indsize); > + bno = be32_to_cpu(indir[(bn/indsize) % (indsize*bn)]) + (bn%indsize); This would break the existing Unixware support - we'll need helpers like the one I just mentioned above instead. > @@ -340,21 +436,61 @@ vxfs_iget(struct super_block *sbp, ino_t ino) > static void vxfs_i_callback(struct rcu_head *head) > { > struct inode *inode = container_of(head, struct inode, i_rcu); > - kmem_cache_free(vxfs_inode_cachep, inode->i_private); > + void *priv = inode->i_private; > + > + inode->i_private = NULL; > + // just in case the same inode was used elsewhere after releasing i_private. > + // if it was then dereferencing NULL is far better than using invalid > + // pointer to memory claimed by something. > + kmem_cache_free(vxfs_inode_cachep, priv); > +} > + > +void vxfs_destroy_inode(struct inode *ip) > +{ > + call_rcu(&ip->i_rcu, vxfs_i_callback); > +} > + > +void vxfs_inode_info_free(struct vxfs_inode_info *vip) > +{ > + kmem_cache_free(vxfs_inode_cachep, vip); > } Can you explain these changes? Being able to explain each change in the changelog is one of the reasons why we prefer multiple small changes, one for each issue. > +{ > + int rc = 0; > + > + if (!setup) { > + vxfs_inode_cachep = kmem_cache_create("vxfs_inode", > + sizeof(struct vxfs_inode_info), 0, > + SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD, NULL); > + > + if (!vxfs_inode_cachep) > + rc = -ENOMEM; > + } else { > + /* > + * Make sure all delayed rcu free inodes are flushed before we > + * destroy cache. > + */ > + rcu_barrier(); > + kmem_cache_destroy(vxfs_inode_cachep); > + } > + > + return rc; > +} I don't think a function that does two different things depending on the argument is a good idea. I suspect you added it to keep vxfs_inode_cachep static in this file? I'm fine with that in general, but please add two function for it then, and split it into a separate patch. > #include <linux/stat.h> > #include <linux/vfs.h> > #include <linux/mount.h> > +#include <linux/byteorder/generic.h> Please use <asm/byteorder.h> instead. -- 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
