[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#6131: [PATCH]: fiemap support for efficient sparse file copy
From: |
jeff.liu |
Subject: |
bug#6131: [PATCH]: fiemap support for efficient sparse file copy |
Date: |
Tue, 21 Sep 2010 14:47:10 +0800 |
User-agent: |
Thunderbird 2.0.0.14 (X11/20080505) |
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
>
>
>
- bug#6131: [PATCH]: fiemap support for efficient sparse file copy, jeff.liu, 2010/09/20
- bug#6131: [PATCH]: fiemap support for efficient sparse file copy, Jim Meyering, 2010/09/20
- bug#6131: [PATCH]: fiemap support for efficient sparse file copy,
jeff.liu <=
- bug#6131: [PATCH]: fiemap support for efficient sparse file copy, jeff.liu, 2010/09/26
- bug#6131: [PATCH]: fiemap support for efficient sparse file copy, Jim Meyering, 2010/09/26
- bug#6131: [PATCH]: fiemap support for efficient sparse file copy, jeff.liu, 2010/09/26
- bug#6131: [PATCH]: fiemap support for efficient sparse file copy, jeff.liu, 2010/09/27
- bug#6131: [PATCH]: fiemap support for efficient sparse file copy, Jim Meyering, 2010/09/28
- bug#6131: [PATCH]: fiemap support for efficient sparse file copy, jeff.liu, 2010/09/29
- bug#6131: [PATCH]: fiemap support for efficient sparse file copy, Jim Meyering, 2010/09/28
- bug#6131: [PATCH]: fiemap support for efficient sparse file copy, jeff.liu, 2010/09/29