Re: freevxfs

[Christoph Hellwig <hch@infradead.org>]

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.