2008-04-11 18:16:23

by Eric Sandeen

Subject: [PATCH 4/5] make extent_goto() deterministic when logical block not found

Make sure that extent_goto() leaves us at the last extent
prior to the requested logical block, if the logical block
requested lands in a hole.

Signed-off-by: Eric Sandeen <[email protected]>
---
lib/ext2fs/extent.c | 13 ++++++++++++-
1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index 9f19e7b..e7b7e85 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -553,6 +553,12 @@ errcode_t ext2fs_extent_free_path(ext2_extent_path_t path)
}
#endif

+/*
+ * Go to the node at leaf_level which contains logical block blk.
+ *
+ * If "blk" has no mapping (hole) then handle is left at last
+ * extent before blk.
+ */
static errcode_t extent_goto(ext2_extent_handle_t handle,
int leaf_level, blk64_t blk)
{
@@ -566,11 +572,16 @@ static errcode_t extent_goto(ext2_extent_handle_t handle,
dbg_print_extent("root", &extent);
while (1) {
if (handle->level - leaf_level == handle->max_depth) {
+ /* block is in this &extent */
if ((blk >= extent.e_lblk) &&
(blk < extent.e_lblk + extent.e_len))
return 0;
- if (blk < extent.e_lblk)
+ if (blk < extent.e_lblk) {
+ retval = ext2fs_extent_get(handle,
+ EXT2_EXTENT_PREV_SIB,
+ &extent);
return EXT2_ET_EXTENT_NOT_FOUND;
+ }
retval = ext2fs_extent_get(handle,
EXT2_EXTENT_NEXT_SIB,
&extent);
--
1.5.4.1

2008-04-17 22:15:52

by Theodore Ts'o

Subject: Re: [PATCH 4/5] make extent_goto() deterministic when logical block not found

On Mon, Apr 07, 2008 at 10:00:59PM -0500, Eric Sandeen wrote:
> Make sure that extent_goto() leaves us at the last extent
> prior to the requested logical block, if the logical block
> requested lands in a hole.

Thanks, applied. You do realize this won't necessarily do the right
thing if the sparse file doesn't have a block mapped at logical block
#0, and you then do an extent_goto() block 0, right? Also, if
extent_get(EXT2_EXTENT_PREV_SIB) fails for various reasons, it still
could be non-deterministic. Code that depends on this may want to do
some sanity checking.

Also, it might be prudent to check the return value here:

> + retval = ext2fs_extent_get(handle,
> + EXT2_EXTENT_PREV_SIB,
> + &extent);
> return EXT2_ET_EXTENT_NOT_FOUND;

and return either the error returned by ext2fs_extent_get, or possibly
invent a new error code, say EXT2_ET_EXTENT_GOTO_BACKUP_FAILED and
return that if retval is non-zero.

I've left this as-is for now but there might be a potential robustness
problem hiding here.

- Ted