Liu Bo
2012-Aug-16 04:42 UTC
[PATCH 1/2] Btrfs: fix a bug in parsing return value in logical resolve
In logical resolve, we parse extent_from_logical()''s
''ret'' as a kind of flag.
It is possible to lose our errors because
(-EXXXX & BTRFS_EXTENT_FLAG_TREE_BLOCK) is true.
I''m not sure if it is on purpose, it just looks too hacky if it is.
I''d rather use a real flag and a ''ret'' to catch
errors.
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
fs/btrfs/backref.c | 24 ++++++++++++++++--------
fs/btrfs/backref.h | 3 ++-
fs/btrfs/ioctl.c | 6 ++++--
fs/btrfs/scrub.c | 14 ++++++++------
fs/btrfs/send.c | 7 ++++---
5 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index a256f3b..08d5385 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1193,7 +1193,8 @@ char *btrfs_iref_to_path(struct btrfs_root *fs_root,
struct btrfs_path *path,
* tree blocks and <0 on error.
*/
int extent_from_logical(struct btrfs_fs_info *fs_info, u64 logical,
- struct btrfs_path *path, struct btrfs_key *found_key)
+ struct btrfs_path *path, struct btrfs_key *found_key,
+ u64 *flags_ret)
{
int ret;
u64 flags;
@@ -1237,10 +1238,17 @@ int extent_from_logical(struct btrfs_fs_info *fs_info,
u64 logical,
(unsigned long long)found_key->objectid,
(unsigned long long)found_key->offset,
(unsigned long long)flags, item_size);
- if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK)
- return BTRFS_EXTENT_FLAG_TREE_BLOCK;
- if (flags & BTRFS_EXTENT_FLAG_DATA)
- return BTRFS_EXTENT_FLAG_DATA;
+
+ WARN_ON(!flags_ret);
+ if (flags_ret) {
+ if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK)
+ *flags_ret = BTRFS_EXTENT_FLAG_TREE_BLOCK;
+ else if (flags & BTRFS_EXTENT_FLAG_DATA)
+ *flags_ret = BTRFS_EXTENT_FLAG_DATA;
+ else
+ BUG_ON(1);
+ return 0;
+ }
return -EIO;
}
@@ -1432,13 +1440,13 @@ int iterate_inodes_from_logical(u64 logical, struct
btrfs_fs_info *fs_info,
{
int ret;
u64 extent_item_pos;
+ u64 flags = 0;
struct btrfs_key found_key;
int search_commit_root = path->search_commit_root;
- ret = extent_from_logical(fs_info, logical, path,
- &found_key);
+ ret = extent_from_logical(fs_info, logical, path, &found_key, &flags);
btrfs_release_path(path);
- if (ret & BTRFS_EXTENT_FLAG_TREE_BLOCK)
+ if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK)
ret = -EINVAL;
if (ret < 0)
return ret;
diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
index 032f4dc..4fda5d8 100644
--- a/fs/btrfs/backref.h
+++ b/fs/btrfs/backref.h
@@ -40,7 +40,8 @@ int inode_item_info(u64 inum, u64 ioff, struct btrfs_root
*fs_root,
struct btrfs_path *path);
int extent_from_logical(struct btrfs_fs_info *fs_info, u64 logical,
- struct btrfs_path *path, struct btrfs_key *found_key);
+ struct btrfs_path *path, struct btrfs_key *found_key,
+ u64 *flags);
int tree_backref_for_extent(unsigned long *ptr, struct extent_buffer *eb,
struct btrfs_extent_item *ei, u32 item_size,
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7bb7556..405b279 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3211,6 +3211,7 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_root
*root,
int ret = 0;
int size;
u64 extent_item_pos;
+ u64 flags = 0;
struct btrfs_ioctl_logical_ino_args *loi;
struct btrfs_data_container *inodes = NULL;
struct btrfs_path *path = NULL;
@@ -3240,10 +3241,11 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_root
*root,
goto out;
}
- ret = extent_from_logical(root->fs_info, loi->logical, path, &key);
+ ret = extent_from_logical(root->fs_info, loi->logical, path, &key,
+ &flags);
btrfs_release_path(path);
- if (ret & BTRFS_EXTENT_FLAG_TREE_BLOCK)
+ if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK)
ret = -ENOENT;
if (ret < 0)
goto out;
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index b223620..5def223 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -352,13 +352,14 @@ static void scrub_print_warning(const char *errstr, struct
scrub_block *sblock)
struct extent_buffer *eb;
struct btrfs_extent_item *ei;
struct scrub_warning swarn;
- u32 item_size;
- int ret;
+ unsigned long ptr = 0;
+ u64 extent_item_pos;
+ u64 flags = 0;
u64 ref_root;
+ u32 item_size;
u8 ref_level;
- unsigned long ptr = 0;
const int bufsize = 4096;
- u64 extent_item_pos;
+ int ret;
path = btrfs_alloc_path();
@@ -375,7 +376,8 @@ static void scrub_print_warning(const char *errstr, struct
scrub_block *sblock)
if (!path || !swarn.scratch_buf || !swarn.msg_buf)
goto out;
- ret = extent_from_logical(fs_info, swarn.logical, path, &found_key);
+ ret = extent_from_logical(fs_info, swarn.logical, path, &found_key,
+ &flags);
if (ret < 0)
goto out;
@@ -387,7 +389,7 @@ static void scrub_print_warning(const char *errstr, struct
scrub_block *sblock)
item_size = btrfs_item_size_nr(eb, path->slots[0]);
btrfs_release_path(path);
- if (ret & BTRFS_EXTENT_FLAG_TREE_BLOCK) {
+ if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK) {
do {
ret = tree_backref_for_extent(&ptr, eb, ei, item_size,
&ref_root, &ref_level);
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index fb5ffe9..7186ae8 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1143,6 +1143,7 @@ static int find_extent_clone(struct send_ctx *sctx,
u64 logical;
u64 num_bytes;
u64 extent_item_pos;
+ u64 flags = 0;
struct btrfs_file_extent_item *fi;
struct extent_buffer *eb = path->nodes[0];
struct backref_ctx backref_ctx;
@@ -1181,13 +1182,13 @@ static int find_extent_clone(struct send_ctx *sctx,
}
logical += btrfs_file_extent_offset(eb, fi);
- ret = extent_from_logical(sctx->send_root->fs_info,
- logical, tmp_path, &found_key);
+ ret = extent_from_logical(sctx->send_root->fs_info, logical, tmp_path,
+ &found_key, &flags);
btrfs_release_path(tmp_path);
if (ret < 0)
goto out;
- if (ret & BTRFS_EXTENT_FLAG_TREE_BLOCK) {
+ if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK) {
ret = -EIO;
goto out;
}
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
We already have a helper, iterate_inodes_from_logical(), for logical resolve,
so just use it.
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
fs/btrfs/ioctl.c | 20 +++-----------------
1 files changed, 3 insertions(+), 17 deletions(-)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 405b279..f1ff21a 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3210,12 +3210,9 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_root
*root,
{
int ret = 0;
int size;
- u64 extent_item_pos;
- u64 flags = 0;
struct btrfs_ioctl_logical_ino_args *loi;
struct btrfs_data_container *inodes = NULL;
struct btrfs_path *path = NULL;
- struct btrfs_key key;
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
@@ -3241,20 +3238,9 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_root
*root,
goto out;
}
- ret = extent_from_logical(root->fs_info, loi->logical, path, &key,
- &flags);
- btrfs_release_path(path);
-
- if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK)
- ret = -ENOENT;
- if (ret < 0)
- goto out;
-
- extent_item_pos = loi->logical - key.objectid;
- ret = iterate_extent_inodes(root->fs_info, key.objectid,
- extent_item_pos, 0, build_ino_list,
- inodes);
-
+ path->search_commit_root = 0;
+ ret = iterate_inodes_from_logical(loi->logical, root->fs_info, path,
+ build_ino_list, inodes);
if (ret < 0)
goto out;
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Jan Schmidt
2012-Aug-16 05:08 UTC
Re: [PATCH 1/2] Btrfs: fix a bug in parsing return value in logical resolve
On Thu, August 16, 2012 at 06:42 (+0200), Liu Bo wrote:> In logical resolve, we parse extent_from_logical()''s ''ret'' as a kind of flag. > > It is possible to lose our errors because > (-EXXXX & BTRFS_EXTENT_FLAG_TREE_BLOCK) is true. > > I''m not sure if it is on purpose, it just looks too hacky if it is. > I''d rather use a real flag and a ''ret'' to catch errors. > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > fs/btrfs/backref.c | 24 ++++++++++++++++-------- > fs/btrfs/backref.h | 3 ++- > fs/btrfs/ioctl.c | 6 ++++-- > fs/btrfs/scrub.c | 14 ++++++++------ > fs/btrfs/send.c | 7 ++++--- > 5 files changed, 34 insertions(+), 20 deletions(-) > > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c > index a256f3b..08d5385 100644 > --- a/fs/btrfs/backref.c > +++ b/fs/btrfs/backref.c > @@ -1193,7 +1193,8 @@ char *btrfs_iref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path, > * tree blocks and <0 on error. > */ > int extent_from_logical(struct btrfs_fs_info *fs_info, u64 logical, > - struct btrfs_path *path, struct btrfs_key *found_key) > + struct btrfs_path *path, struct btrfs_key *found_key, > + u64 *flags_ret) > { > int ret; > u64 flags; > @@ -1237,10 +1238,17 @@ int extent_from_logical(struct btrfs_fs_info *fs_info, u64 logical, > (unsigned long long)found_key->objectid, > (unsigned long long)found_key->offset, > (unsigned long long)flags, item_size); > - if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK) > - return BTRFS_EXTENT_FLAG_TREE_BLOCK; > - if (flags & BTRFS_EXTENT_FLAG_DATA) > - return BTRFS_EXTENT_FLAG_DATA; > + > + WARN_ON(!flags_ret); > + if (flags_ret) { > + if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK) > + *flags_ret = BTRFS_EXTENT_FLAG_TREE_BLOCK; > + else if (flags & BTRFS_EXTENT_FLAG_DATA) > + *flags_ret = BTRFS_EXTENT_FLAG_DATA; > + else > + BUG_ON(1); > + return 0; > + } > > return -EIO; > } > @@ -1432,13 +1440,13 @@ int iterate_inodes_from_logical(u64 logical, struct btrfs_fs_info *fs_info, > { > int ret; > u64 extent_item_pos; > + u64 flags = 0; > struct btrfs_key found_key; > int search_commit_root = path->search_commit_root; > > - ret = extent_from_logical(fs_info, logical, path, > - &found_key); > + ret = extent_from_logical(fs_info, logical, path, &found_key, &flags); > btrfs_release_path(path); > - if (ret & BTRFS_EXTENT_FLAG_TREE_BLOCK) > + if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK) > ret = -EINVAL; > if (ret < 0) > return ret; > diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h > index 032f4dc..4fda5d8 100644 > --- a/fs/btrfs/backref.h > +++ b/fs/btrfs/backref.h > @@ -40,7 +40,8 @@ int inode_item_info(u64 inum, u64 ioff, struct btrfs_root *fs_root, > struct btrfs_path *path); > > int extent_from_logical(struct btrfs_fs_info *fs_info, u64 logical, > - struct btrfs_path *path, struct btrfs_key *found_key); > + struct btrfs_path *path, struct btrfs_key *found_key, > + u64 *flags); > > int tree_backref_for_extent(unsigned long *ptr, struct extent_buffer *eb, > struct btrfs_extent_item *ei, u32 item_size, > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 7bb7556..405b279 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -3211,6 +3211,7 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_root *root, > int ret = 0; > int size; > u64 extent_item_pos; > + u64 flags = 0; > struct btrfs_ioctl_logical_ino_args *loi; > struct btrfs_data_container *inodes = NULL; > struct btrfs_path *path = NULL; > @@ -3240,10 +3241,11 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_root *root, > goto out; > } > > - ret = extent_from_logical(root->fs_info, loi->logical, path, &key); > + ret = extent_from_logical(root->fs_info, loi->logical, path, &key, > + &flags); > btrfs_release_path(path); > > - if (ret & BTRFS_EXTENT_FLAG_TREE_BLOCK) > + if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK) > ret = -ENOENT; > if (ret < 0) > goto out; > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > index b223620..5def223 100644 > --- a/fs/btrfs/scrub.c > +++ b/fs/btrfs/scrub.c > @@ -352,13 +352,14 @@ static void scrub_print_warning(const char *errstr, struct scrub_block *sblock) > struct extent_buffer *eb; > struct btrfs_extent_item *ei; > struct scrub_warning swarn; > - u32 item_size; > - int ret; > + unsigned long ptr = 0; > + u64 extent_item_pos; > + u64 flags = 0; > u64 ref_root; > + u32 item_size; > u8 ref_level; > - unsigned long ptr = 0; > const int bufsize = 4096; > - u64 extent_item_pos; > + int ret; > > path = btrfs_alloc_path(); > > @@ -375,7 +376,8 @@ static void scrub_print_warning(const char *errstr, struct scrub_block *sblock) > if (!path || !swarn.scratch_buf || !swarn.msg_buf) > goto out; > > - ret = extent_from_logical(fs_info, swarn.logical, path, &found_key); > + ret = extent_from_logical(fs_info, swarn.logical, path, &found_key, > + &flags); > if (ret < 0) > goto out; > > @@ -387,7 +389,7 @@ static void scrub_print_warning(const char *errstr, struct scrub_block *sblock) > item_size = btrfs_item_size_nr(eb, path->slots[0]); > btrfs_release_path(path); > > - if (ret & BTRFS_EXTENT_FLAG_TREE_BLOCK) { > + if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK) { > do { > ret = tree_backref_for_extent(&ptr, eb, ei, item_size, > &ref_root, &ref_level); > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > index fb5ffe9..7186ae8 100644 > --- a/fs/btrfs/send.c > +++ b/fs/btrfs/send.c > @@ -1143,6 +1143,7 @@ static int find_extent_clone(struct send_ctx *sctx, > u64 logical; > u64 num_bytes; > u64 extent_item_pos; > + u64 flags = 0; > struct btrfs_file_extent_item *fi; > struct extent_buffer *eb = path->nodes[0]; > struct backref_ctx backref_ctx; > @@ -1181,13 +1182,13 @@ static int find_extent_clone(struct send_ctx *sctx, > } > logical += btrfs_file_extent_offset(eb, fi); > > - ret = extent_from_logical(sctx->send_root->fs_info, > - logical, tmp_path, &found_key); > + ret = extent_from_logical(sctx->send_root->fs_info, logical, tmp_path, > + &found_key, &flags); > btrfs_release_path(tmp_path); > > if (ret < 0) > goto out; > - if (ret & BTRFS_EXTENT_FLAG_TREE_BLOCK) { > + if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK) { > ret = -EIO; > goto out; > }Acked-by: Jan Schmidt <list.btrfs@jan-o-sch.net> Not for the next rc but for 3.7, IMHO. Thanks, -Jan -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, August 16, 2012 at 06:42 (+0200), Liu Bo wrote:> We already have a helper, iterate_inodes_from_logical(), for logical resolve, > so just use it. > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > fs/btrfs/ioctl.c | 20 +++----------------- > 1 files changed, 3 insertions(+), 17 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 405b279..f1ff21a 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -3210,12 +3210,9 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_root *root, > { > int ret = 0; > int size; > - u64 extent_item_pos; > - u64 flags = 0; > struct btrfs_ioctl_logical_ino_args *loi; > struct btrfs_data_container *inodes = NULL; > struct btrfs_path *path = NULL; > - struct btrfs_key key; > > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > @@ -3241,20 +3238,9 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_root *root, > goto out; > } > > - ret = extent_from_logical(root->fs_info, loi->logical, path, &key, > - &flags); > - btrfs_release_path(path); > - > - if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK) > - ret = -ENOENT; > - if (ret < 0) > - goto out; > - > - extent_item_pos = loi->logical - key.objectid; > - ret = iterate_extent_inodes(root->fs_info, key.objectid, > - extent_item_pos, 0, build_ino_list, > - inodes); > - > + path->search_commit_root = 0;I came across that search_commit_root as well when re-reading iterate_inodes_from_logical. Wouldn''t explicitly set the default value here, though. We don''t do it before each and every call to btrfs_search_slot, either.> + ret = iterate_inodes_from_logical(loi->logical, root->fs_info, path, > + build_ino_list, inodes);To maintain the current behavior, we should patch -EINVAL to -ENOENT here.> if (ret < 0) > goto out; >Thanks, -Jan -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/16/2012 01:10 PM, Jan Schmidt wrote:> On Thu, August 16, 2012 at 06:42 (+0200), Liu Bo wrote: >> We already have a helper, iterate_inodes_from_logical(), for logical resolve, >> so just use it. >> >> Signed-off-by: Liu Bo <bo.li.liu@oracle.com> >> --- >> fs/btrfs/ioctl.c | 20 +++----------------- >> 1 files changed, 3 insertions(+), 17 deletions(-) >> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >> index 405b279..f1ff21a 100644 >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c[...]>> - extent_item_pos = loi->logical - key.objectid; >> - ret = iterate_extent_inodes(root->fs_info, key.objectid, >> - extent_item_pos, 0, build_ino_list, >> - inodes); >> - >> + path->search_commit_root = 0; > > I came across that search_commit_root as well when re-reading > iterate_inodes_from_logical. Wouldn''t explicitly set the default value here, > though. We don''t do it before each and every call to btrfs_search_slot, either. >ok, will remove the redundant default set.>> + ret = iterate_inodes_from_logical(loi->logical, root->fs_info, path, >> + build_ino_list, inodes); > > To maintain the current behavior, we should patch -EINVAL to -ENOENT here. >oops, I should have noticed it. Thanks for the comments, Jan :) thanks, liubo -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html