Hi Christoph, Thank you for prompt response. Yes, please tell me what I should do to complete this successfully. In the mean time I will look through the rest of your reply. I couldn't do this yesterday (drove 500km away from city I live in). And my 1st thought of vxfs_fill_super() is that it should check sb magic at two places +1k in case of SCO Unixware and +8k for HP-UX vxfs image to preserve compatibility. Also what is on-disk format of data written by SCO ? HP-UX 10.20 writes everything big endian. (guess it's native to pa-risc 1.1) The information can be utilized later to fine tune more corner case issues with these two images of vxfs. e.g. for vxfs_bmap_ext4() Best regards On Mon, 2016-05-23 at 01:36 -0700, Christoph Hellwig wrote: > 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. -- Krzysztof Blaszkowski -- 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
