On 2023/06/18 15:33:44 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> Generally this looks great and I am fine with you committing this.
> I have some suggestions below which you could follow up on if you
> have time. Otherwise I will get to making some tweaks eventually.

I committed with most of your comments addressed, thank you!

> [...]
> Should we lock the file before deleting it, like remove_packidx() which
> is used by gotadmin cleanup -p?
> In fact, should we call remove_packidx() here of unlinking .idx directly?
> 
> I see that git-repack doesn't seem to bother with such locking but it is
> confusing to have an "unlink_pack" function which doesn't take a lock
> and a "remove_packidx" function which takes a lock.

I'm still not sure about the locking needed here, in general.

I'm think that we should just lock the whole `gotadmin cleanup'
function, which would make locking before the removal of lonely idx
unneeded and fix concurred redundant packfiles removal[*].

I'm not seeing why we should lock the lonely index before removing it,
as we don't seem to check for that lock in other part of the codebase
AFAICS.  haven't checked git yet.

Diff below is what I think the locking should be like, but I know it's
not ok since it includes got_lib_lockfile.h in gotadmin.  I'm open to
suggestion for a got_repository_cleanup_lock() API should the lock
scheme be ok.

This _shouldn't_ make the locking of lonely objects redundant.

I've tested it by unpacking all the objects in (a copy of) got.git and
ran gotadmin cleanup, gotadmin cleanup -n and git gc in parallel.  git
gc errors immediately when it can't take the lock, gotadmin retries
and eventually succeed.  (unpacking was needed to make cleanup slow
enough to run another command.)

one last thing: git is very careful about writing the content of the
lockfile before "committing" it.  Since this is only needed for git gc
--force to provide some verbiage I don't care about partial writes or
races where git reads it before we write.


[*]: as we've discussed on irc, we're using a sorting that doesn't
     depend on the readdir(3) order and is stable wrt pack files with
     the same number of objects, so dangers of parallel "lockfree"
     `gotadmin cleanup' are none I believe.  It is still necessary to
     lock the whole operation however.


diff /home/op/w/got
commit - 9a7c12cfb1e1a7e36813ba8898c5281d5a0dbc30
path + /home/op/w/got
blob - b374606457024caef32ca71ee66bd3057552e4b1
file + gotadmin/gotadmin.c
--- gotadmin/gotadmin.c
+++ gotadmin/gotadmin.c
@@ -21,6 +21,7 @@
 #include <getopt.h>
 #include <err.h>
 #include <errno.h>
+#include <limits.h>
 #include <locale.h>
 #include <inttypes.h>
 #include <sha1.h>
@@ -45,6 +46,9 @@
 #include "got_opentemp.h"
 #include "got_worktree.h"
 
+/* XXX */
+#include "got_lib_lockfile.h"
+
 #ifndef nitems
 #define nitems(_a)	(sizeof((_a)) / sizeof((_a)[0]))
 #endif
@@ -1236,6 +1240,7 @@ cmd_cleanup(int argc, char *argv[])
 	struct got_repository *repo = NULL;
 	int ch, dry_run = 0, npacked = 0, verbosity = 0;
 	int remove_lonely_packidx = 0, ignore_mtime = 0;
+	struct got_lockfile *lf = NULL;
 	struct got_cleanup_progress_arg cpa;
 	struct got_lonely_packidx_progress_arg lpa;
 	off_t loose_before, loose_after;
@@ -1246,6 +1251,7 @@ cmd_cleanup(int argc, char *argv[])
 	char pack_before_scaled[FMT_SCALED_STRSIZE];
 	char pack_after_scaled[FMT_SCALED_STRSIZE];
 	char total_size_scaled[FMT_SCALED_STRSIZE];
+	char myname[HOST_NAME_MAX + 1];
 	int *pack_fds = NULL;
 
 #ifndef PROFILE
@@ -1316,6 +1322,20 @@ cmd_cleanup(int argc, char *argv[])
 		goto done;
 	}
 
+	if (gethostname(myname, sizeof(myname)) == -1) {
+		error = got_error_from_errno("gethostname");
+		goto done;
+	}
+
+	error = got_lockfile_lock(&lf, "gc.pid", got_repo_get_fd(repo));
+	if (error)
+		goto done;
+
+	if (dprintf(lf->fd, "%d %s", getpid(), myname) < 0) {
+		error = got_error_from_errno("dprintf");
+		goto done;
+	}
+
 	memset(&cpa, 0, sizeof(cpa));
 	cpa.last_ncommits = -1;
 	cpa.last_npurged = -1;
@@ -1381,6 +1401,13 @@ done:
 	}
 
 done:
+	if (lf) {
+		const struct got_error *unlock_err;
+
+		unlock_err = got_lockfile_unlock(lf, got_repo_get_fd(repo));
+		if (error == NULL)
+			error = unlock_err;
+	}
 	if (repo)
 		got_repo_close(repo);
 	if (pack_fds) {
blob - e1a1870147bfefcb7e3caaaedf16d6b92e78a5c1
file + lib/lockfile.c
--- lib/lockfile.c
+++ lib/lockfile.c
@@ -55,11 +55,11 @@ got_lockfile_lock(struct got_lockfile **lf, const char
 	do {
 		if (dir_fd != -1) {
 			(*lf)->fd = openat(dir_fd, (*lf)->path,
-			    O_RDONLY | O_CREAT | O_EXCL | O_EXLOCK | O_CLOEXEC,
+			    O_RDWR | O_CREAT | O_EXCL | O_EXLOCK | O_CLOEXEC,
 			    GOT_DEFAULT_FILE_MODE);
 		} else {
 			(*lf)->fd = open((*lf)->path,
-			    O_RDONLY | O_CREAT | O_EXCL | O_EXLOCK | O_CLOEXEC,
+			    O_RDWR | O_CREAT | O_EXCL | O_EXLOCK | O_CLOEXEC,
 			    GOT_DEFAULT_FILE_MODE);
 		}
 		if ((*lf)->fd != -1)
blob - 213bc1e83451612e1e2ba0be6bf664e3f4f44050
file + lib/repository_admin.c
--- lib/repository_admin.c
+++ lib/repository_admin.c
@@ -1419,21 +1419,6 @@ static const struct got_error *
 	return err;
 }
 
-static const struct got_error *
-remove_packidx(int dir_fd, const char *relpath)
-{
-	const struct got_error *err, *unlock_err;
-	struct got_lockfile *lf;
-
-	err = got_lockfile_lock(&lf, relpath, dir_fd);
-	if (err)
-		return err;
-	if (unlinkat(dir_fd, relpath, 0) == -1)
-		err = got_error_from_errno("unlinkat");
-	unlock_err = got_lockfile_unlock(lf, dir_fd);
-	return err ? err : unlock_err;
-}
-
 const struct got_error *
 got_repo_remove_lonely_packidx(struct got_repository *repo, int dry_run,
     got_lonely_packidx_progress_cb progress_cb, void *progress_arg,
@@ -1491,9 +1476,10 @@ got_repo_remove_lonely_packidx(struct got_repository *
 		}
 
 		if (!dry_run) {
-			err = remove_packidx(packdir_fd, dent->d_name);
-			if (err)
+			if (unlinkat(packdir_fd, dent->d_name, 0) == -1) {
+				err = got_error_from_errno("unlinkat");
 				goto done;
+			}
 		}
 		if (progress_cb) {
 			char *path;