GNU bug report logs - #6131
[PATCH]: fiemap support for efficient sparse file copy

Previous Next

Full log

Message #278 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, 21 Sep 2010 14:47:10 +0800

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
> 
> 
> 

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.