From: Linus Torvalds <torvalds@linux-foundation.org>
To: Nicolas Pitre <nico@cam.org>
Cc: Jason McMullan <jason.mcmullan@gmail.com>,
git@vger.kernel.org, bfields@fieldses.org
Subject: Re: "failed to read delta base object at..."
Date: Wed, 27 Aug 2008 12:48:00 -0700 (PDT) [thread overview]
Message-ID: <alpine.LFD.1.10.0808271222250.3363@nehalem.linux-foundation.org> (raw)
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)
next prev parent reply other threads:[~2008-08-27 19:50 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-08-25 16:46 "failed to read delta base object at..." J. Bruce Fields
2008-08-25 18:58 ` Nicolas Pitre
2008-08-25 21:18 ` J. Bruce Fields
2008-08-25 19:01 ` Linus Torvalds
2008-08-25 21:31 ` J. Bruce Fields
2008-08-25 21:37 ` Linus Torvalds
2008-08-25 22:13 ` J. Bruce Fields
2008-08-25 23:59 ` Linus Torvalds
2008-08-26 20:43 ` Jason McMullan
2008-08-26 21:01 ` Jason McMullan
2008-08-27 17:05 ` Linus Torvalds
2008-08-27 19:17 ` Nicolas Pitre
2008-08-27 19:48 ` Linus Torvalds [this message]
2008-08-27 20:46 ` Nicolas Pitre
2008-08-29 2:05 ` [PATCH 0/3] don't let disk corruptions escape pack SHA1 checksum Nicolas Pitre
2008-08-29 2:07 ` [PATCH 1/3] improve reliability of fixup_pack_header_footer() Nicolas Pitre
2008-08-29 2:07 ` [PATCH 2/3] pack-objects: use fixup_pack_header_footer()'s validation mode Nicolas Pitre
2008-08-29 2:07 ` [PATCH 3/3] index-pack: " Nicolas Pitre
2008-08-29 4:44 ` [PATCH 1/3] improve reliability of fixup_pack_header_footer() Shawn O. Pearce
2008-08-29 13:08 ` Nicolas Pitre
2008-08-29 14:30 ` Shawn O. Pearce
2008-08-29 20:07 ` [PATCH 0/5] pack header rewriting improvements Nicolas Pitre
2008-08-29 20:07 ` [PATCH 1/5] pack-objects: improve returned information from write_one() Nicolas Pitre
2008-08-29 20:07 ` [PATCH 2/5] improve reliability of fixup_pack_header_footer() Nicolas Pitre
2008-08-29 20:08 ` [PATCH 3/5] pack-objects: use fixup_pack_header_footer()'s validation mode Nicolas Pitre
2008-08-29 20:08 ` [PATCH 4/5] index-pack: " Nicolas Pitre
2008-08-29 20:08 ` [PATCH 5/5] fixup_pack_header_footer(): use nicely aligned buffer sizes Nicolas Pitre
2008-08-31 7:10 ` Junio C Hamano
2008-08-29 20:14 ` [PATCH 1/3] improve reliability of fixup_pack_header_footer() Nicolas Pitre
2008-08-29 4:55 ` [PATCH 0/3] don't let disk corruptions escape pack SHA1 checksum Shawn O. Pearce
2008-08-26 20:55 ` "failed to read delta base object at..." J. Bruce Fields
2008-08-27 20:14 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LFD.1.10.0808271222250.3363@nehalem.linux-foundation.org \
--to=torvalds@linux-foundation.org \
--cc=bfields@fieldses.org \
--cc=git@vger.kernel.org \
--cc=jason.mcmullan@gmail.com \
--cc=nico@cam.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).