RFC patchmaintenance: separate parallelism safe and unsafe tasks

> Therefore, tasks are marked whether they are async safe or
> not. Async unsafe tasks are run first in the same process before running
> async safe tasks in parallel.
> 
> Since the gc task is partially safe, there are two new tasks -- an async
> safe gc task and an async unsafe gc task. In order to properly invoke
> this in gc, `--run-async-safe` and `--run-async-unsafe` have been added
> as options to gc. Maintenance will only run these two new tasks if it
> was set to detach, otherwise the original gc task runs.
> 
> Additionally, if a user passes in tasks thru `--task`, we do not attempt
> to run separate async/sync tasks since the user sets the order of tasks.
> 
> WIP: automatically run gc unsafe tasks when gc is invoked but not from
>      maintenance
> WIP: edit test in t7900-maintainance.sh to match new functionality
> WIP: add additional documentation for new options and functionality
> 
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>  builtin/gc.c           | 173 ++++++++++++++++++++++++++++++++++++-----
>  t/t7900-maintenance.sh |  24 +++---
>  2 files changed, 168 insertions(+), 29 deletions(-)
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index d52735354c..375d304c42 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c

> @@ -815,7 +824,12 @@ struct repository *repo UNUSED)
>  		atexit(process_log_file_at_exit);
>  	}
>  
> -	gc_before_repack(&opts, &cfg);
> +	if (run_async_unsafe) {
> +		gc_before_repack(&opts, &cfg);
> +		goto out;
> +	} else if (!run_async_safe)
> +		gc_before_repack(&opts, &cfg);
> +	
>  

>  	if (!repository_format_precious_objects) {
>  		struct child_process repack_cmd = CHILD_PROCESS_INIT;
> @@ -1052,6 +1066,46 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts,
>  	return 0;
>  }
>  
> +static int maintenance_task_unsafe_gc(struct maintenance_run_opts *opts,
> +				      struct gc_config *cfg UNUSED)
> +{
> +	struct child_process child = CHILD_PROCESS_INIT;
> +
> +	child.git_cmd = child.close_object_store = 1;
> +	strvec_push(&child.args, "gc");
> +
> +	if (opts->auto_flag)
> +		strvec_push(&child.args, "--auto");
> +	if (opts->quiet)
> +		strvec_push(&child.args, "--quiet");
> +	else
> +		strvec_push(&child.args, "--no-quiet");
> +	strvec_push(&child.args, "--no-detach");
> +	strvec_push(&child.args, "--run-async-unsafe");
> +
> +	return run_command(&child);
> +}
> +
> +static int maintenance_task_safe_gc(struct maintenance_run_opts *opts,
> +				    struct gc_config *cfg UNUSED)
> +{
> +	struct child_process child = CHILD_PROCESS_INIT;
> +
> +	child.git_cmd = child.close_object_store = 1;
> +	strvec_push(&child.args, "gc");
> +
> +	if (opts->auto_flag)
> +		strvec_push(&child.args, "--auto");
> +	if (opts->quiet)
> +		strvec_push(&child.args, "--quiet");
> +	else
> +		strvec_push(&child.args, "--no-quiet");
> +	strvec_push(&child.args, "--no-detach");
> +	strvec_push(&child.args, "--run-async-safe");
> +
> +	return run_command(&child);
> +}

