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

On Tue, 2008-11-04 at 17:14 -0800, Andrew Morton wrote:
> On Fri, 31 Oct 2008 14:27:22 -0700
> Mingming Cao <cmm@us.ibm.com> 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?
> 
> 

Ok, I was following the same style in the quota code, but I will do
that.
> 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?  

Yes, this is the failure case. In case the request exceeds the quota
limit, we will quit with error without block allocation, so the inode
bytes update should be skipped.

> 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.
> 
> 

Attached is the incremental fixes. I will post the  updated series
later.

Thanks,
---
 fs/dquot.c               |   40 +++++++++++++++++-----------------------
 include/linux/quotaops.h |    2 +-
 2 files changed, 18 insertions(+), 24 deletions(-)






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