tag 635683 + patch
thanks
Hi,
On Fri, 12 Aug 2011, Jonathan Nieder wrote:
> Ok, here's a patch with better behavior. On Linux and similar platforms,
> it just opens the file for reading. If fsync fails with errno == EBADF,
> it falls back to opening for writing, and if that fails with errno ==
> ETXTBSY, it skips the fsync under the assumption that the file has already
> been fsynced.
I thought the same at first but all this is just wrong. If you look
carefully, you'll notice that only real files are fsync()ed and hardlinks
are not (look where fnnf_deferred_fsync is added to the flags).
In the tar archive, the first file in a set of hardlinked files appears as
a normal file and the others are then marked as hardlinks of the first
file.
What broke dpkg here is the the fact that hardlinks do not use
deferred renames and are thus put into places before the "target file"
is fsync()ed and renamed. Thus the minimal fix is this one:
> --- a/src/archives.c
> +++ b/src/archives.c
> @@ -851,7 +851,8 @@ tarobject(void *ctx, struct tar_entry *ti)
> * in .dpkg-new.
> */
>
> - if (ti->type == tar_filetype_file || ti->type == tar_filetype_symlink) {
> + if (ti->type == tar_filetype_file || ti->type == tar_filetype_symlink ||
> + ti->type == tar_filetype_hardlink) {
> nifd->namenode->flags |= fnnf_deferred_rename;
>
> debug(dbg_eachfiledetail, "tarobject done and installation deferred");
But from a design point of view, it just seems wrong to not have all files
treated the same. IMO we should be using deferred rename for all files except
directories.
Then one can wonder whether we ought to do something to ensure other types
of files are written on the disk too... and whether adding a fsync() on hardlink
brings anything. Since the hardlinks all point to the same inode, I guess
a fsync() on the hardlink doesn't add anything, the supplementary information
is just the availability of the same file on a different path and to
ensure persistency of this it's the fsync() on the directory that we would
need.
I have attached my suggested patch. It's tested and I confirm that it
fixes this issue.
Cheers,
--
Raphaël Hertzog ◈ Debian Developer
Follow my Debian News ▶ http://RaphaelHertzog.com (English)
▶ http://RaphaelHertzog.fr (Français)
commit 8f91f1da8defc4d8464fda8b100f966f655dceeb
Author: Raphaël Hertzog <hert...@debian.org>
Date: Fri Aug 12 21:23:58 2011 +0200
dpkg: use deferred rename for all files except directories
The decision to defer fsync/rename only of normal files and to
keep renaming the other directly types during the unpack
(commit 9cd41fdda1c27169c52d73b3b3ce71991d724994) has been
wrong first for the symlinks (commit
a766f501f6da46aca070c315e6429e163d188202) and we discover now
that it's also wrong for hardlinks: due to this change hardlinks
are put into place before the "target" file. This opens up the
possibility that the file gets executed (via the hardlink) before
the deferred fsync/rename which then fails with ETXTBSY.
Instead of making yet another exception for hardlinks, it's time
to decide that deferred rename is the norm and only directories
should be exempted from this.
This change also means that when we create hardlinks we can assume the
target file is a .dpkg-new one and we can get rid of the check on
fnnf_deferred_rename to compute the correct path.
Reported-by: Niko Tyni <nt...@debian.org>
diff --git a/debian/changelog b/debian/changelog
index e875ca6..7c6343d 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -98,6 +98,9 @@ dpkg (1.16.1) UNRELEASED; urgency=low
* Fix possible segfault of dpkg in findbreakcycle(). LP: #733414
* dpkg-source now properly cleans up the temporary tarball generated for
native formats in case of unexpected interruption. Closes: #631494
+ * Fix dpkg to defer rename of hardlinks just like normal files. Otherwise
+ we might end up trying to fsync() a file that is already used and
+ fail with ETXTBSY. Closes: #635683
[ Guillem Jover ]
* Install deb-src-control(5) man pages in dpkg-dev. Closes: #620520
diff --git a/src/archives.c b/src/archives.c
index f060e70..c8d80aa 100644
--- a/src/archives.c
+++ b/src/archives.c
@@ -764,8 +764,7 @@ tarobject(void *ctx, struct tar_entry *ti)
varbuf_add_char(&hardlinkfn, '/');
linknode = findnamenode(ti->linkname, 0);
varbuf_add_str(&hardlinkfn, namenodetouse(linknode, tc->pkg)->name);
- if (linknode->flags & fnnf_deferred_rename)
- varbuf_add_str(&hardlinkfn, DPKGNEWEXT);
+ varbuf_add_str(&hardlinkfn, DPKGNEWEXT);
varbuf_end_str(&hardlinkfn);
if (link(hardlinkfn.buf,fnamenewvb.buf))
ohshite(_("error creating hard link `%.255s'"), ti->name);
@@ -851,7 +850,7 @@ tarobject(void *ctx, struct tar_entry *ti)
* in .dpkg-new.
*/
- if (ti->type == tar_filetype_file || ti->type == tar_filetype_symlink) {
+ if (ti->type != tar_filetype_dir) {
nifd->namenode->flags |= fnnf_deferred_rename;
debug(dbg_eachfiledetail, "tarobject done and installation deferred");
