On Tue, Sep 28, 2021 at 4:55 PM Jeff King <peff@xxxxxxxx> wrote: > > On Tue, Sep 28, 2021 at 11:32:43PM +0000, Neeraj Singh via GitGitGadget wrote: > > > If a temporary ODB is active, as determined by GIT_QUARANTINE_PATH > > being set, create object files with their final names. This avoids > > an extra rename beyond what is needed to merge the temporary ODB in > > tmp_objdir_migrate. > > What's our goal here? Is it the performance of avoiding the extra > rename()? Or do we benefit from the simplicity of avoiding it? > > If the former, do we have measurements on how much this matters? > > If the latter, what does the simplicity buy us? I thought maybe it would > make reasoning about fsync() easier, because we don't have to worry > about fsyncing the rename. But we'd eventually have to rename() into the > real object directory anyway. > > The reason I want to push back is... > > > Creating an object file with the expected final name should be okay > > since the git process writing to the temporary object store is the > > only writer, and it only invokes write_loose_object/create_object_file > > after checking that the object doesn't exist. > > ...this seems like a kind-of dangerous assumption. Most of the time, > yeah, I'd expect just a single process to be writing. But one of the > things that happens during the receive-pack quarantine is that we run > hooks, which can run any set of arbitrary Git commands, including > simultaneous readers and writers. It seems like we might be introducing > subtle races there. > > -Peff Yes, the main goal was to avoid an extra rename. I see your concern and I guess we have no way of knowing if someone is really going to get bitten by this or not. On the other hand, we do know in the case of batch_fsync that only Git is running against the objdir at that time. I'll remove this change since it's not where the real perf benefit is. Thanks, Neeraj
