Re: [PATCH v4 2/4] core.fsync: introduce granular fsync control

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: Junio C Hamano <gitster@pobox.com>,
	Neeraj Singh <nksingh85@gmail.com>,
	Neeraj Singh via GitGitGadget <gitgitgadget@gmail.com>,
	Git List <git@vger.kernel.org>,
	"Randall S. Becker" <rsbecker@nexbridge.com>,
	Bagas Sanjaya <bagasdotme@gmail.com>,
	Elijah Newren <newren@gmail.com>,
	"Neeraj K. Singh" <neerajsi@microsoft.com>
Subject: Re: [PATCH v4 2/4] core.fsync: introduce granular fsync control
Date: Wed, 09 Mar 2022 19:50:15 +0100	[thread overview]
Message-ID: <220309.867d93lztw.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <YiiuqK/tCnQOXrSV@ncase>

On Wed, Mar 09 2022, Patrick Steinhardt wrote:

> [[PGP Signed Part:Undecided]]
> On Mon, Feb 14, 2022 at 09:17:31AM -0800, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> 
>> > To summarize my take: while the degree of durability may be something
>> > that's up for discussions, I think that the current defaults for
>> > atomicity are bad for users because they can and do lead to repository
>> > corruption.
>> 
>> Good summary.
>> 
>> If the user cares about fsynching loose object files in the right
>> way, we shouldn't leave loose ref files not following the safe
>> safety level, regardless of how this new core.fsync knobs would look
>> like.
>> 
>> I think we three are in agreement on that.
>
> Is there anything I can specifically do to help out with this topic? We
> have again hit data loss in production because we don't sync loose refs
> to disk before renaming them into place, so I'd really love to sort out
> this issue somehow so that I can revive my patch series which fixes the
> known repository corruption [1].
>
> Alternatively, can we maybe find a way forward with applying a version
> of my patch series without first settling the bigger question of how we
> want the overall design to look like? In my opinion repository
> corruption is a severe bug that needs to be fixed, and it doesn't feel
> sensible to block such a fix over a discussion that potentially will
> take a long time to settle.
>
> Patrick
>
> [1]: http://public-inbox.org/git/cover.1636544377.git.ps@pks.im/

I share that view. I was wondering how this topic fizzled out the other
day, but then promptly forgot about it.

I think the best thing at this point (hint hint!) would be for someone
in the know to (re-)submit the various patches appropriate to move this
forward. Whether that's just this series, part of it, or some/both of
those + patches from you and Eric and this point I don't know/remember.

But just to be explicitly clear, as probably the person most responsible
for pushing this towards the "bigger question of [...] overall
design".

I just wanted to facilitate a discussion that would result in the
various stakeholders who wanted to add some fsync-related config coming
up with something that's mutually compatible, and I think the design
from Neeraj in this series fits that purpose, is Good Enough etc.

I.e. the actually important and IMO blockers were all resolved, e.g. not
having an fsync configuration that older git versions would needlessly
die on, and not painting ourselves into a corner where
e.g. core.fsync=false or something was squatted on by something other
than a "no fsync, whatsoever" etc.

(But I haven't looked at it again just now, so...)

Anyway, just trying to be explicit that to whatever extent this was held
up by questions/comments of mine I'm very happy to see this go forward.
As you (basically) say we shouldn't lose sight of ongoing data loss in
this area because of some config bikeshedding :)