[PATCH 0/2] e2fsprogs/debugfs: do sparse copy when src is a sparse file

[PATCH 0/2] e2fsprogs/debugfs: do sparse copy when src is a sparse file

[Robert Yang]

* There are two patches, one is used for fixing the max length of the 
  argument, the other one is for sparsing copy when src is a sparse
  file.

* BTW., we are trying to use the debugfs to create an ext2/ext3/ext4
  image based on a given directory or file, which is similar to genext2fs,
  please vist here for the script you are interested in it: 

  https://gist.github.com/robertlinux/6034499

  It is an external shell script at the moment, the performance is not good,
  so we are going to:
  - Integrate this function into mke2fs in C language.
  - Add a [-d <directory>] option to specify the initial directory in mke2fs

  I'm not sure whether such a patch is acceptable or not, please feel free to
  give your comments.

Robert Yang
Wind River System

Robert Yang (2):
  debugfs.c: the max length of debugfs argument is too short
  debugfs.c: do sparse copy when src is a sparse file

 debugfs/debugfs.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 61 insertions(+), 11 deletions(-)

-- 
1.8.1.2

[PATCH 1/2] debugfs.c: the max length of debugfs argument is too short

[Robert Yang]

The max length of debugfs argument is 256 which is too short, the
arguments are two paths, the PATH_MAX is 4096 according to
/usr/include/linux/limits.h, use 2048 (4096 / 2) is a reasonable value.

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
Acked-by: Darren Hart <dvhart@linux.intel.com>
---
 debugfs/debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index dcf16e2..b77d0b5 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -2300,7 +2300,7 @@ void do_dump_mmp(int argc EXT2FS_ATTR((unused)), char *argv[])
 static int source_file(const char *cmd_file, int ss_idx)
 {
 	FILE		*f;
-	char		buf[256];
+	char		buf[2048];
 	char		*cp;
 	int		exit_status = 0;
 	int		retval;
-- 
1.8.1.2

[PATCH 2/2] debugfs.c: do sparse copy when src is a sparse file

[Robert Yang]

Let debugfs do sparse copy when src is a sparse file, just like
"cp --sparse=auto"

* For the
  #define IO_BUFSIZE 32*1024

  This is from coreutils-8.13/src/ioblksize.h (GPL V3):
/* As of Mar 2009, 32KiB is determined to be the minimium
   blksize to best minimize system call overhead.
   This can be tested with this script with the results
   shown for a 1.7GHz pentium-m with 2GB of 400MHz DDR2 RAM:

   for i in $(seq 0 10); do
     size=$((8*1024**3)) #ensure this is big enough
     bs=$((1024*2**$i))
     printf "%7s=" $bs
     dd bs=$bs if=/dev/zero of=/dev/null count=$(($size/$bs)) 2>&1 |
     sed -n 's/.* \([0-9.]* [GM]B\/s\)/\1/p'
   done

      1024=734 MB/s
      2048=1.3 GB/s
      4096=2.4 GB/s
      8192=3.5 GB/s
     16384=3.9 GB/s
     32768=5.2 GB/s
     65536=5.3 GB/s
    131072=5.5 GB/s
    262144=5.7 GB/s
    524288=5.7 GB/s
   1048576=5.8 GB/s

   Note that this is to minimize system call overhead.
   Other values may be appropriate to minimize file system
   or disk overhead.  For example on my current GNU/Linux system
   the readahead setting is 128KiB which was read using:

   file="."
   device=$(df -P --local "$file" | tail -n1 | cut -d' ' -f1)
   echo $(( $(blockdev --getra $device) * 512 ))

   However there isn't a portable way to get the above.
   In the future we could use the above method if available
   and default to io_blksize() if not.
 */
enum { IO_BUFSIZE = 32*1024 };

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
Acked-by: Darren Hart <dvhart@linux.intel.com>
---
 debugfs/debugfs.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 60 insertions(+), 10 deletions(-)

diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index b77d0b5..e443703 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -37,6 +37,16 @@ extern char *optarg;
 #include "../version.h"
 #include "jfs_user.h"
 
+/* 32KiB is the minimium blksize to best minimize system call overhead. */
+#ifndef IO_BUFSIZE
+#define IO_BUFSIZE 32*1024
+#endif
+
+/* Block size for `st_blocks' */
+#ifndef S_BLKSIZE
+#define S_BLKSIZE 512
+#endif
+
 ss_request_table *extra_cmds;
 const char *debug_prog_name;
 int sci_idx;
@@ -1571,14 +1581,17 @@ void do_find_free_inode(int argc, char *argv[])
 }
 
 #ifndef READ_ONLY
-static errcode_t copy_file(int fd, ext2_ino_t newfile)
+static errcode_t copy_file(int fd, ext2_ino_t newfile, int bufsize,
+			int make_holes, int *zero_written)
 {
 	ext2_file_t	e2_file;
 	errcode_t	retval;
 	int		got;
 	unsigned int	written;
-	char		buf[8192];
+	char		buf[bufsize];
 	char		*ptr;
+	char		*cp;
+	int		count;
 
 	retval = ext2fs_file_open(current_fs, newfile,
 				  EXT2_FILE_WRITE, &e2_file);
@@ -1594,14 +1607,30 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
 			goto fail;
 		}
 		ptr = buf;
+		cp = ptr;
+		count = got;
 		while (got > 0) {
-			retval = ext2fs_file_write(e2_file, ptr,
-						   got, &written);
-			if (retval)
-				goto fail;
-
-			got -= written;
-			ptr += written;
+			if (make_holes) {
+				/* Check whether all is zero */
+				while (count-- && *cp++ == 0)
+					continue;
+				if (count < 0) {
+					 /* The whole block is zero, make a hole */
+					retval = ext2fs_file_lseek(e2_file, got, EXT2_SEEK_CUR, NULL);
+					if (retval)
+						goto fail;
+					got = 0;
+				}
+			}
+			/* Normal copy */
+			if (got > 0) {
+				*zero_written = 0;
+				retval = ext2fs_file_write(e2_file, ptr, got, &written);
+				if (retval)
+					goto fail;
+				got -= written;
+				ptr += written;
+			}
 		}
 	}
 	retval = ext2fs_file_close(e2_file);
@@ -1620,6 +1649,9 @@ void do_write(int argc, char *argv[])
 	ext2_ino_t	newfile;
 	errcode_t	retval;
 	struct ext2_inode inode;
+	int		bufsize = IO_BUFSIZE;
+	int		make_holes = 0;
+	int 		zero_written = 1;
 
 	if (common_args_process(argc, argv, 3, 3, "write",
 				"<native file> <new file>", CHECK_FS_RW))
@@ -1684,9 +1716,27 @@ void do_write(int argc, char *argv[])
 		return;
 	}
 	if (LINUX_S_ISREG(inode.i_mode)) {
-		retval = copy_file(fd, newfile);
+		if (statbuf.st_blocks < statbuf.st_size / S_BLKSIZE) {
+			make_holes = 1;
+			/*
+			 * Use I/O blocksize as buffer size when
+			 * copying sparse files.
+			 */
+			bufsize = statbuf.st_blksize;
+		}
+		retval = copy_file(fd, newfile, bufsize, make_holes, &zero_written);
 		if (retval)
 			com_err("copy_file", retval, 0);
+
+		if ((inode.i_flags & EXT4_EXTENTS_FL) && zero_written) {
+			/*
+			 * If no data is copied which indicateds that no write
+			 * happens, we need to turn off the EXT4_EXTENTS_FL.
+			 */
+			inode.i_flags &= ~EXT4_EXTENTS_FL;
+			if (debugfs_write_inode(newfile, &inode, argv[0]))
+				close(fd);
+		}
 	}
 	close(fd);
 }
-- 
1.8.1.2

Re: [PATCH 2/2] debugfs.c: do sparse copy when src is a sparse file

[Darrick J. Wong]

