Page MenuHomeFreeBSD

Use real values to calculate Max I/O size instead of guessing.
ClosedPublic

Authored by slm on Jun 30 2016, 8:22 PM.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

slm updated this revision to Diff 18050.Jun 30 2016, 8:22 PM
slm retitled this revision from to Use real values to calculate Max I/O size instead of guessing..
slm updated this object.
slm edited the test plan for this revision. (Show Details)
slm added reviewers: ken, scottl, ambrisko, asomers.
slm set the repository for this revision to rS FreeBSD src repository.
ken accepted this revision.Jun 30 2016, 8:32 PM
ken edited edge metadata.

Great, thank you!

Can you also do the same for mpr(4)?

This revision is now accepted and ready to land.Jun 30 2016, 8:32 PM
slm added a comment.Jun 30 2016, 8:36 PM

Yes, I can do it for mpr too, but it will be a little different due to the changes with Chain Segment Size. I just need to look at it and do the right thing.

scottl edited edge metadata.Jul 1 2016, 4:07 AM

Looks good in principle. Would it be possible to have this value be overridable by a user tunable/sysctl?

scottl accepted this revision.Jul 1 2016, 4:07 AM
scottl edited edge metadata.
slm added a comment.Jul 1 2016, 6:45 PM

Scott, do you mean to make it larger than the calculated value? If larger, there is a danger that the I/O will fail because the number of chains in the I/O could exceed the MaxChainDepth of the controller. But, the calculated value is also based on the smallest SG element allowed for every SG element (PAGE_SIZE), so the max I/O can actually be much larger than the calculated max I/O. Maybe a warning message could be printed if the user's max I/O size is larger than the calculated size. Would that work?

scottl added a comment.Jul 1 2016, 7:08 PM

Smaller than the calculated value is what I meant. It's a convenient backstop against unexpected code, disk, and controller bugs. Just a simple cpi->maxio = min(sc->user_max_io, cpi->maxio);

slm added a comment.Jul 1 2016, 7:09 PM

OK. Sure, that sounds easy enough.

slm added a comment.Jul 1 2016, 7:12 PM

I could default the user value to 0, which would force the driver to use the calculated value. If not 0, then use the user value. Otherwise, we don't really know what to default the value to and we're back to using a guess. Sound OK?

scottl added a comment.Jul 1 2016, 7:22 PM

Sounds good

scottl added a comment.Jul 1 2016, 8:16 PM

Actually I would set the default user value to 0xffffffff, and then do a min() calculation with that and the calculated value. That way you never go above the calculated value, but you can go lower (maybe clip the lower value to something sensible, not allow absurdly low values like '1').

slm updated this revision to Diff 18155.Jul 5 2016, 6:44 PM
slm edited edge metadata.

Rewrote a little to use a new sysctl variable (max_io_pages). This is the max number of pages that a user wants to use per I/O. The driver will use the lesser of this new value and the calculated value from IOCFacts. The default for max_io_pages is -1, meaning the driver will use IOCFacts.

This revision now requires review to proceed.Jul 5 2016, 6:44 PM
scottl accepted this revision.Jul 5 2016, 7:45 PM
scottl edited edge metadata.
This revision is now accepted and ready to land.Jul 5 2016, 7:45 PM
slm updated this revision to Diff 18167.Jul 5 2016, 9:37 PM
slm edited edge metadata.

Adding mps.4 man page changes to the review.

This revision now requires review to proceed.Jul 5 2016, 9:37 PM
slm updated this revision to Diff 18195.Jul 6 2016, 6:27 PM
slm edited edge metadata.

Went ahead and added mpr changes for this to the same review. There are only slight differences between how the mps and mpr drivers handle the max I/O calculation.

ken accepted this revision.Jul 7 2016, 2:28 AM
ken edited edge metadata.

Looks good, thanks!

This revision is now accepted and ready to land.Jul 7 2016, 2:28 AM
This revision was automatically updated to reflect the committed changes.