MESSAGE quota resource implemention

[Greg Banks]

On 01/09/11 22:22, Bron Gondwana wrote:
> On Thu, Sep 01, 2011 at 01:27:00PM +0200, Julien Coloos wrote:
>> Le 01/09/2011 03:03, Greg Banks a écrit :

> 
> But more generally, the "update the quotaroot" is atomic-safe,
> because the mailbox doesn't add/remove things from the quotaroot
> racily - but quota -f IS racy.  The only way around that would
> be a dual pass mark and collect thing, where you marked each
> mailbox as "we're re-calculating, so don't update the quota usage"
> in the first sweep, then came back and removed the mark as you
> read the value.  Tricky, but doable.  The downside is that a
> crash part way through can leave you in a broken state.  But
> that's true of everything.
> 
>> But I agree that in case of underflow detection throwing a warning
>> in syslog might help draw the attention when logs are analysed.
> 
> "when".  haha.
> 
> (maybe at a few sites that care... but for the vast majority of
>  sites, if you're depending on them reading syslog you've already
>  lost.  Software that understands that and is robust in the face
>  of errors is much nicer for the poor suckers on the receiving end
>  of all this)

If the software was robust, underflow would not happen and we would not
need to test for it and handle it.  Thus the log messages are not
operational messages intended for the sysadmin, but warnings about
internal Cyrus problems intended for Cyrus developers, and syslog is a
suitable place for them.

How's about this for a strategy?

When a quota resource is first enabled, (i.e. the limit is changed from
UNLIMITED to some finite value), the usage is stored as some special
value which I'll call INDETERMINATE.

Changing a mailbox' quotaroot to an existing quotaroot, resets all the
quotaroot's usages to INDETERMINATE.

Usage deltas applied to the INDETERMINATE value silently yield
INDETERMINATE back.

Usage deltas which would underflow a finite (i.e. not INDETERMINATE)
usage value are clamped with a warning about an internal consistency
problem.

A regularly run process such as cyr_expire finds quotaroots which have
one or more INDETERMINATE usages and runs quota -f on them.

In this model, INDETERMINATE value acts like your "dual pass mark".


> 
>>> 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).

As far as I can see the only point of that first append_check() call is
to fail early in the case of a permission fail.

 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 ?

Literals, and binary, and urls...

I do think there's an argument to made for moving the maintenance of the
totalsize parameter into the struct appendstate, and once you do that
it's possible to do quota/permission checks at finer grained points.
But why bother?  Why slow down the common case of not exceeding quota?

> 
> There are some bugs about this already.  In particularl the opposite
> case, checking that we actually want to append something before
> aborting a sieve delivery - because it may be discarded or redirected
> or even duplicate suppressed anyway.  Something to keep in mind.
> 

> 
>>> 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 ? 

Yes.


> One place please :)  Ideally I'd like to absorb more of the quota
> stuff into mailbox.c.  Greg and I have some debate about this - how
> much is too much for that file to be doing.  Probably it should be
> abstracted into a couple of layers of stuff - but I really do like
> the consistency of having just a couple of function calls:
> 
> mailbox_append_index_record; and
> mailbox_update_index_record
> 
> which do all the consistency checking and counter updating inside.
> Plus of course a mailbox_check_quota thing that takes a set of
> quota checks to do and sees if there will be space for the
> planned changes!

After implementing the X-ANNOTATION-STORAGE quota, I'm almost completely
convinced I got it very wrong, and should have left all the quota
updating in mailbox_commit_quota().  I was trying hard to avoid adding a
field to the index header to track the storage used by all the
annotations for the mailbox and for messages in the mailbox; but I'm
really not happy with the results :(



-- 
Greg.