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

Previous Next

Full log

View this message in rfc822 format

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: bug#6131: [PATCH]: fiemap support for efficient sparse file copy
Date: Tue, 28 Sep 2010 11:08:24 +0800

jeff.liu wrote:
> Hi Jim,
> 
> Thanks for your prompt response, I will fix this issue when all review done.
Hi Jim,

For my current implementation, I just have another thought to remove the "char *fname" from struct
extent_scan, and add a new item "int errno" to save the errno set by ioctl(2) or lseek(2) if either
call failed.
at first, I am intended to use "char *fname" for the debugging purpose inside, however, maybe its
better to do such things outside of the module according to the return value and errno. this change
can not only reduce the memory allocation for 'fname' but also make a neatly open_extent_scan()
interface.

/* Structure used to reserve extent scan information per file.  */
struct extent_scan
{
 ....
  int errno;
  ....
};

void
open_extent_scan (int src_fd, struct extent_scan **scan);


Is it better?

Thanks,
-Jeff

> 
> 
> Regards,
> -Jeff
> 
> Jim Meyering wrote:
>> Hi Jeff,
>>
>> This function has problems:
>>   - the inner "zeros" declaration shadows the outer one
>>       and ends up being useless.
>>   - the "sizeof zeros" resolves to 4 or 8.  obviously not what you intended.
>>
>> ...
>>>  static bool
>>> +write_zeros (int fd, uint64_t n_bytes)
>>>  {
>>> -  bool last = false;
>>> -  union { struct fiemap f; char c[4096]; } fiemap_buf;
>>> -  struct fiemap *fiemap = &fiemap_buf.f;
>>> -  struct fiemap_extent *fm_ext = &fiemap->fm_extents[0];
>>> -  enum { count = (sizeof fiemap_buf - sizeof *fiemap) / sizeof *fm_ext };
>>> -  verify (count != 0);
>>> +  char *zeros = calloc (IO_BUFSIZE, sizeof (char));
>>> +  if (! zeros)
>>> +    {
>>> +      /* Try a small buffer.  */
>>> +      static char zeros[1024];
>>> +    }
>>>
>>> +  while (n_bytes)
>>> +    {
>>> +      uint64_t n = MIN (sizeof zeros, n_bytes);
>>> +      if ((full_write (fd, zeros, n)) != n)
>>> +        return false;
>>> +      n_bytes -= n;
>>> +    }
>>> +
>>> +  return true;
>>> +}
>> Please use the following instead.
>> I'll review the rest tomorrow or Tuesday.
>>
>> static bool
>> write_zeros (int fd, uint64_t n_bytes)
>> {
>>   static char *zeros;
>>   static size_t nz = IO_BUFSIZE;
>>
>>   if (zeros == NULL)
>>     {
>>       static char fallback[1024];
>>       zeros = calloc (nz, 1);
>>       if (zeros == NULL)
>>         {
>>           zeros = fallback;
>>           nz = sizeof fallback;
>>         }
>>     }
>>
>>   while (n_bytes)
>>     {
>>       uint64_t n = MIN (nz, n_bytes);
>>       if ((full_write (fd, zeros, n)) != n)
>>         return false;
>>       n_bytes -= n;
>>     }
>>
>>   return true;
>> }
> 
> 
> 
> 

Previous Next

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