btrfs: clear 'ret' in btrfs_check_shared() loop
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
@@ -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
Quoted 5 lines · Mark Fasheh
btrfs_check_shared()is leaking a return value of '1' fromfind_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()andfind_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
Quoted 9 lines · Chris Mason
Quoted 8 lines · Mark Fasheh
btrfs_check_shared()is leaking a return value of '1' fromfind_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()andfind_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