Hi,
Raphael Hertzog wrote:
> 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.
Makes perfect sense. Thanks!
Here's the remainder of the train of thought I sent before. I don't
see any reason to apply it unless we have some information (e.g., perf
data) that suggests it helps.
-- >8 --
Subject: dpkg: try opening files for reading when fsync-ing
fsync() is not supposed to affect the file descriptor but the
underlying inode, so on most modern operating systems it does not care
whether it is passed a file descriptor open for reading or for
writing. Opening for reading can be slightly more lightweight --- for
example, on Linux it avoids having to allocate structures related to
quota.
However, some OSes (e.g., AIX) do require a descriptor open for
writing.
Therefore open files for reading, not writing, before fsyncing them
and only fall back to opening for writing if the system requires it.
Signed-off-by: Jonathan Nieder <jrnie...@gmail.com>
---
debian/changelog | 4 ++++
src/archives.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
2 files changed, 46 insertions(+), 9 deletions(-)
diff --git a/debian/changelog b/debian/changelog
index 7c6343d3..2b255171 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -186,6 +186,10 @@ dpkg (1.16.1) UNRELEASED; urgency=low
* Add new --raw-extract option to dpkg-deb combining --control and
--extract. Closes: #552123
+ [ Jonathan Nieder ]
+ * Open extracted files for reading, not writing, before fsyncing them.
+ Only fall back to opening for writing if fsync fails with errno == EBADF.
+
[ Updated dpkg translations ]
* German (Sven Joachim). Closes: #620312
* Swedish (Peter Krefting).
diff --git a/src/archives.c b/src/archives.c
index c8d80aad..c0dac081 100644
--- a/src/archives.c
+++ b/src/archives.c
@@ -911,6 +911,47 @@ tar_writeback_barrier(struct fileinlist *files, struct
pkginfo *pkg)
}
#endif
+static int
+open_checked(const char *path, int flags)
+{
+ int fd = open(path, flags);
+ if (fd < 0)
+ ohshite(_("unable to open '%.255s'"), path);
+ return fd;
+}
+
+static void
+close_checked(int fd, const char *path)
+{
+ if (close(fd))
+ ohshite(_("error closing/writing `%.255s'"), path);
+}
+
+static void
+fsync_path(const char *path)
+{
+ int fd;
+
+ fd = open_checked(path, O_RDONLY);
+ if (fsync(fd) == 0) {
+ close_checked(fd, path);
+ return;
+ }
+ if (errno != EBADF)
+ ohshite(_("unable to sync file '%.255s'"), path);
+
+ /*
+ * fsync() on some Unixen (e.g., AIX) requires "fd" to be
+ * open for writing, failing with EBADF when otherwise.
+ * Try again with O_WRONLY for their sake.
+ */
+ close(fd);
+ fd = open_checked(path, O_WRONLY);
+ if (fsync(fd))
+ ohshite(_("unable to sync file '%.255s'"), path);
+ close_checked(fd, path);
+}
+
void
tar_deferred_extract(struct fileinlist *files, struct pkginfo *pkg)
{
@@ -935,15 +976,7 @@ tar_deferred_extract(struct fileinlist *files, struct
pkginfo *pkg)
int fd;
debug(dbg_eachfiledetail, "deferred extract needs fsync");
-
- fd = open(fnamenewvb.buf, O_WRONLY);
- if (fd < 0)
- ohshite(_("unable to open '%.255s'"), fnamenewvb.buf);
- if (fsync(fd))
- ohshite(_("unable to sync file '%.255s'"), fnamenewvb.buf);
- if (close(fd))
- ohshite(_("error closing/writing `%.255s'"), fnamenewvb.buf);
-
+ fsync_path(fnamenewvb.buf);
cfile->namenode->flags &= ~fnnf_deferred_fsync;
}
--
1.7.6
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
