Page MenuHomeFreeBSD

Fix geli device cleanup
ClosedPublic

Authored by ngie on Apr 9 2019, 12:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 6:27 PM
Unknown Object (File)
Thu, Nov 21, 8:39 AM
Unknown Object (File)
Sat, Nov 2, 1:11 AM
Unknown Object (File)
Oct 23 2024, 4:59 AM
Unknown Object (File)
Oct 20 2024, 11:19 AM
Unknown Object (File)
Oct 19 2024, 4:08 AM
Unknown Object (File)
Oct 8 2024, 8:50 AM
Unknown Object (File)
Oct 6 2024, 10:57 PM
Subscribers

Details

Summary

Final cleanup routines shouldn't be called from testcases; it should be called
from the testcase cleanup routine.

Furthermore, geli_test_cleanup should take care of cleaning up geli providers
and the memory disks used for the geli providers. geli_test_cleanup will always
be executed whereas the equivalent logic in geli_test_body, may not have been
executed if the test failed prior to the logic being run.

Prior to this change, the test case was trying to clean up $md twice: once in
at the end of the test case body function, and the other in the cleanup function.
The cleanup function logic was failing because there wasn't anything to clean up
in the cleanup function and the errors weren't being ignored.

This fixes FreeBSD test suite runs after r345864.

PR: 237128
MFC with: r345864

Test Plan
$ sudo kyua test -k /usr/tests/sys/geom/class/eli/Kyuafile online_resize_test:online_resize; sudo mdconfig -l
online_resize_test:online_resize  ->  passed  [17.098s]

Results file id is usr_tests_sys_geom_class_eli.20190409-005019-784972
Results saved to /root/.kyua/store/results.usr_tests_sys_geom_class_eli.20190409-005019-784972.db

1/1 passed (0 failed)
$

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23582
Build 22573: arc lint + arc unit

Event Timeline

I don't know why you added a blank line at 179, but otherwise LGTM.

This revision is now accepted and ready to land.Apr 9 2019, 3:23 AM

Do you mind elaborating why cleaning up like that is wrong? The reason I did it was to avoid creating multiple md devices. Here we only have 8 iterations of the loop, but I could easly imagine tests were I'd have hundreds and cleaning as I go would be a good thing then, IMHO. But maybe I'm missing something obvious here, so if you could explain that would be great. Thanks.

PS. The patch is fine.

In D19854#426189, @pjd wrote:

Do you mind elaborating why cleaning up like that is wrong? The reason I did it was to avoid creating multiple md devices. Here we only have 8 iterations of the loop, but I could easly imagine tests were I'd have hundreds and cleaning as I go would be a good thing then, IMHO. But maybe I'm missing something obvious here, so if you could explain that would be great. Thanks.

PS. The patch is fine.

Long story short is that it was doubling up the cleanup process.

It would allocate the device in body function, clean it up in the body function, then try to clean up the same device in the cleanup function, failing because it didn't exist in a state that could be deallocated/detached.

I don't know why you added a blank line at 179, but otherwise LGTM.

Fixed :)...

In D19854#426191, @ngie wrote:
In D19854#426189, @pjd wrote:

Do you mind elaborating why cleaning up like that is wrong? The reason I did it was to avoid creating multiple md devices. Here we only have 8 iterations of the loop, but I could easly imagine tests were I'd have hundreds and cleaning as I go would be a good thing then, IMHO. But maybe I'm missing something obvious here, so if you could explain that would be great. Thanks.

PS. The patch is fine.

Long story short is that it was doubling up the cleanup process.

It would allocate the device in body function, clean it up in the body function, then try to clean up the same device in the cleanup function, failing because it didn't exist in a state that could be deallocated/detached.

Hmm, I thought that ignoring exit code for mdconfig and redirecting its stderr to /dev/null won't make it fail the tests. I wanted to leave it in case the tests somehow exited in the middle. Anyway, looks good with your patch.

A more important reason not to clean up in the body is because it won't necessarily get run. If the test fails, the body will exit immediately. That's why cleanup is done in a separate process.

This revision was automatically updated to reflect the committed changes.