On Fri, Jul 19, 2013 at 10:17:37AM +0800, Robert Yang wrote:
> Let debugfs do sparse copy when src is a sparse file, just like
> "cp --sparse=auto"
> 
> * For the
>   #define IO_BUFSIZE 32*1024
> 
>   This is from coreutils-8.13/src/ioblksize.h (GPL V3):
> /* As of Mar 2009, 32KiB is determined to be the minimium
>    blksize to best minimize system call overhead.
>    This can be tested with this script with the results
>    shown for a 1.7GHz pentium-m with 2GB of 400MHz DDR2 RAM:

Um.... GNU updated this to 64K a couple of years ago:
http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/ioblksize.h;h=1ae93255e7d0ccf0855208c7ae5888209997bf16;hb=HEAD

Just for laughs I tried it on a T430 with an i5-3320M and 16G of DDR3-1600 RAM:

   1024=3.7 GB/s
   2048=7.1 GB/s
   4096=8.8 GB/s
   8192=14.9 GB/s
  16384=14.3 GB/s
  32768=13.4 GB/s
  65536=15.8 GB/s
 131072=20.7 GB/s
 262144=16.4 GB/s
 524288=15.9 GB/s
1048576=15.8 GB/s
2097152=15.1 GB/s
4194304=11.7 GB/s
8388608=9.9 GB/s
16777216=9.4 GB/s
33554432=9.3 GB/s
67108864=9.3 GB/s
134217728=8.8 GB/s

For that matter, a 2010-era i7-950/DDR3-1066 system showed this:

   1024=3.4 GB/s
   2048=5.6 GB/s
   4096=7.8 GB/s
   8192=9.5 GB/s
  16384=10.8 GB/s
  32768=11.4 GB/s
  65536=11.6 GB/s
 131072=12.2 GB/s
 262144=11.9 GB/s
 524288=12.3 GB/s
1048576=12.4 GB/s
2097152=12.5 GB/s
4194304=12.5 GB/s
8388608=10.3 GB/s
16777216=8.0 GB/s
33554432=7.6 GB/s
67108864=7.8 GB/s
134217728=7.5 GB/s

And for good measure, a cruddy old T2300 Core Duo from 2006 spat out this:

   1024=1.1 GB/s
   2048=2.1 GB/s
   4096=3.6 GB/s
   8192=5.0 GB/s
  16384=6.3 GB/s
  32768=6.5 GB/s
  65536=6.6 GB/s
 131072=7.0 GB/s
 262144=7.1 GB/s
 524288=7.1 GB/s
1048576=6.8 GB/s
2097152=4.4 GB/s
4194304=2.3 GB/s
8388608=2.0 GB/s
16777216=2.0 GB/s
33554432=2.0 GB/s
67108864=2.0 GB/s
134217728=1.9 GB/s

I suspect you could increase the buffer size to 128K (or possibly even BLKRAGET
size?) without much of a problem...

> 
>    for i in $(seq 0 10); do
>      size=$((8*1024**3)) #ensure this is big enough
>      bs=$((1024*2**$i))
>      printf "%7s=" $bs
>      dd bs=$bs if=/dev/zero of=/dev/null count=$(($size/$bs)) 2>&1 |
>      sed -n 's/.* \([0-9.]* [GM]B\/s\)/\1/p'
>    done
> 
>       1024=734 MB/s
>       2048=1.3 GB/s
>       4096=2.4 GB/s
>       8192=3.5 GB/s
>      16384=3.9 GB/s
>      32768=5.2 GB/s
>      65536=5.3 GB/s
>     131072=5.5 GB/s
>     262144=5.7 GB/s
>     524288=5.7 GB/s
>    1048576=5.8 GB/s
> 
>    Note that this is to minimize system call overhead.
>    Other values may be appropriate to minimize file system
>    or disk overhead.  For example on my current GNU/Linux system
>    the readahead setting is 128KiB which was read using:
> 
>    file="."
>    device=$(df -P --local "$file" | tail -n1 | cut -d' ' -f1)
>    echo $(( $(blockdev --getra $device) * 512 ))
> 
>    However there isn't a portable way to get the above.
>    In the future we could use the above method if available
>    and default to io_blksize() if not.
>  */
> enum { IO_BUFSIZE = 32*1024 };
> 
> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> Acked-by: Darren Hart <dvhart@linux.intel.com>
> ---
>  debugfs/debugfs.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 60 insertions(+), 10 deletions(-)
> 
> diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
> index b77d0b5..e443703 100644
> --- a/debugfs/debugfs.c
> +++ b/debugfs/debugfs.c
> @@ -37,6 +37,16 @@ extern char *optarg;
>  #include "../version.h"
>  #include "jfs_user.h"
>  
> +/* 32KiB is the minimium blksize to best minimize system call overhead. */
> +#ifndef IO_BUFSIZE
> +#define IO_BUFSIZE 32*1024
> +#endif
> +
> +/* Block size for `st_blocks' */
> +#ifndef S_BLKSIZE
> +#define S_BLKSIZE 512
> +#endif
> +
>  ss_request_table *extra_cmds;
>  const char *debug_prog_name;
>  int sci_idx;
> @@ -1571,14 +1581,17 @@ void do_find_free_inode(int argc, char *argv[])
>  }
>  
>  #ifndef READ_ONLY
> -static errcode_t copy_file(int fd, ext2_ino_t newfile)
> +static errcode_t copy_file(int fd, ext2_ino_t newfile, int bufsize,
> +			int make_holes, int *zero_written)
>  {
>  	ext2_file_t	e2_file;
>  	errcode_t	retval;
>  	int		got;
>  	unsigned int	written;
> -	char		buf[8192];
> +	char		buf[bufsize];

...well, I guess it could be more of a problem if you put 128K on the stack.

--D

>  	char		*ptr;
> +	char		*cp;
> +	int		count;
>  
>  	retval = ext2fs_file_open(current_fs, newfile,
>  				  EXT2_FILE_WRITE, &e2_file);
> @@ -1594,14 +1607,30 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
>  			goto fail;
>  		}
>  		ptr = buf;
> +		cp = ptr;
> +		count = got;
>  		while (got > 0) {
> -			retval = ext2fs_file_write(e2_file, ptr,
> -						   got, &written);
> -			if (retval)
> -				goto fail;
> -
> -			got -= written;
> -			ptr += written;
> +			if (make_holes) {
> +				/* Check whether all is zero */
> +				while (count-- && *cp++ == 0)
> +					continue;
> +				if (count < 0) {
> +					 /* The whole block is zero, make a hole */
> +					retval = ext2fs_file_lseek(e2_file, got, EXT2_SEEK_CUR, NULL);
> +					if (retval)
> +						goto fail;
> +					got = 0;
> +				}
> +			}
> +			/* Normal copy */
> +			if (got > 0) {
> +				*zero_written = 0;
> +				retval = ext2fs_file_write(e2_file, ptr, got, &written);
> +				if (retval)
> +					goto fail;
> +				got -= written;
> +				ptr += written;
> +			}
>  		}
>  	}
>  	retval = ext2fs_file_close(e2_file);
> @@ -1620,6 +1649,9 @@ void do_write(int argc, char *argv[])
>  	ext2_ino_t	newfile;
>  	errcode_t	retval;
>  	struct ext2_inode inode;
> +	int		bufsize = IO_BUFSIZE;
> +	int		make_holes = 0;
> +	int 		zero_written = 1;
>  
>  	if (common_args_process(argc, argv, 3, 3, "write",
>  				"<native file> <new file>", CHECK_FS_RW))
> @@ -1684,9 +1716,27 @@ void do_write(int argc, char *argv[])
>  		return;
>  	}
>  	if (LINUX_S_ISREG(inode.i_mode)) {
> -		retval = copy_file(fd, newfile);
> +		if (statbuf.st_blocks < statbuf.st_size / S_BLKSIZE) {
> +			make_holes = 1;
> +			/*
> +			 * Use I/O blocksize as buffer size when
> +			 * copying sparse files.
> +			 */
> +			bufsize = statbuf.st_blksize;
> +		}
> +		retval = copy_file(fd, newfile, bufsize, make_holes, &zero_written);
>  		if (retval)
>  			com_err("copy_file", retval, 0);
> +
> +		if ((inode.i_flags & EXT4_EXTENTS_FL) && zero_written) {
> +			/*
> +			 * If no data is copied which indicateds that no write
> +			 * happens, we need to turn off the EXT4_EXTENTS_FL.
> +			 */
> +			inode.i_flags &= ~EXT4_EXTENTS_FL;
> +			if (debugfs_write_inode(newfile, &inode, argv[0]))
> +				close(fd);
> +		}
>  	}
>  	close(fd);
>  }
> -- 
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] debugfs.c: do sparse copy when src is a sparse file

[Robert Yang]

Hi Darrick,

Thanks for your reply, it seems that 64K is a good idea since put 128K
on the stack might cause problems, I will wait for one or two days for
more comments on other parts of the patches, then send a V2 with the
updates.

// Robert


