Details

Reviewers

imp
chuck
jhb

Group Reviewers

cam

Commits

rGe4416d1e95c4: nvme_sim: signal namespace depature
rGda71efa95d34: nvd: handle namespace changes
rGecfb0e51b212: nda: React to namespace change events
rG0ccc71de6284: nvme: Refactor geom setting to function.
rG2c5edd4860b2: nvme: Notify namespace changes better
rGb7ffac04541f: nvme_sim: signal namespace depature
rG96a7ed1e7fb4: nvd: handle namespace changes
rG9a465b37ea17: nda: React to namespace change events
rG31412fda9fa7: nvme: Refactor geom setting to function.
rGfe6ee00d174e: nvme: Notify namespace changes better
rG4640f5008922: nvme_sim: signal namespace depature
rGbd769e73d8f1: nvd: handle namespace changes
rG86d3ec359a56: nda: React to namespace change events
rGdffd882d12d2: nvme: Refactor geom setting to function.
rG20e94950c54e: nvme: Notify namespace changes better

Summary

Take D32963 and make nvd/nda resize after namespace changed async event trigger.

create a new thread and this thread will update namespace data when needed.

nvd can now call resize_disk() and update the disk size properly.

for nda, after NVME_PROBE_IDENTIFY_NS is finish, fire a AC_GETDEV_CHANGED event,
and it will call resize_disk() to adjust disk size.

Test Plan

When bhyve vm resize disk, verify diskinfo output inside guest, both nvd and nda.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 43079
Build 39967: arc lint + arc unit

Event Timeline

wanpengqian_gmail.com created this revision.Nov 17 2021, 9:03 AM

Herald added a reviewer: cam. · View Herald TranscriptNov 17 2021, 9:03 AM

Herald added a subscriber: dab. · View Herald Transcript

wanpengqian_gmail.com requested review of this revision.Nov 17 2021, 9:03 AM

Harbormaster completed remote builds in B42837: Diff 98634.Nov 17 2021, 9:03 AM

remove nvd modification.

Harbormaster completed remote builds in B42838: Diff 98635.Nov 17 2021, 10:12 AM

Like I said before, we shouldn't be basing this off the async callback, but instead the namespace change stuff.
Namespaces can arrive and depart, and this doesn't cope with that aspect of things, just the size change.

sys/cam/nvme/nvme_da.c
717	You can't do this in the kernel. the stack is too small.
939	I think this should be after we set the other parameters.
sys/dev/nvme/nvme.c
281 ↗	(On Diff #98635)	No. Like I said before, we do not want to parse the NVME log pages in the clients. This change is wrong.
sys/dev/nvme/nvme_ctrlr.c
760	I think this is wrong on many levels.
sys/dev/nvme/nvme_sim.c
381	this is also wrong. It doesn't handle the multiple name spaces arriving or departing. It only handles the size change.

This revision now requires changes to proceed.Nov 17 2021, 4:04 PM

rebase as reviewer suggested.

Harbormaster completed remote builds in B43078: Diff 99279.Dec 1 2021, 8:23 AM

wanpengqian_gmail.com edited the summary of this revision. (Show Details)Dec 1 2021, 8:29 AM

wanpengqian_gmail.com edited the test plan for this revision. (Show Details)

wanpengqian_gmail.com added a parent revision: D32953: bhyve nvme: Inform guests of namespace resize.

fix unrelated change.

Harbormaster completed remote builds in B43079: Diff 99280.Dec 1 2021, 8:35 AM

wanpengqian_gmail.com retitled this revision from nvme: nda call disk_resize() when namespace changed async event happened to nvme: nvd/nda call disk_resize() when namespace changed async event happened.Dec 1 2021, 8:38 AM

wanpengqian_gmail.com edited the summary of this revision. (Show Details)

wanpengqian_gmail.com marked an inline comment as done.Dec 2 2021, 1:36 AM

wanpengqian_gmail.com added inline comments.

sys/cam/nvme/nvme_da.c
717	OK, remove this output.
939	Yes, I did not match the original sequence.
sys/dev/nvme/nvme_ctrlr.c
760	since notify_ns will query the namespace, that will cause kernel panic. here we post a query request to queue. other thread will query and construct_ns and call notify_ns after that.
sys/dev/nvme/nvme_sim.c
381	I will not use async callback this time.

after NVME_PROBE_IDENTIFY_NS is done, fire a AC_GETDEV_CHANGED async event.

Harbormaster completed remote builds in B43106: Diff 99354.Dec 2 2021, 1:13 PM

wanpengqian_gmail.com edited the summary of this revision. (Show Details)Dec 2 2021, 1:15 PM

wanpengqian_gmail.com edited the test plan for this revision. (Show Details)

wanpengqian_gmail.com planned changes to this revision.Aug 26 2022, 12:07 AM

I tested with 14.0-CURRENT Oct 10 2022, and it works. Is it the right direction for dynamically changing the disk size after hosts resize the disk?

Ah, I missed the update on this review. It's largely the right direction and I'm sorry I missed it...
I've since updated different parts of the code, so this isn't quite right... But the glue to the bulk of this code is change, the bulk of this code hasn't changed, so I'll commit this to the tree shortly.

@imp I'm not very familiar with the CAM subsystem, so it would be great if you could make the necessary adjustments and commit this to current. Thank you very much.

This revision was not accepted when it landed; it landed in state Changes Planned.Tue, Nov 18, 9:05 PM

Closed by commit rG20e94950c54e: nvme: Notify namespace changes better (authored by wanpengqian_gmail.com, committed by imp). · Explain Why

This revision was automatically updated to reflect the committed changes.

imp added a commit: rG20e94950c54e: nvme: Notify namespace changes better.

imp added a commit: rGdffd882d12d2: nvme: Refactor geom setting to function..

imp added a commit: rG86d3ec359a56: nda: React to namespace change events.

imp added a commit: rGbd769e73d8f1: nvd: handle namespace changes.

imp added a commit: rG4640f5008922: nvme_sim: signal namespace depature.

imp added a commit: rGfe6ee00d174e: nvme: Notify namespace changes better.Wed, Nov 19, 4:13 AM

imp added a commit: rG31412fda9fa7: nvme: Refactor geom setting to function..

imp added a commit: rG9a465b37ea17: nda: React to namespace change events.

imp added a commit: rG96a7ed1e7fb4: nvd: handle namespace changes.