Bug 235473
| Summary: | geom: add feature: gconcat online append | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | noah.bergbauer | ||||||
| Component: | kern | Assignee: | freebsd-geom (Nobody) <geom> | ||||||
| Status: | Closed FIXED | ||||||||
| Severity: | Affects Only Me | CC: | markj | ||||||
| Priority: | --- | Keywords: | patch | ||||||
| Version: | CURRENT | ||||||||
| Hardware: | Any | ||||||||
| OS: | Any | ||||||||
| Attachments: |
|
||||||||
Created attachment 202078 [details]
GEOM patch v2
So AFAICT the best approach here is to use sx locks (after all that's how gmirror does it).
After testing this with WITNESS I fixed several bugs but still: some feedback would be much appreciated!
Would you mind uploading a copy of the diff to phabricator? Some instructions are here: https://wiki.freebsd.org/Phabricator You'd have to create another account, but it makes commenting on diffs much easier. If you prefer not to, I can upload the diff myself and make comments there. (In reply to Mark Johnston from comment #2) Thanks, done: https://reviews.freebsd.org/D19227 This was committed as https://cgit.freebsd.org/src/commit/?id=d575e81fbcfa0a83848a8b9b09dd02497bfd5b00 , so I believe there's nothing left to do here. Please re-open if that's not the case. |
Created attachment 201701 [details] Initial patch draft It really shouldn't be necessary to rebuild a gconcat volume from scratch just to add another disk. This is particularly inconvenient because it forces one to remount/restart any filesystems/geoms stacked on top of gconcat for no apparent reason. I attached my first attempt at hacking on this. It works! But since I have never worked with geoms (or rather any part of the kernel at all) before I am very unsure about correctness, especially when it comes to locking and race conditions in general. Since the existing mutex appears to have a very specific purpose, I created a new one. One obvious race is keeping the `g_access` levels in sync, so I'm locking over there as well. I would like to avoid locking a mutex in the regular IO path (g_concat_start) as this looks like a performance footgun to me, but given that I have to do array reallocation it seems to me that the only alternative is to leak memory - perhaps an rwlock would be appropriate to solve this? Even after using FreeBSD for several years I never understood the hardcode option so I didn't implement that either. Incrementing G_CONCAT_VERSION didn't work for some reason (userland tool wouldn't pick up the new version), did I miss something? Also I still need to update the manpage. I mean, the patch is quite literally a copy-pasted mix from gmultipath (resize/insert capability), gmirror (metadata update) and gconcat itself. It works but it really needs someone who actually knows how this stuff works to advise me on how to proceed.