2014-01-21 06:21:43

by Darrick J. Wong

[permalink] [raw]
Subject: [PATCH 0/6] miscellaneous e2fsprogs fixes

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


2014-01-21 06:21:50

by Darrick J. Wong

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

Signed-off-by: Darrick J. Wong <[email protected]>
---
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;


2014-01-21 06:21:55

by Darrick J. Wong

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

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

Signed-off-by: Darrick J. Wong <[email protected]>
---
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)


2014-01-21 06:22:02

by Darrick J. Wong

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

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 <[email protected]>
---
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);


2014-01-21 06:22:09

by Darrick J. Wong

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

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

Signed-off-by: Darrick J. Wong <[email protected]>
---
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;


2014-01-21 06:22:16

by Darrick J. Wong

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

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 <[email protected]>
---
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)


2014-01-21 06:22:23

by Darrick J. Wong

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

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 <[email protected]>
---
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:


2014-01-21 07:21:31

by Zheng Liu

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

On Mon, Jan 20, 2014 at 10:21:45PM -0800, Darrick J. Wong wrote:
> Signed-off-by: Darrick J. Wong <[email protected]>

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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-01-21 07:28:18

by Zheng Liu

[permalink] [raw]
Subject: Re: [PATCH 2/6] mke2fs: clean up kernel version tests

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 <[email protected]>

Reviewed-by: Zheng Liu <[email protected]>
- 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-01-21 07:35:48

by Zheng Liu

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

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 <[email protected]>

Reviewed-by: Zheng Liu <[email protected]>
- 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-01-21 07:39:20

by Zheng Liu

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

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 <[email protected]>

Reviewed-by: Zheng Liu <[email protected]>
- 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-01-21 07:41:10

by Zheng Liu

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

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 <[email protected]>

Reviewed-by: Zheng Liu <[email protected]>
- 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-01-21 07:45:27

by Zheng Liu

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

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 <[email protected]>

Reviewed-by: Zheng Liu <[email protected]>
- 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-01-21 18:18:24

by Darrick J. Wong

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

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 <[email protected]>
>
> 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 [email protected]
> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-02-06 20:28:31

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 2/6] mke2fs: clean up kernel version tests

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 <[email protected]>
>
> Reviewed-by: Zheng Liu <[email protected]>

Thanks, applied.

- Ted

2014-02-06 20:30:00

by Theodore Ts'o

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

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 <[email protected]>
>
> Reviewed-by: Zheng Liu <[email protected]>

Thanks, applied.

- Ted

2014-02-06 20:31:42

by Theodore Ts'o

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

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 <[email protected]>
>
> Reviewed-by: Zheng Liu <[email protected]>

Thanks, applied.

- Ted

2014-02-06 20:34:31

by Theodore Ts'o

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

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 <[email protected]>
>
> Reviewed-by: Zheng Liu <[email protected]>

Thanks, applied.

- Ted