Re: [PATCH] copy: adjust fiemap handling of sparse files

[Jim Meyering]

Pádraig Brady wrote:
...
> I'm going to use this "2.6.38" check to only enable FIEMAP_FLAG_SYNC
> before Linux kernel 2.6.38.  It's always worth avoiding sync if possible.
> Proposed patch attached.
> I'll submit my 3 outstanding fiemap patches tomorrow.
...
> Subject: [PATCH] copy: with fiemap copy, only sync when needed
>
> * src/extent-scan.h (struct extent_scan): Add the fm_flags member to
> pass to the fiemap scan.
> * src/extent-scan.c (extent_need_sync): A new function used to
> detect Linux kernels before 2.6.38.
> (extent_scan_init): Add FIEMAP_FLAG_SYNC when needed.
> * tests/cp/sparse-fiemap: Adjust comment.
> * NEWS: Mention the change in behavior.
> Indirectly suggested by Mike Frysinger
> ---
>  NEWS                   |    4 ++++
>  src/extent-scan.c      |   32 +++++++++++++++++++++++++++++++-
>  src/extent-scan.h      |    3 +++
>  tests/cp/sparse-fiemap |    4 ++--
>  4 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 9c4a16f..d1020cb 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -31,6 +31,10 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
>    bytes from the input, and also can efficiently create a hole in the
>    output file when --sparse=always is specified.
>
> +  cp now avoids syncing files when possible, when doing a FIEMAP copy.
> +  The sync in only needed on Linux kernels before 2.6.38.
> +  [The sync was introduced in coreutils-8.10]

Thanks.  This looks good.
Minor suggestions below:

> diff --git a/src/extent-scan.c b/src/extent-scan.c
...

> +/* Work around Linux kernel issues on BTRFS and EXT4 before 2.6.38.  */

Please add a comment here, e.g.,

/* FIXME: remove in 2013, or whenever we're pretty confident
   that the offending, unpatched kernels are no longer in use.  */

> +static bool
> +extent_need_sync (void)
> +{
> +  static int need_sync = -1;
> +
> +  if (need_sync == -1)
> +    {
> +      struct utsname name;
> +      need_sync = 0; /* No workaround by default.  */
> +
> +#ifdef __linux__
> +      if (uname (&name) != -1 && strncmp (name.release, "2.6.", 4) == 0)
> +        {
> +           unsigned long val;
> +           if (xstrtoul (name.release + 4, NULL, 0, &val, NULL) == 
> LONGINT_OK)

That 3rd argument is the conversion base.
Leaving it as 0 lets us accept octal and hexadecimal.
No big risk, obviously, but you can tighten it up by using 10 instead.

> +             {
> +               if (val < 38)
> +                 need_sync = 1;
> +             }
> +        }
> +#endif
> +    }
> +
> +  return need_sync;
> +}