Re: "failed to read delta base object at..."

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Aug 27, 2008, 19:48 UTC
Message-ID: <alpine.LFD.1.10.0808271222250.3363@nehalem.linux-foundation.org>
In-Reply-To: <alpine.LFD.1.10.0808271458320.1624@xanadu.home>

On Wed, 27 Aug 2008, Nicolas Pitre wrote:

> 
> And isn't the bad data block size and alignment a bit odd for a 
> filesystem crash corruption?

Yes. If it was a filesystem issue, I'd expect it to be at least disk block aligned (512 bytes, most of the time) and more likely filesystem block aligned (ie mostly 4kB).

However, if we were to re-write the file afterwards, it could still get non-block-aligned corruption - simply because there was a non-block-aligned rewrite that got lost. But we don't actually ever do that, except for the header and the SHA1 at the end in some unusual cases.

> However, in the pack-objects case, it is almost impossible to have such 
> a corruption since the data is SHA1 summed immediately before being 
> written out.

Yes. Anything that uses the "sha1write()" model (which includes the regular pack-file _and_ the index) should generally be pretty safe.

However, we do have this odd case of fixing up the pack after-the-fact when we receive it from somebody else (because we get a thin pack and don't know how many objects the final result will have). And that case seems to be not as safe, because it

 - re-reads the file to recompute the SHA1

   This is understandable, and it's fairly ok, but it does mean that there 
   is a bigger chance of the SHA1 matching if something has corrupted the 
   file in the meantime!

   (That was not the case of this corruption, obviously, since the SHA1 
   didn't match)

 - but it also forgets to fsync the result, because it only did that in 
   one path rather in all cases of fixup.

   Again, this wasn't actually the cause of this corruption, because the 
   corruption wasn't near the header or tail, so if it had been due to a 
   missed write due to missing an fsync, the pattern would have been 
   different.

Anyway, we should fix the latter problem regardless, even if it's (a) damn unlikely and (b) definietly not the case in this thing.

The fix is trivial - just move the "fsync_or_die()" into the fixup routine rather than doing it in one of the callers.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

		Linus

---
 builtin-pack-objects.c |    1 -
 pack-write.c           |    1 +
 2 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 2dadec1..d394c49 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -499,7 +499,6 @@ static void write_pack_file(void)
 		} else {
 			int fd = sha1close(f, NULL, 0);
 			fixup_pack_header_footer(fd, sha1, pack_tmp_name, nr_written);
-			fsync_or_die(fd, pack_tmp_name);
 			close(fd);
 		}
 
diff --git a/pack-write.c b/pack-write.c
index a8f0269..ddcfd37 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -179,6 +179,7 @@ void fixup_pack_header_footer(int pack_fd,
 
 	SHA1_Final(pack_file_sha1, &c);
 	write_or_die(pack_fd, pack_file_sha1, 20);
+	fsync_or_die(pack_fd, pack_name);
 }
 
 char *index_pack_lockfile(int ip_out)