threads / patch / 5792

patchpack-refs: use lockfile as everybody else does.

Subject: [PATCH] pack-refs: use lockfile as everybody else does.

## tl;dr

4 messages between Oct 3, 2006 and Oct 5, 2006. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Junio C Hamano· Oct 3, 2006, 09:19 UTC · lore
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 I failed to spot a difference from the result/lock-path thing in
 builtin-pack-ref and what lockfile interface does for refs and
 index file.  They seem to do the essentially same thing.  Did I
 miss something?
 Also I am not sure about the "fsync(fd); fclose(refs_file)"
 sequence I did not touch with this patch.  Doesn't stdio still
 have stuff buffered when you run fsync()?  Would adding fflush()
 in between help?
 builtin-pack-refs.c |   21 ++++-----------------
 1 files changed, 4 insertions(+), 17 deletions(-)
Show changes to builtin-pack-refs.c +4 −17
diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
index 4093973..ede4743 100644
--- a/builtin-pack-refs.c
+++ b/builtin-pack-refs.c
@@ -1,7 +1,6 @@
 #include "cache.h"
 #include "refs.h"
 
-static const char *result_path, *lock_path;
 static const char builtin_pack_refs_usage[] =
 "git-pack-refs [--prune]";
 
@@ -17,12 +16,6 @@ struct pack_refs_cb_data {
 	FILE *refs_file;
 };
 
-static void remove_lock_file(void)
-{
-	if (lock_path)
-		unlink(lock_path);
-}
-
 static int do_not_prune(int flags)
 {
 	/* If it is already packed or if it is a symref,
@@ -69,6 +62,8 @@ static void prune_refs(struct ref_to_pru
 	}
 }
 
+static struct lock_file packed;
+
 int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 {
 	int fd, i;
@@ -88,14 +83,7 @@ int cmd_pack_refs(int argc, const char *
 	if (i != argc)
 		usage(builtin_pack_refs_usage);
 
-	result_path = xstrdup(git_path("packed-refs"));
-	lock_path = xstrdup(mkpath("%s.lock", result_path));
-
-	fd = open(lock_path, O_CREAT | O_EXCL | O_WRONLY, 0666);
-	if (fd < 0)
-		die("unable to create new ref-pack file (%s)", strerror(errno));
-	atexit(remove_lock_file);
-
+	fd = hold_lock_file_for_update(&packed, git_path("packed-refs"), 1);
 	cbdata.refs_file = fdopen(fd, "w");
 	if (!cbdata.refs_file)
 		die("unable to create ref-pack file structure (%s)",
@@ -103,9 +91,8 @@ int cmd_pack_refs(int argc, const char *
 	for_each_ref(handle_one_ref, &cbdata);
 	fsync(fd);
 	fclose(cbdata.refs_file);
-	if (rename(lock_path, result_path) < 0)
+	if (commit_lock_file(&packed) < 0)
 		die("unable to overwrite old ref-pack file (%s)", strerror(errno));
-	lock_path = NULL;
 	if (cbdata.prune)
 		prune_refs(cbdata.ref_to_prune);
 	return 0;

-- 
1.4.2.3.gd1e9e
Linus Torvalds· Oct 5, 2006, 02:55 UTC · re: Junio C Hamano · lore

Re: [PATCH] pack-refs: use lockfile as everybody else does.

On Tue, 3 Oct 2006, Junio C Hamano wrote:
Show 5 quoted lines
> 
>  Also I am not sure about the "fsync(fd); fclose(refs_file)"
>  sequence I did not touch with this patch.  Doesn't stdio still
>  have stuff buffered when you run fsync()?  Would adding fflush()
>  in between help?
Yes, there should be a fflush() there before the fsync.
		Linus
Junio C Hamano· Oct 5, 2006, 06:15 UTC · re: Linus Torvalds · lore

Re: [PATCH] pack-refs: use lockfile as everybody else does.

Linus Torvalds <torvalds@osdl.org> writes:
> Yes, there should be a fflush() there before the fsync.

While I have your attention... The code that follows the committing of packed-refs file by calling rename() to move the lock file to its final name prunes already packed refs, and we should really make sure the effect of rename() survives before starting to remove loose refs from the filesystem.

How would one ensure the effect of rename(2) hits the disk platter before proceeding to do something else? We seem to do sync(1) in git-repack.sh for similar reasons, and I wonder if we should do a sync(2) there. I doubt it would be worth it though; the function can return before the actual writing is done.

If the write-out of metainfo is ordered enough that if we rename the packed-refs lock file to its final destination and then we unlink loose refs, it might be reasonably safe to assume that rename(2)'s effect has already hit the disk when the effect of unlink(2) does, of the machine crashes in the meantime, as long as none of the later unlink hits the disk before rename does it is Ok.

Linus Torvalds· Oct 5, 2006, 14:51 UTC · re: Junio C Hamano · lore

Re: [PATCH] pack-refs: use lockfile as everybody else does.

On Wed, 4 Oct 2006, Junio C Hamano wrote:
Show 6 quoted lines
> 
> How would one ensure the effect of rename(2) hits the disk
> platter before proceeding to do something else?  We seem to do
> sync(1) in git-repack.sh for similar reasons, and I wonder if we
> should do a sync(2) there.  I doubt it would be worth it though;
> the function can return before the actual writing is done.

sync() is supposed to wait until the data is on the disk, anything else is likely to be a bad implementation (even if POSIX may not _guarantee_ it).

Also, most filesystems (for various reasons, not the least of which is "speed of fsck") will journal metadata, but not file contents. Which means that if a crash happens and one of the loose refs has already been removed, then (thanks to journaling) that means that on most filesystems the rename would already have been guaranteed to hit the disk - even regardless of any sync().

So in practice, it's much more important to guarantee the file contents (the fsync) part, because the OS already tends to guarantee metadata ordering.

			Linus

← back to recent threads