From owner-svn-src-all@freebsd.org Sat Feb 20 09:37:48 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B89B4AAD3F8; Sat, 20 Feb 2016 09:37:48 +0000 (UTC) (envelope-from se@freebsd.org) Received: from mailout11.t-online.de (mailout11.t-online.de [194.25.134.85]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mailout00.t-online.de", Issuer "TeleSec ServerPass DE-1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 49C789C1; Sat, 20 Feb 2016 09:37:48 +0000 (UTC) (envelope-from se@freebsd.org) Received: from fwd04.aul.t-online.de (fwd04.aul.t-online.de [172.20.26.149]) by mailout11.t-online.de (Postfix) with SMTP id 70E4655598D; Sat, 20 Feb 2016 10:37:40 +0100 (CET) Received: from [192.168.119.17] (GMGY9QZJohooYLvD6khdW5O4fllH-DC8Eua8lkUnEF2Obo0eQImv2trfzRLU4Tvwe9@[87.151.212.124]) by fwd04.t-online.de with (TLSv1.2:ECDHE-RSA-AES256-SHA encrypted) esmtp id 1aX3yk-3Qnvcm0; Sat, 20 Feb 2016 10:37:30 +0100 Subject: Re: svn commit: r295800 - head/usr.bin/cap_mkdb To: Eric van Gyzen , Bryan Drewery , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, dwmalone@FreeBSD.org References: <201602190842.u1J8gDOc015177@repo.freebsd.org> <56C7B60E.8080002@FreeBSD.org> <56C7D019.3020807@FreeBSD.org> From: Stefan Esser X-Enigmail-Draft-Status: N1110 Message-ID: <56C833CD.4040309@freebsd.org> Date: Sat, 20 Feb 2016 10:37:17 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <56C7D019.3020807@FreeBSD.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-ID: GMGY9QZJohooYLvD6khdW5O4fllH-DC8Eua8lkUnEF2Obo0eQImv2trfzRLU4Tvwe9 X-TOI-MSGID: 5f85d3f1-6166-4529-8ed5-33d2bc96c439 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 20 Feb 2016 09:37:48 -0000 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