Bug 235473 - geom: add feature: gconcat online append
Summary: geom: add feature: gconcat online append
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-geom mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-02-03 22:49 UTC by noah.bergbauer
Modified: 2019-02-17 19:50 UTC (History)
1 user (show)

See Also:


Attachments
Initial patch draft (8.24 KB, text/plain)
2019-02-03 22:49 UTC, noah.bergbauer
no flags Details
Patch v2 (12.99 KB, patch)
2019-02-16 23:25 UTC, noah.bergbauer
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
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