Re: [PATCH 4/4] xfs: per-type quota timers and warn limits

From: Eric Sandeen <sandeen@redhat.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: linux-xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH 4/4] xfs: per-type quota timers and warn limits
Date: Mon, 17 Feb 2020 10:03:59 -0600	[thread overview]
Message-ID: <0ed308e8-6a66-c43c-c94d-5ee782781aa3@redhat.com> (raw)
In-Reply-To: <20200217134339.GI31012@infradead.org>

On 2/17/20 7:43 AM, Christoph Hellwig wrote:
>>  	struct xfs_disk_dquot	*d = &dq->q_core;
>> +	struct xfs_def_quota	*defq;
>> +
>>  	ASSERT(d->d_id);
>> +	defq = xfs_get_defquota(dq);
> 
> Move up to the declaration line?

*shrug* ok

>> +	switch (type) {
>> +	case XFS_DQ_USER:
>> +		defq = &qinf->qi_usr_default;
>> +		break;
>> +	case XFS_DQ_GROUP:
>> +		defq = &qinf->qi_grp_default;
>> +		break;
>> +	case XFS_DQ_PROJ:
>> +		defq = &qinf->qi_prj_default;
>> +		break;
>> +	default:
>> +		ASSERT(0);
>> +		/* fall through */
>> +	}
> 
> Should this go into a helper?  Or even better replace the
> qi_*default members with an array that the type can index into?

Like maybe xfs_get_defquota() ;)

Not sure what I was thinking here, whoops.

>> @@ -592,39 +609,31 @@ xfs_qm_init_timelimits(
>>  	 *
>>  	 * Since we may not have done a quotacheck by this point, just read
>>  	 * the dquot without attaching it to any hashtables or lists.
>> -	 *
>> -	 * Timers and warnings are globally set by the first timer found in
>> -	 * user/group/proj quota types, otherwise a default value is used.
>> -	 * This should be split into different fields per quota type.
>>  	 */
>> -	if (XFS_IS_UQUOTA_RUNNING(mp))
>> -		type = XFS_DQ_USER;
>> -	else if (XFS_IS_GQUOTA_RUNNING(mp))
>> -		type = XFS_DQ_GROUP;
>> -	else
>> -		type = XFS_DQ_PROJ;
>>  	error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
>>  	if (error)
>>  		return;
>>  
>>  	ddqp = &dqp->q_core;
>> +	defq = xfs_get_defquota(dqp);
> 
> Isn't the defq variable already initialized earlier in the function?

Oh, uh ... whoops.  Not sure how I messed that up, thanks.

-Eric