MESSAGE quota resource implemention

[Julien Coloos]

Hi, thanks for your comments and reviewing the code

Le 01/09/2011 03:03, Greg Banks a écrit :
>> I think that there are two things that may also be done concerning 
>> quota entries:
>>   - always recompute resource usage when changing its limit from 
>> unlimited to bounded
>>     -> currently it is only done once when setting the usage limit 
>> for the first time
>>     -> that way, it may not be necessary to track resource presence 
>> when reading/writing quota entries
>>     -> but maybe it could be too time consuming in some cases, since 
>> it seems to be possible to associate a quota resource to a whole 
>> domain (recomputing usage for all mailboxes would then take some time)
>>   - do not write resource in quota entry when its usage is unlimited
>>     -> except for the STORAGE resource, for backward compatibility
>>     -> would also help keeping quota entries size to the bare minimum
>>
>> What do you think ?
>
> This one's tough, I wasn't sure what to do.  However, I'm happy to 
> leave it to the administrator to have to manually run quota -f (maybe 
> twice!) if they set a quota on a resource that is already being used.  
> I'm unconvinced that automatically doing the equivalent of -f as a 
> side effect of setting the first limit is necessary or wise.  Perhaps 
> we should a) document it clearly and b) detect the situation and put 
> an obvious message saying something like "you may need to run quota -f 
> ..." where the human making the change will see it.  Also, such a 
> message might be useful when usage underflow is detected.
>
My concern here is that there are times when quota changes are not done 
manually by a human, but happen automatically as part of platform 
provisioning scheme. For example, on many platforms we manage, if a user 
subscribes to some offer his/her mailbox quota will automatically be 
upgraded (IMAP action). So, at least in our case, I though it might be 
useful.

But I agree that in case of underflow detection throwing a warning in 
syslog might help draw the attention when logs are analysed.



Le 01/09/2011 10:15, Greg Banks a écrit :
> commit 2d4fb0 "Added quota MESSAGE resource (RFC 2087) management."
>
> imap/quota.h
>
> Why #include <stdint.h>?  It's not like we're using any of the 
> typedefs there.  There's a good argument to be made that we *should* 
> do so, but until then the #include doesn't seem useful.
>
As you realised in the next commit, I removed QUOTA_REPORT_FMT 
(appearing only in quota.c) to use PRIdMAX/PRIuMAX and cast to 
(u)intmax_t upon formatting. Using those appeared as a future-proof 
solution in the discussion about 32/64-bit variables thread I started 
some time ago :)
So yeah, the include could have been done *only* in quota.c, but I 
remembered that Bron wanted to get rid of (u)quota_t later. So I decided 
to put the include in quota.h for later use.

> I'm unconvinced about the need for quota.sets[].  After a lot of 
> reading it seems to me that (quota.sets[x]) and (quota.limits[x] != 
> QUOTA_UNLIMITED) are identical or should be.  Also, are you setting 
> sets[] anywhere except when loading from the db?
>
These would be perfectly identical if implementing the last point I 
brought up (not writing the resource in the quota entry if it's usage is 
unlimited). But then I need to compute actual quota usage at the right 
time. And when I saw that the code would allow to associate a quota to a 
whole domain (ouch, performance issue foreseen if quota is to be 
computed too often), I decided on the solution I proposed: compute it 
only once the first time it is set, hence the need of quota.sets[] 
(since when changing from bounded to unlimited limit, the resource is 
still kept in the quota entry to prevent recomputing its usage next time).

Otherwise, sets[] is updated when setting quota limits (in 
mboxlist_setquotas).

> imap/quota_db.c
>
> In quota_write(), your sanity check should goto the buf_free() at the 
> end of the function rather than return early.  Otherwise, looks good.
>
Ah, forgot that one.
Actually I took a shortcut here, since buf_free should not be necessary 
with the current code (which does append strings; so if the len is still 
0, no string was malloc'ed). But for sanity reasons you are right, it is 
wiser to goto the end of the function and do the expected cleanup stuff. 
This shall be future-proof.

> imap/imapd.c
>
> In the 1st hunk in cmd_append(), at this point in the code I believe 
> totalsize = 0, so you could easily pass 0 for the new last argument as 
> well.
Yes at this point totalsize is still 0.
The current code, which handles MULTIAPPEND, does a preliminary check to 
see if mailbox is not overquota, then receives all messages, and finally 
checks that all messages can fit in the mailbox.
Since we know for sure at least one message is coming, I though it could 
be a good idea to early-check that at least one more message would fit 
in the mailbox before receiving anything. Otherwise we are staging new 
file only to trash it at the end (when overquota). So actually I wonder 
if the code could be updated to check (LITERAL parameter) that each 
message about to be received would fit in the mailbox, instead of 
checking only once at the end ?

> imap/mailbox.c
>
> I'm not sure it's a good idea to set mailbox->i.exists = 0 in 
> mailbox_delete().  Perhaps it would be better to check for 
> (mailbox->i.options & OPT_MAILBOX_DELETED) when sampling 
> mailbox->i.exists in mailbox_commit_quota().
Hmm, I wondered about it too. At the end of mailbox_delete, upon calling 
mailbox_close, all physical files in the mailbox are deleted, and it 
didn't seem like 'exists' was used anywhere in-between, so I thought it 
would be alright.
But given the complexity of the code at that point (lot of cleaning when 
deleting the mailbox), and unforeseen future usages, I guess you are 
right. Actually maybe even quota_mailbox_use shall be treated that way ?

> I'm thinking that there's now three places in the code which take a 
> mailbox* and fill out an array of quota diffs, interpreting the 
> contents of the struct mailbox.  That should really be centralised.
I'm not sure to see what you have in mind here.
Are you talking about the places where the QUOTA_STORAGE and 
QUOTA_MESSAGE entries of the quota diff array are computed relatively to 
the 'quota_mailbox_used' and 'exists' index fields of the struct mailbox 
? If so I guess some of the code could indeed be centralised.


Regards
Julien Coloos