Bug 235473

Summary: geom: add feature: gconcat online append
Product: Base System Reporter: noah.bergbauer
Component: kernAssignee: freebsd-geom (Nobody) <geom>
Status: New ---    
Severity: Affects Only Me CC: markj
Priority: --- Keywords: patch
Version: CURRENT   
Hardware: Any   
OS: Any   
Description Flags
Initial patch draft
GEOM patch v2 none

Description noah.bergbauer 2019-02-03 22:49:52 UTC
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.
Comment 1 noah.bergbauer 2019-02-16 23:25:30 UTC
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!
Comment 2 Mark Johnston freebsd_committer 2019-02-17 17:02:32 UTC
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.
Comment 3 noah.bergbauer 2019-02-17 19:50:36 UTC
(In reply to Mark Johnston from comment #2)

Thanks, done: https://reviews.freebsd.org/D19227