Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue of incorrect qgroup detection in #29835 #30497

Closed
wants to merge 1 commit into from

Conversation

yongtang
Copy link
Member

@yongtang yongtang commented Jan 27, 2017

In #29835 BTRFS_IOC_INO_LOOKUP was used for qgroup detection.
However, this is not necessarily true because it only indicates the existance of tree id in btrfs.

Overall, the way to test the qgroup is to use btrfs qgroup show <path> and see if it returns something. The following code offers insight in checking qgroup:

https://github.com/kdave/btrfs-progs/blob/v4.9/qgroup.c#L1035

This fix tries to fix the issue by using BTRFS_IOC_TREE_SEARCH with BTRFS_QGROUP_STATUS_KEY instead.

In case qgroup is enabled, the search for BTRFS_QGROUP_STATUS_KEY should be valid.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

/cc @cpuguy83 @tonistiigi @zhuguihua
Please take a look and sorry for the trouble and inconvenience the #29835 caused.

In 29835 `BTRFS_IOC_INO_LOOKUP` was used for qgroup detection.
However, this is not necessarily true because it only indicate
the existance of tree id.

This fix tries to fix the issue by using `BTRFS_IOC_TREE_SEARCH`
with `BTRFS_QGROUP_STATUS_KEY` instead. In case qgroup is enabled,
the search for `BTRFS_QGROUP_STATUS_KEY` should return valid.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Copy link
Member Author

Also, I am not familiar with Jenkins test setup for docker but it might make sense to add integration tests for storage drivers like btrfs. Not sure if it is possible though.

@vdemeester vdemeester added the status/failing-ci Indicates that the PR in its current state fails the test suite label Feb 7, 2017
@yongtang
Copy link
Member Author

This PR could be superseded by #29427.

@vdemeester
Copy link
Member

@yongtang should we close this one ? 👼

@yongtang
Copy link
Member Author

Thanks @vdemeester. I will close this PR for now. Ideally we want to make sure #29427 is merged before the release of 1.14.0, so that the issue could be addressed.

@yongtang yongtang closed this Feb 13, 2017
@yongtang yongtang deleted the 29835-fix branch May 25, 2017 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/failing-ci Indicates that the PR in its current state fails the test suite status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants