Page MenuHomeFreeBSD

Implement gnop delay.
ClosedPublic

Authored by oshogbo on Jul 24 2019, 4:30 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 8:47 PM
Unknown Object (File)
Fri, Mar 22, 8:47 PM
Unknown Object (File)
Fri, Mar 22, 8:47 PM
Unknown Object (File)
Fri, Mar 22, 8:46 PM
Unknown Object (File)
Fri, Mar 22, 8:46 PM
Unknown Object (File)
Fri, Mar 22, 8:46 PM
Unknown Object (File)
Fri, Mar 22, 8:46 PM
Unknown Object (File)
Fri, Mar 22, 8:46 PM
Subscribers

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

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25601
Build 24201: arc lint + arc unit

Event Timeline

Style fix.
Hide structure definition.

Full patch ;/ Sorry about noise.

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/

133

Leave "value" uppercased here.

Fixes sugested by bcr@.
Thanks!

This revision is now accepted and ready to land.Jul 24 2019, 7:13 AM
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
446

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

474

Unrelated change.

oshogbo marked an inline comment as done.

Addres markj@ sugestions.

This revision now requires review to proceed.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.

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.

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
272

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

277

Why does this require Giant?

444

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

Fix style since you're modifying this line anyway?

oshogbo marked an inline comment as done.

Address markj suggestions.

sys/geom/nop/g_nop.c
272

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

277

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

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

sys/geom/nop/g_nop.c
277

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

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.

sys/geom/nop/g_nop.c
443

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

545

Why do you allow a delay probability of -1?

sys/geom/nop/g_nop.c
545

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

markj added inline comments.
sys/geom/nop/g_nop.c
545

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.