bug#6131: [PATCH]: fiemap support for efficient sparse file copy (original) (raw)

[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

From: Jim Meyering
Subject: bug#6131: [PATCH]: fiemap support for efficient sparse file copy
Date: Wed, 09 Jun 2010 08:27:35 +0200

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 (srcfd, extlogical, SEEKSET) < 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[bufsize];

This assumes C99, since bufsize is not known at compile time. Also, isn't there a potential segmentation-violation problem if bufsize 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?

I've just pushed this series to a branch, "fiemap-copy", so others can follow along and contribute more easily.

> char fiemapbuf[4096]; > struct fiemap *fiemap = (struct fiemap *) fiemapbuf; > struct fiemapextent *fmext = &fiemap->fmextents[0]; > uint32t count = ((sizeof fiemapbuf - sizeof (*fiemap)) > / sizeof (struct fiemapextent));

This code isn't portable, since fiemapbuf 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]; } fiemapbuf; struct fiemap *fiemap = &fiemapbuf.f; struct fiemapextent *fmext = &fiemap->fmextents[0]; enum { count = (sizeof fiemapbuf - sizeof *fiemap) / sizeof *fmext }; 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 | 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;

@@ -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;

-- 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; }

-- 1.7.1.501.g23b46