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

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

[Junio C Hamano]

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(-)

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

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

[Linus Torvalds]

On Tue, 3 Oct 2006, Junio C Hamano wrote:
> 
>  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

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

[Junio C Hamano]

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.

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

[Linus Torvalds]

On Wed, 4 Oct 2006, Junio C Hamano wrote:
> 
> 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