From patchwork Fri Nov 8 17:31:12 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Calvin Wan X-Patchwork-Id: 13868524 Received: from mail-pg1-f201.google.com (mail-pg1-f201.google.com [209.85.215.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1AA021DFD1 for ; Fri, 8 Nov 2024 17:31:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.215.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1731087091; cv=none; b=fJKT2mMJDbtBEi/dSgeR5XyGh3NqfpUIIkFR/DwHtZSGRqT5yxPRFaLByLFRam92DrslwBxG5Wmz5xToZ5IqZX90wrtvns3I6mNKyHMoiWFSNMQPTQWRXWPZbSihDdwazrvE/ueNZWbMlUTOvRyDSDXgZ5ulyFXSXub4OKxoqQw= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1731087091; c=relaxed/simple; bh=7phob1aWWWCSuG96OgEDxjTUy2E6mTtBL2wlDD6Tq/k=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=f8OLCMtaOhzgt3BvEblt6UjYOZcHC50LbgMcnTEUlc7ge8LGhYJESLUzC0CG2iXtxfBUMA/mZRJAE5wsM5IyoUo3CCuXEhlIRWWNV8tiVCkXcESH/K/lGUxM0GczGPDfXHHbQHuP1Kp4ts54SqALEPmsI9bkrhWo/zcg+3xKNIU= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--calvinwan.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=tvY5FZTD; arc=none smtp.client-ip=209.85.215.201 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--calvinwan.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="tvY5FZTD" Received: by mail-pg1-f201.google.com with SMTP id 41be03b00d2f7-7eb0a32fc5aso2053362a12.2 for ; Fri, 08 Nov 2024 09:31:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1731087089; x=1731691889; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=zdaXBeNWyZ7c/i1bDtqSJBAzKjVQd4iRvFi0ww8IGu8=; b=tvY5FZTDc7AgCpk3lN5jpbbymFQIHGrrIU1RO9IV7DQlgC1Bq1C9cIcR0RRM9thyON EhMMJ+aJpubo/pjDcOWjPbbL/9aPzGHa190/5I6Y/7doJslbG1cge4yANE2qSbW3x+w1 uE5wRRFt1Wxaz5tFT70LcxCGcaPXDlLPzqD0WHVsNChXxbaFOGCZHCATjNf60SWl8P06 SefSLAelAN/W+HlAYqt9ynWlgf8zCOiI6LA5e6ZYjN7hXtq0cV2tTGqvc2ENnBaiGOtw BhUAYw5eCIOnZOxIpc7Q+fFf7EZvR+uYiV6H53ncTalVDX03BwnnZpoD0+cNpha2DRYs KAiQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731087089; x=1731691889; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=zdaXBeNWyZ7c/i1bDtqSJBAzKjVQd4iRvFi0ww8IGu8=; b=qJ3UHF1tgbGhw5krjNRU10xnQ22S65gOzAGqw2sG4PfDdb7zOdAN+ST3Ki+JyjvKDk 11c6hWCP8AbnTJT7JyWp3almKkNeQBBkwy6VXcrHNiCSGtz7WoGfDV2chctcosuNHYHa YjYHwUNXzmreLAZqbe3kkx3+15SwCn5vQ5I3tlJK8y/1iR+YiARj6Kc49B1+WJounb1k D8op+yDaV4DkfoqdizUjkHlngXVLFOj44gP+fQMp3ACYmdbZLGw2hyW2lopI8t+3FyFe 28TZidwPj4jwjn8O+pZ4+zJWyM67n9OE/2fjnKJzPq7ipTsuee/zMbwNnj2fY+cEzSAc eS5Q== X-Gm-Message-State: AOJu0Yz4EpUMgC17/c+k9GrHJv8HKG6pTvrcAopn9G11GGtXgaxjX1jU iEsGcCI0n36WsOsBOdPrBn5+XLYeJQhllXAA/rORAijEIev3GdMTL3DX/zYoGcSmEn0aTzPF1Uz rJfKrV/H7neOEAkNZ/jArOvd0W9YlKrsGyltfQm/9OkKX0rDb5+4vaXjwtayEs7vxahDC68kzcg Wir7aGXpXErC3k70Z3kPktlDMfeWoh04eGVfXQgVwrydfb X-Google-Smtp-Source: AGHT+IEtfx13XXxnqCu8Jua9YbiGFF1RaXUopxFBuDK3OGSxSnNYic4KS5VXZ3Y9jzdV0WzgdtedF9msK33FgnM= X-Received: from barleywine.c.googlers.com ([fda3:e722:ac3:cc00:ef:85c8:ac13:4239]) (user=calvinwan job=sendgmr) by 2002:a65:5cc1:0:b0:7c6:af2b:196f with SMTP id 41be03b00d2f7-7f430af7482mr19130a12.6.1731087086398; Fri, 08 Nov 2024 09:31:26 -0800 (PST) Date: Fri, 8 Nov 2024 17:31:12 +0000 In-Reply-To: <20241108173112.1240584-1-calvinwan@google.com> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20241108173112.1240584-1-calvinwan@google.com> X-Mailer: git-send-email 2.47.0.277.g8800431eea-goog Message-ID: <20241108173112.1240584-2-calvinwan@google.com> Subject: [RFC PATCH 1/1] maintenance: separate parallelism safe and unsafe tasks From: Calvin Wan To: git@vger.kernel.org Cc: Calvin Wan , steamdon@google.com, emrass@google.com, ps@pks.im, me@ttaylorr.com, stolee@gmail.com Certain maintenance tasks and subtasks within gc are unsafe to run in parallel with other commands because they lock up files such as HEAD. 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 --- 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 @@ -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")); if (prune_expire_arg != prune_expire_sentinel) { free(cfg.prune_expire); @@ -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, }, [TASK_GC] = { "gc", maintenance_task_gc, need_to_gc, + UNSAFE, 1, }, [TASK_COMMIT_GRAPH] = { "commit-graph", maintenance_task_commit_graph, should_write_commit_graph, + SAFE, }, [TASK_PACK_REFS] = { "pack-refs", maintenance_task_pack_refs, pack_refs_condition, + UNSAFE, }, }; @@ -1411,10 +1494,18 @@ static int compare_tasks_by_selection(const void *a_, const void *b_) return b->selected_order - a->selected_order; } +static int compare_tasks_by_safeness(const void *a_, const void *b_) +{ + const struct maintenance_task *a = a_; + const struct maintenance_task *b = b_; + + return a->daemonize_safe - b->daemonize_safe; +} + static int maintenance_run_tasks(struct maintenance_run_opts *opts, struct gc_config *cfg) { - int i, found_selected = 0; + int i, j, found_selected = 0; int result = 0; struct lock_file lk; struct repository *r = the_repository; @@ -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; + } + + QSORT(tasks, TASK__COUNT, compare_tasks_by_safeness); + + for (j = 0; j < TASK__COUNT; j++) { + if (tasks[j].daemonize_safe == SAFE) + break; + + if (found_selected && tasks[j].selected_order < 0) + continue; + + if (!found_selected && !tasks[j].enabled) + continue; + + if (opts->auto_flag && + (!tasks[j].auto_condition || + !tasks[j].auto_condition(cfg))) + continue; + + if (opts->schedule && tasks[j].schedule < opts->schedule) + continue; + + trace2_region_enter("maintenance", tasks[j].name, r); + if (tasks[j].fn(opts, cfg)) { + error(_("task '%s' failed"), tasks[j].name); + result = 1; + } + trace2_region_leave("maintenance", tasks[j].name, r); + } + } + /* Failure to daemonize is ok, we'll continue in foreground. */ if (opts->detach > 0) { trace2_region_enter("maintenance", "detach", the_repository); @@ -1443,33 +1585,28 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts, trace2_region_leave("maintenance", "detach", the_repository); } - 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); - - for (i = 0; i < TASK__COUNT; i++) { - if (found_selected && tasks[i].selected_order < 0) + for (j = j; j < TASK__COUNT; j++) { + if (found_selected && tasks[j].selected_order < 0) continue; - if (!found_selected && !tasks[i].enabled) + if (!found_selected && !tasks[j].enabled) continue; if (opts->auto_flag && - (!tasks[i].auto_condition || - !tasks[i].auto_condition(cfg))) + (!tasks[j].auto_condition || + !tasks[j].auto_condition(cfg))) continue; - if (opts->schedule && tasks[i].schedule < opts->schedule) + if (opts->schedule && tasks[j].schedule < opts->schedule) continue; - trace2_region_enter("maintenance", tasks[i].name, r); - if (tasks[i].fn(opts, cfg)) { - error(_("task '%s' failed"), tasks[i].name); + // fprintf(stderr, "running %i: %s\n",j , tasks[j].name); + trace2_region_enter("maintenance", tasks[j].name, r); + if (tasks[j].fn(opts, cfg)) { + error(_("task '%s' failed"), tasks[j].name); result = 1; } - trace2_region_leave("maintenance", tasks[i].name, r); + trace2_region_leave("maintenance", tasks[j].name, r); } rollback_lock_file(&lk); diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index c224c8450c..5bbd07ec30 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -43,17 +43,19 @@ test_expect_success 'help text' ' test_grep "usage: git maintenance" err ' -test_expect_success 'run [--auto|--quiet]' ' - GIT_TRACE2_EVENT="$(pwd)/run-no-auto.txt" \ - git maintenance run 2>/dev/null && - GIT_TRACE2_EVENT="$(pwd)/run-auto.txt" \ - git maintenance run --auto 2>/dev/null && - GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \ - git maintenance run --no-quiet 2>/dev/null && - test_subcommand git gc --quiet --no-detach /dev/null && +# GIT_TRACE2_EVENT="$(pwd)/run-auto.txt" \ +# git maintenance run --auto 2>/dev/null && +# GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \ +# git maintenance run --no-quiet 2>/dev/null && +# test_subcommand git gc --quiet --no-detach