svn commit: r295800 - head/usr.bin/cap_mkdb

[Stefan Esser]

Am 20.02.2016 um 03:31 schrieb Eric van Gyzen:
> On 2/19/16 6:40 PM, Bryan Drewery wrote:
>> On 2/19/2016 12:42 AM, Stefan Esser wrote:
>>> Author: se
>>> Date: Fri Feb 19 08:42:13 2016
>>> New Revision: 295800
>>> URL: https://svnweb.freebsd.org/changeset/base/295800
>>>
>>> Log:
>>>    Remove O_SYNC from the options passed to dbmopen().
>> Uh, this is a full revert of r293312's changes to cap_mkdb which were
>> made for good reason. So this seems simply wrong without a better fix.
>>
>>>       The output file is created as a temporary file that is moved
>>> over the
>>>    existing file after completion. Thus there is no need to immediately
>>>    flush all created db records to the temporary file.
>> This is not right either. Depending on the use of soft updates /
>> journaling the data and metadata (file name / rename) may be written at
>> different times. It is entirely possible to get a renamed file with no
>> or junk content without an fsync. That's exactly what r293312 mentions
>> in its commit message.

I had performed multiple tests and found, that the file was always
created identical whether with or without O_SYNC. But I've got to
admit, that I did not check the SVN log.

After reading the commit log entry for r293312, I understand there is
the risk of an empty db file if a system crash happens before all data
and metadata has been flushed.

I had assumed that one of the guarantees soft-updates makes is that
data and meta-data operations are ordered relative to each other.

I'm not sure, whether this is a hypothetical case, or whether such a
case had been observed in reality, but I understand that you don't
want to take risk when operating on critical system files.

O_SYNC was first added to updates of the password db, and I think that
was the file with the biggest risk of accidental damage due to a crash.

But services_mkdb and cap_mkdb are only invoked by the administrator
after the text files are modified and it is highly unlikely (but of
course not impossible) that a crash occurs at just that moment.

> dwmalone@ plans to put the fsync() in the db close method, which makes a
> lot of sense, and would fix this in a better way.
> 
>     https://reviews.freebsd.org/D5186
> 
> This commit probably should have waited for D5186 to be committed, but
> at least that seems imminent.

Yes, I mentioned review D5186 in the commit log and was aware, that
there was consensus that dwmalone's change to hash.c is about to be
committed. I was afraid that D5186 could be committed without the
removal of O_SYNC and that D5186 might be merged to 10.3, but that
cap_mkdb kept O_SYNC and would be slow throughout the lifetime of
10.3.

So the commit was expected to be followed by the fsync() patch in
hash.c very soon and I had mentioned, that the MFC of the mkdb patch
should be performed in conjunction (or after) the MFC of D5186.

I think it is important that the mkdb utilities do not take tens of
seconds and up to several minutes in 10.3 ...

Regards, STefan