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.
Message #308 received at submit <at> debbugs.gnu.org (full text, mbox):
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: Re: bug#6131: [PATCH]: fiemap support for efficient sparse file copy Date: Tue, 12 Oct 2010 13:15:11 +0800
Jim Meyering wrote:
> jeff.liu wrote:
>
>> 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?
>
> Thanks,
>
> Here are 5 changes on top of yours.
> I'll definitely adjust logs and maybe merge one or two before
> pushing anything. Just to be sure people understand, this series
> will not be in the upcoming release.
>
> Quick summary:
> - don't let write failure go unreported
> - make "distcheck" pass once again
> - rename functions to start with "extent_scan_"
> - remove unnecessary #include directives (part of "make syntax-check")
>
> None of this is pushed yet, but at least now it passes "make distcheck".
Thanks for the update, the new functions name looks better than me.
Regards,
-Jeff
>
> From ef3c2fe3760b11bc143b36246ee458ec86c484c9 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering <at> redhat.com>
> Date: Mon, 11 Oct 2010 11:19:02 +0200
> Subject: [PATCH 1/5] rename extent_scan member
>
> * extent-scan.h [struct extent_scan]: Rename member:
> s/hit_last_extent/hit_final_extent/. "final" is clearer,
> since "last" can be interpreted as "preceding".
> ---
> src/copy.c | 4 ++--
> src/extent-scan.c | 6 +++---
> src/extent-scan.h | 2 +-
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/copy.c b/src/copy.c
> index 43eeb74..1e1360e 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -208,7 +208,7 @@ extent_copy (int src_fd, int dest_fd, size_t buf_size,
> bool ok = get_extents_info (&scan);
> if (! ok)
> {
> - if (scan.hit_last_extent)
> + if (scan.hit_final_extent)
> break;
>
> if (scan.initial_scan_failed)
> @@ -302,7 +302,7 @@ extent_copy (int src_fd, int dest_fd, size_t buf_size,
>
> /* Release the space allocated to scan->ext_info. */
> free_extents_info (&scan);
> - } while (! scan.hit_last_extent);
> + } while (! scan.hit_final_extent);
>
> /* Do nothing now. */
> close_extent_scan (&scan);
> diff --git a/src/extent-scan.c b/src/extent-scan.c
> index f371b87..b0345f5 100644
> --- a/src/extent-scan.c
> +++ b/src/extent-scan.c
> @@ -40,7 +40,7 @@ open_extent_scan (int src_fd, struct extent_scan *scan)
> scan->ei_count = 0;
> scan->scan_start = 0;
> scan->initial_scan_failed = false;
> - scan->hit_last_extent = false;
> + scan->hit_final_extent = false;
> }
>
> #ifdef __linux__
> @@ -81,7 +81,7 @@ get_extents_info (struct extent_scan *scan)
> /* If 0 extents are returned, then more get_extent_table() are not needed. */
> if (fiemap->fm_mapped_extents == 0)
> {
> - scan->hit_last_extent = true;
> + scan->hit_final_extent = true;
> return false;
> }
>
> @@ -100,7 +100,7 @@ get_extents_info (struct extent_scan *scan)
> i--;
> if (scan->ext_info[i].ext_flags & FIEMAP_EXTENT_LAST)
> {
> - scan->hit_last_extent = true;
> + scan->hit_final_extent = true;
> return true;
> }
>
> diff --git a/src/extent-scan.h b/src/extent-scan.h
> index 07c2e5b..0c9c199 100644
> --- a/src/extent-scan.h
> +++ b/src/extent-scan.h
> @@ -50,7 +50,7 @@ struct extent_scan
> bool initial_scan_failed;
>
> /* If ture, the total extent scan per file has been finished. */
> - bool hit_last_extent;
> + bool hit_final_extent;
>
> /* Extent information. */
> struct extent_info *ext_info;
> --
> 1.7.3.1.104.gc752e
>
>
> From 7f38fe3ab8d1ee08f8ca4a96457df39da5bd1f70 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering <at> redhat.com>
> Date: Mon, 11 Oct 2010 11:44:12 +0200
> Subject: [PATCH 2/5] rename extent-scan functions to start with extent_scan_
>
> ---
> src/copy.c | 12 +++++-------
> src/extent-scan.c | 10 +++++-----
> src/extent-scan.h | 22 ++++++++++++----------
> 3 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/src/copy.c b/src/copy.c
> index 1e1360e..a7d10b8 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -201,11 +201,11 @@ extent_copy (int src_fd, int dest_fd, size_t buf_size,
> uint64_t last_ext_len = 0;
> uint64_t last_read_size = 0;
>
> - open_extent_scan (src_fd, &scan);
> + extent_scan_init (src_fd, &scan);
>
> do
> {
> - bool ok = get_extents_info (&scan);
> + bool ok = extent_scan_read (&scan);
> if (! ok)
> {
> if (scan.hit_final_extent)
> @@ -213,7 +213,6 @@ extent_copy (int src_fd, int dest_fd, size_t buf_size,
>
> if (scan.initial_scan_failed)
> {
> - close_extent_scan (&scan);
> *require_normal_copy = true;
> return false;
> }
> @@ -301,11 +300,10 @@ extent_copy (int src_fd, int dest_fd, size_t buf_size,
> }
>
> /* Release the space allocated to scan->ext_info. */
> - free_extents_info (&scan);
> - } while (! scan.hit_final_extent);
> + extent_scan_free (&scan);
>
> - /* Do nothing now. */
> - close_extent_scan (&scan);
> + }
> + while (! scan.hit_final_extent);
>
> /* If a file ends up with holes, the sum of the last extent logical offset
> and the read-returned size or the last extent length will be shorter than
> diff --git a/src/extent-scan.c b/src/extent-scan.c
> index b0345f5..97bb792 100644
> --- a/src/extent-scan.c
> +++ b/src/extent-scan.c
> @@ -32,9 +32,9 @@
> #endif
>
> /* Allocate space for struct extent_scan, initialize the entries if
> - necessary and return it as the input argument of get_extents_info(). */
> + necessary and return it as the input argument of extent_scan_read(). */
> extern void
> -open_extent_scan (int src_fd, struct extent_scan *scan)
> +extent_scan_init (int src_fd, struct extent_scan *scan)
> {
> scan->fd = src_fd;
> scan->ei_count = 0;
> @@ -50,14 +50,13 @@ open_extent_scan (int src_fd, struct extent_scan *scan)
> /* 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)
> +extent_scan_read (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
> @@ -88,6 +87,7 @@ get_extents_info (struct extent_scan *scan)
> scan->ei_count = fiemap->fm_mapped_extents;
> scan->ext_info = xnmalloc (scan->ei_count, sizeof (struct extent_info));
>
> + unsigned int i;
> for (i = 0; i < scan->ei_count; i++)
> {
> assert (fm_extents[i].fe_logical <= OFF_T_MAX);
> @@ -109,5 +109,5 @@ get_extents_info (struct extent_scan *scan)
> return true;
> }
> #else
> -extern bool get_extents_info (ignored) { errno = ENOTSUP; return false; }
> +extern bool extent_scan_read (ignored) { errno = ENOTSUP; return false; }
> #endif
> diff --git a/src/extent-scan.h b/src/extent-scan.h
> index 0c9c199..3119c8d 100644
> --- a/src/extent-scan.h
> +++ b/src/extent-scan.h
> @@ -19,7 +19,7 @@
> #ifndef EXTENT_SCAN_H
> # define EXTENT_SCAN_H
>
> -/* Structure used to reserve information of each extent. */
> +/* Structure used to store information of each extent. */
> struct extent_info
> {
> /* Logical offset of an extent. */
> @@ -44,25 +44,27 @@ struct extent_scan
> /* 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. */
> + /* 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. */
> + /* If true, the total extent scan per file has been finished. */
> bool hit_final_extent;
>
> - /* Extent information. */
> + /* Extent information: a malloc'd array of ei_count structs. */
> struct extent_info *ext_info;
> };
>
> void
> -open_extent_scan (int src_fd, struct extent_scan *scan);
> +extent_scan_init (int src_fd, struct extent_scan *scan);
>
> bool
> -get_extents_info (struct extent_scan *scan);
> +extent_scan_read (struct extent_scan *scan);
>
> -#define free_extents_info(ext_scan) free ((ext_scan)->ext_info)
> -#define close_extent_scan(ext_scan) /* empty */
> +static inline void
> +extent_scan_free (struct extent_scan *scan)
> +{
> + free (scan->ext_info);
> +}
>
> #endif /* EXTENT_SCAN_H */
> --
> 1.7.3.1.104.gc752e
>
>
> From e33ec433eb36b1a777f9591a63bcaee1b9e6c1bf Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering <at> redhat.com>
> Date: Mon, 11 Oct 2010 11:55:46 +0200
> Subject: [PATCH 3/5] distribute extent-scan.h, too
>
> * src/Makefile.am (copy_sources): Also distribute extent-scan.h.
> ---
> src/Makefile.am | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 7187596..de4c828 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -450,7 +450,7 @@ uninstall-local:
> fi; \
> fi
>
> -copy_sources = copy.c cp-hash.c extent-scan.c
> +copy_sources = copy.c cp-hash.c extent-scan.c extent-scan.h
>
> # Use `ginstall' in the definition of PROGRAMS and in dependencies to avoid
> # confusion with the `install' target. The install rule transforms `ginstall'
> --
> 1.7.3.1.104.gc752e
>
>
> From b0a1374189800a6e8edc2cfb5154199fe970ccd7 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering <at> redhat.com>
> Date: Mon, 11 Oct 2010 11:55:58 +0200
> Subject: [PATCH 4/5] formatting
>
> ---
> src/extent-scan.c | 7 ++++++-
> src/extent-scan.h | 6 ++----
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/src/extent-scan.c b/src/extent-scan.c
> index 97bb792..5160975 100644
> --- a/src/extent-scan.c
> +++ b/src/extent-scan.c
> @@ -109,5 +109,10 @@ extent_scan_read (struct extent_scan *scan)
> return true;
> }
> #else
> -extern bool extent_scan_read (ignored) { errno = ENOTSUP; return false; }
> +extern bool
> +extent_scan_read (struct extent_scan *scan ATTRIBUTE_UNUSED)
> +{
> + errno = ENOTSUP;
> + return false;
> +}
> #endif
> diff --git a/src/extent-scan.h b/src/extent-scan.h
> index 3119c8d..ac9e500 100644
> --- a/src/extent-scan.h
> +++ b/src/extent-scan.h
> @@ -55,11 +55,9 @@ struct extent_scan
> struct extent_info *ext_info;
> };
>
> -void
> -extent_scan_init (int src_fd, struct extent_scan *scan);
> +void extent_scan_init (int src_fd, struct extent_scan *scan);
>
> -bool
> -extent_scan_read (struct extent_scan *scan);
> +bool extent_scan_read (struct extent_scan *scan);
>
> static inline void
> extent_scan_free (struct extent_scan *scan)
> --
> 1.7.3.1.104.gc752e
>
>
> From f4513c41e656af44859587060ce9658241988cb1 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering <at> redhat.com>
> Date: Mon, 11 Oct 2010 12:00:07 +0200
> Subject: [PATCH 5/5] extent-scan.c: don't include error.h or quote.h
>
> * src/extent-scan.c: Don't include error.h or quote.h. Neither is used.
> ---
> src/extent-scan.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/src/extent-scan.c b/src/extent-scan.c
> index 5160975..3bb0d53 100644
> --- a/src/extent-scan.c
> +++ b/src/extent-scan.c
> @@ -24,8 +24,6 @@
>
> #include "system.h"
> #include "extent-scan.h"
> -#include "error.h"
> -#include "quote.h"
>
> #ifndef HAVE_FIEMAP
> # include "fiemap.h"
> --
> 1.7.3.1.104.gc752e
>
>
>
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.