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: jeff.liu
Subject: bug#6131: [PATCH]: fiemap support for efficient sparse file copy
Date: Wed, 09 Jun 2010 17:07:33 +0800
User-agent: Thunderbird 2.0.0.14 (X11/20080505)

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 (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? 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 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 (fiemapcopy): 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 @@ fiemapcopy (int srcfd, int destfd, sizet bufsize, char const *dstname, bool *normalcopyrequired) { bool last = false; - char fiemapbuf[4096]; - struct fiemap *fiemap = (struct fiemap *) fiemapbuf; + union { struct fiemap f; char c[4096]; } fiemapbuf; + struct fiemap *fiemap = &fiemapbuf.f; struct fiemapextent *fmext = &fiemap->fmextents[0]; - uint32t count = ((sizeof fiemapbuf - sizeof (*fiemap)) - / sizeof (struct fiemapextent)); + enum { count = (sizeof fiemapbuf - sizeof *fiemap) / sizeof *fmext }; + verify (count != 0); + offt lastextlogical = 0; uint64t lastextlen = 0; uint64t lastreadsize = 0; @@ -185,7 +186,7 @@ fiemapcopy (int srcfd, int destfd, sizet bufsize, /* This is required at least to initialize fiemap->fmstart, but also serves (in mid 2010) to appease valgrind, which appears not to know the semantics of the FIEMAP ioctl. */ - memset (fiemapbuf, 0, sizeof fiemapbuf); + memset (&fiemapbuf, 0, sizeof fiemapbuf);

do { @@ -220,13 +221,13 @@ fiemapcopy (int srcfd, int destfd, sizet bufsize, offt extlogical = fmext[i].felogical; uint64t extlen = fmext[i].felength;

- if (lseek (srcfd, extlogical, SEEKSET) < 0LL) + if (lseek (srcfd, extlogical, SEEKSET) < 0) { error (0, errno, ("cannot lseek %s"), quote (srcname)); return false; }

- if (lseek (destfd, extlogical, SEEKSET) < 0LL) + if (lseek (destfd, extlogical, SEEKSET) < 0) { error (0, errno, ("cannot lseek %s"), quote (dstname)); return false; -- 1.7.1.501.g23b46

Also, I've squashed this clean-up patch onto Jeff's original, since extlen is unsigned (of type uint64t).

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 @@ fiemapcopy (int srcfd, int destfd, sizet bufsize, last = true; }

- while (0 < extlen) + while (extlen) { char buf[bufsize];

-- 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.