bug#6131: [PATCH]: fiemap support for efficient sparse file copy

[jeff.liu]

Jim Meyering wrote:
> Paul Eggert wrote:
>> Jeff Liu and Jim Meyering wrote:
>>> diff --git a/src/fiemap.h b/src/fiemap.h
>>> new file mode 100644
>>> index 0000000..d33293b
>>> --- /dev/null
>>> +++ b/src/fiemap.h
> 
> Hi Paul,
> 
> Thanks for the review.
> 
>> Why is this file necessary?  fiemap.h is included only if it exists,
>> right?  Shouldn't we just use the kernel's fiemap.h rather than
>> copying it here and assuming kernel internals?
> 
> The ioctl is available/usable in 2.6.27 and newer that do not publish
> this file.  For example, it's in F13's (2.6.33's) /usr/include/linux/fiemap.h,
> as well as the one in debian unstable's 2.6.32, but probably
> not in much older kernels.
> 
> Hmm..  I see that it's available even in F11's 2.6.30.9-x
> 
> It would be better to include <linux/fiemap.h> when present,
> else our copy of that header.  Then, eventually, the else
> clause will become unused.  Note that even on newer kernels,
> the linux/* headers are optional.
> 
> Eventually we'll have a hard requirement on kernel headers --
> at least when building against a linux kernel.
> 
>>>          if (lseek (src_fd, ext_logical, SEEK_SET) < 0LL)
>> For this sort of thing, please just use "0" rather than "0LL".
>> "0" is easier to read and it has the same effect here.
> 
> Included in the patch below.
> 
>>>              char buf[buf_size];
>> This assumes C99, since buf_size is not known at compile time.
>> Also, isn't there a potential segmentation-violation problem
>> if buf_size is sufficiently large?
>>
>> More generally, since the caller is already allocating a buffer of the
>> appropiate size, shouldn't we just reuse that buffer, rather than
>> allocating a new one?  That would avoid the problems of assuming
>> C99 and possible segmentation violations.
> 
> Good point.  Thanks.
> We can definitely avoid that allocation.
> Do you feel like writing the patch?
Thanks for pointing this out!

Hi Paul,

I am appreciated if you have time for writing the patch.  Or else, I will 
follow up sometime in the
next few days since I have an urgent task need to handle over at present.


Regards,
-Jeff

> 
> I've just pushed this series to a branch, "fiemap-copy",
> so others can follow along and contribute more easily.
> 
>>>   char fiemap_buf[4096];
>>>   struct fiemap *fiemap = (struct fiemap *) fiemap_buf;
>>>   struct fiemap_extent *fm_ext = &fiemap->fm_extents[0];
>>>   uint32_t count = ((sizeof fiemap_buf - sizeof (*fiemap))
>>>                     / sizeof (struct fiemap_extent));
>> This code isn't portable, since fiemap_buf is only char-aligned, and
>> struct fiemap may well require stricter alignment.  The code will work
>> on the x86 despite the alignment problem, but that's just a happy
>> coincidence.
>>
>> A lesser point: the code assumes that 'struct fiemap' is sufficiently
>> small (considerably less than 4096 bytes in size); I expect that this
>> is universally true but we might as well check this assumption, since
>> it's easy to do so without any runtime overhead.
>>
>> So I propose something like this instead:
>>
>>    union { struct fiemap f; char c[4096]; } fiemap_buf;
>>    struct fiemap *fiemap = &fiemap_buf.f;
>>    struct fiemap_extent *fm_ext = &fiemap->fm_extents[0];
>>    enum { count = (sizeof fiemap_buf - sizeof *fiemap) / sizeof *fm_ext };
>>    verify (count != 0);
> 
> I've done this in your name:
> 
> From fffa2e8661a27978927fcc8afb6873631a753292 Mon Sep 17 00:00:00 2001
> From: Paul Eggert <address@hidden>
> Date: Wed, 9 Jun 2010 08:15:07 +0200
> Subject: [PATCH] copy.c: ensure proper alignment of fiemap buffer
> 
> * src/copy.c (fiemap_copy): Ensure that our fiemap buffer
> is large enough and well-aligned.
> Replace "0LL" with equivalent "0" as 3rd argument to lseek.
> ---
>  src/copy.c |   15 ++++++++-------
>  1 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/src/copy.c b/src/copy.c
> index f629771..27e083a 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -171,11 +171,12 @@ fiemap_copy (int src_fd, int dest_fd, size_t buf_size,
>               char const *dst_name, bool *normal_copy_required)
>  {
>    bool last = false;
> -  char fiemap_buf[4096];
> -  struct fiemap *fiemap = (struct fiemap *) fiemap_buf;
> +  union { struct fiemap f; char c[4096]; } fiemap_buf;
> +  struct fiemap *fiemap = &fiemap_buf.f;
>    struct fiemap_extent *fm_ext = &fiemap->fm_extents[0];
> -  uint32_t count = ((sizeof fiemap_buf - sizeof (*fiemap))
> -                    / sizeof (struct fiemap_extent));
> +  enum { count = (sizeof fiemap_buf - sizeof *fiemap) / sizeof *fm_ext };
> +  verify (count != 0);
> +
>    off_t last_ext_logical = 0;
>    uint64_t last_ext_len = 0;
>    uint64_t last_read_size = 0;
> @@ -185,7 +186,7 @@ fiemap_copy (int src_fd, int dest_fd, size_t buf_size,
>    /* This is required at least to initialize fiemap->fm_start,
>       but also serves (in mid 2010) to appease valgrind, which
>       appears not to know the semantics of the FIEMAP ioctl. */
> -  memset (fiemap_buf, 0, sizeof fiemap_buf);
> +  memset (&fiemap_buf, 0, sizeof fiemap_buf);
> 
>    do
>      {
> @@ -220,13 +221,13 @@ fiemap_copy (int src_fd, int dest_fd, size_t buf_size,
>            off_t ext_logical = fm_ext[i].fe_logical;
>            uint64_t ext_len = fm_ext[i].fe_length;
> 
> -          if (lseek (src_fd, ext_logical, SEEK_SET) < 0LL)
> +          if (lseek (src_fd, ext_logical, SEEK_SET) < 0)
>              {
>                error (0, errno, _("cannot lseek %s"), quote (src_name));
>                return false;
>              }
> 
> -          if (lseek (dest_fd, ext_logical, SEEK_SET) < 0LL)
> +          if (lseek (dest_fd, ext_logical, SEEK_SET) < 0)
>              {
>                error (0, errno, _("cannot lseek %s"), quote (dst_name));
>                return false;
> --
> 1.7.1.501.g23b46
> 
> 
> Also, I've squashed this clean-up patch onto Jeff's original,
> since ext_len is unsigned (of type uint64_t).
> 
> From bad13e737c683757a2ed05404564d8863c5da30e Mon Sep 17 00:00:00 2001
> From: Jim Meyering <address@hidden>
> Date: Wed, 9 Jun 2010 08:24:39 +0200
> Subject: [PATCH] remove 0 <
> 
> ---
>  src/copy.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/src/copy.c b/src/copy.c
> index 27e083a..f149be4 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -240,7 +240,7 @@ fiemap_copy (int src_fd, int dest_fd, size_t buf_size,
>                last = true;
>              }
> 
> -          while (0 < ext_len)
> +          while (ext_len)
>              {
>                char buf[buf_size];
> 
> --
> 1.7.1.501.g23b46
> 
> 
> 


-- 
With Windows 7, Microsoft is asserting legal control over your computer and is 
using this power to
abuse computer users.