Re: [PATCH v2 2/3] wrapper: provide function to sync directories
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Nov 10, 2021, 14:40 UTC
- Message-ID
- <211110.86zgqcgj98.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <3ac9d4d7abd224a4c0991f1036f2d95eedb9ceac.1636544377.git.ps@pks.im>
On Wed, Nov 10 2021, Patrick Steinhardt wrote:
Show 45 quoted lines
> [[PGP Signed Part:Undecided]]
> In ec983eb5d2 (core.fsyncobjectfiles: batched disk flushes, 2021-10-04),
> we have introduced batched syncing of object files. This mode works by
> only requesting a writeback of the page cache backing the file on
> written files, followed by a single hardware-flush via a temporary file
> created in the directory we want to flush. Given modern journaling file
> systems, this pattern is expected to be durable.
>
> While it's possible to reuse the `git_fsync()` helper to synchronize the
> page cache only, there is no helper which would allow for doing a
> hardware flush of a directory by creating a temporary file. Other
> callers which want to follow the same pattern would thus have to repeat
> this logic.
>
> Extract a new helper `git_fsync_dir()` from the object files code which
> neatly encapsulates this logic such that it can be reused.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> bulk-checkin.c | 13 +++----------
> git-compat-util.h | 7 +++++++
> wrapper.c | 21 +++++++++++++++++++++
> 3 files changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 4deee1af46..e6ebdd1db5 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -98,16 +98,9 @@ static void do_batch_fsync(void)
> * hardware.
> */
>
> - if (needs_batch_fsync) {
> - struct strbuf temp_path = STRBUF_INIT;
> - struct tempfile *temp;
> -
> - strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX", get_object_directory());
> - temp = xmks_tempfile(temp_path.buf);
> - fsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));
> - delete_tempfile(&temp);
> - strbuf_release(&temp_path);
> - }
> + if (needs_batch_fsync &&
> + git_fsync_dir(get_object_directory()) < 0)
> + die_errno("fsyncing object directory");Nit: Similar to 1/3, but this message is new: We say "fsyncing object directory", but it would be better to pass in some "verbose" flag to git_fsync_dir() so we can say e.g.:
error_errno(_("couldn't create core.fsyncRefFiles=batch tempfile '%s' in '%s'"), ...)
error_errno(_("couldn't fsync() core.fsyncRefFiles=batch tempfile '%s' in '%s'"), ...)I.e. being able to say specifically why we failed, permission error or the tempfile? fsync() didn't work etc?
Looking at the underlying APIs maybe they already have a mode to "die" or "warn" appropriately? Or...
Show 13 quoted lines
> +int git_fsync_dir(const char *path)
> +{
> + struct strbuf temp_path = STRBUF_INIT;
> + struct tempfile *temp;
> +
> + strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX", path);
> +
> + temp = mks_tempfile(temp_path.buf);
> + if (!temp)
> + return -1;
> +
> + if (git_fsync(get_tempfile_fd(temp), FSYNC_HARDWARE_FLUSH) < 0)
> + return -1;...if they do maybe we should use their non-fatal mode, because with/without that these "return -1" need to be "goto cleanup" so we can attempt to clean up after ourselves here.
I think this whole thing would be better if we generalized tmp-objdir.h a bit, so it could create and manage an arbitrary file in an arbitrary directory, and that API should really be generalized to a user of tempfile.c.
I.e. we'd then create this file, sync it optionally, whine if it does't work, and be guaranteed to try to clean anything that goes wrong up atexit().