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

[jeff.liu]

Hi Jim,

Thanks for your prompt response and kindly suggestion!
I totally agree with your review comments, I will post next round patches 
according to that soon.


Regards,
-Jeff

Jim Meyering wrote:
> 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
> 
> 
>