Page MenuHomeFreeBSD

mpi3mr: Fix EINPROGRESS errors hanging the card
ClosedPublic

Authored by imp on Nov 10 2023, 12:19 AM.
Tags
None
Referenced Files
F102698845: D42543.id130166.diff
Sat, Nov 16, 12:41 AM
F102698632: D42543.id130134.diff
Sat, Nov 16, 12:37 AM
F102698216: D42543.id130168.diff
Sat, Nov 16, 12:28 AM
F102695995: D42543.id130164.diff
Fri, Nov 15, 11:45 PM
F102695152: D42543.id130726.diff
Fri, Nov 15, 11:29 PM
F102687782: D42543.diff
Fri, Nov 15, 9:29 PM
Unknown Object (File)
Tue, Nov 12, 8:26 PM
Unknown Object (File)
Sun, Oct 20, 11:08 PM
Subscribers
None

Details

Summary

Move enqueueing of commands to bus_dmamap_load_ccb callback

Fix fundamental difference between FreeBSD and Linux. On Linux, your dma
load callback always happends before it returns, so drivers are written
to load the map, then submit to hardware. On FreeBSD, the callback may
be deferred and return EINPROGRESS. This means the callback is
responsible for queueing the request to the hardware is done after the
SGL list is created. Make a number of interrelated cahnages:

At the end of mpi3mr_prepare_sgls, add a call to mpi3mr_enqueue_request.

Split the hardware submission out from the end of mpi3mr_action_scsiio
and move it into a new routine mpi3mr_enqueue_request.

Move all error completion from the end of mpi3mr_action_scsiio to where
the error is detected. We cannot pass errors back from the
mpi3mr_enqueue_request to do this on a 'failed' mpi3mr in a centralized
place (since it has to be fire and forget).

Add comments about zero length SGLs never making it into
mpi3mr_prepare_sgls. Keep the code there for the moment, but we only set
cm->data to non-NULL when scsiio_req->DataLength is not zero. So the
datalength can't be zero and we can't send the zero SGLs.

Add commentts about other "impossible" tests in mpi3mr_prepare_sgls that
really should be simple asserts of some flavor.

Eliminate cm->error_code, since we can't pass data back from the
mpi3mr_prepare_sgl callback anymore.

In mpi3mr_map_request, call mpi3mr_enqueue_request for the no data case.
This seems to work even though we've not done the special zero length
handling that was in mpi3mr_prepare_sgls, giving further evidence to it
not actually being needed. This is needed for SCSI CDBs that have no
data to pass to the drive like TEST UNIT READY.

With this change, and the prior ones, we're now able to run with mpi3mr
on 128GB systems and very heavy disk load (so many buffers land > 4GB:
the driver instructs busdma to never use memory abouve 4GB, which may be
too conservative, but an issue for another time).

Sponsored by: Netflix

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 54443
Build 51333: arc lint + arc unit

Event Timeline

imp requested review of this revision.Nov 10 2023, 12:19 AM
imp created this revision.

All these changes are being tested on a Netflix workload.

sys/dev/mpi3mr/mpi3mr_cam.c
135

Here, below and proably in other places CAM_REQ_INVALID is wrong. It makes CAM to try different SCSI commands. Lets use CAM_REQ_CMP_ERR unless we know better.

148

This generally should not happen. busdma should ensure it. We may just assert it.

171

Lets not check for impossible. Lets either assert or remove.

171

nsegs > MPI3MR_SG_DEPTH is already checked above. How many times do we need it? IMO zero.

imp marked 4 inline comments as done.Nov 15 2023, 3:54 PM
imp added inline comments.
sys/dev/mpi3mr/mpi3mr_cam.c
135

Ah, I'd forgotten that detail... Yes, will fix. Double check that I caught all the ones you were worried about.

148

switched to asserts for this and other issues you raised.

imp marked 2 inline comments as done.

separate out trivial trailing whitespace fixes.

This is just doing bus_dma as intended. :)

This revision is now accepted and ready to land.Nov 15 2023, 4:23 PM
sys/dev/mpi3mr/mpi3mr_cam.c
150

Would be nice to print the value.

163–164

I guess getting here in that case and especially with nsegs > 0 asserted above is even more unlikely. Shall we just remove it?

better kassert
Add note that mav and I both think this if clause can be safely removed

This revision now requires review to proceed.Nov 15 2023, 9:33 PM

But I am still not getting sense of remaining scsiio_req->DataLength == 0 check.

This revision is now accepted and ready to land.Nov 15 2023, 9:47 PM

Only sleep if we've not had an exit request.

This revision now requires review to proceed.Nov 15 2023, 10:10 PM
sys/dev/mpi3mr/mpi3mr.c
3115 ↗(On Diff #130166)

I am not sure how this expression supposed to work, plus seems it got into wrong review.

remove stray commit I 'folded' to the wrong change (and somehow manage to mangle in the process)

sys/dev/mpi3mr/mpi3mr_cam.c
163–164

Driver calls mpi3mr_perpare_sgl() only when cm->data is non-NULL. And if cm->data is not NULL , then scsiio_req->DataLength can never be zero. So We can remove this check. Even this check is not needed in mpi3mr_map_request also.

This revision is now accepted and ready to land.Nov 21 2023, 5:19 PM
This revision was automatically updated to reflect the committed changes.