>  static int maintenance_task_gc(struct maintenance_run_opts *opts,
>  			       struct gc_config *cfg UNUSED)
>  {
> @@ -1350,6 +1404,7 @@ struct maintenance_task {
>  	const char *name;
>  	maintenance_task_fn *fn;
>  	maintenance_auto_fn *auto_condition;
> +	unsigned daemonize_safe;

>  	unsigned enabled:1;
>  
>  	enum schedule_priority schedule;
> @@ -1362,6 +1417,8 @@ enum maintenance_task_label {
>  	TASK_PREFETCH,
>  	TASK_LOOSE_OBJECTS,
>  	TASK_INCREMENTAL_REPACK,
> +	TASK_UNSAFE_GC,
> +	TASK_SAFE_GC,
>  	TASK_GC,
>  	TASK_COMMIT_GRAPH,
>  	TASK_PACK_REFS,
> @@ -1370,36 +1427,62 @@ enum maintenance_task_label {
>  	TASK__COUNT
>  };
>  
> +enum maintenance_task_daemonize_safe {
> +	UNSAFE,
> +	SAFE,
> +};

>  static struct maintenance_task tasks[] = {
>  	[TASK_PREFETCH] = {
>  		"prefetch",
>  		maintenance_task_prefetch,
> +		NULL,
> +		SAFE,
>  	},

>  	[TASK_LOOSE_OBJECTS] = {
>  		"loose-objects",
>  		maintenance_task_loose_objects,
>  		loose_object_auto_condition,
> +		SAFE,
>  	},
>  	[TASK_INCREMENTAL_REPACK] = {
>  		"incremental-repack",
>  		maintenance_task_incremental_repack,
>  		incremental_repack_auto_condition,
> +		SAFE,
> +	},
> +	[TASK_UNSAFE_GC] = {
> +		"unsafe-gc",
> +		maintenance_task_unsafe_gc,
> +		need_to_gc,
> +		UNSAFE,
> +		0,
> +	},
> +	[TASK_SAFE_GC] = {
> +		"safe-gc",
> +		maintenance_task_safe_gc,
> +		need_to_gc,
> +		SAFE,
> +		0,
>  	},

> @@ -1436,6 +1527,57 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
>  	}
>  	free(lock_path);
>  
> +	for (i = 0; !found_selected && i < TASK__COUNT; i++)
> +		found_selected = tasks[i].selected_order >= 0;
> +
> +	if (found_selected)
> +		QSORT(tasks, TASK__COUNT, compare_tasks_by_selection);
> +	else if (opts->detach > 0) {
> +		/* 
> +		 * Part of gc is unsafe to run in the background, so
> +		 * separate out gc into unsafe and safe tasks.
> +		 * 
> +		 * Run unsafe tasks, including unsafe maintenance tasks,
> +		 * before daemonizing and running safe tasks.
> +		 */
> +
> +		if (tasks[TASK_GC].enabled) {
> +			tasks[TASK_GC].enabled = 0;
> +			tasks[TASK_UNSAFE_GC].enabled = 1;
> +			tasks[TASK_UNSAFE_GC].schedule = tasks[TASK_GC].schedule;
> +			tasks[TASK_SAFE_GC].enabled = 1;
> +			tasks[TASK_SAFE_GC].schedule = tasks[TASK_GC].schedule;
> +		}

>
> On Fri, Nov 08, 2024 at 05:31:12PM +0000, Calvin Wan wrote:
> > Certain maintenance tasks and subtasks within gc are unsafe to run in
> > parallel with other commands because they lock up files such as
> > HEAD.
>
> I don't think it is fair to classify this as "unsafe". Nothing is unsafe
> here: we take locks to guard us against concurrent modifications.
> What you're having problems with is the fact that this safety mechanism
> works as expected and keeps other processes from modifying locked the
> data.
>
> > Therefore, tasks are marked whether they are async safe or
> > not. Async unsafe tasks are run first in the same process before running
> > async safe tasks in parallel.
> >
> > Since the gc task is partially safe, there are two new tasks -- an async
> > safe gc task and an async unsafe gc task. In order to properly invoke
> > this in gc, `--run-async-safe` and `--run-async-unsafe` have been added
> > as options to gc. Maintenance will only run these two new tasks if it
> > was set to detach, otherwise the original gc task runs.
> >
> > Additionally, if a user passes in tasks thru `--task`, we do not attempt
> > to run separate async/sync tasks since the user sets the order of tasks.
> >
> > WIP: automatically run gc unsafe tasks when gc is invoked but not from
> >      maintenance
> > WIP: edit test in t7900-maintainance.sh to match new functionality
> > WIP: add additional documentation for new options and functionality
> >
> > Signed-off-by: Calvin Wan <calvinwan@google.com>
> > ---
> >  builtin/gc.c           | 173 ++++++++++++++++++++++++++++++++++++-----
> >  t/t7900-maintenance.sh |  24 +++---
> >  2 files changed, 168 insertions(+), 29 deletions(-)
> >
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index d52735354c..375d304c42 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
>
> It might make sense to split out the git-gc(1) changes into a
> preparatory commit with its own set of tests.
>
> > @@ -815,7 +824,12 @@ struct repository *repo UNUSED)
> >               atexit(process_log_file_at_exit);
> >       }
> >
> > -     gc_before_repack(&opts, &cfg);
> > +     if (run_async_unsafe) {
> > +             gc_before_repack(&opts, &cfg);
> > +             goto out;
> > +     } else if (!run_async_safe)
> > +             gc_before_repack(&opts, &cfg);
> > +
> >
>
> Style: there should be curly braces around the `else if` here.
>
> >       if (!repository_format_precious_objects) {
> >               struct child_process repack_cmd = CHILD_PROCESS_INIT;
> > @@ -1052,6 +1066,46 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts,
> >       return 0;
> >  }
> >
> > +static int maintenance_task_unsafe_gc(struct maintenance_run_opts *opts,
> > +                                   struct gc_config *cfg UNUSED)
> > +{
> > +     struct child_process child = CHILD_PROCESS_INIT;
> > +
> > +     child.git_cmd = child.close_object_store = 1;
> > +     strvec_push(&child.args, "gc");
> > +
> > +     if (opts->auto_flag)
> > +             strvec_push(&child.args, "--auto");
> > +     if (opts->quiet)
> > +             strvec_push(&child.args, "--quiet");
> > +     else
> > +             strvec_push(&child.args, "--no-quiet");
> > +     strvec_push(&child.args, "--no-detach");
> > +     strvec_push(&child.args, "--run-async-unsafe");
> > +
> > +     return run_command(&child);
> > +}
> > +
> > +static int maintenance_task_safe_gc(struct maintenance_run_opts *opts,
> > +                                 struct gc_config *cfg UNUSED)
> > +{
> > +     struct child_process child = CHILD_PROCESS_INIT;
> > +
> > +     child.git_cmd = child.close_object_store = 1;
> > +     strvec_push(&child.args, "gc");
> > +
> > +     if (opts->auto_flag)
> > +             strvec_push(&child.args, "--auto");
> > +     if (opts->quiet)
> > +             strvec_push(&child.args, "--quiet");
> > +     else
> > +             strvec_push(&child.args, "--no-quiet");
> > +     strvec_push(&child.args, "--no-detach");
> > +     strvec_push(&child.args, "--run-async-safe");
> > +
> > +     return run_command(&child);
> > +}
>
> These two functions and `maintenance_task_gc()` all look exactly the
> same. We should deduplicate them.
>
> >  static int maintenance_task_gc(struct maintenance_run_opts *opts,
> >                              struct gc_config *cfg UNUSED)
> >  {
> > @@ -1350,6 +1404,7 @@ struct maintenance_task {
> >       const char *name;
> >       maintenance_task_fn *fn;
> >       maintenance_auto_fn *auto_condition;
> > +     unsigned daemonize_safe;
>
> We can use the enum here to give readers a better hint what this
> variable is about.
>
> >       unsigned enabled:1;
> >
> >       enum schedule_priority schedule;
> > @@ -1362,6 +1417,8 @@ enum maintenance_task_label {
> >       TASK_PREFETCH,
> >       TASK_LOOSE_OBJECTS,
> >       TASK_INCREMENTAL_REPACK,
> > +     TASK_UNSAFE_GC,
> > +     TASK_SAFE_GC,
> >       TASK_GC,
> >       TASK_COMMIT_GRAPH,
> >       TASK_PACK_REFS,
> > @@ -1370,36 +1427,62 @@ enum maintenance_task_label {
> >       TASK__COUNT
> >  };
> >
> > +enum maintenance_task_daemonize_safe {
> > +     UNSAFE,
> > +     SAFE,
> > +};
>
> These names can conflict quite fast. Do we maybe want to rename them to
> e.g. `MAINTENANCE_TASK_DAEMONIZE_(SAFE|UNSAFE)`?
>
> >  static struct maintenance_task tasks[] = {
> >       [TASK_PREFETCH] = {
> >               "prefetch",
> >               maintenance_task_prefetch,
> > +             NULL,
> > +             SAFE,
> >       },
>
> It might make sense to prepare these to take designated field
> initializers in a preparatory commit.

>
> >       [TASK_LOOSE_OBJECTS] = {
> >               "loose-objects",
> >               maintenance_task_loose_objects,
> >               loose_object_auto_condition,
> > +             SAFE,
> >       },
> >       [TASK_INCREMENTAL_REPACK] = {
> >               "incremental-repack",
> >               maintenance_task_incremental_repack,
> >               incremental_repack_auto_condition,
> > +             SAFE,
> > +     },
> > +     [TASK_UNSAFE_GC] = {
> > +             "unsafe-gc",
> > +             maintenance_task_unsafe_gc,
> > +             need_to_gc,
> > +             UNSAFE,
> > +             0,
> > +     },
> > +     [TASK_SAFE_GC] = {
> > +             "safe-gc",
> > +             maintenance_task_safe_gc,
> > +             need_to_gc,
> > +             SAFE,
> > +             0,
> >       },
>
> Hm. I wonder whether we really want to expose additional tasks to
> address the issue, which feels like we're leaking implementation details
> to our users. Would it maybe be preferable to instead introduce a new
> optional callback function for every task that handles the pre-detach
> logic?

> On Sun, Nov 10, 2024 at 11:07 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >       [TASK_LOOSE_OBJECTS] = {
> > >               "loose-objects",
> > >               maintenance_task_loose_objects,
> > >               loose_object_auto_condition,
> > > +             SAFE,
> > >       },
> > >       [TASK_INCREMENTAL_REPACK] = {
> > >               "incremental-repack",
> > >               maintenance_task_incremental_repack,
> > >               incremental_repack_auto_condition,
> > > +             SAFE,
> > > +     },
> > > +     [TASK_UNSAFE_GC] = {
> > > +             "unsafe-gc",
> > > +             maintenance_task_unsafe_gc,
> > > +             need_to_gc,
> > > +             UNSAFE,
> > > +             0,
> > > +     },
> > > +     [TASK_SAFE_GC] = {
> > > +             "safe-gc",
> > > +             maintenance_task_safe_gc,
> > > +             need_to_gc,
> > > +             SAFE,
> > > +             0,
> > >       },
> >
> > Hm. I wonder whether we really want to expose additional tasks to
> > address the issue, which feels like we're leaking implementation details
> > to our users. Would it maybe be preferable to instead introduce a new
> > optional callback function for every task that handles the pre-detach
> > logic?
> 
> This does sound like a good idea. However, would there be any issue
> with running all pre-detach logic before running post-detach logic?
> I'm thinking if pre-detach logic from a different function could
> affect post-detach logic from another. If not, I do agree this would
> be the best solution going forward.

>
> On Mon, Nov 11, 2024 at 10:06:10AM -0800, Calvin Wan wrote:
> > On Sun, Nov 10, 2024 at 11:07 PM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > Hm. I wonder whether we really want to expose additional tasks to
> > > address the issue, which feels like we're leaking implementation details
> > > to our users. Would it maybe be preferable to instead introduce a new
> > > optional callback function for every task that handles the pre-detach
> > > logic?
> >
> > This does sound like a good idea. However, would there be any issue
> > with running all pre-detach logic before running post-detach logic?
> > I'm thinking if pre-detach logic from a different function could
> > affect post-detach logic from another. If not, I do agree this would
> > be the best solution going forward.
>
> Sure, in theory these can interact with each other. But is that any
> different when you represent this with tasks instead? The conflict would
> still exist there. It's also not any different to how things work right
> now: the "gc" task will impact the "repack" task, so configuring them
> both at the same time does not really make much sense.

> .... I think for now it makes sense to hold off
> on rerolling this series (at least in the form of auto
> backgrounding/foregrounding tasks) since the purported benefits
> currently aren't worth the churn. Thanks again for the comments on
> this series

> On Mon, Nov 11, 2024 at 10:29 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Mon, Nov 11, 2024 at 10:06:10AM -0800, Calvin Wan wrote:
> > > On Sun, Nov 10, 2024 at 11:07 PM Patrick Steinhardt <ps@pks.im> wrote:
> > > >
> > > > Hm. I wonder whether we really want to expose additional tasks to
> > > > address the issue, which feels like we're leaking implementation details
> > > > to our users. Would it maybe be preferable to instead introduce a new
> > > > optional callback function for every task that handles the pre-detach
> > > > logic?
> > >
> > > This does sound like a good idea. However, would there be any issue
> > > with running all pre-detach logic before running post-detach logic?
> > > I'm thinking if pre-detach logic from a different function could
> > > affect post-detach logic from another. If not, I do agree this would
> > > be the best solution going forward.
> >
> > Sure, in theory these can interact with each other. But is that any
> > different when you represent this with tasks instead? The conflict would
> > still exist there. It's also not any different to how things work right
> > now: the "gc" task will impact the "repack" task, so configuring them
> > both at the same time does not really make much sense.
> 
> No you are correct that this is no different than how these tasks are
> currently run. However, I have just received some numbers that the
> repack, when gc'ing in Android, is the longest operation so even if we
> were able to run repack first in the foreground, ultimately it
> wouldn't save a significant amount of time compared to running gc
> entirely in the foreground. I think for now it makes sense to hold off
> on rerolling this series (at least in the form of auto
> backgrounding/foregrounding tasks) since the purported benefits
> currently aren't worth the churn. Thanks again for the comments on
> this series

> Since the gc task is partially safe, there are two new tasks -- an async
> safe gc task and an async unsafe gc task. In order to properly invoke
> this in gc, `--run-async-safe` and `--run-async-unsafe` have been added
> as options to gc. Maintenance will only run these two new tasks if it
> was set to detach, otherwise the original gc task runs.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index d52735354c..375d304c42 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -668,6 +668,8 @@ struct repository *repo UNUSED)
>  	pid_t pid;
>  	int daemonized = 0;
>  	int keep_largest_pack = -1;
> +	int run_async_safe = 0;
> +	int run_async_unsafe = 0;
>  	timestamp_t dummy;
>  	struct child_process rerere_cmd = CHILD_PROCESS_INIT;
>  	struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
> @@ -694,6 +696,10 @@ struct repository *repo UNUSED)
>  			   PARSE_OPT_NOCOMPLETE),
>  		OPT_BOOL(0, "keep-largest-pack", &keep_largest_pack,
>  			 N_("repack all other packs except the largest pack")),
> +		OPT_BOOL(0, "run-async-safe", &run_async_safe,
> +			 N_("run only background safe gc tasks, should only be invoked thru maintenance")),
> +		OPT_BOOL(0, "run-async-unsafe", &run_async_unsafe,
> +			 N_("run only background unsafe gc tasks, should only be invoked thru maintenance")),
>  		OPT_END()
>  	};
>  
> @@ -718,6 +724,9 @@ struct repository *repo UNUSED)
>  			     builtin_gc_usage, 0);
>  	if (argc > 0)
>  		usage_with_options(builtin_gc_usage, builtin_gc_options);
> +	
> +	if (run_async_safe && run_async_unsafe)
> +		die(_("--run-async-safe cannot be used with --run-async-unsafe"));

