[PATCH 0/6] miscellaneous e2fsprogs fixes

[PATCH 0/6] miscellaneous e2fsprogs fixes

[Darrick J. Wong]

Here are a few miscellaneous fixes for e2fsprogs-next.  Eventually
they'll end up in the patchbomb rollup, though for now I'm taking a
break on messing with e2fsprogs outside of pushing the rest of the
patchbomb into upstream.

The first couple of patches fix resource leaks and cleans up the Linux
kernel tests in mke2fs.

The next two patches fix some more errors in the punch routines that
resulted in premature exiting from the punch routine, or extent tree
breakage.

The final two patches deals with extent tree truncation problems when
there is an error while splitting an extent into three parts.

I've tested these e2fsprogs changes against the -next branch as of
1/16.  These days, I use an 8GB ramdisk and a 20T "disk" I constructed
out of dm-snapshot to test in an x64 VM.  The make check tests should
pass.

Comments and questions are, as always, welcome.

--D

[PATCH 1/6] misc: fix resource leaks in e2fsprogs

[Darrick J. Wong]

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 lib/ext2fs/icount.c |    2 ++
 1 file changed, 2 insertions(+)


diff --git a/lib/ext2fs/icount.c b/lib/ext2fs/icount.c
index a3b20f0..7d1b3d5 100644
--- a/lib/ext2fs/icount.c
+++ b/lib/ext2fs/icount.c
@@ -198,6 +198,7 @@ errcode_t ext2fs_create_icount_tdb(ext2_filsys fs, char *tdb_dir,
 	fd = mkstemp(fn);
 	if (fd < 0) {
 		retval = errno;
+		ext2fs_free_mem(&fn);
 		goto errout;
 	}
 	umask(save_umask);
@@ -216,6 +217,7 @@ errcode_t ext2fs_create_icount_tdb(ext2_filsys fs, char *tdb_dir,
 	close(fd);
 	if (icount->tdb == NULL) {
 		retval = errno;
+		ext2fs_free_mem(&fn);
 		goto errout;
 	}
 	*ret = icount;

[PATCH 2/6] mke2fs: clean up kernel version tests

[Darrick J. Wong]

Refactor the running kernel version checks to hide the details of
version code checking, etc.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 misc/mke2fs.c |   39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)


diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 527f25b..f8232d4 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -27,6 +27,7 @@
 #include <time.h>
 #ifdef __linux__
 #include <sys/utsname.h>
+#include <linux/version.h>
 #endif
 #ifdef HAVE_GETOPT_H
 #include <getopt.h>
@@ -169,7 +170,28 @@ static int parse_version_number(const char *s)
 	rev = strtol(cp, &endptr, 10);
 	if (cp == endptr)
 		return 0;
-	return ((((major * 256) + minor) * 256) + rev);
+	return KERNEL_VERSION(major, minor, rev);
+}
+
+static int is_before_linux_ver(unsigned int major, unsigned int minor)
+{
+	struct		utsname ut;
+	int		linux_version_code = 0;
+
+	if (uname(&ut)) {
+		perror("uname");
+		exit(1);
+	}
+	linux_version_code = parse_version_number(ut.release);
+	if (linux_version_code == 0)
+		return 0;
+
+	return linux_version_code < KERNEL_VERSION(major, minor, 0);
+}
+#else
+static int is_before_linux_ver(unsigned int major, unsigned int minor)
+{
+	return 0;
 }
 #endif
 
@@ -1362,9 +1384,6 @@ static void PRS(int argc, char *argv[])
 	 * Finally, we complain about fs_blocks_count > 2^32 on a non-64bit fs.
 	 */
 	blk64_t		fs_blocks_count = 0;
-#ifdef __linux__
-	struct 		utsname ut;
-#endif
 	long		sysval;
 	int		s_opt = -1, r_opt = -1;
 	char		*fs_features = 0;
@@ -1430,15 +1449,8 @@ profile_error:
 	memset(&fs_param, 0, sizeof(struct ext2_super_block));
 	fs_param.s_rev_level = 1;  /* Create revision 1 filesystems now */
 
