Page MenuHomeFreeBSD

Implement aliases in a way that actually work.
ClosedPublic

Authored by imp on May 8 2020, 4:39 PM.

Details

Reviewers
allanjude
cem
rpokala
Group Reviewers
cam
Summary

The alias needs to be part of the provider instead of the geom. To bind the DEV
geom, we need to look at the provider's names and aliases and create the dev
entries from there. If this lives in the GEOM, then it won't propigate down the
tree properly. Remove it from geom, add it provider.

Update geli, gmountver, gnop, gpart, and guzip to use it, which handles the bulk
of the uses in FreeBSD. I think this is all the providers that apply a suffix to
the parent name.

Add nvd alias back to nda now that it actually works.

Test Plan

boot with a complicated setup:

% ls -l '/dev/n[dv][ad]0*'
crw-r-----  1 root  operator  0x5f May  8 16:13 /dev/nda0
crw-r-----  1 root  operator  0x61 May  8 16:13 /dev/nda0p1
crw-r-----  1 root  operator  0x63 May  8 16:13 /dev/nda0p2
crw-r-----  1 root  operator  0x65 May  8 16:13 /dev/nda0p3
crw-r-----  1 root  operator  0x67 May  8 16:13 /dev/nda0p4
crw-r-----  1 root  operator  0x69 May  8 16:13 /dev/nda0p5
crw-r-----  1 root  operator  0xb8 May  8 16:13 /dev/nda0p5.eli
crw-r-----  1 root  operator  0x6b May  8 16:13 /dev/nda0p6
crw-r-----  1 root  operator  0x6d May  8 16:13 /dev/nda0p7
crw-r-----  1 root  operator  0x6f May  8 16:13 /dev/nda0p8
crw-r-----  1 root  operator  0x71 May  8 16:13 /dev/nda0p9
lrwxr-xr-x  1 root  wheel        4 May  8 16:13 /dev/nvd0 -> nda0
lrwxr-xr-x  1 root  wheel        6 May  8 16:13 /dev/nvd0p1 -> nda0p1
lrwxr-xr-x  1 root  wheel        6 May  8 16:13 /dev/nvd0p2 -> nda0p2
lrwxr-xr-x  1 root  wheel        6 May  8 16:13 /dev/nvd0p3 -> nda0p3
lrwxr-xr-x  1 root  wheel        6 May  8 16:13 /dev/nvd0p4 -> nda0p4
lrwxr-xr-x  1 root  wheel        6 May  8 16:13 /dev/nvd0p5 -> nda0p5
lrwxr-xr-x  1 root  wheel       10 May  8 16:13 /dev/nvd0p5.eli -> nda0p5.eli
lrwxr-xr-x  1 root  wheel        6 May  8 16:13 /dev/nvd0p6 -> nda0p6
lrwxr-xr-x  1 root  wheel        6 May  8 16:13 /dev/nvd0p7 -> nda0p7
lrwxr-xr-x  1 root  wheel        6 May  8 16:13 /dev/nvd0p8 -> nda0p8
lrwxr-xr-x  1 root  wheel        6 May  8 16:13 /dev/nvd0p9 -> nda0p9
% ls -l /dev/n[dv][ad]3*
crw-r-----  1 root  operator  0x89 May  8 16:13 /dev/nda3
crw-r-----  1 root  operator  0xa3 May  8 17:17 /dev/nda3s1
crw-r-----  1 root  operator  0xa5 May  8 17:17 /dev/nda3s1a
crw-r-----  1 root  operator  0xa7 May  8 17:17 /dev/nda3s1b
crw-r-----  1 root  operator  0xa9 May  8 17:17 /dev/nda3s1d
crw-r-----  1 root  operator  0xab May  8 17:17 /dev/nda3s1e
lrwxr-xr-x  1 root  wheel        4 May  8 16:13 /dev/nvd3 -> nda3
lrwxr-xr-x  1 root  wheel        6 May  8 17:17 /dev/nvd3s1 -> nda3s1
lrwxr-xr-x  1 root  wheel        7 May  8 17:17 /dev/nvd3s1a -> nda3s1a
lrwxr-xr-x  1 root  wheel        7 May  8 17:17 /dev/nvd3s1b -> nda3s1b
lrwxr-xr-x  1 root  wheel        7 May  8 17:17 /dev/nvd3s1d -> nda3s1d
lrwxr-xr-x  1 root  wheel        7 May  8 17:17 /dev/nvd3s1e -> nda3s1e

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 30982
Build 28689: arc lint + arc unit

Event Timeline

imp requested review of this revision.May 8 2020, 4:39 PM
imp added a reviewer: allanjude.
sys/cam/nvme/nvme_da.c
953

Yes. the old code only worked for the single partition case... And even then 'worked' was likely too strong a characterization...

All of my suggestions are stylistic/ non-functional. Functionally looks great. Thanks!

sys/geom/geom_subr.c
652

The cast is not needed in C and in this case there is no really compelling reason to have it in spite of that.

I’d suggest replacing the sizeof expression with sizeof(*gap) but that is just personal preference.

654

Given the known length of an sbuf, this could be memcpy instead, which doesn’t need to nul-check every byte in the source.

You do not even need sbuf_len()+1 to capture the trailing nul due to M_ZERO allocation.

There was another similar instance of this earlier in the patch.

656

This is kind of an odd construction. ga_alias could just be a trailing variable length array, and avoid wasting 8 bytes for no reason.

sys/geom/part/g_part.c
492

“Before” is now stale

This revision is now accepted and ready to land.May 8 2020, 5:44 PM
sys/geom/geom_subr.c
652

yea this is copied from elsewhere.... I can remove the cast... the *gap vs what it is matters more in larger functions... I'll consider it here...

654

It's a string, I'm not going to hyper-optimize this to obscure that fact. It makes the code more obscure.

656

It's done elsewhere exactly like this in geom. If we ever want to add other notations and such to the aliases, or need two strings, we'd have to rework a lot. Based on these to things, this is fine.

sys/geom/part/g_part.c
492

True. This needs to be reworded a bit since this was for the old geom implementation and we needed to do that much to get the single partition case to work (which, in hindsight is (a) the only thing I tested and (b) the only thing that could work).

I note this is now creating an alias in NVMECAM, as well as in the GEOM layer; why both?

Also, it appears you have added support for aliases to some providers that didn't previously support them (g_eli, g_mountver, g_nop, g_uzip), but not all; why that subset?

I note this is now creating an alias in NVMECAM, as well as in the GEOM layer; why both?

nda does it for the disk it creates. The geom part layer (and others) do it for the layers they create. Basically, anywhere we create names based on the upper layer's name, we also have to create aliases based on the upper layer's alaises. We can't automate this because name creation belongs to the provider.

Also, it appears you have added support for aliases to some providers that didn't previously support them (g_eli, g_mountver, g_nop, g_uzip), but not all; why that subset?

It's my belief that I've done all the providers that create their lower level name from the upper layer name (eg nda0 -> nda0p1) as opposed to getting the same from elsewhere (eg nda0p2, nda1p2 -> mirror/fred). Did I miss one?

sys/cam/nvme/nvme_da.c
953

This likely needs a sysctl/tunable so people/automation that have migrated or are in the process of migrating don't get confused by both there if they aren't prepared for it.

sys/geom/geom_subr.c
652

I've tightened it up a little, though

654

memcpy avoid strcpy false positive in static analysis though, so sure.

sys/geom/part/g_part.c
492

I've reworded.

Changes look fine, and I'm satisfied with your answers to my question.