On Fri, Aug 08, 2008 at 04:00:20PM +0200, Jim Meyering wrote:
> Olaf Hering <o...@suse.de> wrote:
> > On Wed, Aug 06, Jim Meyering wrote:
> >> Can I expect you to adjust your patch to detect
> >> fsync and close failures?
> >
> > How should it handle the failures?
> > parted cant do anything about the error in practice.
>
> Parted should report the write error, and propagate the failure "up" the
> call tree. Any time Parted ignores a write failure, that is a potential
> for serious data loss. An obvious bug.
>
> The hard part (given the current implementation) is making a write
> failure translate to a parted exit status that is nonzero. For now,
> if you would at least make it diagnose any failure, that'd be enough.
>
> For example, _do_fsync detects fsync failure and reports it.
> Speaking of _do_fsync, the added fsync calls in your patch end up
> being redundant with the fsync call performed by _do_fsync in some
> code paths, but that's probably not worth worrying about.
It's not clear that anyone ever made the changes Jim requested. How
about the attached patch, which incorporates Olaf's previous patch and
turns fsync/close failures into a retry/ignore exception? I tested this
with the following LD_PRELOADable wrapper; compile with 'gcc -Wall -c -o
break-fsync.o break-fsync.c && gcc -shared -o break-fsync.so
break-fsync.o -ldl', and run with 'LD_PRELOAD=/path/to/break-fsync.so'.
#define _GNU_SOURCE
#include <errno.h>
#include <dlfcn.h>
#include <unistd.h>
static int (*_fsync)(int fd) = NULL;
int fsync(int fd)
{
if (!_fsync)
_fsync = dlsym(RTLD_NEXT, "fsync");
_fsync(fd);
errno = EIO;
return -1;
}
Thanks,
--
Colin Watson [cjwat...@ubuntu.com]
>From 0f8f1c9dbaae6f3ccd4840a3b0d3815bffeb3f46 Mon Sep 17 00:00:00 2001
From: Colin Watson <cjwat...@ubuntu.com>
Date: Fri, 24 Jul 2009 18:25:22 +0100
Subject: [PATCH] Use fsync rather than O_DIRECT
O_DIRECT doesn't work on all filesystems (e.g. tmpfs), and fsync does
just as good a job to ensure that buffers are flushed.
Based on
http://lists.alioth.debian.org/pipermail/parted-devel/2008-August/002392.html
by Olaf Hering, but with added fsync/close error checking.
---
libparted/arch/linux.c | 27 +++++++++++++++++++--------
1 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
index f875581..06b9327 100644
--- a/libparted/arch/linux.c
+++ b/libparted/arch/linux.c
@@ -66,15 +66,9 @@
#define HDIO_GETGEO 0x0301 /* get device geometry */
#define HDIO_GET_IDENTITY 0x030d /* get IDE identification info */
-#if defined(O_DIRECT) && !(defined(__s390__) || defined(__s390x__))
-#define RD_MODE (O_RDONLY | O_DIRECT)
-#define WR_MODE (O_WRONLY | O_DIRECT)
-#define RW_MODE (O_RDWR | O_DIRECT)
-#else
#define RD_MODE (O_RDONLY)
#define WR_MODE (O_WRONLY)
#define RW_MODE (O_RDWR)
-#endif
struct hd_geometry {
unsigned char heads;
@@ -1408,7 +1402,16 @@ _flush_cache (PedDevice* dev)
fd = open (name, WR_MODE, 0);
if (fd > 0) {
ioctl (fd, BLKFLSBUF);
- close (fd);
+retry:
+ if (fsync (fd) < 0 || close (fd) < 0)
+ if (ped_exception_throw (
+ PED_EXCEPTION_WARNING,
+ PED_EXCEPTION_RETRY +
+ PED_EXCEPTION_IGNORE,
+ _("Error fsyncing/closing %s: %s"),
+ name, strerror (errno))
+ == PED_EXCEPTION_RETRY)
+ goto retry;
}
}
free (name);
@@ -1470,7 +1473,15 @@ linux_close (PedDevice* dev)
if (dev->dirty)
_flush_cache (dev);
- close (arch_specific->fd);
+retry:
+ if (fsync (arch_specific->fd) < 0 || close (arch_specific->fd) < 0)
+ if (ped_exception_throw (
+ PED_EXCEPTION_WARNING,
+ PED_EXCEPTION_RETRY + PED_EXCEPTION_IGNORE,
+ _("Error fsyncing/closing %s: %s"),
+ dev->path, strerror (errno))
+ == PED_EXCEPTION_RETRY)
+ goto retry;
return 1;
}
--
1.6.3.3
_______________________________________________
bug-parted mailing list
bug-parted@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-parted
