Skip to contentSkip to conversation

btrfs: clear 'ret' in btrfs_check_shared() loop

PATCH Superseded Mark Fasheh 2 participants 3 messages Apr 21, 2015 – Apr 22, 2015 Btrfs file system

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 btrfs: clear 'ret' in btrfs_check_shared() loop

btrfs_check_shared() is leaking a return value of '1' from find_parent_nodes(). As a result, callers (in this case, extent_fiemap()) are told extents are shared when they are not. This in turn broke fiemap on btrfs for kernels v3.18 and up.

The fix is simple - we just have to clear 'ret' after we are done processing the results of find_parent_nodes().

It wasn't clear to me at first what was happening with return values in btrfs_check_shared() and find_parent_nodes() - thanks to Josef for the help on irc. I added documentation to both functions to make things more clear for the next hacker who might come across them.

---
 fs/btrfs/backref.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
fs/btrfs/backref.c +17 -0
index f55721f..8d47380 100644
@@ -880,6 +880,8 @@ static int __add_keyed_refs(struct btrfs_fs_info *fs_info,
  * indirect refs to their parent bytenr.
  * When roots are found, they're added to the roots list
  *
+ * NOTE: This can return values > 0
+ *
  * FIXME some caching might speed things up
  */
 static int find_parent_nodes(struct btrfs_trans_handle *trans,
@@ -1198,6 +1200,19 @@ int btrfs_find_all_roots(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
+/**
+ * btrfs_check_shared - tell us whether an extent is shared
+ *
+ * @trans: optional trans handle
+ *
+ * btrfs_check_shared uses the backref walking code but will short
+ * circuit as soon as it finds a root or inode that doesn't match the
+ * one passed in. This provides a significant performance benefit for
+ * callers (such as fiemap) which want to know whether the extent is
+ * shared but do not need a ref count.
+ *
+ * Return: 0 if extent is not shared, 1 if it is shared, < 0 on error.
+ */
 int btrfs_check_shared(struct btrfs_trans_handle *trans,
 		       struct btrfs_fs_info *fs_info, u64 root_objectid,
 		       u64 inum, u64 bytenr)
@@ -1226,11 +1241,13 @@ int btrfs_check_shared(struct btrfs_trans_handle *trans,
 		ret = find_parent_nodes(trans, fs_info, bytenr, elem.seq, tmp,
 					roots, NULL, root_objectid, inum);
 		if (ret == BACKREF_FOUND_SHARED) {
+			/* this is the only condition under which we return 1 */
 			ret = 1;
 			break;
 		}
 		if (ret < 0 && ret != -ENOENT)
 			break;
+		ret = 0;
 		node = ulist_next(tmp, &uiter);
 		if (!node)
 			break;
Signature · 1 line
2.1.2
#1 lore raw
Quoted 5 lines · Mark Fasheh

btrfs_check_shared() is leaking a return value of '1' from find_parent_nodes(). As a result, callers (in this case, extent_fiemap()) are told extents are shared when they are not. This in turn broke fiemap on btrfs for kernels v3.18 and up.

Quoted 8 lines

The fix is simple - we just have to clear 'ret' after we are done processing the results of find_parent_nodes().

It wasn't clear to me at first what was happening with return values in btrfs_check_shared() and find_parent_nodes() - thanks to Josef for the help on irc. I added documentation to both functions to make things more clear for the next hacker who might come across them.

Thanks Mark, any reason not to tag this for stable?

-chris

#2 lore raw
Quoted 9 lines · Chris Mason
Quoted 8 lines · Mark Fasheh

btrfs_check_shared() is leaking a return value of '1' from find_parent_nodes(). As a result, callers (in this case, extent_fiemap()) are told extents are shared when they are not. This in turn broke fiemap on btrfs for kernels v3.18 and up.

The fix is simple - we just have to clear 'ret' after we are done processing the results of find_parent_nodes().

Quoted 7 lines
Quoted 5 lines

It wasn't clear to me at first what was happening with return values in btrfs_check_shared() and find_parent_nodes() - thanks to Josef for the help on irc. I added documentation to both functions to make things more clear for the next hacker who might come across them.

Thanks Mark, any reason not to tag this for stable?

If you all are ok with the patch then yes please send it to -stable too. It breaks duperemove on kernels from 3.18 onward:

https://github.com/markfasheh/duperemove/issues/55

so I'm understandbly interested in seeing it backported :)

Thanks,

	--Mark
Quoted 7 lines

-chris

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Mark Fasheh