Re: [parted-devel] [PATCH] Fall back to not using O_DIRECT

[Jim Meyering]

Colin Watson wrote:
> On Fri, Aug 08, 2008 at 04:00:20PM +0200, Jim Meyering wrote:
>> Olaf Hering <address@hidden> 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                                       address@hidden
>
>>From 0f8f1c9dbaae6f3ccd4840a3b0d3815bffeb3f46 Mon Sep 17 00:00:00 2001
> From: Colin Watson <address@hidden>
> 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(-)

Hi Colin,

Thanks a lot for finishing that change.
It looks fine and passes the tests, so I pushed it to "next".