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

From: Neeraj Singh <nksingh85@gmail.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: "Junio C Hamano" <gitster@pobox.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>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Neeraj K. Singh" <neerajsi@microsoft.com>
Subject: Re: [PATCH v4 2/4] core.fsync: introduce granular fsync control
Date: Wed, 9 Mar 2022 12:05:09 -0800	[thread overview]
Message-ID: <CANQDOdfHYnKKFfQ6ptPcLacx=HX5cnXGRuL55Ttfr+zKCSBQJg@mail.gmail.com> (raw)
In-Reply-To: <YiiuqK/tCnQOXrSV@ncase>

On Wed, Mar 9, 2022 at 5:42 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> 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/

Hi Patrick,
Thanks for reviving this discussion.  I've updated the PR on
GitGitGadget with a rebase
onto the current 'main' branch and some minor build fixes. I've also
revamped the aggregate options
and documentation to be more inline with Junio's suggestion of having
'levels of safety' that we steer
the user towards. I'm still keeping the detailed options, but
hopefully the guidance is clear enough to
avoid confusion.

I'd be happy to make any point fixes as necessary to get that branch
into proper shape for
upstream, if we've gotten to the point where we don't want to change
the fundamental design.

I agree with Patrick that the detailed knobs are primarily for use by
hosters like GitLab and GitHub.

Please expect a v5 today.

Thanks,
Neeraj