| Message ID | 20131001012740.28415.55304.stgit@birch.djwong.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
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
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
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;
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