Re: [RFC][patch 4/7] v9fs: VFS superblock operations (2.0-rc6)

[Pekka Enberg <penberg@gmail.com>]

Hi,

Some more comments follow.

On 5/24/05, ericvh@gmail.com <ericvh@gmail.com> wrote:
> +static int __init init_v9fs(void)
> +{
> +	int error;
> +
> +	v9fs_error_init();
> +
> +	printk(KERN_INFO "Installing v9fs 9P2000 file system support\n");
> +
> +	if ((error = register_filesystem(&v9fs_fs_type))) {
> +		printk(KERN_ERR "Could not register v9fs\n");
> +		return error;
> +	}
> +
> +	v9fs_rpcreq_slab = kmem_cache_create("v9fs_rpcreq_info",
> +					     sizeof(struct v9fs_rpcreq), 0,
> +					     SLAB_HWCACHE_ALIGN, NULL, NULL);
> +
> +	if (!v9fs_rpcreq_slab) {
> +		eprintk(KERN_WARNING, "Couldn't allocate v9fs_rpcreq cache\n");
> +		return -1;

Module initialization fails and you don't do unregister_filesystem. Please
shuffle the code so that you do all other initialization _before_ calling
register_filesystem. And don't forget to release the slab on error.

> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * v9fs_init - shutdown module
> + *
> + */
> +
> +static void __exit exit_v9fs(void)
> +{
> +	struct list_head *p, *n;
> +	struct v9fs_slab *s;
> +
> +	list_for_each_safe(p, n, &v9fs_slab_list) {
> +		s = list_entry(p, struct v9fs_slab, list);
> +		kmem_cache_destroy(s->slab);
> +		list_del(&s->list);
> +	}
> +
> +	kmem_cache_destroy(v9fs_rpcreq_slab);
> +	unregister_filesystem(&v9fs_fs_type);

This looks racy. You should unregister the filesystem before releasing its
resources.

> +}
> +
> +module_init(init_v9fs)
> +module_exit(exit_v9fs)
> +
> +MODULE_AUTHOR("Eric Van Hensbergen <ericvh@gmail.com>");
> +MODULE_AUTHOR("Ron Minnich <rminnich@lanl.gov>");
> +MODULE_LICENSE("GPL");

> Index: fs/9p/vfs_super.c
> ===================================================================
> --- /dev/null  (tree:0bf32353105286a5624aeea862d35a4bbae09851)
> +++ 178666ee376655ef8ec19a2ffc0490241b428110/fs/9p/vfs_super.c  (mode:100644)
> +static struct super_block *v9fs_get_sb(struct file_system_type
> +				       *fs_type, int flags,
> +				       const char *dev_name, void *data)
> +{
> +	struct super_block *sb = NULL;
> +	struct v9fs_fcall *fcall = NULL;
> +	struct inode *inode = NULL;
> +	struct dentry *root = NULL;
> +	struct v9fs_session_info *v9ses = NULL;
> +	struct v9fs_fid *root_fid = NULL;
> +	int mode = S_IRWXUGO | S_ISVTX;
> +	uid_t uid = current->fsuid;
> +	gid_t gid = current->fsgid;
> +	int stat_result = 0;
> +	int newfid = 0;
> +
> +	dprintk(DEBUG_VFS, " \n");
> +
> +	v9ses = kmalloc(sizeof(struct v9fs_session_info), GFP_KERNEL);
> +	if (!v9ses)
> +		return ERR_PTR(-ENOMEM);
> +	memset(v9ses, 0, sizeof(struct v9fs_session_info));

Please consider using kcalloc().

> +
> +	if ((newfid = v9fs_session_init(v9ses, dev_name, data)) < 0) {
> +		dprintk(DEBUG_ERROR, "problem initiating session\n");
> +		sb = ERR_PTR(newfid);
> +		goto free_session;
> +	}
> +
> +	sb = sget(fs_type, NULL, v9fs_set_super, v9ses);
> +
> +	sb->s_maxbytes = MAX_LFS_FILESIZE;
> +	sb->s_blocksize =
> +	    v9fs_block_bits(v9ses->maxdata, &sb->s_blocksize_bits);
> +	sb->s_magic = V9FS_WIRE_MAGIC;
> +	sb->s_op = &v9fs_super_ops;

Please consider extracting these to a v9fs_fill_super() function to follow
the style set by other filesystems.

> +
> +	sb->s_flags |=
> +	    MS_ACTIVE | MS_SYNCHRONOUS | MS_DIRSYNC | MS_NODIRATIME |
> +	    MS_NOATIME;
> +
> +	inode = v9fs_get_inode(sb, S_IFDIR | mode);
> +	if ((!inode) || IS_ERR(inode)) {
> +		sb = ERR_PTR(-EINVAL);

It is a good idea to return the same error (to propagate the error code)
but in this case it seems a lot simpler to make v9fs_get_inode always return
NULL on failure. You are being bit too defensive in your programming for
the kernel in other places as well.

> +
> +/**
> + * v9fs_kill_super - Kill Superblock
> + * @s: superblock
> + * 
> + */
> +
> +static void v9fs_kill_super(struct super_block *s)
> +{
> +	struct v9fs_session_info *v9ses =
> +	    (struct v9fs_session_info *)s->s_fs_info;

Redundant cast.

		Pekka