Re: [PATCH 6/8] exofs: super_operations and file_system_type

[Evgeniy Polyakov <zbr@ioremap.net>]

Hi.

On Mon, Feb 09, 2009 at 03:25:53PM +0200, Boaz Harrosh (bharrosh@panasas.com) wrote:
> +static int parse_options(char *options, struct exofs_mountopt *opts)
> +{
> +	char *p;
> +	substring_t args[MAX_OPT_ARGS];
> +	int option;
> +	bool s_pid = false;
> +
> +	EXOFS_DBGMSG("parse_options %s\n", options);
> +	/* defaults */
> +	memset(opts, 0, sizeof(*opts));
> +	opts->timeout = BLK_DEFAULT_SG_TIMEOUT;
> +
> +	while ((p = strsep(&options, ",")) != NULL) {
> +		int token;
> +		char str[32];
> +
> +		if (!*p)
> +			continue;
> +
> +		token = match_token(p, tokens, args);
> +		switch (token) {
> +		case Opt_pid:
> +			if (0 == match_strlcpy(str, &args[0], sizeof(str)))
> +				return -EINVAL;
> +			opts->pid = simple_strtoull(str, NULL, 0);
> +			if (opts->pid < EXOFS_MIN_PID) {
> +				EXOFS_ERR("Partition ID must be >= %u",
> +					  EXOFS_MIN_PID);
> +				return -EINVAL;
> +			}
> +			s_pid = 1;
> +			break;
> +		case Opt_to:
> +			if (match_int(&args[0], &option))
> +				return -EINVAL;
> +			if (option <= 0) {
> +				EXOFS_ERR("Timout must be > 0");
> +				return -EINVAL;
> +			}
> +			opts->timeout = option * HZ;

Is it intentional to be a different timeouton systems with different HX
but the same mount option?

> +static struct inode *exofs_alloc_inode(struct super_block *sb)
> +{
> +	struct exofs_i_info *oi;
> +
> +	oi = kmem_cache_alloc(exofs_inode_cachep, GFP_KERNEL);

I'm curious if this should be GFP_NOFS or not?

> +	if (!oi)
> +		return NULL;
> +
> +	oi->vfs_inode.i_version = 1;
> +	return &oi->vfs_inode;
> +}

> +static void exofs_put_super(struct super_block *sb)
> +{
> +	int num_pend;
> +	struct exofs_sb_info *sbi = sb->s_fs_info;
> +
> +	/* make sure there are no pending commands */
> +	for (num_pend = atomic_read(&sbi->s_curr_pending); num_pend > 0;
> +	     num_pend = atomic_read(&sbi->s_curr_pending)) {

This rises a question. Let's check exofs_new_inode() for example (it is
a bad example, since inode can not be created when we already in the
put_super() callback, but still there are others), it increments
s_curr_pending way after inode was created, so is it possible that
some in-flight callback is about to be executed and its subsequent
s_curr_pending manipulation will not be detected by this loop?

Should s_curr_pending increment be audited all over the code to be
increased before the potential postponing command starts (which is not
the case in exofs_new_inode() above)?

> +		wait_queue_head_t wq;
> +		init_waitqueue_head(&wq);
> +		wait_event_timeout(wq,
> +				  (atomic_read(&sbi->s_curr_pending) == 0),
> +				  msecs_to_jiffies(100));
> +	}
> +
> +	osduld_put_device(sbi->s_dev);
> +	kfree(sb->s_fs_info);
> +	sb->s_fs_info = NULL;
> +}

-- 
	Evgeniy Polyakov