On 07/20/2013 02:55 AM, Darrick J. Wong wrote:
> On Fri, Jul 19, 2013 at 10:17:37AM +0800, Robert Yang wrote:
>> Let debugfs do sparse copy when src is a sparse file, just like
>> "cp --sparse=auto"
>>
>> * For the
>>    #define IO_BUFSIZE 32*1024
>>
>>    This is from coreutils-8.13/src/ioblksize.h (GPL V3):
>> /* As of Mar 2009, 32KiB is determined to be the minimium
>>     blksize to best minimize system call overhead.
>>     This can be tested with this script with the results
>>     shown for a 1.7GHz pentium-m with 2GB of 400MHz DDR2 RAM:
>
> Um.... GNU updated this to 64K a couple of years ago:
> http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/ioblksize.h;h=1ae93255e7d0ccf0855208c7ae5888209997bf16;hb=HEAD
>
> Just for laughs I tried it on a T430 with an i5-3320M and 16G of DDR3-1600 RAM:
>
>     1024=3.7 GB/s
>     2048=7.1 GB/s
>     4096=8.8 GB/s
>     8192=14.9 GB/s
>    16384=14.3 GB/s
>    32768=13.4 GB/s
>    65536=15.8 GB/s
>   131072=20.7 GB/s
>   262144=16.4 GB/s
>   524288=15.9 GB/s
> 1048576=15.8 GB/s
> 2097152=15.1 GB/s
> 4194304=11.7 GB/s
> 8388608=9.9 GB/s
> 16777216=9.4 GB/s
> 33554432=9.3 GB/s
> 67108864=9.3 GB/s
> 134217728=8.8 GB/s
>
> For that matter, a 2010-era i7-950/DDR3-1066 system showed this:
>
>     1024=3.4 GB/s
>     2048=5.6 GB/s
>     4096=7.8 GB/s
>     8192=9.5 GB/s
>    16384=10.8 GB/s
>    32768=11.4 GB/s
>    65536=11.6 GB/s
>   131072=12.2 GB/s
>   262144=11.9 GB/s
>   524288=12.3 GB/s
> 1048576=12.4 GB/s
> 2097152=12.5 GB/s
> 4194304=12.5 GB/s
> 8388608=10.3 GB/s
> 16777216=8.0 GB/s
> 33554432=7.6 GB/s
> 67108864=7.8 GB/s
> 134217728=7.5 GB/s
>
> And for good measure, a cruddy old T2300 Core Duo from 2006 spat out this:
>
>     1024=1.1 GB/s
>     2048=2.1 GB/s
>     4096=3.6 GB/s
>     8192=5.0 GB/s
>    16384=6.3 GB/s
>    32768=6.5 GB/s
>    65536=6.6 GB/s
>   131072=7.0 GB/s
>   262144=7.1 GB/s
>   524288=7.1 GB/s
> 1048576=6.8 GB/s
> 2097152=4.4 GB/s
> 4194304=2.3 GB/s
> 8388608=2.0 GB/s
> 16777216=2.0 GB/s
> 33554432=2.0 GB/s
> 67108864=2.0 GB/s
> 134217728=1.9 GB/s
>
> I suspect you could increase the buffer size to 128K (or possibly even BLKRAGET
> size?) without much of a problem...
>
>>
>>     for i in $(seq 0 10); do
>>       size=$((8*1024**3)) #ensure this is big enough
>>       bs=$((1024*2**$i))
>>       printf "%7s=" $bs
>>       dd bs=$bs if=/dev/zero of=/dev/null count=$(($size/$bs)) 2>&1 |
>>       sed -n 's/.* \([0-9.]* [GM]B\/s\)/\1/p'
>>     done
>>
>>        1024=734 MB/s
>>        2048=1.3 GB/s
>>        4096=2.4 GB/s
>>        8192=3.5 GB/s
>>       16384=3.9 GB/s
>>       32768=5.2 GB/s
>>       65536=5.3 GB/s
>>      131072=5.5 GB/s
>>      262144=5.7 GB/s
>>      524288=5.7 GB/s
>>     1048576=5.8 GB/s
>>
>>     Note that this is to minimize system call overhead.
>>     Other values may be appropriate to minimize file system
>>     or disk overhead.  For example on my current GNU/Linux system
>>     the readahead setting is 128KiB which was read using:
>>
>>     file="."
>>     device=$(df -P --local "$file" | tail -n1 | cut -d' ' -f1)
>>     echo $(( $(blockdev --getra $device) * 512 ))
>>
>>     However there isn't a portable way to get the above.
>>     In the future we could use the above method if available
>>     and default to io_blksize() if not.
>>   */
>> enum { IO_BUFSIZE = 32*1024 };
>>
>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
>> Acked-by: Darren Hart <dvhart@linux.intel.com>
>> ---
>>   debugfs/debugfs.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 60 insertions(+), 10 deletions(-)
>>
>> diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
>> index b77d0b5..e443703 100644
>> --- a/debugfs/debugfs.c
>> +++ b/debugfs/debugfs.c
>> @@ -37,6 +37,16 @@ extern char *optarg;
>>   #include "../version.h"
>>   #include "jfs_user.h"
>>
>> +/* 32KiB is the minimium blksize to best minimize system call overhead. */
>> +#ifndef IO_BUFSIZE
>> +#define IO_BUFSIZE 32*1024
>> +#endif
>> +
>> +/* Block size for `st_blocks' */
>> +#ifndef S_BLKSIZE
>> +#define S_BLKSIZE 512
>> +#endif
>> +
>>   ss_request_table *extra_cmds;
>>   const char *debug_prog_name;
>>   int sci_idx;
>> @@ -1571,14 +1581,17 @@ void do_find_free_inode(int argc, char *argv[])
>>   }
>>
>>   #ifndef READ_ONLY
>> -static errcode_t copy_file(int fd, ext2_ino_t newfile)
>> +static errcode_t copy_file(int fd, ext2_ino_t newfile, int bufsize,
>> +			int make_holes, int *zero_written)
>>   {
>>   	ext2_file_t	e2_file;
>>   	errcode_t	retval;
>>   	int		got;
>>   	unsigned int	written;
>> -	char		buf[8192];
>> +	char		buf[bufsize];
>
> ...well, I guess it could be more of a problem if you put 128K on the stack.
>
> --D
>
>>   	char		*ptr;
>> +	char		*cp;
>> +	int		count;
>>
>>   	retval = ext2fs_file_open(current_fs, newfile,
>>   				  EXT2_FILE_WRITE, &e2_file);
>> @@ -1594,14 +1607,30 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
>>   			goto fail;
>>   		}
>>   		ptr = buf;
>> +		cp = ptr;
>> +		count = got;
>>   		while (got > 0) {
>> -			retval = ext2fs_file_write(e2_file, ptr,
>> -						   got, &written);
>> -			if (retval)
>> -				goto fail;
>> -
>> -			got -= written;
>> -			ptr += written;
>> +			if (make_holes) {
>> +				/* Check whether all is zero */
>> +				while (count-- && *cp++ == 0)
>> +					continue;
>> +				if (count < 0) {
>> +					 /* The whole block is zero, make a hole */
>> +					retval = ext2fs_file_lseek(e2_file, got, EXT2_SEEK_CUR, NULL);
>> +					if (retval)
>> +						goto fail;
>> +					got = 0;
>> +				}
>> +			}
>> +			/* Normal copy */
>> +			if (got > 0) {
>> +				*zero_written = 0;
>> +				retval = ext2fs_file_write(e2_file, ptr, got, &written);
>> +				if (retval)
>> +					goto fail;
>> +				got -= written;
>> +				ptr += written;
>> +			}
>>   		}
>>   	}
>>   	retval = ext2fs_file_close(e2_file);
>> @@ -1620,6 +1649,9 @@ void do_write(int argc, char *argv[])
>>   	ext2_ino_t	newfile;
>>   	errcode_t	retval;
>>   	struct ext2_inode inode;
>> +	int		bufsize = IO_BUFSIZE;
>> +	int		make_holes = 0;
>> +	int 		zero_written = 1;
>>
>>   	if (common_args_process(argc, argv, 3, 3, "write",
>>   				"<native file> <new file>", CHECK_FS_RW))
>> @@ -1684,9 +1716,27 @@ void do_write(int argc, char *argv[])
>>   		return;
>>   	}
>>   	if (LINUX_S_ISREG(inode.i_mode)) {
>> -		retval = copy_file(fd, newfile);
>> +		if (statbuf.st_blocks < statbuf.st_size / S_BLKSIZE) {
>> +			make_holes = 1;
>> +			/*
>> +			 * Use I/O blocksize as buffer size when
>> +			 * copying sparse files.
>> +			 */
>> +			bufsize = statbuf.st_blksize;
>> +		}
>> +		retval = copy_file(fd, newfile, bufsize, make_holes, &zero_written);
>>   		if (retval)
>>   			com_err("copy_file", retval, 0);
>> +
>> +		if ((inode.i_flags & EXT4_EXTENTS_FL) && zero_written) {
>> +			/*
>> +			 * If no data is copied which indicateds that no write
>> +			 * happens, we need to turn off the EXT4_EXTENTS_FL.
>> +			 */
>> +			inode.i_flags &= ~EXT4_EXTENTS_FL;
>> +			if (debugfs_write_inode(newfile, &inode, argv[0]))
>> +				close(fd);
>> +		}
>>   	}
>>   	close(fd);
>>   }
>> --
>> 1.8.1.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH 2/2] debugfs.c: do sparse copy when src is a sparse file

[Darrick J. Wong]

On Sun, Jul 21, 2013 at 10:38:12AM +0800, Robert Yang wrote:
> 
> Hi Darrick,
> 
> Thanks for your reply, it seems that 64K is a good idea since put 128K
> on the stack might cause problems, I will wait for one or two days for
> more comments on other parts of the patches, then send a V2 with the
> updates.

Well in that case I'll review harder. :)

(Actually I thought of a few more things this morning.)

