Details

Reviewers

bcr
allanjude
markj

Group Reviewers

manpages

Commits

rS350471: gnop: Introduce requests delay.

Summary

Implement a delay of the request in the geom nop.
This allows to simulated disk that are responding slow to the IO requests.

Early version of code reviowed by: pjd

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25484
Build 24103: arc lint + arc unit

Event Timeline

oshogbo created this revision.Jul 24 2019, 4:30 AM

Harbormaster completed remote builds in B25484: Diff 60079.Jul 24 2019, 4:30 AM

oshogbo added reviewers: bcr, allanjude, markj.Jul 24 2019, 4:30 AM

Style fix.
Hide structure definition.

Harbormaster completed remote builds in B25485: Diff 60080.Jul 24 2019, 4:36 AM

Full patch ;/ Sorry about noise.

Harbormaster completed remote builds in B25486: Diff 60081.Jul 24 2019, 4:38 AM

Two easy fixes for the man page. Looks like a nice feature to have.
Thanks for implementing it.

lib/geom/nop/gnop.8
123	s/millisecond/milliseconds/
131	Leave "value" uppercased here.

Fixes sugested by bcr@.
Thanks!

Harbormaster completed remote builds in B25487: Diff 60082.Jul 24 2019, 6:55 AM

oshogbo marked 2 inline comments as done.Jul 24 2019, 6:55 AM

OK from manpages!

This revision is now accepted and ready to land.Jul 24 2019, 7:13 AM

markj added inline comments.Jul 24 2019, 2:03 PM

lib/geom/nop/gnop.8
47	Swap with wfailprob to preserve sorting.
58	Ditto.
123	Is the delay applied before or after the command is completed?
sys/geom/nop/g_nop.c
440	This can all be written in one line: `while ((dthis = TAILQ_FIRST(&sc->sc_head_delay)) != NULL) {`
476	Unrelated change.

Addres markj@ sugestions.

This revision now requires review to proceed.Jul 24 2019, 3:03 PM

Harbormaster completed remote builds in B25496: Diff 60093.Jul 24 2019, 3:03 PM

oshogbo added inline comments.Jul 24 2019, 3:03 PM

lib/geom/nop/gnop.8
47	It should be sorted by the variable name? Not by the option?
123	Before.

markj added inline comments.Jul 24 2019, 6:27 PM

lib/geom/nop/gnop.8
47	Oops, sorry.
123	I believe it should be specified in the documentation.

Add information when the request will be delayed.

Harbormaster completed remote builds in B25561: Diff 60229.Jul 29 2019, 9:58 AM

markj added inline comments.Jul 29 2019, 5:35 PM

lib/geom/nop/gnop.8
124	"Note that requests will be delayed before they are sent to the backing device."
sys/geom/nop/g_nop.c
264	Shouldn't we just pass the I/O through without delays in this case?
269	Why does this require Giant?
438	I think this loop should not be necessary: GEOM must not invoke providergone while I/O to the provider is still pending. Another way to think about it: suppose there are entries in the delay list. That means that gnop is still waiting for I/O requests to the lower layers to complete. But immediately after this queue is cleared, we free the gnop provider, so we cannot handle I/O completions. GEOM uses nstart and nend fields of the provider to track pending I/O, and doesn't call providergone while those numbers are not equal. And while there are entries pending in the delay queue, we must have nstart > nend.
sys/geom/nop/g_nop.h
94	Fix style since you're modifying this line anyway?

Address markj suggestions.

Harbormaster completed remote builds in B25580: Diff 60258.Jul 29 2019, 9:24 PM

oshogbo added inline comments.Jul 29 2019, 9:24 PM

sys/geom/nop/g_nop.c
264	Interesting question. This is an test suit so I don't think there is really reason to fail at this point.
269	I implemented this first and then ask pjd about that. TBH pjd told me that this lock is used only in case when the configuration is changed and when we are doing request so this should have any impact on this.
438	I was observing this behavior, but I wasn't sure what will happen when the provider will be orphaned (for example the memstick was removed).

markj added inline comments.Jul 29 2019, 10:36 PM

sys/geom/nop/g_nop.c
269	Sorry, I don't follow. The lock is acquired when the callout fires. We should avoid introducing new users of Giant and instead either use the softc lock, or do not specify a lock at all. (I think the latter is fine.)
438	Pending BIOs will bounce back through the provider with an error in that case. Regardless, this loop cannot be correct unless I am misunderstanding something. I believe you can simply assert that the queue is empty. If not, then there is some larger problem and this loop cannot be sufficient for handling it correctly, for the reason I stated above.