FastMail Cyrus Patches for upstream

[Bron Gondwana]

On Thu, 27 Aug 2009 21:19 -0400, "Brian Awood" <bawood at umich.edu> wrote:
> 
> On Thursday 27 August 2009 @ 18:55, Bron Gondwana wrote:
> >
> > a) it's a pain to update.  For cache_crc I've special-cased "0" so
> > we don't have to read every single cache record.  You'd have to do
> > the same here, or have an "options" value like pop3_uidl so that a
> > mailbox could be "upgraded" during a process_records run or
> > something.  Don't worry, I've already considered this for my "get
> > shared seen back into the index file rather than in a bloody huge
> > non-findable skiplist file that can't be replicated to a new server
> > easily".  It's doable.
> 
> True, it would be a pain to update existing files.  A low impact way 
> might be to only generate it during things like an append or 
> reconstruct.

Yep - that's pretty much how the CRC32 on the cache records
already works.

> > b) it's un-necessary.  If the record is not broken, then parsing
> > out the length only takes 10 operations, and it's not like we have
> > to do it that often.
> 
> Also true,  but if you had the length, it could be: read cache data of 
> length x, calculate the crc of the buffer, verify and then parse it 
> if it's valid.  I suppose it's 6 of one and a 1/2 dozen of the other, 
> it just makes more sense to me if the data was verified before 
> parsing.

Yeah - it does make a bit more sense when you put it like that.
Does it make 8 bytes per record worth of sense though? (we need
to pad to 8 byte multiples for 64 bit modseq alignment.)  I'll have
another look at that in a second.

> > Ahh - there were a couple of bugs.  One that wrote everything
> > between two records to the file, and another that truncated
> > incorrectly on failed append, leaving all sorts of extra junk in
> > cache files.  They should both be fixed in 2.3.14 I think.  Though
> > cache files could still contain junk on disk until a rewrite gets
> > the whole thing!
> 
> We're running 2.3.14 + local & some Fastmail patches.  I think the 
> truncation of the file on a failed append works correctly, but it 
> seems like extra records are still getting copied.  

Right - that would be a bug then.  I'll poke around the code.
Just to confirm - the symptom is copied messages out of
order causing the intermediate cache records to be copied
as well?

> > Unexpunge is the only thing I can think of that might give out of
> > order cache records.  Though I _think_ it may actually be totally
> > broken with respect to cache records at the moment.  I would
> > recommend a reconstruct after any unexpunge at the moment.
> 
> unexpunge seems to be ok, based on some simple testing.  It doesn't 
> touch the cache file at all, and seems to just be moving records back 
> to the index.  I think we'll be reconstructing unexpunged mailboxes 
> for now anyway.

Yeah, that's the problem though, because if the copied-back records
might have the wrong offsets unless everything was kept in sync
properly.

> > Ok - so that's a bug that needs fixing, if it's still present in
> > CVS.
> >
> > I _thought_ I had fixed it.  Fixing this bug doesn't require having
> > the record length in the index file though.  It can be done just by
> > reading the record itself - which we now have a nice easy API for,
> > which returns the length and does CRC and internal structure checks
> > at the same time!
> 
> Great! I'm looking forward to testing out this code, cache files have 
> been our main ongoing issue. 

Cool.  Sounds good!  We've been OK with cache files for a while now,
but we had a RAID incident (tech pulled the wrong drive) which made
us want better integrity checking just-in-case[tm].

Bron.
-- 
  Bron Gondwana
  brong at fastmail.fm