bug#6131: [PATCH]: fiemap support for efficient sparse file copy

[Jim Meyering]

jeff.liu wrote:
> Hi Jim and All,
>
> Do you have any comments for the current implementation?

Hi Jeff,

Thanks for the reminder.
I've just gone back and looked at those patches:

    http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/20534/focus=21008

There are some superficial problems.

First, whenever you move code from one place to another,
the commit that performs the move should be careful to
induce no other change.  In this case, the change should
remove code from copy.c and create the new .c file with
code that is essentially identical.  You'll have to remove
a "static" attribute on the primary function(s), and add
#include directives in the new file, but those are inevitable.
Also, in copy.c, you will remove the function body
and associated #include directives, adding an #include
of the new .h file.  Of course, this change must also update
src/Makefile.am, and the result should pass "make distcheck".

But perhaps you require new functions like init_extent_scan
in order to use the abstracted function properly.
In that case, your first commit would add the new functions
in copy.c and make use of them there.  Then you would move
all of the functions to their new file in the next commit,
making no semantic change.

Note however, that this copying code is intended to be usable in a
multi-threaded application, and thus must avoid using internal static
state.  Your patch adds a few new static variables, each of which
would cause trouble in such an application:

  +static size_t current_scanned_extents_count = 0;
  +  static uint64_t next_map_start = 0;
  +  static size_t i = 0;

While you're rearranging your patch along the lines above,
please eliminate those static variables, too.

Also, this new function should be adjusted:

  +/* Write zeros as holes to the destination file.  */
  +static bool
  +fill_with_holes_ok (int dest_fd, const char *dst_name,
  +                    char *buf, size_t buf_size,
  +                    uint64_t holes_len)

Its signature is unnecessarily complicated for a function
that does nothing more than write N zero bytes to a file descriptor.
Also, the function name is misleading (as is its holes_len parameter),
since it certainly does not create holes.

Hmm... now I'm suspicious: could the second use of your fill_with_holes_ok
write from an uninitialized "buf"?  It appears that is possible,
but I confess not to have applied the patch.

What do you think of this signature,

  static bool
  write_zeros (int fd, uint64_t n_bytes)

That would require a buffer full of zeros, preferably of optimal size.
It could use a body like this,

{
  static char zero[32 * 1024];
  while (n_bytes)
    {
      uint64_t n = MIN (sizeof zero, n_bytes);
      if (full_write (fd, zero, n)) != n)
        return false;
      n_bytes -= n;
    }
  return true;
}

or even calloc an IO_BUFSIZE-byte buffer on the first call
and use that.  Yes, using calloc appears better, since this code
will end up being used relatively infrequently.  Or perhaps better
still, do use calloc, but if the allocation fails, fall back on
using a smaller static buffer, of size say 1024 bytes.

Of course, simplifying the function means each caller
will have to diagnose failure, but imho, that's preferable
in this case.

Jim