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

[Jim Meyering]

Olaf Hering <address@hidden> wrote:
> On Tue, Aug 05, Soren Hansen wrote:
>
>> +#if defined(O_DIRECT)
>
> It is safe to drop the entire thing.
> ifarch like that, in generic code, asks for trouble.

Hi Olaf,

Thanks for the patch.

I agree wholeheartedly that arch-specific #ifdefs are best
avoided, but am a little leery of removing O_DIRECT altogether.
However, I'm currently leaning towards accepting this.
Has this change been tested much?

Also, it'd be better to check for fsync failure (i.e. it can
fail with EIO, which would be serious).  If you'd also like
to check those close calls for failure, I wouldn't complain ;-)

> ---
>  libparted/arch/linux.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> --- a/libparted/arch/linux.c
> +++ b/libparted/arch/linux.c
> @@ -65,15 +65,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;
> @@ -1253,6 +1247,7 @@ _flush_cache (PedDevice* dev)
>                          fd = open (name, WR_MODE, 0);
>                          if (fd > 0) {
>                                  ioctl (fd, BLKFLSBUF);
> +                             fsync (fd);
>                                  close (fd);
>                          }
>                  }
> @@ -1315,6 +1310,7 @@ linux_close (PedDevice* dev)
>
>          if (dev->dirty)
>                  _flush_cache (dev);
> +        fsync (arch_specific->fd);
>          close (arch_specific->fd);
>          return 1;
>  }
>
> _______________________________________________
> parted-devel mailing list
> address@hidden
> http://lists.alioth.debian.org/mailman/listinfo/parted-devel