btrfs: clear 'ret' in btrfs_check_shared() loop
diff mbox

Message ID 20150421214733.GO17170@wotan.suse.de
State Superseded
Headers show

Commit Message

Mark Fasheh April 21, 2015, 9:47 p.m. UTC
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.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/backref.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Chris Mason April 22, 2015, 12:49 a.m. UTC | #1
On 04/21/2015 05:47 PM, Mark Fasheh wrote:
> 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.
> 

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

-chris

--
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
Mark Fasheh April 22, 2015, 1:49 a.m. UTC | #2
On Tue, Apr 21, 2015 at 08:49:08PM -0400, Chris Mason wrote:
> On 04/21/2015 05:47 PM, Mark Fasheh wrote:
> > 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.
> > 
> 
> 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

> 
> -chris
> 
> --
> 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
--
Mark Fasheh
--
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

Patch
diff mbox

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index f55721f..8d47380 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -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;