| Message ID | 20150421214733.GO17170@wotan.suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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;
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(+)