Re: [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge

[Dmitry Monakhov <dmonakhov@openvz.org>]

Jan Kara <jack@suse.cz> writes:

> On Thu 08-04-10 22:04:22, Dmitry Monakhov wrote:
>> Due to previous IO error or other bugs an inode may not has quota
>> pointer. This definite sign of quota inconsistency/corruption.
>> In fact this condition must being checked in all charge/uncharge
>> functions. And if error was detected it is reasonable to fail whole
>> operation. But uncharge(free_space/claim_space/free_inode) functions
>> has no fail nature. This is because caller can't handle errors from
>> such functions. How can you handle error from following call-trace?
>> write_page()->quota_claim_space()
>> 
>> So alloc_space() and alloc_inode() are the only functions which we may
>> reliably abort in case of any errors.
>> 
>> This patch add corresponding checks only for charge functions.
>> 
>> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
>> ---
>>  fs/quota/dquot.c |   39 +++++++++++++++++++++++++++++++++------
>>  1 files changed, 33 insertions(+), 6 deletions(-)
>> 
>> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
>> index 3f4541e..7480e03 100644
>> --- a/fs/quota/dquot.c
>> +++ b/fs/quota/dquot.c
>> @@ -1529,13 +1529,27 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number,
>>  	}
>>  
>>  	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
>> +	/* Now recheck reliably when holding dqptr_sem */
>> +	if (!(active = sb_any_quota_active(inode->i_sb)) || IS_NOQUOTA(inode)) {
>   Please don't use assignment in conditions... It's against kernel's coding
> style.
>
>> +		inode_incr_space(inode, number, reserve);
>> +		goto out;
>> +	}
>> +
>>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++)
>>  		warntype[cnt] = QUOTA_NL_NOWARN;
>>  
>>  	spin_lock(&dq_data_lock);
>>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
>> -		if (!inode->i_dquot[cnt])
>> +		if (!(active & (1 << cnt)))
>>  			continue;
>> +		/*
>> +		 * Given quota type is active, so quota must being
>> +		 * initialized for this inode
>> +		 */
>> +		if (!inode->i_dquot[cnt]) {
>> +			ret = -EIO;
>   Umm, I don't like this. I think we should just silently skip the quota
> modification. The error has already been passed to the filesystem during
> dquot_initialize and this would effectively disallow any allocation on the
> filesystem if quota structure isn't available which might be a bit
> impractical.
  In fact in many places ret code from dquot_initialize() is simply
  ignored, because before this patch-set init was void, so it takes
  significant amount of time to fix all callers. And charge callers are
  usually do care about error code, so IMHO we have to signal what
  quota is broken if it is actually broken.
  Another example: If filesystem becomes RO (due to journal abort, and etc)
  we do care about this, and check that condition in almost all places and
  return appropriate error, instead of simply skip write() syscall.
  User is able to disable quota if it becomes broken, and continue with
  hope that this is just an quota issue, but not disk failure :)