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

[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 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, so use BUFSIZ (which is 8192 on Linux
systems), that's also what the ss library uses.

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

diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index 2660218..a6bc932 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -37,6 +37,10 @@ extern char *optarg;
 #include "../version.h"
 #include "jfs_user.h"
 
+#ifndef BUFSIZ
+#define BUFSIZ 8192
+#endif
+
 ss_request_table *extra_cmds;
 const char *debug_prog_name;
 int sci_idx;
@@ -2311,7 +2315,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[BUFSIZ];
 	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 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

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

[Robert Yang]

Any comments, please ?

// Robert

On 07/29/2013 05:06 PM, Robert Yang wrote:
> === 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(-)
>