Conversation
|
Hello @prajnoha I'm not saying that we are not going to merge this but it would take time before it is deeply validated. Did you tested it with exernal metadata? @bkucman could you please test this with IMSM? |
Yes, I have tested with IMSM (with |
6d40337 to
fc587cb
Compare
I'm more concerned about boot flows with OS installed on IMSM raid here is why I asked bkucman to verify (imsm is owned by graid now). |
|
Yes, this change requires testing on real test case scenarios, I’ll internally plan such tests on the hardware with IMSM. |
|
Hey @bkucman could you please give an testing ETA here? |
mwilck
left a comment
mwilck
left a comment
There was a problem hiding this comment.
Minor remarks from my side. Sorry for the late review.
| /* We need to create the device */ | ||
| map_lock(&map); | ||
| mdfd = create_mddev(ident->devname, ident->name, LOCAL, chosen_name, 1); | ||
| udev_blocked = udev_is_available(); |
There was a problem hiding this comment.
Instead of calling udev_is_available explicitly in callers (which can easily be forgotten), why not keep the call in create_mddev(), and add an int * (or bool *) parameter in which create_mddev() can store the current state?
There was a problem hiding this comment.
Let me add that I think the entire udev_block() logic is racy by design.
We call udev_is_available() in many places now. Apparently the idea is that the availability of udev can change while mdadm is running (otherwise, one call to udev_is_available() at startup would be sufficient). But if we assume that this can happen, it can happen any time, so remembering this state in a variable is moot.
TBH, I don't understand why we need the udev_is_available() check at all. Here at least. If udev was not running, creating files under /run/mdadm and deleting them later wouldn't do any harm. So we could just create and delete them always and skip the test if udev is running. Or what am I overlooking here?
There was a problem hiding this comment.
Instead of calling
udev_is_availableexplicitly in callers (which can easily be forgotten), why not keep the call increate_mddev(), and add anint *(orbool *) parameter in whichcreate_mddev()can store the current state?
Yes, sure, we can do that instead - that would do the job as well.
Let me add that I think the entire
udev_block()logic is racy by design. We calludev_is_available()in many places now. Apparently the idea is that the availability of udev can change while mdadm is running (otherwise, one call toudev_is_available()at startup would be sufficient). But if we assume that this can happen, it can happen any time, so remembering this state in a variable is moot.
Honestly, I'm not a big fan of using any files in the filesystem for any notification purposes, because then it's critical for us to be sure we always clean it up properly. And yes, it is usually prone to various races too. However, this is the existing mechanism we have. Surely, we could start thinking of improving it and look for better future mechanism. For now, this patchset is just a fixup/cleanup for existing one (which I came to and noticed an opportunity for immediate smaller improvements).
TBH, I don't understand why we need the
udev_is_available()check at all. Here at least. If udev was not running, creating files under/run/mdadmand deleting them later wouldn't do any harm. So we could just create and delete them always and skip the test if udev is running. Or what am I overlooking here?
For me, that would be just a matter of not executing useless code. BUT udev_is_available now executes stat for /dev/.udev and /run/udev each time, which itself seems superfluous - running it at the start should suffice. I'd double check the error paths though, where certain operation starts with "udev enabled" and then later in the process, it encounters a situation where udev is gone (like, not waiting for a signal from udev indefinitely or similar). Just in case.
Overall, sure, we can start thinking about more broader cleanups and improvements here for future changes.
There was a problem hiding this comment.
is_udev_available is a 2 years ago reworked verification that has been always here. I never tried to challenge that and I trust your judgement. Definitely, there is a log of space to rework.
There was a problem hiding this comment.
With udev and the limited experience and support we always had, we (previously at Intel) at Graid always minimized risk of regression in this area.
I'm glad to see you guys talking about this and challenging this legacy code. You have my support but we have limited validation capabilities for a while, we are still building everything.
Sorry @prajnoha for keeping you waiting for regression testing.
There was a problem hiding this comment.
Sorry @prajnoha for keeping you waiting for regression testing.
Personally, I see no risk of regeressions here.
There was a problem hiding this comment.
I'm more careful here as any mistake may lead to mdmon blocked or not stated which is critical.
Not sure what other options we have. we could use
Yes. But it's useless code one way or the other. Either we create and delete files under /run pointlessly, or we repeatedly check for udev, also pointlessly. Either operation is rather light-weight, I think. |
Move the symlink wait to the common exit path, after udev is unblocked and the uevent is sent. The re-add loop does not need the symlink. A flag separates a successful start from the "not enough disks" case, so the latter does not wait and time out. The redundant rv = 0 goes with it, rv is already 0 in that branch. The array file descriptor has to stay open for the wait, so it is closed after it rather than before. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
The wait comes before the uevent, so it can only succeed if udev already created the symlink in response to an earlier event. Send the uevent first and wait after it. The array file descriptor has to stay open for the wait, so it is closed after it as well. Drop the udev_unblock() at the end of the path, it repeats the one already done before the uevent is sent. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
Sysfs rule application does not need the symlink. Move the symlink wait after it so it is the last step before return. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
Create() waits for the /dev/md/<name> link before sending the event that announces the array is ready, so nothing it has done by then can create that link. The add event from create_mddev cannot either, the array is still clear or inactive and udev-md-raid-arrays.rules gives up before it reaches the symlink rules. The wait only succeeds if udev happens to process the kernel's own change event from array start in time, which shows up as a spurious "timeout waiting for ..." when udev is busy. Send the event first and wait after it, the same ordering the incremental paths use. A flag records whether the array reached a state worth waiting for, so the "not enough devices" case does not wait at all. The container branch already had this ordering, it sent its own uevent because a container is never run and so gets no event from the kernel. That call is now redundant. The array file descriptor has to stay open for the wait, so it is closed after it rather than before. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
Several error paths return without unblocking udev, leaving the blocking file behind. Route them through a single error exit that unblocks. That exit is also reached when udev was never blocked, either because the caller did not ask for it or because we failed before udev_block(). It needs no guard, udev_unblock() does nothing when there is no file to remove. Drop the safety net in Incremental_container(), create_mddev() now cleans up after itself. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
When a container member is skipped by the filter, udev is not unblocked, leaking the blocking file on disk. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
The release path sends a uevent unconditionally, but subarrays already get their own after assembly and error paths have nothing to signal. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
Add udev_ready() for the common "device is ready" path, where udev is unblocked and a change event is sent so that udev re-evaluates the array. It replaces the recurring pattern of calling both in sequence. Call sites that only need udev_unblock() (error, abort and skip paths) keep it, they are a distinct case. At Incremental()'s common exit it moves into an else branch, so it still runs when there is no sra to send the event on. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
Checking whether udev runs means looking for the udev directories and reading an environment variable. udev can be stopped at any time, so the answer is never a promise that it is still there when the caller acts on it, and repeating the check only moves that race around. Since no number of checks gives a guarantee, do it once on startup and remember the answer. udev_detect() does the check and stores it, udev_is_available() only reports what was stored. The whole invocation then acts on a single view of the system. Monitor is the exception, it runs long enough for the answer to go stale for good rather than momentarily. udev_is_available() used to stat() on every call, which kept Monitor current on each pass through its event loop. Keep that property by refreshing the state there. mdmon needs no such call, no code path it executes asks whether udev is available. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
create_mddev() cleared its block_udev argument when udev was not running, so udev_block() was only ever reached with udev present. That leaves every caller to remember the same rule. Do the check in udev_block() instead and let callers ask for blocking unconditionally. Blocking a udev that is not running trivially succeeds, there is nothing to block. Callers do not have to track whether udev ended up blocked either. The blocking file is the state, so udev_unblock() knows on its own that there is nothing to remove and is safe on any cleanup path. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
The check lived in udev_initialize(), which only runs on the first call, so Monitor had to repeat it before every wait. Without that the second pass would go straight to the monitor socket with no udev behind it, and the first would complain "No udev." on a system that simply does not run udev. Do the check in udev_wait_for_events() instead. A udev that is not running is not an error worth reporting, there is just nothing to wait for, so return UDEV_STATUS_ERROR_NO_UDEV and let the caller sleep another way. Monitor keeps its udev_detect() call, that one refreshes the answer because Monitor runs long enough for it to change. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
Only the 'abort' label unblocks udev, but most error paths in Create() jump directly to 'abort_locked' and leave the blocking file behind. Unblock at 'abort_locked' instead, which is reached from both labels. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
fc587cb to
2ae1aef
Compare
|
Hi! I have refreshed the patchset and addressed some of the points from our discussion - mainly the I'm also looking at |
The copy is bounded by the size of the source pointer instead of the size of the destination buffer, so it stops after sizeof(char *) - 1 bytes, 7 on a 64-bit build. Longer names are cut short and the following udev_ready() writes the change event to a wrong sysfs path, or to none at all. Use the size of the destination buffer instead. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
The uevent in udev_ready() belongs to the unblocking, it tells udev to look at the device it was kept away from. Sending it when nothing was blocked is not part of that and it can repeat an event udev already received. That is the case when incremental assembly completes an already existing array. The array node is only opened, udev is never blocked, and the event sent on exit repeats the one the kernel emitted when the array started. Return from udev_unblock() whether udev was blocked and send the uevent only then. Code that needs to refresh udev state for another reason can call sysfs_uevent() on its own. Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
2ae1aef to
f850c6d
Compare
4 participants
This patchset cleans up udev blocking lifecycle management across Incremental, Create, and Assemble, fixing several bugs and a race condition along the way.
Problems addressed:
Symlink wait vs. uevent ordering race. In the Incremental container path, wait_for() ran before the change uevent was sent. udev could see the initial add event before metadata was set, skip symlink creation, and cause a ~3.6s timeout. The same misordering existed in the non-container path and in Assemble (relative to sysfs_rules_apply()).
Leaked udev blocking files. Several error paths in create_mddev() and Incremental_container() returned without calling udev_unblock(), leaving the blocking file on disk. Skipped container members also leaked the blocking file.
Redundant/spurious udev calls. udev_unblock() and sysfs_uevent() were called unconditionally even when udev was not available or when the code path never called udev_block() (e.g. a second disk arriving for an already-existing IMSM container). The container release path sent a redundant uevent that subarrays had already sent individually.