Date: Sat, 20 Feb 2016 10:37:17 +0100 From: Stefan Esser <se@freebsd.org> To: Eric van Gyzen <vangyzen@FreeBSD.org>, Bryan Drewery <bdrewery@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, dwmalone@FreeBSD.org Subject: Re: svn commit: r295800 - head/usr.bin/cap_mkdb Message-ID: <56C833CD.4040309@freebsd.org> In-Reply-To: <56C7D019.3020807@FreeBSD.org> References: <201602190842.u1J8gDOc015177@repo.freebsd.org> <56C7B60E.8080002@FreeBSD.org> <56C7D019.3020807@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?56C833CD.4040309>