Page MenuHomeFreeBSD

Implement gnop delay.
ClosedPublic

Authored by oshogbo on Jul 24 2019, 4:30 AM.

Details

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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

oshogbo created this revision.Jul 24 2019, 4:30 AM
oshogbo updated this revision to Diff 60080.Jul 24 2019, 4:36 AM

Style fix.
Hide structure definition.

oshogbo updated this revision to Diff 60081.Jul 24 2019, 4:38 AM

Full patch ;/ Sorry about noise.

bcr added a comment.Jul 24 2019, 6:44 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 ↗(On Diff #60081)

s/millisecond/milliseconds/

131 ↗(On Diff #60081)

Leave "value" uppercased here.

oshogbo updated this revision to Diff 60082.Jul 24 2019, 6:55 AM

Fixes sugested by bcr@.
Thanks!

oshogbo marked 2 inline comments as done.Jul 24 2019, 6:55 AM
bcr accepted this revision.Jul 24 2019, 7:13 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 ↗(On Diff #60082)

Swap with wfailprob to preserve sorting.

58 ↗(On Diff #60082)

Ditto.

123 ↗(On Diff #60081)

Is the delay applied before or after the command is completed?

sys/geom/nop/g_nop.c
446 ↗(On Diff #60082)

This can all be written in one line: while ((dthis = TAILQ_FIRST(&sc->sc_head_delay)) != NULL) {

482 ↗(On Diff #60082)

Unrelated change.

oshogbo updated this revision to Diff 60093.Jul 24 2019, 3:03 PM
oshogbo marked an inline comment as done.

Addres markj@ sugestions.

This revision now requires review to proceed.Jul 24 2019, 3:03 PM
oshogbo added inline comments.Jul 24 2019, 3:03 PM
lib/geom/nop/gnop.8
47 ↗(On Diff #60082)

It should be sorted by the variable name? Not by the option?

123 ↗(On Diff #60081)

Before.

markj added inline comments.Jul 24 2019, 6:27 PM
lib/geom/nop/gnop.8
47 ↗(On Diff #60082)

Oops, sorry.

123 ↗(On Diff #60081)

I believe it should be specified in the documentation.

oshogbo updated this revision to Diff 60229.Jul 29 2019, 9:58 AM

Add information when the request will be delayed.

markj added inline comments.Jul 29 2019, 5:35 PM
lib/geom/nop/gnop.8
124 ↗(On Diff #60229)

"Note that requests will be delayed before they are sent to the backing device."

sys/geom/nop/g_nop.c
270 ↗(On Diff #60229)

Shouldn't we just pass the I/O through without delays in this case?

275 ↗(On Diff #60229)

Why does this require Giant?

444 ↗(On Diff #60229)

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

Fix style since you're modifying this line anyway?

oshogbo updated this revision to Diff 60258.Jul 29 2019, 9:24 PM
oshogbo marked an inline comment as done.

Address markj suggestions.

oshogbo added inline comments.Jul 29 2019, 9:24 PM
sys/geom/nop/g_nop.c
270 ↗(On Diff #60229)

Interesting question.
This is an test suit so I don't think there is really reason to fail at this point.

275 ↗(On Diff #60229)

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.

444 ↗(On Diff #60229)

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

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

444 ↗(On Diff #60229)

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.

oshogbo updated this revision to Diff 60300.Jul 30 2019, 6:20 PM

Fix markj@ suggestions.

markj added inline comments.Jul 30 2019, 6:34 PM
sys/geom/nop/g_nop.c
443 ↗(On Diff #60300)

Maybe qualify: "delayed request list is not empty".

545 ↗(On Diff #60300)

Why do you allow a delay probability of -1?

oshogbo added inline comments.Jul 30 2019, 6:36 PM
sys/geom/nop/g_nop.c
545 ↗(On Diff #60300)

It the same as wfailprob.
it means 'do not change'.
Is set when the user will not provide the options.

oshogbo updated this revision to Diff 60301.Jul 30 2019, 6:37 PM

Fix KASSERT text.

markj accepted this revision.Jul 30 2019, 7:01 PM
markj added inline comments.
sys/geom/nop/g_nop.c
545 ↗(On Diff #60300)

Ah, I see. It would be nice to use nvlists for GEOM configuration, so that absence of a key would indicate "do not change" instead.

This revision is now accepted and ready to land.Jul 30 2019, 7:01 PM
This revision was automatically updated to reflect the committed changes.