> 
> // Robert
> 
> 
> On 07/20/2013 02:55 AM, Darrick J. Wong wrote:
> >On Fri, Jul 19, 2013 at 10:17:37AM +0800, Robert Yang wrote:
> >>Let debugfs do sparse copy when src is a sparse file, just like
> >>"cp --sparse=auto"
> >>
> >>* For the
> >>   #define IO_BUFSIZE 32*1024
> >>
> >>   This is from coreutils-8.13/src/ioblksize.h (GPL V3):
> >>/* As of Mar 2009, 32KiB is determined to be the minimium
> >>    blksize to best minimize system call overhead.
> >>    This can be tested with this script with the results
> >>    shown for a 1.7GHz pentium-m with 2GB of 400MHz DDR2 RAM:
> >
> >Um.... GNU updated this to 64K a couple of years ago:
> >http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/ioblksize.h;h=1ae93255e7d0ccf0855208c7ae5888209997bf16;hb=HEAD
> >
> >Just for laughs I tried it on a T430 with an i5-3320M and 16G of DDR3-1600 RAM:
> >
> >    1024=3.7 GB/s
> >    2048=7.1 GB/s
> >    4096=8.8 GB/s
> >    8192=14.9 GB/s
> >   16384=14.3 GB/s
> >   32768=13.4 GB/s
> >   65536=15.8 GB/s
> >  131072=20.7 GB/s
> >  262144=16.4 GB/s
> >  524288=15.9 GB/s
> >1048576=15.8 GB/s
> >2097152=15.1 GB/s
> >4194304=11.7 GB/s
> >8388608=9.9 GB/s
> >16777216=9.4 GB/s
> >33554432=9.3 GB/s
> >67108864=9.3 GB/s
> >134217728=8.8 GB/s
> >
> >For that matter, a 2010-era i7-950/DDR3-1066 system showed this:
> >
> >    1024=3.4 GB/s
> >    2048=5.6 GB/s
> >    4096=7.8 GB/s
> >    8192=9.5 GB/s
> >   16384=10.8 GB/s
> >   32768=11.4 GB/s
> >   65536=11.6 GB/s
> >  131072=12.2 GB/s
> >  262144=11.9 GB/s
> >  524288=12.3 GB/s
> >1048576=12.4 GB/s
> >2097152=12.5 GB/s
> >4194304=12.5 GB/s
> >8388608=10.3 GB/s
> >16777216=8.0 GB/s
> >33554432=7.6 GB/s
> >67108864=7.8 GB/s
> >134217728=7.5 GB/s
> >
> >And for good measure, a cruddy old T2300 Core Duo from 2006 spat out this:
> >
> >    1024=1.1 GB/s
> >    2048=2.1 GB/s
> >    4096=3.6 GB/s
> >    8192=5.0 GB/s
> >   16384=6.3 GB/s
> >   32768=6.5 GB/s
> >   65536=6.6 GB/s
> >  131072=7.0 GB/s
> >  262144=7.1 GB/s
> >  524288=7.1 GB/s
> >1048576=6.8 GB/s
> >2097152=4.4 GB/s
> >4194304=2.3 GB/s
> >8388608=2.0 GB/s
> >16777216=2.0 GB/s
> >33554432=2.0 GB/s
> >67108864=2.0 GB/s
> >134217728=1.9 GB/s
> >
> >I suspect you could increase the buffer size to 128K (or possibly even BLKRAGET
> >size?) without much of a problem...
> >
> >>
> >>    for i in $(seq 0 10); do
> >>      size=$((8*1024**3)) #ensure this is big enough
> >>      bs=$((1024*2**$i))
> >>      printf "%7s=" $bs
> >>      dd bs=$bs if=/dev/zero of=/dev/null count=$(($size/$bs)) 2>&1 |
> >>      sed -n 's/.* \([0-9.]* [GM]B\/s\)/\1/p'
> >>    done
> >>
> >>       1024=734 MB/s
> >>       2048=1.3 GB/s
> >>       4096=2.4 GB/s
> >>       8192=3.5 GB/s
> >>      16384=3.9 GB/s
> >>      32768=5.2 GB/s
> >>      65536=5.3 GB/s
> >>     131072=5.5 GB/s
> >>     262144=5.7 GB/s
> >>     524288=5.7 GB/s
> >>    1048576=5.8 GB/s
> >>
> >>    Note that this is to minimize system call overhead.
> >>    Other values may be appropriate to minimize file system
> >>    or disk overhead.  For example on my current GNU/Linux system
> >>    the readahead setting is 128KiB which was read using:
> >>
> >>    file="."
> >>    device=$(df -P --local "$file" | tail -n1 | cut -d' ' -f1)
> >>    echo $(( $(blockdev --getra $device) * 512 ))
> >>
> >>    However there isn't a portable way to get the above.
> >>    In the future we could use the above method if available
> >>    and default to io_blksize() if not.
> >>  */
> >>enum { IO_BUFSIZE = 32*1024 };
> >>
> >>Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> >>Acked-by: Darren Hart <dvhart@linux.intel.com>
> >>---
> >>  debugfs/debugfs.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++--------
> >>  1 file changed, 60 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
> >>index b77d0b5..e443703 100644
> >>--- a/debugfs/debugfs.c
> >>+++ b/debugfs/debugfs.c
> >>@@ -37,6 +37,16 @@ extern char *optarg;
> >>  #include "../version.h"
> >>  #include "jfs_user.h"
> >>
> >>+/* 32KiB is the minimium blksize to best minimize system call overhead. */
> >>+#ifndef IO_BUFSIZE
> >>+#define IO_BUFSIZE 32*1024
> >>+#endif
> >>+
> >>+/* Block size for `st_blocks' */
> >>+#ifndef S_BLKSIZE
> >>+#define S_BLKSIZE 512
> >>+#endif
> >>+
> >>  ss_request_table *extra_cmds;
> >>  const char *debug_prog_name;
> >>  int sci_idx;
> >>@@ -1571,14 +1581,17 @@ void do_find_free_inode(int argc, char *argv[])
> >>  }
> >>
> >>  #ifndef READ_ONLY
> >>-static errcode_t copy_file(int fd, ext2_ino_t newfile)
> >>+static errcode_t copy_file(int fd, ext2_ino_t newfile, int bufsize,
> >>+			int make_holes, int *zero_written)
> >>  {
> >>  	ext2_file_t	e2_file;
> >>  	errcode_t	retval;
> >>  	int		got;
> >>  	unsigned int	written;
> >>-	char		buf[8192];
> >>+	char		buf[bufsize];

I wonder, do we allow variable length arrays?  I recall Ted was trying to get
rid of these.

> >
> >...well, I guess it could be more of a problem if you put 128K on the stack.
> >
> >--D
> >
> >>  	char		*ptr;
> >>+	char		*cp;
> >>+	int		count;
> >>
> >>  	retval = ext2fs_file_open(current_fs, newfile,
> >>  				  EXT2_FILE_WRITE, &e2_file);
> >>@@ -1594,14 +1607,30 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
> >>  			goto fail;
> >>  		}
> >>  		ptr = buf;
> >>+		cp = ptr;
> >>+		count = got;
> >>  		while (got > 0) {
> >>-			retval = ext2fs_file_write(e2_file, ptr,
> >>-						   got, &written);
> >>-			if (retval)
> >>-				goto fail;
> >>-
> >>-			got -= written;
> >>-			ptr += written;
> >>+			if (make_holes) {
> >>+				/* Check whether all is zero */
> >>+				while (count-- && *cp++ == 0)
> >>+					continue;

I suspect that calloc()ing a zero buffer and calling memcmp() would be faster
than a byte-for-byte comparison.

> >>+				if (count < 0) {
> >>+					 /* The whole block is zero, make a hole */
> >>+					retval = ext2fs_file_lseek(e2_file, got, EXT2_SEEK_CUR, NULL);
> >>+					if (retval)
> >>+						goto fail;
> >>+					got = 0;

I think the entire make_holes clause could be lifted out of the inner while and
placed in the outer while, since the is-zero-buffer test depends only on the
input.

You could use FIEMAP/FIBMAP or SEEK_DATA or something to efficiently walk the
allocated regions of the incoming file.  If they're available...

> >>+				}
> >>+			}
> >>+			/* Normal copy */
> >>+			if (got > 0) {

Then you don't need the test here.

> >>+				*zero_written = 0;
> >>+				retval = ext2fs_file_write(e2_file, ptr, got, &written);
> >>+				if (retval)
> >>+					goto fail;
> >>+				got -= written;
> >>+				ptr += written;
> >>+			}
> >>  		}
> >>  	}
> >>  	retval = ext2fs_file_close(e2_file);
> >>@@ -1620,6 +1649,9 @@ void do_write(int argc, char *argv[])
> >>  	ext2_ino_t	newfile;
> >>  	errcode_t	retval;
> >>  	struct ext2_inode inode;
> >>+	int		bufsize = IO_BUFSIZE;
> >>+	int		make_holes = 0;
> >>+	int 		zero_written = 1;
> >>
> >>  	if (common_args_process(argc, argv, 3, 3, "write",
> >>  				"<native file> <new file>", CHECK_FS_RW))
> >>@@ -1684,9 +1716,27 @@ void do_write(int argc, char *argv[])
> >>  		return;
> >>  	}
> >>  	if (LINUX_S_ISREG(inode.i_mode)) {
> >>-		retval = copy_file(fd, newfile);
> >>+		if (statbuf.st_blocks < statbuf.st_size / S_BLKSIZE) {

Well, that's one way to detect a sparse file coming in -- but do we care about
the case of copying in a non-sparse file that contains a lot of zero regions?

Maybe we could add a flag to the 'write' command to force make_holes=1?

(Or just figure it out ourselves via fiemap as suggested above.)

> >>+			make_holes = 1;
> >>+			/*
> >>+			 * Use I/O blocksize as buffer size when
> >>+			 * copying sparse files.
> >>+			 */
> >>+			bufsize = statbuf.st_blksize;
> >>+		}
> >>+		retval = copy_file(fd, newfile, bufsize, make_holes, &zero_written);
> >>  		if (retval)
> >>  			com_err("copy_file", retval, 0);
> >>+
> >>+		if ((inode.i_flags & EXT4_EXTENTS_FL) && zero_written) {
> >>+			/*
> >>+			 * If no data is copied which indicateds that no write
> >>+			 * happens, we need to turn off the EXT4_EXTENTS_FL.

I don't think removing the extents flag is necessary; "touch /mnt/emptyfile"
creates an empty flag with the extents flag set.

--D
> >>+			 */
> >>+			inode.i_flags &= ~EXT4_EXTENTS_FL;
> >>+			if (debugfs_write_inode(newfile, &inode, argv[0]))
> >>+				close(fd);
> >>+		}
> >>  	}
> >>  	close(fd);
> >>  }
> >>--
> >>1.8.1.2
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] debugfs.c: do sparse copy when src is a sparse file

[Robert Yang]

Hi Darrick,

Thank you very much for your very detailed review, I will fix them
one by one and send a V2 sooner.

// Robert


