Page MenuHomeFreeBSD

mpr: fix freeze / release mismatch in timeout code
ClosedPublic

Authored by imp on Nov 18 2021, 6:53 PM.
Tags
None
Referenced Files
F81932730: D33054.diff
Tue, Apr 23, 9:52 AM
Unknown Object (File)
Mar 7 2024, 7:00 PM
Unknown Object (File)
Feb 1 2024, 8:46 AM
Unknown Object (File)
Dec 20 2023, 6:29 AM
Unknown Object (File)
Dec 18 2023, 8:57 PM
Unknown Object (File)
Dec 2 2023, 2:47 PM
Unknown Object (File)
Oct 4 2023, 5:02 AM
Unknown Object (File)
Sep 22 2023, 11:54 PM

Details

Summary

So, if we're processing a timeout, and we've sent an ABORT to the firmware
for that timeout, but not yet received the response from the firmware, AND
we get another timeout, we queue the timeout and freeze the queue. However,
when we've finally processed them all, we only release the queue once. This
causes all I/O to halt as the devq remains frozen forever.

Instead, only freeze the queue when we start the process (eg set INRESET
on the target). This will allow the release when all the timed out I/Os have
finished ABORTing.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

imp requested review of this revision.Nov 18 2021, 6:53 PM
imp retitled this revision from better freeze / release to mpr: fix freeze / release mismatch in timeout code.Nov 18 2021, 7:10 PM
imp edited the summary of this revision. (Show Details)
imp added reviewers: mav, scottl, ken.

I believe that this fixes 9781c28c6d63cfa8438d1aa31f512a6b217a6b2b because that's where this xpt_freeze_devq was introduced.

Warner, would you please point me where exactly do you see an asymmetry? As I see, the queue is frozen for every task management request sent to device inside mprsas_prepare_for_tm() and should be released for each one inside mprsas_free_tm(). Is there case when those two call don't match?

PS: And I don't like pulling debug messages into the level enabled by default.

Ah. I think I see. mprsas_logical_unit_reset_complete() and mprsas_abort_complete() reused the tm by calling mprsas_send_abort() another time for another timed out command without freeing tm. Your patch may have sense.

This revision is now accepted and ready to land.Nov 20 2021, 12:24 AM

Yes. I think you have it.

schedule cm1 and cm2 at the same time.
timeout cm1
send abort cm1
devq freeze
timeout cm2 (put on queue)
abort cm1 finishes
send abort cm2
devq freeze
abort cm2 finishes
devq release

is the sequence I captured in the wild. Sometimes with as many as 4 cm at the same time.

If that's the sequence you see, then all we need to do is chat about the xinfo -> info changes.
I'd like all the freeze / release messages to be at least under recovery, but I could do xinfo | recovery instead.
that helps trace out what happens here.
Comments on that movement?