On 7/1/20 12:51 PM, Darrick J. Wong wrote:
> On Wed, Jul 01, 2020 at 09:56:21AM +0100, Christoph Hellwig wrote:
>> On Tue, Jun 30, 2020 at 08:43:20AM -0700, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>>>
>>> Refactor the open-coded test for whether or not we're over quota.
>>>
>>> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>>> ---
>>> fs/xfs/xfs_dquot.c | 95 ++++++++++++++++------------------------------------
>>> 1 file changed, 30 insertions(+), 65 deletions(-)
>>>
>>>
>>> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
>>> index 35a113d1b42b..ef34c82c28a0 100644
>>> --- a/fs/xfs/xfs_dquot.c
>>> +++ b/fs/xfs/xfs_dquot.c
>>> @@ -97,6 +97,33 @@ xfs_qm_adjust_dqlimits(
>>> xfs_dquot_set_prealloc_limits(dq);
>>> }
>>>
>>> +/*
>>> + * Determine if this quota counter is over either limit and set the quota
>>> + * timers as appropriate.
>>> + */
>>> +static inline void
>>> +xfs_qm_adjust_res_timer(
>>> + struct xfs_dquot_res *res,
>>> + struct xfs_def_qres *dres)
>>> +{
>>> + bool over;
>>> +
>>> +#ifdef DEBUG
>>> + if (res->hardlimit)
>>> + ASSERT(res->softlimit <= res->hardlimit);
>>> +#endif
>>
>> Maybe:
>> ASSERRT(!res->hardlimit || res->softlimit <= res->hardlimit);
>
> Changed.
>
>>
>>> +
>>> + over = (res->softlimit && res->count > res->softlimit) ||
>>> + (res->hardlimit && res->count > res->hardlimit);
>>> +
>>> + if (over && res->timer == 0)
>>> + res->timer = ktime_get_real_seconds() + dres->timelimit;
>>> + else if (!over && res->timer != 0)
>>> + res->timer = 0;
>>> + else if (!over && res->timer == 0)
>>> + res->warnings = 0;
>>
>> What about:
>>
>> if ((res->softlimit && res->count > res->softlimit) ||
>> (res->hardlimit && res->count > res->hardlimit)) {
>> if (res->timer == 0)
>> res->timer = ktime_get_real_seconds() + dres->timelimit;
>> } else {
>> if (res->timer)
>> res->timer = 0;
>> else
>> res->warnings = 0;
>> }
>
> I don't care either way, but the last time I sent this patch out, Eric
> and Amir seemed to want a flatter if structure:
>
> https://lore.kernel.org/linux-xfs/b979d33d-361b-88cd-699c-7e5f1c621698@xxxxxxxxxxx/
> https://lore.kernel.org/linux-xfs/CAOQ4uxiveTQu8_7UOvN07=P4o9hBBZTCyu4sSw5UpbrNPQL2pQ@xxxxxxxxxxxxxx/
>
> Granted that was before I pulled the whole thing into a separate helper
> function, so maybe the context is different here...?
I think it is different. I'm not too hung up about either way and
can't promise to dedicate time to thinking about it soon, so -
as you wish. :)
-Eric
