Package: coreutils;
Reported by: "jeff.liu" <jeff.liu <at> oracle.com>
Date: Fri, 7 May 2010 14:16:02 UTC
Severity: normal
Tags: patch
Done: Jim Meyering <jim <at> meyering.net>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: "jeff.liu" <jeff.liu <at> oracle.com> To: Jim Meyering <jim <at> meyering.net> Cc: Sunil Mushran <sunil.mushran <at> oracle.com>, Paul Eggert <eggert <at> CS.UCLA.EDU>, bug-coreutils <at> gnu.org, Joel Becker <Joel.Becker <at> oracle.com>, Tao Ma <tao.ma <at> oracle.com>, Pádraig Brady <P <at> draigBrady.com>, Chris Mason <chris.mason <at> oracle.com> Subject: bug#6131: [PATCH]: fiemap support for efficient sparse file copy Date: Wed, 29 Sep 2010 16:11:41 +0800
Jim Meyering wrote:
> jeff.liu wrote:
>> Sorry for the delay.
>>
>> This is the new patch to isolate the stuff regarding to extents reading to a new module. and teach
>> cp(1) to make use of it.
>
> Jeff,
>
> I applied your patch to my rebased fiemap-copy branch.
> My first step was to run the usual
>
> ./bootstrap && ./configure && make && make check
>
> "make check" failed on due to a double free in your new code:
> (x86_64, Fedora 13, ext4 working directory)
>
> To get details, I made this temporary modification:
Hi Jim,
I am sorry for the fault, it fixed at the patch below.
Would you please revie at your convenience?
Changes:
========
1. fix write_zeros() as Jim's comments, thanks for pointing this out.
2. remove char const *fname from struct extent_scan.
3. change the signature of open_extent_scan() from "void open_extent_scan(struct extent_scan
**scan)" to "void open_extent_scan(struct extent_scan *scan)"; the reason is I'd like to reduce once
memory allocation for the extent_scan variable, instead, using stack to save it.
4. remove close_extent_scan() from a function defined at extent-scan.c to extent-scan.h as a Macro
definination, but it does nothing for now, since initial extent scan defined at stack.
5. add a macro "free_extents_info()" defined at extent-scan.h to release the memory allocated to
extent info which should be called combine with get_extents_info(), it just one line, so IMHO,
define it as macro should be ok.
I have done the memory check via `valgrind`, no issue found.
make test against cp/sparse-fiemap failed at the extent compare stage, but the file content is
identical to each other by comparing those two files "j1/j2" manually.
Is it make sense if we verify them through diff(1) since the testing file is in small size?
or we have to merge the contig extents from the output of `filefrag', I admit I have not dig into
the filefrag-extent-compare at the moment, I need to recall the perl language syntax. :-P.
From 50a3338db06442fa2d789fd65175172d140cc96e Mon Sep 17 00:00:00 2001
From: Jie Liu <jeff.liu <at> oracle.com>
Date: Wed, 29 Sep 2010 15:35:43 +0800
Subject: [PATCH 1/1] cp: add a new module for scanning extents
* src/extent-scan.c: Source code for scanning extents.
Call open_extent_scan() to initialize extent scan.
Call get_extents_info() to get a number of extents for each iteration.
* src/extent-scan.h: Header file of extent-scan.c.
Wrap free_extent_info() as macro define to release the space allocated extent_info per extent scan.
Wrap close_extent_scan() as macro define but do nothing at the moment.
* src/Makefile.am: Reference it and link it to copy_source.
* src/copy.c: Make use of the new module, replace fiemap_copy() with extent_copy().
Signed-off-by: Jie Liu <jeff.liu <at> oracle.com>
---
src/Makefile.am | 2 +-
src/copy.c | 197 ++++++++++++++++++++++++++++++----------------------
src/extent-scan.c | 113 ++++++++++++++++++++++++++++++
src/extent-scan.h | 68 ++++++++++++++++++
4 files changed, 296 insertions(+), 84 deletions(-)
create mode 100644 src/extent-scan.c
create mode 100644 src/extent-scan.h
diff --git a/src/Makefile.am b/src/Makefile.am
index 7d56312..2412b16 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -459,7 +459,7 @@ uninstall-local:
fi; \
fi
-copy_sources = copy.c cp-hash.c
+copy_sources = copy.c cp-hash.c extent-scan.c
# Use `ginstall' in the definition of PROGRAMS and in dependencies to avoid
# confusion with the `install' target. The install rule transforms `ginstall'
diff --git a/src/copy.c b/src/copy.c
index f48c74d..42ec300 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -35,6 +35,7 @@
#include "buffer-lcm.h"
#include "copy.h"
#include "cp-hash.h"
+#include "extent-scan.h"
#include "error.h"
#include "fcntl--.h"
#include "file-set.h"
@@ -63,10 +64,6 @@
#include <sys/ioctl.h>
-#ifndef HAVE_FIEMAP
-# include "fiemap.h"
-#endif
-
#ifndef HAVE_FCHOWN
# define HAVE_FCHOWN false
# define fchown(fd, uid, gid) (-1)
@@ -153,74 +150,79 @@ clone_file (int dest_fd, int src_fd)
#endif
}
-#ifdef __linux__
-# ifndef FS_IOC_FIEMAP
-# define FS_IOC_FIEMAP _IOWR ('f', 11, struct fiemap)
-# endif
-/* Perform a FIEMAP copy, if possible.
- Call ioctl(2) with FS_IOC_FIEMAP (available in linux 2.6.27) to
- obtain a map of file extents excluding holes. This avoids the
- overhead of detecting holes in a hole-introducing/preserving copy,
- and thus makes copying sparse files much more efficient. Upon a
- successful copy, return true. If the initial ioctl fails, set
- *NORMAL_COPY_REQUIRED to true and return false. Upon any other
- failure, set *NORMAL_COPY_REQUIRED to false and return false. */
static bool
-fiemap_copy (int src_fd, int dest_fd, size_t buf_size,
- off_t src_total_size, char const *src_name,
- char const *dst_name, bool *normal_copy_required)
+write_zeros (int fd, uint64_t n_bytes)
{
- bool last = false;
- 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);
+ static char *zeros;
+ static size_t nz = IO_BUFSIZE;
+
+ if (zeros == NULL)
+ {
+ static char fallback[1024];
+ zeros = calloc (nz, 1);
+ if (zeros == NULL)
+ {
+ zeros = fallback;
+ nz = sizeof fallback;
+ }
+ }
+
+ while (n_bytes)
+ {
+ uint64_t n = MIN (sizeof nz, n_bytes);
+ if ((full_write (fd, zeros, n)) != n)
+ return false;
+ n_bytes -= n;
+ }
+
+ return true;
+}
+/* Perform an efficient extent copy, if possible. This avoids
+ the overhead of detecting holes in hole-introducing/preserving
+ copy, and thus makes copying sparse files much more efficient.
+ Upon a successful copy, return true. If the initial extent scan
+ fails, set *NORMAL_COPY_REQUIRED to true and return false.
+ Upon any other failure, set *NORMAL_COPY_REQUIRED to false and
+ return false. */
+static bool
+extent_copy (int src_fd, int dest_fd, size_t buf_size,
+ off_t src_total_size, bool make_holes,
+ char const *src_name, char const *dst_name,
+ bool *require_normal_copy)
+{
+ struct extent_scan scan;
off_t last_ext_logical = 0;
uint64_t last_ext_len = 0;
uint64_t last_read_size = 0;
- unsigned int i = 0;
- *normal_copy_required = false;
+ unsigned int i;
+ bool ok = true;
- /* 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);
+ open_extent_scan (src_fd, &scan);
do
{
- fiemap->fm_length = FIEMAP_MAX_OFFSET;
- fiemap->fm_flags = FIEMAP_FLAG_SYNC;
- fiemap->fm_extent_count = count;
-
- /* When ioctl(2) fails, fall back to the normal copy only if it
- is the first time we met. */
- if (ioctl (src_fd, FS_IOC_FIEMAP, fiemap) < 0)
+ ok = get_extents_info (&scan);
+ if (! ok)
{
- /* If the first ioctl fails, tell the caller that it is
- ok to proceed with a normal copy. */
- if (i == 0)
- *normal_copy_required = true;
- else
+ if (scan.hit_last_extent)
+ break;
+
+ if (scan.initial_scan_failed)
{
- /* If the second or subsequent ioctl fails, diagnose it,
- since it ends up causing the entire copy/cp to fail. */
- error (0, errno, _("%s: FIEMAP ioctl failed"), quote (src_name));
+ close_extent_scan (&scan);
+ *require_normal_copy = true;
+ return false;
}
+
+ error (0, errno, _("failed to get extents info %s"), quote (src_name));
return false;
}
- /* If 0 extents are returned, then more ioctls are not needed. */
- if (fiemap->fm_mapped_extents == 0)
- break;
-
- for (i = 0; i < fiemap->fm_mapped_extents; i++)
+ for (i = 0; i < scan.ei_count; i++)
{
- assert (fm_ext[i].fe_logical <= OFF_T_MAX);
-
- off_t ext_logical = fm_ext[i].fe_logical;
- uint64_t ext_len = fm_ext[i].fe_length;
+ off_t ext_logical = scan.ext_info[i].ext_logical;
+ uint64_t ext_len = scan.ext_info[i].ext_length;
if (lseek (src_fd, ext_logical, SEEK_SET) < 0)
{
@@ -228,27 +230,37 @@ fiemap_copy (int src_fd, int dest_fd, size_t buf_size,
return false;
}
- if (lseek (dest_fd, ext_logical, SEEK_SET) < 0)
+ if (make_holes)
{
- error (0, errno, _("cannot lseek %s"), quote (dst_name));
- return false;
+ if (lseek (dest_fd, ext_logical, SEEK_SET) < 0)
+ {
+ error (0, errno, _("cannot lseek %s"), quote (dst_name));
+ return false;
+ }
}
-
- if (fm_ext[i].fe_flags & FIEMAP_EXTENT_LAST)
+ else
{
- last_ext_logical = ext_logical;
- last_ext_len = ext_len;
- last = true;
+ /* If not making a sparse file, write zeros to the destination
+ file if there is a hole between the last and current extent. */
+ if (last_ext_logical + last_ext_len < ext_logical)
+ {
+ uint64_t holes_len = ext_logical - last_ext_logical - last_ext_len;
+ if (! write_zeros (dest_fd, holes_len))
+ return false;
+ }
}
+ last_ext_logical = ext_logical;
+ last_ext_len = ext_len;
+ last_read_size = 0;
+
while (ext_len)
{
char buf[buf_size];
/* Avoid reading into the holes if the left extent
length is shorter than the buffer size. */
- if (ext_len < buf_size)
- buf_size = ext_len;
+ buf_size = MIN (ext_len, buf_size);
ssize_t n_read = read (src_fd, buf, buf_size);
if (n_read < 0)
@@ -258,12 +270,12 @@ fiemap_copy (int src_fd, int dest_fd, size_t buf_size,
continue;
#endif
error (0, errno, _("reading %s"), quote (src_name));
- return false;
+ return false;
}
if (n_read == 0)
{
- /* Figure out how many bytes read from the last extent. */
+ /* Figure out how many bytes read from the previous extent. */
last_read_size = last_ext_len - ext_len;
break;
}
@@ -278,27 +290,44 @@ fiemap_copy (int src_fd, int dest_fd, size_t buf_size,
}
}
- fiemap->fm_start = fm_ext[i - 1].fe_logical + fm_ext[i - 1].fe_length;
+ /* Release the space allocated to scan->ext_info. */
+ free_extents_info (&scan);
+ } while (! scan.hit_last_extent);
- } while (! last);
+ /* Do nothing now. */
+ close_extent_scan (&scan);
/* If a file ends up with holes, the sum of the last extent logical offset
- and the read-returned size will be shorter than the actual size of the
- file. Use ftruncate to extend the length of the destination file. */
- if (last_ext_logical + last_read_size < src_total_size)
+ and the read-returned size or the last extent length will be shorter than
+ the actual size of the file. Use ftruncate to extend the length of the
+ destination file if make_holes, or write zeros up to the actual size of the
+ file. */
+ if (make_holes)
{
- if (ftruncate (dest_fd, src_total_size) < 0)
+ if (last_ext_logical + last_read_size < src_total_size)
{
- error (0, errno, _("failed to extend %s"), quote (dst_name));
- return false;
+ if (ftruncate (dest_fd, src_total_size) < 0)
+ {
+ error (0, errno, _("failed to extend %s"), quote (dst_name));
+ return false;
+ }
+ }
+ }
+ else
+ {
+ if (last_ext_logical + last_ext_len < src_total_size)
+ {
+ uint64_t holes_len = src_total_size - last_ext_logical - last_ext_len;
+ if (0 < holes_len)
+ {
+ if (! write_zeros (dest_fd, holes_len))
+ return false;
+ }
}
}
return true;
}
-#else
-static bool fiemap_copy (ignored) { errno == ENOTSUP; return false; }
-#endif
/* FIXME: describe */
/* FIXME: rewrite this to use a hash table so we avoid the quadratic
@@ -833,11 +862,13 @@ copy_reg (char const *src_name, char const *dst_name,
if (make_holes)
{
bool require_normal_copy;
- /* Perform efficient FIEMAP copy for sparse files, fall back to the
- standard copy only if the ioctl(2) fails. */
- if (fiemap_copy (source_desc, dest_desc, buf_size,
- src_open_sb.st_size, src_name,
- dst_name, &require_normal_copy))
+ /* Perform efficient extent copy for sparse file, fall back to the
+ standard copy only if the initial extent scan fails. If the
+ '--sparse=never' option was specified, we writing all data but
+ use extent copy if available to efficiently read. */
+ if (extent_copy (source_desc, dest_desc, buf_size,
+ src_open_sb.st_size, make_holes,
+ src_name, dst_name, &require_normal_copy))
goto preserve_metadata;
else
{
diff --git a/src/extent-scan.c b/src/extent-scan.c
new file mode 100644
index 0000000..f371b87
--- /dev/null
+++ b/src/extent-scan.c
@@ -0,0 +1,113 @@
+/* extent-scan.c -- core functions for scanning extents
+ Copyright (C) 2010 Free Software Foundation, Inc.
+
+ This program is free software: you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation, either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+ Written by Jie Liu (jeff.liu <at> oracle.com). */
+
+#include <config.h>
+#include <stdio.h>
+#include <sys/types.h>
+#include <sys/ioctl.h>
+#include <assert.h>
+
+#include "system.h"
+#include "extent-scan.h"
+#include "error.h"
+#include "quote.h"
+
+#ifndef HAVE_FIEMAP
+# include "fiemap.h"
+#endif
+
+/* Allocate space for struct extent_scan, initialize the entries if
+ necessary and return it as the input argument of get_extents_info(). */
+extern void
+open_extent_scan (int src_fd, struct extent_scan *scan)
+{
+ scan->fd = src_fd;
+ scan->ei_count = 0;
+ scan->scan_start = 0;
+ scan->initial_scan_failed = false;
+ scan->hit_last_extent = false;
+}
+
+#ifdef __linux__
+# ifndef FS_IOC_FIEMAP
+# define FS_IOC_FIEMAP _IOWR ('f', 11, struct fiemap)
+# endif
+/* Call ioctl(2) with FS_IOC_FIEMAP (available in linux 2.6.27) to
+ obtain a map of file extents excluding holes. */
+extern bool
+get_extents_info (struct extent_scan *scan)
+{
+ union { struct fiemap f; char c[4096]; } fiemap_buf;
+ struct fiemap *fiemap = &fiemap_buf.f;
+ struct fiemap_extent *fm_extents = &fiemap->fm_extents[0];
+ enum { count = (sizeof fiemap_buf - sizeof *fiemap) / sizeof *fm_extents };
+ verify (count != 0);
+ unsigned int i;
+
+ /* 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);
+
+ fiemap->fm_start = scan->scan_start;
+ fiemap->fm_flags = FIEMAP_FLAG_SYNC;
+ fiemap->fm_extent_count = count;
+ fiemap->fm_length = FIEMAP_MAX_OFFSET - scan->scan_start;
+
+ /* Fall back to the standard copy if call ioctl(2) failed for the
+ the first time. */
+ if (ioctl (scan->fd, FS_IOC_FIEMAP, fiemap) < 0)
+ {
+ if (scan->scan_start == 0)
+ scan->initial_scan_failed = true;
+ return false;
+ }
+
+ /* If 0 extents are returned, then more get_extent_table() are not needed. */
+ if (fiemap->fm_mapped_extents == 0)
+ {
+ scan->hit_last_extent = true;
+ return false;
+ }
+
+ scan->ei_count = fiemap->fm_mapped_extents;
+ scan->ext_info = xnmalloc (scan->ei_count, sizeof (struct extent_info));
+
+ for (i = 0; i < scan->ei_count; i++)
+ {
+ assert (fm_extents[i].fe_logical <= OFF_T_MAX);
+
+ scan->ext_info[i].ext_logical = fm_extents[i].fe_logical;
+ scan->ext_info[i].ext_length = fm_extents[i].fe_length;
+ scan->ext_info[i].ext_flags = fm_extents[i].fe_flags;
+ }
+
+ i--;
+ if (scan->ext_info[i].ext_flags & FIEMAP_EXTENT_LAST)
+ {
+ scan->hit_last_extent = true;
+ return true;
+ }
+
+ scan->scan_start = fm_extents[i].fe_logical + fm_extents[i].fe_length;
+
+ return true;
+}
+#else
+extern bool get_extents_info (ignored) { errno = ENOTSUP; return false; }
+#endif
diff --git a/src/extent-scan.h b/src/extent-scan.h
new file mode 100644
index 0000000..07c2e5b
--- /dev/null
+++ b/src/extent-scan.h
@@ -0,0 +1,68 @@
+/* core functions for efficient reading sparse files
+ Copyright (C) 2010 Free Software Foundation, Inc.
+
+ This program is free software: you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation, either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+ Written by Jie Liu (jeff.liu <at> oracle.com). */
+
+#ifndef EXTENT_SCAN_H
+# define EXTENT_SCAN_H
+
+/* Structure used to reserve information of each extent. */
+struct extent_info
+{
+ /* Logical offset of an extent. */
+ off_t ext_logical;
+
+ /* Extent length. */
+ uint64_t ext_length;
+
+ /* Extent flags, use it for FIEMAP only, or set it to zero. */
+ uint32_t ext_flags;
+};
+
+/* Structure used to reserve extent scan information per file. */
+struct extent_scan
+{
+ /* File descriptor of extent scan run against. */
+ int fd;
+
+ /* Next scan start offset. */
+ off_t scan_start;
+
+ /* How many extent info returned for a scan. */
+ uint32_t ei_count;
+
+ /* If true, fall back to a normal copy, either
+ set by the failure of ioctl(2) for FIEMAP or
+ lseek(2) with SEEK_DATA. */
+ bool initial_scan_failed;
+
+ /* If ture, the total extent scan per file has been finished. */
+ bool hit_last_extent;
+
+ /* Extent information. */
+ struct extent_info *ext_info;
+};
+
+void
+open_extent_scan (int src_fd, struct extent_scan *scan);
+
+bool
+get_extents_info (struct extent_scan *scan);
+
+#define free_extents_info(ext_scan) free ((ext_scan)->ext_info)
+#define close_extent_scan(ext_scan) /* empty */
+
+#endif /* EXTENT_SCAN_H */
--
1.5.4.3
Regards,
-Jeff
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.