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 for partition races #64

Merged
merged 2 commits into from May 26, 2016
Merged

Fix for partition races #64

merged 2 commits into from May 26, 2016

Conversation

mvollmer
Copy link
Contributor

No description provided.

…ions"

This reverts commit eb4b994.

The BLKRRPART ioctl is dangerous in that it causes pairs of remove/add
uevents for all partitions which in turn confuse our internal data
structures and cause the block devices to briefly disappear from the
filesystem.

Rereading the partition table is also unnecessary since parted,
libblockdev, and udevd do that already.

Thus, eb4b994 might actually make things worse instead of fixing the
bug it is intended to fix.
@mvollmer
Copy link
Contributor Author

mvollmer commented May 26, 2016

I have confirmed that this works via the check-storage-luks test case of Cockpit:

https://github.com/cockpit-project/cockpit/blob/master/test/verify/check-storage-luks

This test shows the following behavior on Debian unstable:

With storaged 2.5.0: reliably fails
With storaged 2.5.0 and a artificial "flock -s /dev/sda" in the background: reliably passes
With storaged 2.5.1: reliably fails
With storaged 2.5.1 and a artificial "flock -s /dev/sda" in the background: reliably fails
With storaged 2.5.1 and this patch: reliably passes

This means that flock -s helps, that the recent "re-read partitions" fix doesn't help, and that this patch actually prevents "flock -s" from helping and thus needs to be reverted.

A consistent conclusion can be found with Fedora:

With storaged 2.5.1: reliably fails
With storaged 2.5.1 and a artificial "flock -s /dev/sda" in the background: reliably fails
With storaged 2.5.1 and this patch: reliably passes

@tsmetana
Copy link
Contributor

Hm... Isn't this basically a workaround of an udev issue? Not that I would not want to merge the patch (thanks! btw.). Just trying to understand it...

@mvollmer
Copy link
Contributor Author

Isn't this basically a workaround of an udev issue?

Yes, you could say that.

Real fix 1: We can remove the BLKRRPART call from udev. This is a change in behavior and might break existing code.

Real fix 2: We can change BLKRRPART to not remove everything and then put everything back, but instead to only add new partitions, and only remove partitions that have really disappeared.

@tsmetana tsmetana merged commit 408cb57 into storaged-project:master May 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants