Re: [PATCH 1/3] RFC quota: add generic quota error handlers.

[Dmitry Monakhov <dmonakhov@openvz.org>]

Jan Kara <jack@suse.cz> writes:

> On Wed 31-03-10 13:00:35, Dmitry Monakhov wrote:
>> @@ -1023,8 +1022,12 @@ static inline void dquot_resv_space(struct dquot *dquot, qsize_t number)
>>   */
>>  static void dquot_claim_reserved_space(struct dquot *dquot, qsize_t number)
>>  {
>> -	if (dquot->dq_dqb.dqb_rsvspace < number) {
>> -		WARN_ON_ONCE(1);
>> +	if (unlikely(dquot->dq_dqb.dqb_rsvspace < number)) {
>> +		quota_error(dquot->dq_sb, "Incorrect quota reservation "
>> +			"for quota id:%d, rsvspace:%lld, claim:%lld, "
>> +			"thus quota information is probably inconsistent. "
>> +			"Please run quotacheck(8).",
>> +			dquot->dq_id, dquot->dq_dqb.dqb_rsvspace, number);
>>  		number = dquot->dq_dqb.dqb_rsvspace;
>>  	}
>   We shouldn't spam syslog with errors about quotas (which might happen
> once quota gets inconsistent). So we could maybe warn at most once per
> dquot structure (by using an error flag in dq_flags) or even once per
> superblock + type (by using flags in info) - using info would also have
> the advantage that it would be also usable for checks about inodes.
Ok i'll stick to dquot->dq_flags approach like follows:
int quota_handle_error(struct super_block *sb, struct dquot *dquot)
{
        int ret;
        ret = test_and_set_bit(_DQUOT_ERROR, &sb_dqopt(sb)->flags);
        if (dquot)
                ret = test_and_set_bit(DQ_ERROR_B, dquot->dq_flags);
        return ret;
}
and use it like this.
if (!quota_handle_error(sb, dquot) || force_dump)
   dump_error(...)
>
>> diff --git a/fs/quota/quota.c b/fs/quota/quota.c
>> index 95388f9..ebdce30 100644
>> --- a/fs/quota/quota.c
>> +++ b/fs/quota/quota.c
>> @@ -19,6 +19,41 @@
>>  #include <linux/types.h>
>>  #include <linux/writeback.h>
>>  
>> +static void quota_handle_error(struct super_block *sb)
>> +{
>> +
>> +	set_bit(_DQUOT_ERROR, &sb_dqopt(sb)->flags);
>> +	/* XXX: Currently it is no impossible to signall fs about error */
>> +}
>   I guess we can make all quota allocation and freeing functions to return
> an error (EIO) if they spot some problem. That should be enough for a
> filesystem to find out something is wrong...
This result significant paradigm changes, because almost all fs-related
syscalls result int quota alloc/free which return -EIO (i.e in almost
unusable filesystem). Users with incorrect system-scripts will hate us
for this.
>
>> +void __quota_error(struct super_block * sb, const char * function,
>> +		const char * fmt, ...)>> +{
>> +	va_list args;
>> +
>> +	va_start(args, fmt);
>> +	printk(KERN_CRIT "QUOTA: error (device %s): %s: ",sb->s_id, function);
>                                                           ^ space after ','
>
>> +void __quota_warning(struct super_block * sb, const char * func,
>> +		const char * fmt, ...)
>> +{
>> +	va_list args;
>> +
>> +	va_start(args, fmt);
>> +	printk(KERN_WARNING "QUOTA: warning (device %s): %s: ",sb->s_id, func);
>                                                                ^ space
>
>> diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
>> index f81f4bc..da0de94 100644
>> --- a/fs/quota/quota_tree.c
>> +++ b/fs/quota/quota_tree.c
>> @@ -44,7 +44,7 @@ static char *getdqbuf(size_t size)
>>  	char *buf = kmalloc(size, GFP_NOFS);
>>  	if (!buf)
>>  		printk(KERN_WARNING
>> -		       "VFS: Not enough memory for quota buffers.\n");
>> +		       "VFS: Not enough memory for quota buffers.");
>   Why have you removed '\n'?
>
> 								Honza