On 07/23/2013 01:30 AM, Darrick J. Wong wrote:
> On Sun, Jul 21, 2013 at 10:38:12AM +0800, Robert Yang wrote:
>>
>> Hi Darrick,
>>
>> Thanks for your reply, it seems that 64K is a good idea since put 128K
>> on the stack might cause problems, I will wait for one or two days for
>> more comments on other parts of the patches, then send a V2 with the
>> updates.
>
> Well in that case I'll review harder. :)
>
> (Actually I thought of a few more things this morning.)
>
>>
>> // Robert
>>
>>
>> On 07/20/2013 02:55 AM, Darrick J. Wong wrote:
>>> On Fri, Jul 19, 2013 at 10:17:37AM +0800, Robert Yang wrote:
>>>> Let debugfs do sparse copy when src is a sparse file, just like
>>>> "cp --sparse=auto"
>>>>
>>>> * For the
>>>>    #define IO_BUFSIZE 32*1024
>>>>
>>>>    This is from coreutils-8.13/src/ioblksize.h (GPL V3):
>>>> /* As of Mar 2009, 32KiB is determined to be the minimium
>>>>     blksize to best minimize system call overhead.
>>>>     This can be tested with this script with the results
>>>>     shown for a 1.7GHz pentium-m with 2GB of 400MHz DDR2 RAM:
>>>
>>> Um.... GNU updated this to 64K a couple of years ago:
>>> http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/ioblksize.h;h=1ae93255e7d0ccf0855208c7ae5888209997bf16;hb=HEAD
>>>
>>> Just for laughs I tried it on a T430 with an i5-3320M and 16G of DDR3-1600 RAM:
>>>
>>>     1024=3.7 GB/s
>>>     2048=7.1 GB/s
>>>     4096=8.8 GB/s
>>>     8192=14.9 GB/s
>>>    16384=14.3 GB/s
>>>    32768=13.4 GB/s
>>>    65536=15.8 GB/s
>>>   131072=20.7 GB/s
>>>   262144=16.4 GB/s
>>>   524288=15.9 GB/s
>>> 1048576=15.8 GB/s
>>> 2097152=15.1 GB/s
>>> 4194304=11.7 GB/s
>>> 8388608=9.9 GB/s
>>> 16777216=9.4 GB/s
>>> 33554432=9.3 GB/s
>>> 67108864=9.3 GB/s
>>> 134217728=8.8 GB/s
>>>
>>> For that matter, a 2010-era i7-950/DDR3-1066 system showed this:
>>>
>>>     1024=3.4 GB/s
>>>     2048=5.6 GB/s
>>>     4096=7.8 GB/s
>>>     8192=9.5 GB/s
>>>    16384=10.8 GB/s
>>>    32768=11.4 GB/s
>>>    65536=11.6 GB/s
>>>   131072=12.2 GB/s
>>>   262144=11.9 GB/s
>>>   524288=12.3 GB/s
>>> 1048576=12.4 GB/s
>>> 2097152=12.5 GB/s
>>> 4194304=12.5 GB/s
>>> 8388608=10.3 GB/s
>>> 16777216=8.0 GB/s
>>> 33554432=7.6 GB/s
>>> 67108864=7.8 GB/s
>>> 134217728=7.5 GB/s
>>>
>>> And for good measure, a cruddy old T2300 Core Duo from 2006 spat out this:
>>>
>>>     1024=1.1 GB/s
>>>     2048=2.1 GB/s
>>>     4096=3.6 GB/s
>>>     8192=5.0 GB/s
>>>    16384=6.3 GB/s
>>>    32768=6.5 GB/s
>>>    65536=6.6 GB/s
>>>   131072=7.0 GB/s
>>>   262144=7.1 GB/s
>>>   524288=7.1 GB/s
>>> 1048576=6.8 GB/s
>>> 2097152=4.4 GB/s
>>> 4194304=2.3 GB/s
>>> 8388608=2.0 GB/s
>>> 16777216=2.0 GB/s
>>> 33554432=2.0 GB/s
>>> 67108864=2.0 GB/s
>>> 134217728=1.9 GB/s
>>>
>>> I suspect you could increase the buffer size to 128K (or possibly even BLKRAGET
>>> size?) without much of a problem...
>>>
>>>>
>>>>     for i in $(seq 0 10); do
>>>>       size=$((8*1024**3)) #ensure this is big enough
>>>>       bs=$((1024*2**$i))
>>>>       printf "%7s=" $bs
>>>>       dd bs=$bs if=/dev/zero of=/dev/null count=$(($size/$bs)) 2>&1 |
>>>>       sed -n 's/.* \([0-9.]* [GM]B\/s\)/\1/p'
>>>>     done
>>>>
>>>>        1024=734 MB/s
>>>>        2048=1.3 GB/s
>>>>        4096=2.4 GB/s
>>>>        8192=3.5 GB/s
>>>>       16384=3.9 GB/s
>>>>       32768=5.2 GB/s
>>>>       65536=5.3 GB/s
>>>>      131072=5.5 GB/s
>>>>      262144=5.7 GB/s
>>>>      524288=5.7 GB/s
>>>>     1048576=5.8 GB/s
>>>>
>>>>     Note that this is to minimize system call overhead.
>>>>     Other values may be appropriate to minimize file system
>>>>     or disk overhead.  For example on my current GNU/Linux system
>>>>     the readahead setting is 128KiB which was read using:
>>>>
>>>>     file="."
>>>>     device=$(df -P --local "$file" | tail -n1 | cut -d' ' -f1)
>>>>     echo $(( $(blockdev --getra $device) * 512 ))
>>>>
>>>>     However there isn't a portable way to get the above.
>>>>     In the future we could use the above method if available
>>>>     and default to io_blksize() if not.
>>>>   */
>>>> enum { IO_BUFSIZE = 32*1024 };
>>>>
>>>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
>>>> Acked-by: Darren Hart <dvhart@linux.intel.com>
>>>> ---
>>>>   debugfs/debugfs.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++--------
>>>>   1 file changed, 60 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
>>>> index b77d0b5..e443703 100644
>>>> --- a/debugfs/debugfs.c
>>>> +++ b/debugfs/debugfs.c
>>>> @@ -37,6 +37,16 @@ extern char *optarg;
>>>>   #include "../version.h"
>>>>   #include "jfs_user.h"
>>>>
>>>> +/* 32KiB is the minimium blksize to best minimize system call overhead. */
>>>> +#ifndef IO_BUFSIZE
>>>> +#define IO_BUFSIZE 32*1024
>>>> +#endif
>>>> +
>>>> +/* Block size for `st_blocks' */
>>>> +#ifndef S_BLKSIZE
>>>> +#define S_BLKSIZE 512
>>>> +#endif
>>>> +
>>>>   ss_request_table *extra_cmds;
>>>>   const char *debug_prog_name;
>>>>   int sci_idx;
>>>> @@ -1571,14 +1581,17 @@ void do_find_free_inode(int argc, char *argv[])
>>>>   }
>>>>
>>>>   #ifndef READ_ONLY
>>>> -static errcode_t copy_file(int fd, ext2_ino_t newfile)
>>>> +static errcode_t copy_file(int fd, ext2_ino_t newfile, int bufsize,
>>>> +			int make_holes, int *zero_written)
>>>>   {
>>>>   	ext2_file_t	e2_file;
>>>>   	errcode_t	retval;
>>>>   	int		got;
>>>>   	unsigned int	written;
>>>> -	char		buf[8192];
>>>> +	char		buf[bufsize];
>
> I wonder, do we allow variable length arrays?  I recall Ted was trying to get
> rid of these.
>
>>>
>>> ...well, I guess it could be more of a problem if you put 128K on the stack.
>>>
>>> --D
>>>
>>>>   	char		*ptr;
>>>> +	char		*cp;
>>>> +	int		count;
>>>>
>>>>   	retval = ext2fs_file_open(current_fs, newfile,
>>>>   				  EXT2_FILE_WRITE, &e2_file);
>>>> @@ -1594,14 +1607,30 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
>>>>   			goto fail;
>>>>   		}
>>>>   		ptr = buf;
>>>> +		cp = ptr;
>>>> +		count = got;
>>>>   		while (got > 0) {
>>>> -			retval = ext2fs_file_write(e2_file, ptr,
>>>> -						   got, &written);
>>>> -			if (retval)
>>>> -				goto fail;
>>>> -
>>>> -			got -= written;
>>>> -			ptr += written;
>>>> +			if (make_holes) {
>>>> +				/* Check whether all is zero */
>>>> +				while (count-- && *cp++ == 0)
>>>> +					continue;
>
> I suspect that calloc()ing a zero buffer and calling memcmp() would be faster
> than a byte-for-byte comparison.
>
>>>> +				if (count < 0) {
>>>> +					 /* The whole block is zero, make a hole */
>>>> +					retval = ext2fs_file_lseek(e2_file, got, EXT2_SEEK_CUR, NULL);
>>>> +					if (retval)
>>>> +						goto fail;
>>>> +					got = 0;
>
> I think the entire make_holes clause could be lifted out of the inner while and
> placed in the outer while, since the is-zero-buffer test depends only on the
> input.
>
> You could use FIEMAP/FIBMAP or SEEK_DATA or something to efficiently walk the
> allocated regions of the incoming file.  If they're available...
>
>>>> +				}
>>>> +			}
>>>> +			/* Normal copy */
>>>> +			if (got > 0) {
>
> Then you don't need the test here.
>
>>>> +				*zero_written = 0;
>>>> +				retval = ext2fs_file_write(e2_file, ptr, got, &written);
>>>> +				if (retval)
>>>> +					goto fail;
>>>> +				got -= written;
>>>> +				ptr += written;
>>>> +			}
>>>>   		}
>>>>   	}
>>>>   	retval = ext2fs_file_close(e2_file);
>>>> @@ -1620,6 +1649,9 @@ void do_write(int argc, char *argv[])
>>>>   	ext2_ino_t	newfile;
>>>>   	errcode_t	retval;
>>>>   	struct ext2_inode inode;
>>>> +	int		bufsize = IO_BUFSIZE;
>>>> +	int		make_holes = 0;
>>>> +	int 		zero_written = 1;
>>>>
>>>>   	if (common_args_process(argc, argv, 3, 3, "write",
>>>>   				"<native file> <new file>", CHECK_FS_RW))
>>>> @@ -1684,9 +1716,27 @@ void do_write(int argc, char *argv[])
>>>>   		return;
>>>>   	}
>>>>   	if (LINUX_S_ISREG(inode.i_mode)) {
>>>> -		retval = copy_file(fd, newfile);
>>>> +		if (statbuf.st_blocks < statbuf.st_size / S_BLKSIZE) {
>
> Well, that's one way to detect a sparse file coming in -- but do we care about
> the case of copying in a non-sparse file that contains a lot of zero regions?
>
> Maybe we could add a flag to the 'write' command to force make_holes=1?
>
> (Or just figure it out ourselves via fiemap as suggested above.)
>
>>>> +			make_holes = 1;
>>>> +			/*
>>>> +			 * Use I/O blocksize as buffer size when
>>>> +			 * copying sparse files.
>>>> +			 */
>>>> +			bufsize = statbuf.st_blksize;
>>>> +		}
>>>> +		retval = copy_file(fd, newfile, bufsize, make_holes, &zero_written);
>>>>   		if (retval)
>>>>   			com_err("copy_file", retval, 0);
>>>> +
>>>> +		if ((inode.i_flags & EXT4_EXTENTS_FL) && zero_written) {
>>>> +			/*
>>>> +			 * If no data is copied which indicateds that no write
>>>> +			 * happens, we need to turn off the EXT4_EXTENTS_FL.
>
> I don't think removing the extents flag is necessary; "touch /mnt/emptyfile"
> creates an empty flag with the extents flag set.
>
> --D
>>>> +			 */
>>>> +			inode.i_flags &= ~EXT4_EXTENTS_FL;
>>>> +			if (debugfs_write_inode(newfile, &inode, argv[0]))
>>>> +				close(fd);
>>>> +		}
>>>>   	}
>>>>   	close(fd);
>>>>   }
>>>> --
>>>> 1.8.1.2
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH 2/2] debugfs.c: do sparse copy when src is a sparse file

[Robert Yang]

Hi Darrick,

Thank you very much, I've pulled in most of your suggestions, the V2 is
coming, please see my comments in line, and please feel free to give
your comments.

On 07/23/2013 01:30 AM, Darrick J. Wong wrote:
> On Sun, Jul 21, 2013 at 10:38:12AM +0800, Robert Yang wrote:
>>
>>>> @@ -1594,14 +1607,30 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
>>>>   			goto fail;
>>>>   		}
>>>>   		ptr = buf;
>>>> +		cp = ptr;
>>>> +		count = got;
>>>>   		while (got > 0) {
>>>> -			retval = ext2fs_file_write(e2_file, ptr,
>>>> -						   got, &written);
>>>> -			if (retval)
>>>> -				goto fail;
>>>> -
>>>> -			got -= written;
>>>> -			ptr += written;
>>>> +			if (make_holes) {
>>>> +				/* Check whether all is zero */
>>>> +				while (count-- && *cp++ == 0)
>>>> +					continue;
>
> I suspect that calloc()ing a zero buffer and calling memcmp() would be faster
> than a byte-for-byte comparison.
>
>>>> +				if (count < 0) {
>>>> +					 /* The whole block is zero, make a hole */
>>>> +					retval = ext2fs_file_lseek(e2_file, got, EXT2_SEEK_CUR, NULL);
>>>> +					if (retval)
>>>> +						goto fail;
>>>> +					got = 0;
>
> I think the entire make_holes clause could be lifted out of the inner while and
> placed in the outer while, since the is-zero-buffer test depends only on the
> input.
>
> You could use FIEMAP/FIBMAP or SEEK_DATA or something to efficiently walk the
> allocated regions of the incoming file.  If they're available...
>

It seems that the "ioctl(fd, FIBMAP, &b)" requires the root privileges, so I
didn't use it, others are fixed.

>>>> +				}
>>>> +			}
>>>> +			/* Normal copy */
>>>> +			if (got > 0) {
>
> Then you don't need the test here.
>
>>>> +				*zero_written = 0;
>>>> +				retval = ext2fs_file_write(e2_file, ptr, got, &written);
>>>> +				if (retval)
>>>> +					goto fail;
>>>> +				got -= written;
>>>> +				ptr += written;
>>>> +			}
>>>>   		}
>>>>   	}
>>>>   	retval = ext2fs_file_close(e2_file);
>>>> @@ -1620,6 +1649,9 @@ void do_write(int argc, char *argv[])
>>>>   	ext2_ino_t	newfile;
>>>>   	errcode_t	retval;
>>>>   	struct ext2_inode inode;
>>>> +	int		bufsize = IO_BUFSIZE;
>>>> +	int		make_holes = 0;
>>>> +	int 		zero_written = 1;
>>>>
>>>>   	if (common_args_process(argc, argv, 3, 3, "write",
>>>>   				"<native file> <new file>", CHECK_FS_RW))
>>>> @@ -1684,9 +1716,27 @@ void do_write(int argc, char *argv[])
>>>>   		return;
>>>>   	}
>>>>   	if (LINUX_S_ISREG(inode.i_mode)) {
>>>> -		retval = copy_file(fd, newfile);
>>>> +		if (statbuf.st_blocks < statbuf.st_size / S_BLKSIZE) {
>
> Well, that's one way to detect a sparse file coming in -- but do we care about
> the case of copying in a non-sparse file that contains a lot of zero regions?
>

I think that we need to care about whether it is a sparse file or not:
1) We need to use the statbuf.st_blksize as the buffer size to check
    whether the entire block is full of holes, and for the non-sparse
    file, statbuf.st_blksize (usually 512B) is too small to be the buffer.

2) For performance reason, we need to avoid checking whether it is a sparse
    file or not in copy_file() if we know that it is not a sparse file.

