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.
