MESSAGE quota resource implemention

[Bron Gondwana]

On Thu, Sep 01, 2011 at 01:27:00PM +0200, Julien Coloos wrote:
> Le 01/09/2011 03:03, Greg Banks a écrit :
> >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.

A provisioning system could run quota -f itself after making the
change, of course.

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)

> 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 don't mind keeping quota_t around.  Using it makes it a bit
clearer that you intend to store a quota in this variable.  It's
like free documentation.  Just uquota_t is pointless in a 64bit
required world.

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

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.

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

FYI - I'm planning to be a bit more lazy about mailbox deletes
at some point.  It would be super-cool to not even move the files
until someone trys to create a new mailbox with the same name,
otherwise just clean them up in the regular course of cyr_expire.

Need to think about that some more though.

Actually, really I'd like to create a new UNIQUEID - and store
all the files in paths based on uniqueid rather than on folder
name.  This would not only mean renames could be fast and
atomic, but that delayed delete would be fast.  The downside is
a more opaque filesystem layout.  Oh, another upside - file path
limitations don't exist so much any more.

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

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!

Bron.