>
> So if the caller wants to eventually run both, it has to spawn this
> program twice, the first time with one option and the second time
> with the other option?  Somehow it feels a bit unsatisfying.  If the
> caller says "I want to run both classes", and if your safe/unsafe
> classification system knows which task belongs to which class and
> the classification system that unsafe ones should be run before safe
> ones (or whatever), wouldn't it be easier to use for the caller to
> be able to say "run both", and let your classification system take
> care of the ordering?

> However, this is not the case as discovered earlier[1] -- certain
> maintenance and gc tasks are not safe to run in parallel with other
> commands. The consequences of such are that scripts with commands that
> trigger maintenance/gc can race and crash.
>
> Users can also run into
> unexpected errors from porcelain commands that touch common files such
> as HEAD.lock, unaware that a background maintenance/gc task is the one
> holding the lock.

>
> Calvin Wan <calvinwan@google.com> writes:
>
> > However, this is not the case as discovered earlier[1] -- certain
> > maintenance and gc tasks are not safe to run in parallel with other
> > commands. The consequences of such are that scripts with commands that
> > trigger maintenance/gc can race and crash.
> >
> > Users can also run into
> > unexpected errors from porcelain commands that touch common files such
> > as HEAD.lock, unaware that a background maintenance/gc task is the one
> > holding the lock.
>
> The symptom looks more like a controlled refusal of execution than a
> crash.