[PATCH 1/2] Btrfs: fix a bug in parsing return value in logical resolve

[PATCH 1/2] Btrfs: fix a bug in parsing return value in logical resolve

[Liu Bo]

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

[PATCH 2/2] Btrfs: use helper for logical resolve

[Liu Bo]

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

Re: [PATCH 1/2] Btrfs: fix a bug in parsing return value in logical resolve

[Jan Schmidt]

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

Re: [PATCH 2/2] Btrfs: use helper for logical resolve

[Jan Schmidt]

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

Re: [PATCH 2/2] Btrfs: use helper for logical resolve

[Liu Bo]

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