Patchwork [09/31] libext2fs: Rewind extent pointer when totally deleting an extent

login
register
mail settings
Submitter Darrick J. Wong
Date Oct. 1, 2013, 1:27 a.m.
Message ID <20131001012740.28415.55304.stgit@birch.djwong.org>
Download mbox | patch
Permalink /patch/279288/
State Accepted
Headers show

Comments

Darrick J. Wong - Oct. 1, 2013, 1:27 a.m.
During a punch operation, if we decide to delete an extent out of the extent
tree, the subsequent extents are moved on top of the current extent (that is to
say, they're memmmove'd down one slot).  Therefore it is not correct to advance
to the next leaf because that means we miss half the extents in the range!
Rereading the current pointer should be fine.

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



--
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
Theodore Ts'o - Oct. 7, 2013, 1:37 p.m.
On Mon, Sep 30, 2013 at 06:27:40PM -0700, Darrick J. Wong wrote:
> During a punch operation, if we decide to delete an extent out of the extent
> tree, the subsequent extents are moved on top of the current extent (that is to
> say, they're memmmove'd down one slot).  Therefore it is not correct to advance
> to the next leaf because that means we miss half the extents in the range!
> Rereading the current pointer should be fine.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks, applied.

BTW, in the future, this is the sort of change where creating a
regression test would be highly appreciated --- especially since you
presumably had to create test cases while you were creating the patch,
so it's much less effort to encapsulate it into a test case while
developing the patch.

					- Ted
--
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
Darrick J. Wong - Oct. 7, 2013, 6:24 p.m.
On Mon, Oct 07, 2013 at 09:37:25AM -0400, Theodore Ts'o wrote:
> On Mon, Sep 30, 2013 at 06:27:40PM -0700, Darrick J. Wong wrote:
> > During a punch operation, if we decide to delete an extent out of the extent
> > tree, the subsequent extents are moved on top of the current extent (that is to
> > say, they're memmmove'd down one slot).  Therefore it is not correct to advance
> > to the next leaf because that means we miss half the extents in the range!
> > Rereading the current pointer should be fine.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Thanks, applied.
> 
> BTW, in the future, this is the sort of change where creating a
> regression test would be highly appreciated --- especially since you
> presumably had to create test cases while you were creating the patch,
> so it's much less effort to encapsulate it into a test case while
> developing the patch.

I do have one, but it's hung up (with a bunch of other tests) in that icsum.sh
script.  I'm working on turning that into make check tests, but there's nearly
70 of them.

(Alternately, fire up fuse2fs and try to truncate a fragmented extent file.)

--D
> 
> 					- Ted
> --
> 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

diff --git a/lib/ext2fs/punch.c b/lib/ext2fs/punch.c
index b53653a..11c7668 100644
--- a/lib/ext2fs/punch.c
+++ b/lib/ext2fs/punch.c
@@ -186,6 +186,7 @@  static errcode_t ext2fs_punch_extent(ext2_filsys fs, ext2_ino_t ino,
 	blk64_t			free_start, next;
 	__u32			free_count, newlen;
 	int			freed = 0;
+	int			op;
 
 	retval = ext2fs_extent_open2(fs, ino, inode, &handle);
 	if (retval)
@@ -195,6 +196,7 @@  static errcode_t ext2fs_punch_extent(ext2_filsys fs, ext2_ino_t ino,
 	if (retval)
 		goto errout;
 	while (1) {
+		op = EXT2_EXTENT_NEXT_LEAF;
 		dbg_print_extent("main loop", &extent);
 		next = extent.e_lblk + extent.e_len;
 		dbg_printf("start %llu, end %llu, next %llu\n",
@@ -256,8 +258,23 @@  static errcode_t ext2fs_punch_extent(ext2_filsys fs, ext2_ino_t ino,
 			dbg_print_extent("replacing", &extent);
 			retval = ext2fs_extent_replace(handle, 0, &extent);
 		} else {
+			struct ext2fs_extent	newex;
 			dbg_printf("deleting current extent%s\n", "");
 			retval = ext2fs_extent_delete(handle, 0);
+			if (retval)
+				goto errout;
+			/*
+			 * We just moved the next extent into the current
+			 * extent's position, so re-read the extent next time.
+			 */
+			retval = ext2fs_extent_get(handle,
+						   EXT2_EXTENT_PREV_LEAF,
+						   &newex);
+			/* Can't go back? Just reread current. */
+			if (retval == EXT2_ET_EXTENT_NO_PREV) {
+				retval = 0;
+				op = EXT2_EXTENT_CURRENT;
+			}
 		}
 		if (retval)
 			goto errout;
@@ -268,9 +285,10 @@  static errcode_t ext2fs_punch_extent(ext2_filsys fs, ext2_ino_t ino,
 			freed++;
 		}
 	next_extent:
-		retval = ext2fs_extent_get(handle, EXT2_EXTENT_NEXT_LEAF,
+		retval = ext2fs_extent_get(handle, op,
 					   &extent);
-		if (retval == EXT2_ET_EXTENT_NO_NEXT)
+		if (retval == EXT2_ET_EXTENT_NO_NEXT ||
+		    retval == EXT2_ET_NO_CURRENT_NODE)
 			break;
 		if (retval)
 			goto errout;