prototype mappedfile_abort()

[elliefm]

Prototyping mappedfile_abort() API.

[elliefm]

Another old forgotten half-finished piece of work(!). Can I please get some eyes on this to see whether it's of any value, before I finish it off?

I think the intent was meant to be a way of closing a mappedfile during post-error-cleanup -- when you know the file might be dirty (and so you can't simply "close" it without it asserting) but you don't want to "commit" it, either. (The file is probably corrupt at this point, but probably was anyway due to the error you're recovering from...)

I think the "WIP..." commit is called "WIP" because I meant to go and update the other places that use mappedfile to also use mappedfile_abort in their error handling, and prometheus was just where I started?

So if we think this looks viable, then I'll carry on updating the rest of the mappedfile usages like what I did with the prometheus one, and get it merged.

[rsto]

I think it's good to have an abort function. I'm just wondering about the API, as the abort function signature is inconsistent with the commit function.

The abort function arguments is a pointer to pointer and closes the mapped file. The commit function takes a pointer and does not cIose the file. Is there any use of a mappedfile after commit? I've grepped the current codebase for use of mappedfile_commit and don't have an answer, yet.

[elliefm]

The abort function arguments is a pointer to pointer and closes the mapped file. The commit function takes a pointer and does not cIose the file. Is there any use of a mappedfile after commit? I've grepped the current codebase for use of mappedfile_commit and don't have an answer, yet.

The commit function only fsyncs the file, and then (if that works) clears the dirty flag. It doesn't close it, or even unlock it. I don't know if we ever do this, but I think something like this would be reasonable:

mappedfile_open();
while(...) {
    mappedfile_writelock();
    do_some_work();
    mappedfile_pwrite(...);
    mappedfile_commit();
    mappedfile_unlock();
    do_some_other_stuff();
}
mappedfile_close();

In contrast though, if anything has been partially written, mappedfile_abort() will result in a corrupted file -- it's not a smart transactional thing. In fact, if anything has been only partially written, the file is already corrupt, whether you mappedfile_commit() it or not.

I had another look through the LP task that this shook out of, and the problem worked out like this (in prometheus):

• [a disk filled up]
• mappedfile_pwrite() failed due to disk full, so we jumped to the error handler
• mappedfile_unlock() asserted because the file was dirty

If we were to skip the mappedfile_unlock() call on errors, then the mappedfile_close() after it would assert because it's still locked.

If we were to call mappedfile_commit(); mappedfile_unlock(); mappedfile_close; on errors to make sure it's not dirty or locked before we close it, then we'd end up with a sequence like:

• [a disk fills up]
• mappedfile_pwrite() fails due to disk full, so we jump to the error handler
• mappedfile_commit() fails to fsync because of the disk full, and still doesn't clear the dirty flag
• mappedfile_unlock() asserts because the file is still dirty...

So, the goal is a dedicated "it's bad, just get the hell outta here" function.

I think it's good to have an abort function. I'm just wondering about the API, as the abort function signature is inconsistent with the commit function.

I would expect commit/abort to have consistent signatures if this was a transactional thing (where you either get the whole change, or none of it), but it's not and can't be. Maybe "commit" is badly named, since it implies transactionality, when it's really just "flush to disk now and clear the dirty flag".

[rsto]

I'm going to remove myself from the reviewers here. Feel free to add me again when this gets ready for review.