Re: freevxfs

["Krzysztof Błaszkowski" <kb@sysmikro.com.pl>]

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