// Robert

> Maybe we could add a flag to the 'write' command to force make_holes=1?
>
> (Or just figure it out ourselves via fiemap as suggested above.)
>
>>>> +			make_holes = 1;
>>>> +			/*
>>>> +			 * Use I/O blocksize as buffer size when
>>>> +			 * copying sparse files.
>>>> +			 */
>>>> +			bufsize = statbuf.st_blksize;
>>>> +		}
>>>> +		retval = copy_file(fd, newfile, bufsize, make_holes, &zero_written);
>>>>   		if (retval)
>>>>   			com_err("copy_file", retval, 0);
>>>> +
>>>> +		if ((inode.i_flags & EXT4_EXTENTS_FL) && zero_written) {
>>>> +			/*
>>>> +			 * If no data is copied which indicateds that no write
>>>> +			 * happens, we need to turn off the EXT4_EXTENTS_FL.
>
> I don't think removing the extents flag is necessary; "touch /mnt/emptyfile"
> creates an empty flag with the extents flag set.
>
> --D
>>>> +			 */
>>>> +			inode.i_flags &= ~EXT4_EXTENTS_FL;
>>>> +			if (debugfs_write_inode(newfile, &inode, argv[0]))
>>>> +				close(fd);
>>>> +		}
>>>>   	}
>>>>   	close(fd);
>>>>   }
>>>> --
>>>> 1.8.1.2
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

[PATCH 0/2 V2] e2fsprogs/debugfs: do sparse copy when src is a sparse file

[Robert Yang]

=== V2:
* Use 64K for the IO_BUFSIZE and use malloc() to allocate the memory for
  the buffer respect to Darrick's comments.

* Use calloc() and memcmp() to check the sparse block as Darrick
  suggested.

* Adjust the frame and remove a few un-needed code as Darrick
  suggested.

=== V1:
* There are two patches, one is used for fixing the max length of the 
  argument, the other one is for sparsing copy when src is a sparse
  file.

* BTW., we are trying to use the debugfs to create an ext2/ext3/ext4
  image based on a given directory or file, which is similar to genext2fs,
  please vist here for the script you are interested in it: 

  https://gist.github.com/robertlinux/6034499

  It is an external shell script at the moment, the performance is not good,
  so we are going to:
  - Integrate this function into mke2fs in C language.
  - Add a [-d <directory>] option to specify the initial directory in mke2fs

  I'm not sure whether such a patch is acceptable or not, please feel free to
  give your comments.

Robert Yang
Wind River System

Robert Yang (2):
  debugfs.c: the max length of debugfs argument is too short
  debugfs.c: do sparse copy when src is a sparse file

 debugfs/debugfs.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 6 deletions(-)

-- 
1.8.1.2

[PATCH 2/2] debugfs.c: do sparse copy when src is a sparse file

[Robert Yang]

Let debugfs do sparse copy when src is a sparse file, just like
"cp --sparse=auto"

* For the:
  #define IO_BUFSIZE 64*1024
  this is a suggested value from gnu coreutils:
  http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/ioblksize.h;h=1ae93255e7d0ccf0855208c7ae5888209997bf16;hb=HEAD

* Use malloc() to allocate memory for the buffer since put 64K (or
  more) on the stack seems not a good idea.

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
Acked-by: Darren Hart <dvhart@linux.intel.com>
---
 debugfs/debugfs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 53 insertions(+), 5 deletions(-)

diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index b77d0b5..0379f38 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -37,6 +37,16 @@ extern char *optarg;
 #include "../version.h"
 #include "jfs_user.h"
 
+/* 64KiB is the minimium blksize to best minimize system call overhead. */
+#ifndef IO_BUFSIZE
+#define IO_BUFSIZE 64*1024
+#endif
+
+/* Block size for `st_blocks' */
+#ifndef S_BLKSIZE
+#define S_BLKSIZE 512
+#endif
+
 ss_request_table *extra_cmds;
 const char *debug_prog_name;
 int sci_idx;
@@ -1571,22 +1581,28 @@ void do_find_free_inode(int argc, char *argv[])
 }
 
 #ifndef READ_ONLY
-static errcode_t copy_file(int fd, ext2_ino_t newfile)
+static errcode_t copy_file(int fd, ext2_ino_t newfile, int bufsize, int make_holes)
 {
 	ext2_file_t	e2_file;
 	errcode_t	retval;
 	int		got;
 	unsigned int	written;
-	char		buf[8192];
+	char		*buf;
 	char		*ptr;
+	char		*zeromem;
+	int		cmp;
 
 	retval = ext2fs_file_open(current_fs, newfile,
 				  EXT2_FILE_WRITE, &e2_file);
 	if (retval)
 		return retval;
 
+	if (!(buf = (char *) malloc(bufsize))){
+		fprintf(stderr, "copy_file: can't allocate buffer\n");
+		return;
+	}
 	while (1) {
-		got = read(fd, buf, sizeof(buf));
+		got = read(fd, buf, bufsize);
 		if (got == 0)
 			break;
 		if (got < 0) {
@@ -1594,20 +1610,42 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
 			goto fail;
 		}
 		ptr = buf;
+
+		/* Sparse copy */
+		if (make_holes) {
+			if (!(zeromem = (char *) calloc(got, 1))) {
+				fprintf(stderr, "copy_file: can't allocate buffer\n");
+				return;
+			}
+			/* Check whether all is zero */
+			cmp = memcmp(ptr, zeromem, got);
+			/* Free it as early as possible */
+			free(zeromem);
+			if (cmp == 0) {
+				 /* The whole block is zero, make a hole */
+				retval = ext2fs_file_lseek(e2_file, got, EXT2_SEEK_CUR, NULL);
+				if (retval)
+					goto fail;
+				got = 0;
+			}
+		}
+
+		/* Normal copy */
 		while (got > 0) {
 			retval = ext2fs_file_write(e2_file, ptr,
 						   got, &written);
 			if (retval)
 				goto fail;
-
 			got -= written;
 			ptr += written;
 		}
 	}
+	free(buf);
 	retval = ext2fs_file_close(e2_file);
 	return retval;
 
 fail:
+	free(buf);
 	(void) ext2fs_file_close(e2_file);
 	return retval;
 }
@@ -1620,6 +1658,8 @@ void do_write(int argc, char *argv[])
 	ext2_ino_t	newfile;
 	errcode_t	retval;
 	struct ext2_inode inode;
+	int		bufsize = IO_BUFSIZE;
+	int		make_holes = 0;
 
 	if (common_args_process(argc, argv, 3, 3, "write",
 				"<native file> <new file>", CHECK_FS_RW))
@@ -1684,7 +1724,15 @@ void do_write(int argc, char *argv[])
 		return;
 	}
 	if (LINUX_S_ISREG(inode.i_mode)) {
-		retval = copy_file(fd, newfile);
+		if (statbuf.st_blocks < statbuf.st_size / S_BLKSIZE) {
+			make_holes = 1;
+			/*
+			 * Use I/O blocksize as buffer size when
+			 * copying sparse files.
+			 */
+			bufsize = statbuf.st_blksize;
+		}
+		retval = copy_file(fd, newfile, bufsize, make_holes);
 		if (retval)
 			com_err("copy_file", retval, 0);
 	}
-- 
1.8.1.2

Re: [PATCH 2/2] debugfs.c: do sparse copy when src is a sparse file

[Darrick J. Wong]

