Re: [PATCH 6/8] core.fsync: add `fsync_component()` wrapper which doesn't die

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2022, 17:34 UTC
Message-ID: <xmqqwnh1g19q.fsf@gitster.g>
In-Reply-To: <50e39f698a7c0cc06d3bc060e6dbc539ea693241.1646905589.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> We have a `fsync_component_or_die()` helper function which only syncs
> changes to disk in case the corresponding config is enabled by the user.
> This wrapper will always die on an error though, which makes it
> insufficient for new callsites we are about to add.

You can replace "which makes it ..." part with a bit more concrete description to save suspense from the readers.

    fsync_component_or_die() that dies upon an error is not useful
    for callers with their own error handling or recovery logic,
    like ref transaction API.

    Split fsync_component() out that returns an error to help them.

Show 17 quoted lines

> -void fsync_or_die(int fd, const char *);
> +int maybe_fsync(int fd);
> ...
> +static inline int fsync_component(enum fsync_component component, int fd)
> +{
> +	if (fsync_components & component)
> +		return maybe_fsync(fd);
> +	return 0;
> +}
>  
>  static inline void fsync_component_or_die(enum fsync_component component, int fd, const char *msg)
>  {
> -	if (fsync_components & component)
> -		fsync_or_die(fd, msg);
> +	if (fsync_component(component, fd) < 0)
> +		die_errno("fsync error on '%s'", msg);
>  }

I think in the eventuall reroll, these "static inline" functions on the I/O code path will become real functions in write-or-die.c but other than that this reorganization looks sensible.

Thanks.

Show 30 quoted lines

> diff --git a/write-or-die.c b/write-or-die.c
> index 9faa5f9f56..4a5455ce46 100644
> --- a/write-or-die.c
> +++ b/write-or-die.c
> @@ -56,19 +56,21 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)
>  	}
>  }
>  
> -void fsync_or_die(int fd, const char *msg)
> +int maybe_fsync(int fd)
>  {
>  	if (use_fsync < 0)
>  		use_fsync = git_env_bool("GIT_TEST_FSYNC", 1);
>  	if (!use_fsync)
> -		return;
> +		return 0;
>  
>  	if (fsync_method == FSYNC_METHOD_WRITEOUT_ONLY &&
>  	    git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0)
> -		return;
> +		return 0;
>  
>  	if (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0)
> -		die_errno("fsync error on '%s'", msg);
> +		return -1;
> +
> +	return 0;
>  }
>  
>  void write_or_die(int fd, const void *buf, size_t count)