-#ifdef __linux__
-	if (uname(&ut)) {
-		perror("uname");
-		exit(1);
-	}
-	linux_version_code = parse_version_number(ut.release);
-	if (linux_version_code && linux_version_code < (2*65536 + 2*256))
+	if (is_before_linux_ver(2, 2))
 		fs_param.s_rev_level = 0;
-#endif
 
 	if (argc && *argv) {
 		program_name = get_progname(*argv);
@@ -1876,8 +1888,7 @@ profile_error:
 
 		if (use_bsize == -1) {
 			use_bsize = sys_page_size;
-			if ((linux_version_code < (2*65536 + 6*256)) &&
-			    (use_bsize > 4096))
+			if (is_before_linux_ver(2, 6) && use_bsize > 4096)
 				use_bsize = 4096;
 		}
 		if (lsector_size && use_bsize < lsector_size)

[PATCH 3/6] libext2fs: iterate past lower extents during punch

[Darrick J. Wong]

When we're iterating extents during a punch operation, the loop exits
if the punch region is entirely to the right of the extent we're
looking at.  This can happen if the punch region starts in the middle
of a hole and covers mapped extents.  When this happens, we want to
skip to the next extent, because it might be punchable.

Also, if we've totally passed the punch range, stop.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 lib/ext2fs/punch.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)


diff --git a/lib/ext2fs/punch.c b/lib/ext2fs/punch.c
index 25d7953..657cb53 100644
--- a/lib/ext2fs/punch.c
+++ b/lib/ext2fs/punch.c
@@ -288,8 +288,12 @@ static errcode_t ext2fs_punch_extent(ext2_filsys fs, ext2_ino_t ino,
 			   (unsigned long long) end,
 			   (unsigned long long) next);
 		if (start <= extent.e_lblk) {
+			/*
+			 * Have we iterated past the end of the punch region?
+			 * If so, we can stop.
+			 */
 			if (end < extent.e_lblk)
-				goto next_extent;
+				break;
 			dbg_printf("Case #%d\n", 1);
 			/* Start of deleted region before extent; 
 			   adjust beginning of extent */
@@ -303,8 +307,13 @@ static errcode_t ext2fs_punch_extent(ext2_filsys fs, ext2_ino_t ino,
 			extent.e_lblk += free_count;
 			extent.e_pblk += free_count;
 		} else if (end >= next-1) {
+			/*
+			 * Is the punch region beyond this extent?  This can
+			 * happen if start is already inside a hole.  Try to
+			 * advance to the next extent if this is the case.
+			 */
 			if (start >= next)
-				break;
+				goto next_extent;
 			/* End of deleted region after extent;
 			   adjust end of extent */
 			dbg_printf("Case #%d\n", 2);

[PATCH 4/6] libext2fs: during punch, fix parent extents after modifying extent

[Darrick J. Wong]

When modifying/removing an extent during punch, don't forget to update
the extent's parents.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 lib/ext2fs/punch.c |    8 ++++++++
 1 file changed, 8 insertions(+)


diff --git a/lib/ext2fs/punch.c b/lib/ext2fs/punch.c
index 657cb53..a3d020e 100644
--- a/lib/ext2fs/punch.c
+++ b/lib/ext2fs/punch.c
@@ -353,6 +353,9 @@ static errcode_t ext2fs_punch_extent(ext2_filsys fs, ext2_ino_t ino,
 		if (extent.e_len) {
 			dbg_print_extent("replacing", &extent);
 			retval = ext2fs_extent_replace(handle, 0, &extent);
+			if (retval)
+				goto errout;
+			retval = ext2fs_extent_fix_parents(handle);
 		} else {
 			struct ext2fs_extent	newex;
 			blk64_t			old_lblk, next_lblk;
@@ -387,6 +390,11 @@ static errcode_t ext2fs_punch_extent(ext2_filsys fs, ext2_ino_t ino,
 			if (retval)
 				goto errout;
 
+			retval = ext2fs_extent_fix_parents(handle);
+			if (retval && retval != EXT2_ET_NO_CURRENT_NODE)
+				goto errout;
+			retval = 0;
+
 			/* Jump forward to the next extent. */
 			ext2fs_extent_goto(handle, next_lblk);
 			op = EXT2_EXTENT_CURRENT;

[PATCH 5/6] libext2fs: don't hang on to unmapped block if extent tree update fails

[Darrick J. Wong]

If we're doing a BMAP_ALLOC allocation and the extent tree update
fails, there's no point in hanging on to the newly allocated block.
So, free it to make fsck happy.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 lib/ext2fs/bmap.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


diff --git a/lib/ext2fs/bmap.c b/lib/ext2fs/bmap.c
index b944c27..db2fd72 100644
--- a/lib/ext2fs/bmap.c
+++ b/lib/ext2fs/bmap.c
@@ -255,8 +255,10 @@ got_block:
 	set_extent:
 		retval = ext2fs_extent_set_bmap(handle, block,
 						blk64, 0);
-		if (retval)
+		if (retval) {
+			ext2fs_block_alloc_stats2(fs, blk64, -1);
 			return retval;
+		}
 		/* Update inode after setting extent */
 		retval = ext2fs_read_inode(fs, ino, inode);
 		if (retval)

[PATCH 6/6] libext2fs: try to roll back when splitting an extent fails

[Darrick J. Wong]

If a client asks us to remap a block in the middle of an extent, we
potentially have to allocate a fair number of blocks to handle extent
tree splits.  A failure in either of the ext2fs_extent_insert calls
leaves us with an extent tree that no longer maps the logical block in
question and everything that came after it!  Therefore, try to roll
back the extent tree changes before returning an error code.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 lib/ext2fs/extent.c |   25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)


diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index 2928695..87921e6 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -1438,13 +1438,17 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
 			goto done;
 	} else {
 		__u32	orig_length;
+		blk64_t	orig_lblk;
+		struct ext2fs_extent orig_extent;
+		errcode_t r2;
 
 #ifdef DEBUG
 		printf("(re/un)mapping in middle of extent\n");
 #endif
 		/* need to split this extent; later */
-
+		orig_lblk = extent.e_lblk;
 		orig_length = extent.e_len;
+		orig_extent = extent;
 
 		/* shorten pre-split extent */
 		extent.e_len = (logical - extent.e_lblk);
@@ -1456,8 +1460,13 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
 			/* insert new extent after current */
 			retval = ext2fs_extent_insert(handle,
 					EXT2_EXTENT_INSERT_AFTER, &newextent);
-			if (retval)
+			if (retval) {
+				r2 = ext2fs_extent_goto(handle, orig_lblk);
+				if (r2 == 0)
+					ext2fs_extent_replace(handle, 0,
+							      &orig_extent);
 				goto done;
+			}
 		}
 		/* add post-split extent */
 		extent.e_pblk += extent.e_len + 1;
@@ -1465,8 +1474,18 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
 		extent.e_len = orig_length - extent.e_len - 1;
 		retval = ext2fs_extent_insert(handle,
 				EXT2_EXTENT_INSERT_AFTER, &extent);
-		if (retval)
+		if (retval) {
+			if (physical) {
+				r2 = ext2fs_extent_goto(handle,
+							newextent.e_lblk);
+				if (r2 == 0)
+					ext2fs_extent_delete(handle, 0);
+			}
+			r2 = ext2fs_extent_goto(handle, orig_lblk);
+			if (r2 == 0)
+				ext2fs_extent_replace(handle, 0, &orig_extent);
 			goto done;
+		}
 	}
 
 done:

Re: [PATCH 1/6] misc: fix resource leaks in e2fsprogs

[Zheng Liu]

On Mon, Jan 20, 2014 at 10:21:45PM -0800, Darrick J. Wong wrote:
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

It seems that we will free 'fn' in ext2fs_free_icount().

  void ext2fs_free_icount(ext2_icount_t icount)
  {
          if (!icount)
                  return;
  
          icount->magic = 0;
          if (icount->list)
                  ext2fs_free_mem(&icount->list);
          if (icount->single)
                  ext2fs_free_inode_bitmap(icount->single);
          if (icount->multiple)
                  ext2fs_free_inode_bitmap(icount->multiple);
          if (icount->tdb)
                  tdb_close(icount->tdb);
          if (icount->tdb_fn) {
                  unlink(icount->tdb_fn);
                  free(icount->tdb_fn);
          }
  
          ext2fs_free_mem(&icount);
  }

Regards,
                                                - Zheng

> ---
>  lib/ext2fs/icount.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> 
> diff --git a/lib/ext2fs/icount.c b/lib/ext2fs/icount.c
> index a3b20f0..7d1b3d5 100644
> --- a/lib/ext2fs/icount.c
> +++ b/lib/ext2fs/icount.c
> @@ -198,6 +198,7 @@ errcode_t ext2fs_create_icount_tdb(ext2_filsys fs, char *tdb_dir,
>  	fd = mkstemp(fn);
>  	if (fd < 0) {
>  		retval = errno;
> +		ext2fs_free_mem(&fn);
>  		goto errout;
>  	}
>  	umask(save_umask);
> @@ -216,6 +217,7 @@ errcode_t ext2fs_create_icount_tdb(ext2_filsys fs, char *tdb_dir,
>  	close(fd);
>  	if (icount->tdb == NULL) {
>  		retval = errno;
> +		ext2fs_free_mem(&fn);
>  		goto errout;
>  	}
>  	*ret = icount;
> 
> --
> 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/6] mke2fs: clean up kernel version tests

[Zheng Liu]

On Mon, Jan 20, 2014 at 10:21:51PM -0800, Darrick J. Wong wrote:
> Refactor the running kernel version checks to hide the details of
> version code checking, etc.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>
                                                - Zheng

> ---
>  misc/mke2fs.c |   39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 527f25b..f8232d4 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -27,6 +27,7 @@
>  #include <time.h>
>  #ifdef __linux__
>  #include <sys/utsname.h>
> +#include <linux/version.h>
>  #endif
>  #ifdef HAVE_GETOPT_H
>  #include <getopt.h>
> @@ -169,7 +170,28 @@ static int parse_version_number(const char *s)
>  	rev = strtol(cp, &endptr, 10);
>  	if (cp == endptr)
>  		return 0;
> -	return ((((major * 256) + minor) * 256) + rev);
> +	return KERNEL_VERSION(major, minor, rev);
> +}
> +
> +static int is_before_linux_ver(unsigned int major, unsigned int minor)
> +{
> +	struct		utsname ut;
> +	int		linux_version_code = 0;
> +
> +	if (uname(&ut)) {
> +		perror("uname");
> +		exit(1);
> +	}
> +	linux_version_code = parse_version_number(ut.release);
> +	if (linux_version_code == 0)
> +		return 0;
> +
> +	return linux_version_code < KERNEL_VERSION(major, minor, 0);
> +}
> +#else
> +static int is_before_linux_ver(unsigned int major, unsigned int minor)
> +{
> +	return 0;
>  }
>  #endif
>  
> @@ -1362,9 +1384,6 @@ static void PRS(int argc, char *argv[])
>  	 * Finally, we complain about fs_blocks_count > 2^32 on a non-64bit fs.
>  	 */
>  	blk64_t		fs_blocks_count = 0;
> -#ifdef __linux__
> -	struct 		utsname ut;
> -#endif
>  	long		sysval;
>  	int		s_opt = -1, r_opt = -1;
>  	char		*fs_features = 0;
> @@ -1430,15 +1449,8 @@ profile_error:
>  	memset(&fs_param, 0, sizeof(struct ext2_super_block));
>  	fs_param.s_rev_level = 1;  /* Create revision 1 filesystems now */
>  
> -#ifdef __linux__
> -	if (uname(&ut)) {
> -		perror("uname");
> -		exit(1);
> -	}
> -	linux_version_code = parse_version_number(ut.release);
> -	if (linux_version_code && linux_version_code < (2*65536 + 2*256))
> +	if (is_before_linux_ver(2, 2))
>  		fs_param.s_rev_level = 0;
> -#endif
>  
>  	if (argc && *argv) {
>  		program_name = get_progname(*argv);
> @@ -1876,8 +1888,7 @@ profile_error:
>  
>  		if (use_bsize == -1) {
>  			use_bsize = sys_page_size;
> -			if ((linux_version_code < (2*65536 + 6*256)) &&
> -			    (use_bsize > 4096))
> +			if (is_before_linux_ver(2, 6) && use_bsize > 4096)
>  				use_bsize = 4096;
>  		}
>  		if (lsector_size && use_bsize < lsector_size)
> 
> --
> 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 3/6] libext2fs: iterate past lower extents during punch

[Zheng Liu]

On Mon, Jan 20, 2014 at 10:21:58PM -0800, Darrick J. Wong wrote:
> When we're iterating extents during a punch operation, the loop exits
> if the punch region is entirely to the right of the extent we're
> looking at.  This can happen if the punch region starts in the middle
> of a hole and covers mapped extents.  When this happens, we want to
> skip to the next extent, because it might be punchable.
> 
> Also, if we've totally passed the punch range, stop.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>
                                                - Zheng

> ---
>  lib/ext2fs/punch.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/lib/ext2fs/punch.c b/lib/ext2fs/punch.c
> index 25d7953..657cb53 100644
> --- a/lib/ext2fs/punch.c
> +++ b/lib/ext2fs/punch.c
> @@ -288,8 +288,12 @@ static errcode_t ext2fs_punch_extent(ext2_filsys fs, ext2_ino_t ino,
>  			   (unsigned long long) end,
>  			   (unsigned long long) next);
>  		if (start <= extent.e_lblk) {
> +			/*
> +			 * Have we iterated past the end of the punch region?
> +			 * If so, we can stop.
> +			 */
>  			if (end < extent.e_lblk)
> -				goto next_extent;
> +				break;
>  			dbg_printf("Case #%d\n", 1);
>  			/* Start of deleted region before extent; 
>  			   adjust beginning of extent */
> @@ -303,8 +307,13 @@ static errcode_t ext2fs_punch_extent(ext2_filsys fs, ext2_ino_t ino,
>  			extent.e_lblk += free_count;
>  			extent.e_pblk += free_count;
>  		} else if (end >= next-1) {
> +			/*
> +			 * Is the punch region beyond this extent?  This can
> +			 * happen if start is already inside a hole.  Try to
> +			 * advance to the next extent if this is the case.
> +			 */
>  			if (start >= next)
> -				break;
> +				goto next_extent;
>  			/* End of deleted region after extent;
>  			   adjust end of extent */
>  			dbg_printf("Case #%d\n", 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 4/6] libext2fs: during punch, fix parent extents after modifying extent

[Zheng Liu]

On Mon, Jan 20, 2014 at 10:22:04PM -0800, Darrick J. Wong wrote:
> When modifying/removing an extent during punch, don't forget to update
> the extent's parents.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>
                                                - Zheng

> ---
>  lib/ext2fs/punch.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> 
> diff --git a/lib/ext2fs/punch.c b/lib/ext2fs/punch.c
> index 657cb53..a3d020e 100644
> --- a/lib/ext2fs/punch.c
> +++ b/lib/ext2fs/punch.c
> @@ -353,6 +353,9 @@ static errcode_t ext2fs_punch_extent(ext2_filsys fs, ext2_ino_t ino,
>  		if (extent.e_len) {
>  			dbg_print_extent("replacing", &extent);
>  			retval = ext2fs_extent_replace(handle, 0, &extent);
> +			if (retval)
> +				goto errout;
> +			retval = ext2fs_extent_fix_parents(handle);
>  		} else {
>  			struct ext2fs_extent	newex;
>  			blk64_t			old_lblk, next_lblk;
> @@ -387,6 +390,11 @@ static errcode_t ext2fs_punch_extent(ext2_filsys fs, ext2_ino_t ino,
>  			if (retval)
>  				goto errout;
>  
> +			retval = ext2fs_extent_fix_parents(handle);
> +			if (retval && retval != EXT2_ET_NO_CURRENT_NODE)
> +				goto errout;
> +			retval = 0;
> +
>  			/* Jump forward to the next extent. */
>  			ext2fs_extent_goto(handle, next_lblk);
>  			op = EXT2_EXTENT_CURRENT;
> 
> --
> 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 5/6] libext2fs: don't hang on to unmapped block if extent tree update fails

[Zheng Liu]

On Mon, Jan 20, 2014 at 10:22:11PM -0800, Darrick J. Wong wrote:
> If we're doing a BMAP_ALLOC allocation and the extent tree update
> fails, there's no point in hanging on to the newly allocated block.
> So, free it to make fsck happy.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>
                                                - Zheng

> ---
>  lib/ext2fs/bmap.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/lib/ext2fs/bmap.c b/lib/ext2fs/bmap.c
> index b944c27..db2fd72 100644
> --- a/lib/ext2fs/bmap.c
> +++ b/lib/ext2fs/bmap.c
> @@ -255,8 +255,10 @@ got_block:
>  	set_extent:
>  		retval = ext2fs_extent_set_bmap(handle, block,
>  						blk64, 0);
> -		if (retval)
> +		if (retval) {
> +			ext2fs_block_alloc_stats2(fs, blk64, -1);
>  			return retval;
> +		}
>  		/* Update inode after setting extent */
>  		retval = ext2fs_read_inode(fs, ino, inode);
>  		if (retval)
> 
> --
> 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 6/6] libext2fs: try to roll back when splitting an extent fails

[Zheng Liu]

On Mon, Jan 20, 2014 at 10:22:17PM -0800, Darrick J. Wong wrote:
> If a client asks us to remap a block in the middle of an extent, we
> potentially have to allocate a fair number of blocks to handle extent
> tree splits.  A failure in either of the ext2fs_extent_insert calls
> leaves us with an extent tree that no longer maps the logical block in
> question and everything that came after it!  Therefore, try to roll
> back the extent tree changes before returning an error code.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>
                                                - Zheng

> ---
>  lib/ext2fs/extent.c |   25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
> index 2928695..87921e6 100644
> --- a/lib/ext2fs/extent.c
> +++ b/lib/ext2fs/extent.c
> @@ -1438,13 +1438,17 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
>  			goto done;
>  	} else {
>  		__u32	orig_length;
> +		blk64_t	orig_lblk;
> +		struct ext2fs_extent orig_extent;
> +		errcode_t r2;
>  
>  #ifdef DEBUG
>  		printf("(re/un)mapping in middle of extent\n");
>  #endif
>  		/* need to split this extent; later */
> -
> +		orig_lblk = extent.e_lblk;
>  		orig_length = extent.e_len;
> +		orig_extent = extent;
>  
>  		/* shorten pre-split extent */
>  		extent.e_len = (logical - extent.e_lblk);
> @@ -1456,8 +1460,13 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
>  			/* insert new extent after current */
>  			retval = ext2fs_extent_insert(handle,
>  					EXT2_EXTENT_INSERT_AFTER, &newextent);
> -			if (retval)
> +			if (retval) {
> +				r2 = ext2fs_extent_goto(handle, orig_lblk);
> +				if (r2 == 0)
> +					ext2fs_extent_replace(handle, 0,
> +							      &orig_extent);
>  				goto done;
> +			}
>  		}
>  		/* add post-split extent */
>  		extent.e_pblk += extent.e_len + 1;
> @@ -1465,8 +1474,18 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
>  		extent.e_len = orig_length - extent.e_len - 1;
>  		retval = ext2fs_extent_insert(handle,
>  				EXT2_EXTENT_INSERT_AFTER, &extent);
> -		if (retval)
> +		if (retval) {
> +			if (physical) {
> +				r2 = ext2fs_extent_goto(handle,
> +							newextent.e_lblk);
> +				if (r2 == 0)
> +					ext2fs_extent_delete(handle, 0);
> +			}
> +			r2 = ext2fs_extent_goto(handle, orig_lblk);
> +			if (r2 == 0)
> +				ext2fs_extent_replace(handle, 0, &orig_extent);
>  			goto done;
> +		}
>  	}
>  
>  done:
> 
> --
> 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 1/6] misc: fix resource leaks in e2fsprogs

[Darrick J. Wong]

On Tue, Jan 21, 2014 at 03:25:54PM +0800, Zheng Liu wrote:
> On Mon, Jan 20, 2014 at 10:21:45PM -0800, Darrick J. Wong wrote:
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> It seems that we will free 'fn' in ext2fs_free_icount().

Oh, so it is.  Ignore this patch, please.

--D
> 
>   void ext2fs_free_icount(ext2_icount_t icount)
>   {
>           if (!icount)
>                   return;
>   
>           icount->magic = 0;
>           if (icount->list)
>                   ext2fs_free_mem(&icount->list);
>           if (icount->single)
>                   ext2fs_free_inode_bitmap(icount->single);
>           if (icount->multiple)
>                   ext2fs_free_inode_bitmap(icount->multiple);
>           if (icount->tdb)
>                   tdb_close(icount->tdb);
>           if (icount->tdb_fn) {
>                   unlink(icount->tdb_fn);
>                   free(icount->tdb_fn);
>           }
>   
>           ext2fs_free_mem(&icount);
>   }
> 
> Regards,
>                                                 - Zheng
> 
> > ---
> >  lib/ext2fs/icount.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > 
> > diff --git a/lib/ext2fs/icount.c b/lib/ext2fs/icount.c
> > index a3b20f0..7d1b3d5 100644
> > --- a/lib/ext2fs/icount.c
> > +++ b/lib/ext2fs/icount.c
> > @@ -198,6 +198,7 @@ errcode_t ext2fs_create_icount_tdb(ext2_filsys fs, char *tdb_dir,
> >  	fd = mkstemp(fn);
> >  	if (fd < 0) {
> >  		retval = errno;
> > +		ext2fs_free_mem(&fn);
> >  		goto errout;
> >  	}
> >  	umask(save_umask);
> > @@ -216,6 +217,7 @@ errcode_t ext2fs_create_icount_tdb(ext2_filsys fs, char *tdb_dir,
> >  	close(fd);
> >  	if (icount->tdb == NULL) {
> >  		retval = errno;
> > +		ext2fs_free_mem(&fn);
> >  		goto errout;
> >  	}
> >  	*ret = icount;
> > 
> > --
> > 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/6] mke2fs: clean up kernel version tests

[Theodore Ts'o]

On Tue, Jan 21, 2014 at 03:32:40PM +0800, Zheng Liu wrote:
> On Mon, Jan 20, 2014 at 10:21:51PM -0800, Darrick J. Wong wrote:
> > Refactor the running kernel version checks to hide the details of
> > version code checking, etc.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>

Thanks, applied.

					- Ted

Re: [PATCH 3/6] libext2fs: iterate past lower extents during punch

[Theodore Ts'o]

On Tue, Jan 21, 2014 at 03:40:11PM +0800, Zheng Liu wrote:
> On Mon, Jan 20, 2014 at 10:21:58PM -0800, Darrick J. Wong wrote:
> > When we're iterating extents during a punch operation, the loop exits
> > if the punch region is entirely to the right of the extent we're
> > looking at.  This can happen if the punch region starts in the middle
> > of a hole and covers mapped extents.  When this happens, we want to
> > skip to the next extent, because it might be punchable.
> > 
> > Also, if we've totally passed the punch range, stop.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>

Thanks, applied.

					- Ted

Re: [PATCH 4/6] libext2fs: during punch, fix parent extents after modifying extent

[Theodore Ts'o]

On Tue, Jan 21, 2014 at 03:43:43PM +0800, Zheng Liu wrote:
> On Mon, Jan 20, 2014 at 10:22:04PM -0800, Darrick J. Wong wrote:
> > When modifying/removing an extent during punch, don't forget to update
> > the extent's parents.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>

Thanks, applied.

					- Ted

Re: [PATCH 6/6] libext2fs: try to roll back when splitting an extent fails

[Theodore Ts'o]

On Tue, Jan 21, 2014 at 03:49:46PM +0800, Zheng Liu wrote:
> On Mon, Jan 20, 2014 at 10:22:17PM -0800, Darrick J. Wong wrote:
> > If a client asks us to remap a block in the middle of an extent, we
> > potentially have to allocate a fair number of blocks to handle extent
> > tree splits.  A failure in either of the ext2fs_extent_insert calls
> > leaves us with an extent tree that no longer maps the logical block in
> > question and everything that came after it!  Therefore, try to roll
> > back the extent tree changes before returning an error code.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>

Thanks, applied.

						- Ted