On Fri, Jul 26, 2013 at 06:30:57PM +0800, Robert Yang wrote:
> Let debugfs do sparse copy when src is a sparse file, just like
> "cp --sparse=auto"
> 
> * For the:
>   #define IO_BUFSIZE 64*1024
>   this is a suggested value from gnu coreutils:
>   http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/ioblksize.h;h=1ae93255e7d0ccf0855208c7ae5888209997bf16;hb=HEAD
> 
> * Use malloc() to allocate memory for the buffer since put 64K (or
>   more) on the stack seems not a good idea.
> 
> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> Acked-by: Darren Hart <dvhart@linux.intel.com>
> ---
>  debugfs/debugfs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
> index b77d0b5..0379f38 100644
> --- a/debugfs/debugfs.c
> +++ b/debugfs/debugfs.c
> @@ -37,6 +37,16 @@ extern char *optarg;
>  #include "../version.h"
>  #include "jfs_user.h"
>  
> +/* 64KiB is the minimium blksize to best minimize system call overhead. */
> +#ifndef IO_BUFSIZE
> +#define IO_BUFSIZE 64*1024
> +#endif
> +
> +/* Block size for `st_blocks' */
> +#ifndef S_BLKSIZE
> +#define S_BLKSIZE 512
> +#endif
> +
>  ss_request_table *extra_cmds;
>  const char *debug_prog_name;
>  int sci_idx;
> @@ -1571,22 +1581,28 @@ void do_find_free_inode(int argc, char *argv[])
>  }
>  
>  #ifndef READ_ONLY
> -static errcode_t copy_file(int fd, ext2_ino_t newfile)
> +static errcode_t copy_file(int fd, ext2_ino_t newfile, int bufsize, int make_holes)
>  {
>  	ext2_file_t	e2_file;
>  	errcode_t	retval;
>  	int		got;
>  	unsigned int	written;
> -	char		buf[8192];
> +	char		*buf;
>  	char		*ptr;
> +	char		*zeromem;
> +	int		cmp;
>  
>  	retval = ext2fs_file_open(current_fs, newfile,
>  				  EXT2_FILE_WRITE, &e2_file);
>  	if (retval)
>  		return retval;
>  
> +	if (!(buf = (char *) malloc(bufsize))){
> +		fprintf(stderr, "copy_file: can't allocate buffer\n");
> +		return;
> +	}
>  	while (1) {
> -		got = read(fd, buf, sizeof(buf));
> +		got = read(fd, buf, bufsize);
>  		if (got == 0)
>  			break;
>  		if (got < 0) {
> @@ -1594,20 +1610,42 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
>  			goto fail;
>  		}
>  		ptr = buf;
> +
> +		/* Sparse copy */
> +		if (make_holes) {
> +			if (!(zeromem = (char *) calloc(got, 1))) {

Maybe ext2fs_get_memzero()?  And I suspect you can allocate a bufsize-sized
zeromem outside the while loop.  Chances are pretty good that if make_holes,
then buf_size won't be greater than 4096 anyway.

--D

> +				fprintf(stderr, "copy_file: can't allocate buffer\n");
> +				return;
> +			}
> +			/* Check whether all is zero */
> +			cmp = memcmp(ptr, zeromem, got);
> +			/* Free it as early as possible */
> +			free(zeromem);
> +			if (cmp == 0) {
> +				 /* The whole block is zero, make a hole */
> +				retval = ext2fs_file_lseek(e2_file, got, EXT2_SEEK_CUR, NULL);
> +				if (retval)
> +					goto fail;
> +				got = 0;
> +			}
> +		}
> +
> +		/* Normal copy */
>  		while (got > 0) {
>  			retval = ext2fs_file_write(e2_file, ptr,
>  						   got, &written);
>  			if (retval)
>  				goto fail;
> -
>  			got -= written;
>  			ptr += written;
>  		}
>  	}
> +	free(buf);
>  	retval = ext2fs_file_close(e2_file);
>  	return retval;
>  
>  fail:
> +	free(buf);
>  	(void) ext2fs_file_close(e2_file);
>  	return retval;
>  }
> @@ -1620,6 +1658,8 @@ void do_write(int argc, char *argv[])
>  	ext2_ino_t	newfile;
>  	errcode_t	retval;
>  	struct ext2_inode inode;
> +	int		bufsize = IO_BUFSIZE;
> +	int		make_holes = 0;
>  
>  	if (common_args_process(argc, argv, 3, 3, "write",
>  				"<native file> <new file>", CHECK_FS_RW))
> @@ -1684,7 +1724,15 @@ void do_write(int argc, char *argv[])
>  		return;
>  	}
>  	if (LINUX_S_ISREG(inode.i_mode)) {
> -		retval = copy_file(fd, newfile);
> +		if (statbuf.st_blocks < statbuf.st_size / S_BLKSIZE) {
> +			make_holes = 1;
> +			/*
> +			 * Use I/O blocksize as buffer size when
> +			 * copying sparse files.
> +			 */
> +			bufsize = statbuf.st_blksize;
> +		}
> +		retval = copy_file(fd, newfile, bufsize, make_holes);
>  		if (retval)
>  			com_err("copy_file", retval, 0);
>  	}
> -- 
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] debugfs.c: do sparse copy when src is a sparse file

[Robert Yang]

On 07/27/2013 12:02 AM, Darrick J. Wong wrote:
> On Fri, Jul 26, 2013 at 06:30:57PM +0800, Robert Yang wrote:
>> Let debugfs do sparse copy when src is a sparse file, just like
>> "cp --sparse=auto"
>>
>> * For the:
>>    #define IO_BUFSIZE 64*1024
>>    this is a suggested value from gnu coreutils:
>>    http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/ioblksize.h;h=1ae93255e7d0ccf0855208c7ae5888209997bf16;hb=HEAD
>>
>> * Use malloc() to allocate memory for the buffer since put 64K (or
>>    more) on the stack seems not a good idea.
>>
>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
>> Acked-by: Darren Hart <dvhart@linux.intel.com>
>> ---
>>   debugfs/debugfs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 53 insertions(+), 5 deletions(-)
>>
>> diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
>> index b77d0b5..0379f38 100644
>> --- a/debugfs/debugfs.c
>> +++ b/debugfs/debugfs.c
>> @@ -37,6 +37,16 @@ extern char *optarg;
>>   #include "../version.h"
>>   #include "jfs_user.h"
>>
>> +/* 64KiB is the minimium blksize to best minimize system call overhead. */
>> +#ifndef IO_BUFSIZE
>> +#define IO_BUFSIZE 64*1024
>> +#endif
>> +
>> +/* Block size for `st_blocks' */
>> +#ifndef S_BLKSIZE
>> +#define S_BLKSIZE 512
>> +#endif
>> +
>>   ss_request_table *extra_cmds;
>>   const char *debug_prog_name;
>>   int sci_idx;
>> @@ -1571,22 +1581,28 @@ void do_find_free_inode(int argc, char *argv[])
>>   }
>>
>>   #ifndef READ_ONLY
>> -static errcode_t copy_file(int fd, ext2_ino_t newfile)
>> +static errcode_t copy_file(int fd, ext2_ino_t newfile, int bufsize, int make_holes)
>>   {
>>   	ext2_file_t	e2_file;
>>   	errcode_t	retval;
>>   	int		got;
>>   	unsigned int	written;
>> -	char		buf[8192];
>> +	char		*buf;
>>   	char		*ptr;
>> +	char		*zeromem;
>> +	int		cmp;
>>
>>   	retval = ext2fs_file_open(current_fs, newfile,
>>   				  EXT2_FILE_WRITE, &e2_file);
>>   	if (retval)
>>   		return retval;
>>
>> +	if (!(buf = (char *) malloc(bufsize))){
>> +		fprintf(stderr, "copy_file: can't allocate buffer\n");
>> +		return;
>> +	}
>>   	while (1) {
>> -		got = read(fd, buf, sizeof(buf));
>> +		got = read(fd, buf, bufsize);
>>   		if (got == 0)
>>   			break;
>>   		if (got < 0) {
>> @@ -1594,20 +1610,42 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
>>   			goto fail;
>>   		}
>>   		ptr = buf;
>> +
>> +		/* Sparse copy */
>> +		if (make_holes) {
>> +			if (!(zeromem = (char *) calloc(got, 1))) {
>
> Maybe ext2fs_get_memzero()?  And I suspect you can allocate a bufsize-sized
> zeromem outside the while loop.  Chances are pretty good that if make_holes,
> then buf_size won't be greater than 4096 anyway.

Thanks, sounds great, my testing shows that it is faster for copying the
large sparse file after move the ext2fs_get_memzero() out of the while loop,
and didn't have any obvious impaction for non-sparse file, I will update it
and send a V3.

// Robert

>
> --D
>
>> +				fprintf(stderr, "copy_file: can't allocate buffer\n");
>> +				return;
>> +			}
>> +			/* Check whether all is zero */
>> +			cmp = memcmp(ptr, zeromem, got);
>> +			/* Free it as early as possible */
>> +			free(zeromem);
>> +			if (cmp == 0) {
>> +				 /* The whole block is zero, make a hole */
>> +				retval = ext2fs_file_lseek(e2_file, got, EXT2_SEEK_CUR, NULL);
>> +				if (retval)
>> +					goto fail;
>> +				got = 0;
>> +			}
>> +		}
>> +
>> +		/* Normal copy */
>>   		while (got > 0) {
>>   			retval = ext2fs_file_write(e2_file, ptr,
>>   						   got, &written);
>>   			if (retval)
>>   				goto fail;
>> -
>>   			got -= written;
>>   			ptr += written;
>>   		}
>>   	}
>> +	free(buf);
>>   	retval = ext2fs_file_close(e2_file);
>>   	return retval;
>>
>>   fail:
>> +	free(buf);
>>   	(void) ext2fs_file_close(e2_file);
>>   	return retval;
>>   }
>> @@ -1620,6 +1658,8 @@ void do_write(int argc, char *argv[])
>>   	ext2_ino_t	newfile;
>>   	errcode_t	retval;
>>   	struct ext2_inode inode;
>> +	int		bufsize = IO_BUFSIZE;
>> +	int		make_holes = 0;
>>
>>   	if (common_args_process(argc, argv, 3, 3, "write",
>>   				"<native file> <new file>", CHECK_FS_RW))
>> @@ -1684,7 +1724,15 @@ void do_write(int argc, char *argv[])
>>   		return;
>>   	}
>>   	if (LINUX_S_ISREG(inode.i_mode)) {
>> -		retval = copy_file(fd, newfile);
>> +		if (statbuf.st_blocks < statbuf.st_size / S_BLKSIZE) {
>> +			make_holes = 1;
>> +			/*
>> +			 * Use I/O blocksize as buffer size when
>> +			 * copying sparse files.
>> +			 */
>> +			bufsize = statbuf.st_blksize;
>> +		}
>> +		retval = copy_file(fd, newfile, bufsize, make_holes);
>>   		if (retval)
>>   			com_err("copy_file", retval, 0);
>>   	}
>> --
>> 1.8.1.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

[PATCH 0/2 V3] e2fsprogs/debugfs: do sparse copy when src is a sparse file

[Robert Yang]

=== V3: 
* Use BUFSIZ (which is 8192 on Linux systems) for the max argument
  length as Ted suggested.

* Use ext2fs_get_memzero() and move it out of the while loop to get a
  better performance as Darrick suggested.

=== V2: 
* Use 64K for the IO_BUFSIZE and use malloc() to allocate the memory for 
  the buffer respect to Darrick's comments.

* Use calloc() and memcmp() to check the sparse block as Darrick
  suggested.

* Adjust the frame and remove a few un-needed code as Darrick
  suggested.

=== V1: 
* There are two patches, one is used for fixing the max length of the 
  argument, the other one is for sparsing copy when src is a sparse
  file.

* BTW., we are trying to use the debugfs to create an ext2/ext3/ext4
  image based on a given directory or file, which is similar to genext2fs,
  please vist here for the script you are interested in it: 

  https://gist.github.com/robertlinux/6034499

  It is an external shell script at the moment, the performance is not good,
  so we are going to: 
  - Integrate this function into mke2fs in C language.
  - Add a [-d <directory>] option to specify the initial directory in mke2fs

  I'm not sure whether such a patch is acceptable or not, please feel free to
  give your comments.

