Page MenuHomeFreeBSD

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

Authored by imp on Aug 5 2017, 6:19 AM.
Tags
None
Referenced Files
F103221601: D11873.id31708.diff
Fri, Nov 22, 9:13 AM
Unknown Object (File)
Thu, Nov 21, 11:35 AM
Unknown Object (File)
Tue, Nov 19, 12:00 AM
Unknown Object (File)
Mon, Nov 18, 3:57 PM
Unknown Object (File)
Sun, Nov 17, 3:10 AM
Unknown Object (File)
Sun, Nov 10, 2:59 AM
Unknown Object (File)
Sun, Nov 3, 12:56 PM
Unknown Object (File)
Oct 20 2024, 4:37 PM
Subscribers
None

Details

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

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

imp added reviewers: scottl, rpokala, cem, kibab, ken.
imp edited the summary of this revision. (Show Details)
sys/geom/geom.h
162 ↗(On Diff #31611)

Upon reflection, I suppose I need to hack the XML generator to report this, eh?

sys/geom/part/g_part.c
1930–1932 ↗(On Diff #31611)

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 ↗(On Diff #31611)

... to implement the diskX concept ...

sys/geom/part/g_part.c
447 ↗(On Diff #31611)

"created"

sys/cam/nvme/nvme_da.c
811 ↗(On Diff #31611)

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 ↗(On Diff #31611)

Do you anticipate adding multiple (not one) aliases will ever be useful?

sys/geom/geom_disk.c
903 ↗(On Diff #31611)

What if this fails?

sys/geom/geom_subr.c
1224 ↗(On Diff #31611)

What if this fails?

sys/geom/part/g_part.c
1930–1932 ↗(On Diff #31611)

This is easily visible from the code itself. I think it's better not to add anything to the comment.

sys/geom/part/g_part.c
1931 ↗(On Diff #31611)

Since you're in this function, this would be a fine time to correct the typo: "Obtain".

sys/geom/geom.h
128 ↗(On Diff #31611)

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 ↗(On Diff #31611)

WAITOK -> No failure.

sys/geom/geom_subr.c
1224 ↗(On Diff #31611)

WAITOK -> no failure

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).

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 ↗(On Diff #31611)

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
imp edited edge metadata.
imp edited the test plan for this revision. (Show Details)

Update, per review.

This revision now requires review to proceed.Aug 7 2017, 4:24 PM
imp marked 2 inline comments as done.

comments

imp marked 5 inline comments as done.

more

imp marked an inline comment as done.Aug 7 2017, 4:33 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 ↗(On Diff #31611)

I don't think so. There's other places in the tree that don't do these things.

This revision was automatically updated to reflect the committed changes.