Re: [PATCH V2 1/3] quota: Add reservation support for delayed block allocation

[Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>]

On Fri, 31 Oct 2008 14:27:22 -0700
Mingming Cao <cmm@xxxxxxxxxx> wrote:

> +int dquot_alloc_space(struct inode *inode, qsize_t number, int warn)
> +{
> +	int cnt, ret = QUOTA_OK;
> +
> +	/*
> +	 * First test before acquiring mutex - solves deadlocks when we
> +         * re-enter the quota code and are already holding the mutex
> +         */
> +	if (IS_NOQUOTA(inode)) {
> +		inode_add_bytes(inode, number);
> +		return ret;
> +	}
> +
> +	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> +	if (IS_NOQUOTA(inode))	{
> +		/* Now we can do reliable test... */
> +		up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> +		inode_add_bytes(inode, number);
> +		return ret;
> +	}
> +
> +	ret = __dquot_alloc_space(inode, number, warn, 0);
> +	if (ret == NO_QUOTA) {
> +		up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> +		return ret;
> +	}
> +
> +	/* Dirtify all the dquots - this can block when journalling */
> +	for (cnt = 0; cnt < MAXQUOTAS; cnt++)
> +		if (inode->i_dquot[cnt])
> +			mark_dquot_dirty(inode->i_dquot[cnt]);
> +	up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> +	inode_add_bytes(inode, number);
> +	return ret;
> +}

I'm going to have to call "ug" on that code.

Multiple return points per function really is a maintenance problem. 
It's a great source of code duplication, locking errors and resource
leaks as the code evolves.

Can we please rework this code (and any other similar code here) to use
the usual `goto out' pattern?


int dquot_alloc_space(struct inode *inode, qsize_t number, int warn)
{
	int cnt, ret = QUOTA_OK;

	/*
	 * First test before acquiring mutex - solves deadlocks when we
         * re-enter the quota code and are already holding the mutex
         */
	if (IS_NOQUOTA(inode))
		goto out;

	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
	if (IS_NOQUOTA(inode))	/* Now we can do reliable test... */
		goto out_unlock;

	ret = __dquot_alloc_space(inode, number, warn, 0);

	if (ret == NO_QUOTA) {
		up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
		return ret;
	}

<<

I don't know what to do here - did we really want to skip the
inode_add_bytes?  If so, we could do

	number = 0;
	goto out_unlock;

>>

	/* Dirtify all the dquots - this can block when journalling */
	for (cnt = 0; cnt < MAXQUOTAS; cnt++)
		if (inode->i_dquot[cnt])
			mark_dquot_dirty(inode->i_dquot[cnt]);
out_unlock:
	up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
out:
	inode_add_bytes(inode, number);
	return ret;
}

or something like that.


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html