Details

Reviewers

scottl
rpokala
cem
kibab
ken

Commits

rS322199: Add nvd alias to nda ndoes.
rS322198: Expose API to allow disks to ask for alias names in devfs.
rS322197: Add alias support to gpart.
rS322196: Add aliasing concept to geom.

Summary

Implement disk_add_alias to allow aliases to be added to disks. This allows one to alias nda device nodes to nvd.

Add an alias name list to geoms. Use them in geom_dev to create aliases.

Add alias support to gpart.

When we're creating new providers for each of the partitions, add
aliases to the geom before we create the provider so when geom_dev
tastes the provider, the aliases are in place so the proper /dev
entries are created. So foo5p6 gets created as an alias for bar5p6
when foo is an alias for bar.

(this is done as three commits: add to geom, add to gpart, add disk API)

Test Plan

ls -l /dev/n*
crw-r----- 1 root operator 0x89 Aug 5 00:09 /dev/nda0
crw-r----- 1 root operator 0x92 Aug 5 00:09 /dev/nda0p1
crw------- 1 root wheel 0xe Aug 5 00:09 /dev/netmap
crw------- 1 root kmem 0x19 Aug 5 00:09 /dev/nfslock
crw-rw-rw- 1 root wheel 0x10 Aug 5 00:11 /dev/null
lrwxr-xr-x 1 root wheel 4 Aug 5 00:09 /dev/nvd0 -> nda0
lrwxr-xr-x 1 root wheel 6 Aug 5 00:09 /dev/nvd0p1 -> nda0p1
crw------- 1 root wheel 0x2b Aug 5 00:09 /dev/nvme0
crw------- 1 root wheel 0x56 Aug 5 00:09 /dev/nvme0ns1

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 10926
Build 11314: arc lint + arc unit

Event Timeline

imp created this revision.Aug 5 2017, 6:19 AM

Harbormaster completed remote builds in B10872: Diff 31611.Aug 5 2017, 6:19 AM

imp edited the test plan for this revision. (Show Details)Aug 5 2017, 6:21 AM

imp added reviewers: scottl, rpokala, cem, kibab, ken.

imp edited the summary of this revision. (Show Details)

imp added inline comments.Aug 5 2017, 6:27 AM

sys/geom/geom.h
162	Upon reflection, I suppose I need to hack the XML generator to report this, eh?
sys/geom/part/g_part.c
1930–1932	Reviewers: Should I add "and inherit aliases from the container geom" here?

Shouldn't the aliasing be documented in nvd(4), nda(4), or both?

Do we want the aliasing to be done unconditionally, or do we want a knob for it?

Just to make sure I'm understanding the code flow:

ndaregister("ndaX") calls disk_add_alias("nvdX")
disk_add_alias() puts the alias on the list of disk aliases in the struct disk
<other stuff>
g_disk_create() walks the list of aliases in the struct disk and calls g_geom_add_alias()
g_geom_add_alias() puts the alias on the list of geom aliases in the struct g_geom
<other stuff>
g_dev_taste() tells devfs to create the symlink for each alias in the struct g_geom

share/man/man9/disk.9
255	... to implement the diskX concept ...
sys/geom/part/g_part.c
447	"created"

kibab added inline comments.Aug 5 2017, 8:35 AM

sys/cam/nvme/nvme_da.c
811	Please add a comment here why you are doing this and when i t should be removed (eg when we decide that it's time to kill nvd completely / that migration is finished)
sys/geom/geom.h
128	Do you anticipate adding multiple (not one) aliases will ever be useful?
sys/geom/geom_disk.c
903	What if this fails?
sys/geom/geom_subr.c
1224	What if this fails?
sys/geom/part/g_part.c
1930–1932	This is easily visible from the code itself. I think it's better not to add anything to the comment.

rpokala added inline comments.Aug 5 2017, 9:27 PM

sys/geom/part/g_part.c
1931	Since you're in this function, this would be a fine time to correct the typo: "Obtain".

imp added inline comments.Aug 5 2017, 9:30 PM

sys/geom/geom.h
128	I was originally going to do just one. However, Scott Long convinced me that we may someday wind up in a situation where we'd want to unify many drivers. It's not yet clear to me that's not useful, so I made these lists.
sys/geom/geom_disk.c
903	WAITOK -> No failure.
sys/geom/geom_subr.c
1224	WAITOK -> no failure

In D11873#246237, @rpokala wrote:

Shouldn't the aliasing be documented in nvd(4), nda(4), or both?

Yes. I'll update the patch in a few with this as well as the XML changes I think we should have. Though maybe tomorrow.

Do we want the aliasing to be done unconditionally, or do we want a knob for it?

I think unconditionally for now, but I can see a need in the future. Also, the nda/nvd selection now is a bit weak, so maybe options for both in the future.

Just to make sure I'm understanding the code flow:

ndaregister("ndaX") calls disk_add_alias("nvdX")

disk_add_alias() puts the alias on the list of disk aliases in the struct disk

<other stuff>

g_disk_create() walks the list of aliases in the struct disk and calls g_geom_add_alias()

g_geom_add_alias() puts the alias on the list of geom aliases in the struct g_geom

<other stuff>

g_dev_taste() tells devfs to create the symlink for each alias in the struct g_geom

Correct.

Gpart propagates the aliases within the geoms it creates for the disk label scheme and anthing nested (so like bsd label inside of a MBR slice).

In D11873#246237, @rpokala wrote:

Shouldn't the aliasing be documented in nvd(4), nda(4), or both?

There's no nda(4). That should be fixed. That's where this should be documented. That's beyond the scope of this fix.

It's an interesting question if forward compat should be added to nvd.

Looks fine to me. Add the aliases to the mesh XML dump somehow. One minor nit below.

sys/geom/geom_dev.c
372–374	Could any of this stuff be moved inside `make_dev_alias_p`?

This revision is now accepted and ready to land.Aug 7 2017, 4:20 PM

Update, per review.

This revision now requires review to proceed.Aug 7 2017, 4:24 PM

Harbormaster completed remote builds in B10923: Diff 31708.Aug 7 2017, 4:25 PM

comment too

Harbormaster completed remote builds in B10924: Diff 31709.Aug 7 2017, 4:27 PM

comments

Harbormaster completed remote builds in B10926: Diff 31711.Aug 7 2017, 4:32 PM

At this point, I think I've take care of everything, with the possible exception of cem's suggestion (which might have merit I'm yet to fully appreciate, but is better done in a separate commit). Lemme know if I missed something.

sys/geom/geom_dev.c
372–374	I don't think so. There's other places in the tree that don't do these things.

Closed by commit rS322196: Add aliasing concept to geom. (authored by imp). · Explain WhyAug 7 2017, 9:12 PM

This revision was automatically updated to reflect the committed changes.

Create mechanism to allow nvd to be an alias for nda
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 31711

share/man/man9/disk.9

sys/cam/nvme/nvme_da.c

sys/geom/geom.h

sys/geom/geom_dev.c

sys/geom/geom_disk.h

sys/geom/geom_disk.c

sys/geom/geom_dump.c

sys/geom/geom_subr.c

sys/geom/part/g_part.c

Create mechanism to allow nvd to be an alias for ndaClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 31711

share/man/man9/disk.9

sys/cam/nvme/nvme_da.c

sys/geom/geom.h

sys/geom/geom_dev.c

sys/geom/geom_disk.h

sys/geom/geom_disk.c

sys/geom/geom_dump.c

sys/geom/geom_subr.c

sys/geom/part/g_part.c

Create mechanism to allow nvd to be an alias for nda
ClosedPublic
Actions

Revision Contents
Changeset List