Robert Yang
Wind River System

Robert Yang (2):
  debugfs.c: the max length of debugfs argument is too short
  debugfs.c: do sparse copy when src is a sparse file

 debugfs/debugfs.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 62 insertions(+), 5 deletions(-)

-- 
1.8.1.2

[PATCH 2/2] debugfs.c: do sparse copy when src is a sparse file

[Robert Yang]

Let debugfs do sparse copy when src is a sparse file, just like
"cp --sparse=auto"

* For the:
  #define IO_BUFSIZE 64*1024
  this is a suggested value from gnu coreutils:
  http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/ioblksize.h;h=1ae93255e7d0ccf0855208c7ae5888209997bf16;hb=HEAD

* Use malloc() to allocate memory for the buffer since put 64K (or
  more) on the stack seems not a good idea.

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
Acked-by: Darren Hart <dvhart@linux.intel.com>
---
 debugfs/debugfs.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 57 insertions(+), 4 deletions(-)

diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index a6bc932..d35fecc 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -41,6 +41,16 @@ extern char *optarg;
 #define BUFSIZ 8192
 #endif
 
+/* 64KiB is the minimium blksize to best minimize system call overhead. */
+#ifndef IO_BUFSIZE
+#define IO_BUFSIZE 64*1024
+#endif
+
+/* Block size for `st_blocks' */
+#ifndef S_BLKSIZE
+#define S_BLKSIZE 512
+#endif
+
 ss_request_table *extra_cmds;
 const char *debug_prog_name;
 int sci_idx;
@@ -1575,22 +1585,36 @@ void do_find_free_inode(int argc, char *argv[])
 }
 
 #ifndef READ_ONLY
-static errcode_t copy_file(int fd, ext2_ino_t newfile)
+static errcode_t copy_file(int fd, ext2_ino_t newfile, int bufsize, int make_holes)
 {
 	ext2_file_t	e2_file;
 	errcode_t	retval;
 	int		got;
 	unsigned int	written;
-	char		buf[8192];
+	char		*buf;
 	char		*ptr;
+	char		*zero_buf;
+	int		cmp;
 
 	retval = ext2fs_file_open(current_fs, newfile,
 				  EXT2_FILE_WRITE, &e2_file);
 	if (retval)
 		return retval;
 
+	if (!(buf = (char *) malloc(bufsize))){
+		com_err("copy_file", errno, "can't allocate buffer\n");
+		return;
+	}
+
+        /* This is used for checking whether the whole block is zero */
+	retval = ext2fs_get_memzero(bufsize, &zero_buf);
+	if (retval) {
+		com_err("copy_file", retval, "can't allocate buffer\n");
+		return retval;
+	}
+
 	while (1) {
-		got = read(fd, buf, sizeof(buf));
+		got = read(fd, buf, bufsize);
 		if (got == 0)
 			break;
 		if (got < 0) {
@@ -1598,6 +1622,21 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
 			goto fail;
 		}
 		ptr = buf;
+
+		/* Sparse copy */
+		if (make_holes) {
+			/* Check whether all is zero */
+			cmp = memcmp(ptr, zero_buf, got);
+			if (cmp == 0) {
+				 /* The whole block is zero, make a hole */
+				retval = ext2fs_file_lseek(e2_file, got, EXT2_SEEK_CUR, NULL);
+				if (retval)
+					goto fail;
+				got = 0;
+			}
+		}
+
+		/* Normal copy */
 		while (got > 0) {
 			retval = ext2fs_file_write(e2_file, ptr,
 						   got, &written);
@@ -1608,10 +1647,14 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
 			ptr += written;
 		}
 	}
+	free(buf);
+	ext2fs_free_mem(&zero_buf);
 	retval = ext2fs_file_close(e2_file);
 	return retval;
 
 fail:
+	free(buf);
+	ext2fs_free_mem(&zero_buf);
 	(void) ext2fs_file_close(e2_file);
 	return retval;
 }
@@ -1624,6 +1667,8 @@ void do_write(int argc, char *argv[])
 	ext2_ino_t	newfile;
 	errcode_t	retval;
 	struct ext2_inode inode;
+	int		bufsize = IO_BUFSIZE;
+	int		make_holes = 0;
 
 	if (common_args_process(argc, argv, 3, 3, "write",
 				"<native file> <new file>", CHECK_FS_RW))
@@ -1699,7 +1744,15 @@ void do_write(int argc, char *argv[])
 		return;
 	}
 	if (LINUX_S_ISREG(inode.i_mode)) {
-		retval = copy_file(fd, newfile);
+		if (statbuf.st_blocks < statbuf.st_size / S_BLKSIZE) {
+			make_holes = 1;
+			/*
+			 * Use I/O blocksize as buffer size when
+			 * copying sparse files.
+			 */
+			bufsize = statbuf.st_blksize;
+		}
+		retval = copy_file(fd, newfile, bufsize, make_holes);
 		if (retval)
 			com_err("copy_file", retval, 0);
 	}
-- 
1.8.1.2

Re: [PATCH 2/2] debugfs.c: do sparse copy when src is a sparse file

[Darren Hart]

Ted, Darrick,

Any more thoughts on this series from Robert?

We'd like to pull this in to the Yocto Project, but are trying hard not
to include patches that are not at least acked by the upstream
maintainers.

I believe Robert as addressed the concerns raised?

Thanks!

Darren

On Mon, 2013-07-29 at 17:06 +0800, Robert Yang wrote:
> Let debugfs do sparse copy when src is a sparse file, just like
> "cp --sparse=auto"
> 
> * For the:
>   #define IO_BUFSIZE 64*1024
>   this is a suggested value from gnu coreutils:
>   http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/ioblksize.h;h=1ae93255e7d0ccf0855208c7ae5888209997bf16;hb=HEAD
> 
> * Use malloc() to allocate memory for the buffer since put 64K (or
>   more) on the stack seems not a good idea.
> 
> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> Acked-by: Darren Hart <dvhart@linux.intel.com>
> ---
>  debugfs/debugfs.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
> index a6bc932..d35fecc 100644
> --- a/debugfs/debugfs.c
> +++ b/debugfs/debugfs.c
> @@ -41,6 +41,16 @@ extern char *optarg;
>  #define BUFSIZ 8192
>  #endif
>  
> +/* 64KiB is the minimium blksize to best minimize system call overhead. */
> +#ifndef IO_BUFSIZE
> +#define IO_BUFSIZE 64*1024
> +#endif
> +
> +/* Block size for `st_blocks' */
> +#ifndef S_BLKSIZE
> +#define S_BLKSIZE 512
> +#endif
> +
>  ss_request_table *extra_cmds;
>  const char *debug_prog_name;
>  int sci_idx;
> @@ -1575,22 +1585,36 @@ void do_find_free_inode(int argc, char *argv[])
>  }
>  
>  #ifndef READ_ONLY
> -static errcode_t copy_file(int fd, ext2_ino_t newfile)
> +static errcode_t copy_file(int fd, ext2_ino_t newfile, int bufsize, int make_holes)
>  {
>  	ext2_file_t	e2_file;
>  	errcode_t	retval;
>  	int		got;
>  	unsigned int	written;
> -	char		buf[8192];
> +	char		*buf;
>  	char		*ptr;
> +	char		*zero_buf;
> +	int		cmp;
>  
>  	retval = ext2fs_file_open(current_fs, newfile,
>  				  EXT2_FILE_WRITE, &e2_file);
>  	if (retval)
>  		return retval;
>  
> +	if (!(buf = (char *) malloc(bufsize))){
> +		com_err("copy_file", errno, "can't allocate buffer\n");
> +		return;
> +	}
> +
> +        /* This is used for checking whether the whole block is zero */
> +	retval = ext2fs_get_memzero(bufsize, &zero_buf);
> +	if (retval) {
> +		com_err("copy_file", retval, "can't allocate buffer\n");
> +		return retval;
> +	}
> +
>  	while (1) {
> -		got = read(fd, buf, sizeof(buf));
> +		got = read(fd, buf, bufsize);
>  		if (got == 0)
>  			break;
>  		if (got < 0) {
> @@ -1598,6 +1622,21 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
>  			goto fail;
>  		}
>  		ptr = buf;
> +
> +		/* Sparse copy */
> +		if (make_holes) {
> +			/* Check whether all is zero */
> +			cmp = memcmp(ptr, zero_buf, got);
> +			if (cmp == 0) {
> +				 /* The whole block is zero, make a hole */
> +				retval = ext2fs_file_lseek(e2_file, got, EXT2_SEEK_CUR, NULL);
> +				if (retval)
> +					goto fail;
> +				got = 0;
> +			}
> +		}
> +
> +		/* Normal copy */
>  		while (got > 0) {
>  			retval = ext2fs_file_write(e2_file, ptr,
>  						   got, &written);
> @@ -1608,10 +1647,14 @@ static errcode_t copy_file(int fd, ext2_ino_t newfile)
>  			ptr += written;
>  		}
>  	}
> +	free(buf);
> +	ext2fs_free_mem(&zero_buf);
>  	retval = ext2fs_file_close(e2_file);
>  	return retval;
>  
>  fail:
> +	free(buf);
> +	ext2fs_free_mem(&zero_buf);
>  	(void) ext2fs_file_close(e2_file);
>  	return retval;
>  }
> @@ -1624,6 +1667,8 @@ void do_write(int argc, char *argv[])
>  	ext2_ino_t	newfile;
>  	errcode_t	retval;
>  	struct ext2_inode inode;
> +	int		bufsize = IO_BUFSIZE;
> +	int		make_holes = 0;
>  
>  	if (common_args_process(argc, argv, 3, 3, "write",
>  				"<native file> <new file>", CHECK_FS_RW))
> @@ -1699,7 +1744,15 @@ void do_write(int argc, char *argv[])
>  		return;
>  	}
>  	if (LINUX_S_ISREG(inode.i_mode)) {
> -		retval = copy_file(fd, newfile);
> +		if (statbuf.st_blocks < statbuf.st_size / S_BLKSIZE) {
> +			make_holes = 1;
> +			/*
> +			 * Use I/O blocksize as buffer size when
> +			 * copying sparse files.
> +			 */
> +			bufsize = statbuf.st_blksize;
> +		}
> +		retval = copy_file(fd, newfile, bufsize, make_holes);
>  		if (retval)
>  			com_err("copy_file", retval, 0);
>  	}

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel