From 052124090e67753775ebebb325285d2e13ed04d2 Mon Sep 17 00:00:00 2001 From: Andrea Biardi Date: Fri, 23 Jun 2023 12:28:39 +0100 Subject: [PATCH] Fixes issue #146 This patch modifies the logic of ext2fs_extent_set_bmap() so that: - Handling of "only block in extent" special case is removed - This affects files during a shrink with resize2fs; for example, when a file has an extent of 100 blocks and all 100 are relocated, the current (unpatched) code does this: - (ref.: inode_scan_and_fix() -> ext2fs_block_iterate3() -> process_block() which remaps each block and returns BLOCK_CHANGED to ext2fs_block_iterate3(), which then calls ext2fs_extent_set_bmap() to update the extent tree). - A new extent is inserted for the same lblk (because the pblk has moved), and the original extent is pushed back/shrunk by 1 block (start pblk is incremented by 1, e_len reduced by 1). - All other blocks in the extent (except the last one) are appended to the newly-inserted extent, and the original extent is pushed back/shrunk accordingly (one block at a time). - When it's time to process the last remaining block, the code notices an extent of e_len = 1 (the last one remaining with the old pblk) and "optimizes" this by rewriting it in place (instead of appending it to the new extent). - The end result is that *every* file that has an extent with e_len > 1 is *always* split into two extents (the second one consisting of a single block). - I think the intention here was to make the code more efficient by making the update in place, but the way resize2fs calls this function practically causes the number of extents to double up during a shrink; in my testcase a large file with ~8300 extents before the resize ends up with ~8200 after resize (compared with ~15k extents with the unpatched code). - More preference is given to growing the previous extent's end, by swapping two if blocks - This helps compacting the extents during resize2fs (swapping the blocks is necessary because of the removal above). - When an existing extent is shrunk and its e_len becomes zero, it is deleted. - I am not clear if an extent with length == 0 is valid; judging from https://www.mail-archive.com/ptxdist@pengutronix.de/msg20364.html, probably not. - Without this change, "extent.e_len += (extent.e_lblk - start)" in ext2fs_extent_fix_parents() may overflow and throw the end of the extent way outside the size of the filesystem. Check with the e2 image referenced in this bug. In summary, this patch fixes https://github.com/tytso/e2fsprogs/issues/146 and optimizes the allocation of extents after a shrinking resize2fs. Signed-off-by: Andrea Biardi --- lib/ext2fs/extent.c | 94 ++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 53 deletions(-) diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c index 82e75ccd7..983954b33 100644 --- a/lib/ext2fs/extent.c +++ b/lib/ext2fs/extent.c @@ -1439,21 +1439,49 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle, retval = ext2fs_extent_fix_parents(handle); if (retval) goto done; - } else if ((logical == extent.e_lblk) && (extent.e_len == 1)) { + } else if (logical == extent.e_lblk) { #ifdef DEBUG - printf("(re/un)mapping only block in extent\n"); + printf("(re/un)mapping first block in extent\n"); #endif if (physical) { - retval = ext2fs_extent_replace(handle, 0, &newextent); - } else { - retval = ext2fs_extent_delete(handle, 0); + if (has_prev && + (logical == (prev_extent.e_lblk + + prev_extent.e_len)) && + (physical == (prev_extent.e_pblk + + prev_extent.e_len)) && + (new_uninit == prev_uninit) && + ((int) prev_extent.e_len < max_len-1)) { + retval = ext2fs_extent_get(handle, + EXT2_EXTENT_PREV_LEAF, &prev_extent); + if (retval) + goto done; + prev_extent.e_len++; + retval = ext2fs_extent_replace(handle, 0, + &prev_extent); + } else + retval = ext2fs_extent_insert(handle, + 0, &newextent); + if (retval) + goto done; + retval = ext2fs_extent_fix_parents(handle); + if (retval) + goto done; + retval = ext2fs_extent_get(handle, + EXT2_EXTENT_NEXT_LEAF, + &extent); if (retval) goto done; - ec = ext2fs_extent_fix_parents(handle); - if (ec != EXT2_ET_NO_CURRENT_NODE) - retval = ec; } - + extent.e_pblk++; + extent.e_lblk++; + extent.e_len--; + if (extent.e_len) + retval = ext2fs_extent_replace(handle, 0, &extent); + else + retval = ext2fs_extent_delete(handle, 0); + if (retval) + goto done; + retval = ext2fs_extent_fix_parents(handle); if (retval) goto done; } else if (logical == extent.e_lblk + extent.e_len - 1) { @@ -1500,50 +1528,10 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle, if (retval) goto done; } - extent.e_len--; - retval = ext2fs_extent_replace(handle, 0, &extent); - if (retval) - goto done; - } else if (logical == extent.e_lblk) { -#ifdef DEBUG - printf("(re/un)mapping first block in extent\n"); -#endif - if (physical) { - if (has_prev && - (logical == (prev_extent.e_lblk + - prev_extent.e_len)) && - (physical == (prev_extent.e_pblk + - prev_extent.e_len)) && - (new_uninit == prev_uninit) && - ((int) prev_extent.e_len < max_len-1)) { - retval = ext2fs_extent_get(handle, - EXT2_EXTENT_PREV_LEAF, &prev_extent); - if (retval) - goto done; - prev_extent.e_len++; - retval = ext2fs_extent_replace(handle, 0, - &prev_extent); - } else - retval = ext2fs_extent_insert(handle, - 0, &newextent); - if (retval) - goto done; - retval = ext2fs_extent_fix_parents(handle); - if (retval) - goto done; - retval = ext2fs_extent_get(handle, - EXT2_EXTENT_NEXT_LEAF, - &extent); - if (retval) - goto done; - } - extent.e_pblk++; - extent.e_lblk++; - extent.e_len--; - retval = ext2fs_extent_replace(handle, 0, &extent); - if (retval) - goto done; - retval = ext2fs_extent_fix_parents(handle); + if (--extent.e_len) + retval = ext2fs_extent_replace(handle, 0, &extent); + else + retval = ext2fs_extent_delete(handle, 0); if (retval) goto done; } else {