Page MenuHomeFreeBSD
Differential D15517

We can't release the refcount outside of the periph lock.
ClosedPublic
Actions

Authored by imp on May 21 2018, 11:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 28 2024, 10:03 PM
Unknown Object (File)
Dec 20 2023, 11:16 PM
Unknown Object (File)
Dec 20 2023, 3:48 AM
Unknown Object (File)
Dec 15 2023, 9:17 PM
Unknown Object (File)
Dec 9 2023, 7:09 AM
Unknown Object (File)
Nov 29 2023, 6:23 PM
Unknown Object (File)
Nov 16 2023, 11:57 PM
Unknown Object (File)
Oct 29 2023, 6:29 PM
View All 40 Files
Subscribers
kbowling
zeising

Details

Reviewers
mav
Commits
rS334166: We can't release the refcount outside of the periph lock.
Summary

We're dropping the periph lock then dropping the refcount. However,
that violates the locking protocol and is racy. This seems to be
the cause of weird occasional panics with a bogus assert.

Sponsored by: Netflix

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

imp created this revision.May 21 2018, 11:06 PM
Harbormaster completed remote builds in B16748: Diff 42821.May 21 2018, 11:06 PM
imp updated this revision to Diff 42822.May 21 2018, 11:09 PM

Fix missed part of the patch

Harbormaster completed remote builds in B16749: Diff 42822.May 21 2018, 11:09 PM
imp updated this revision to Diff 42823.May 21 2018, 11:11 PM

fix bogus mdastart -> ndastart

Harbormaster completed remote builds in B16750: Diff 42823.May 21 2018, 11:11 PM
imp updated this revision to Diff 42825.May 21 2018, 11:37 PM

Regroup

Harbormaster completed remote builds in B16751: Diff 42825.May 21 2018, 11:37 PM
kbowling added a subscriber: kbowling.May 22 2018, 11:38 AM
imp updated this revision to Diff 42851.May 22 2018, 5:34 PM

doh! compile with invariants

Harbormaster completed remote builds in B16761: Diff 42851.May 22 2018, 5:34 PM
imp updated this revision to Diff 42852.May 22 2018, 5:36 PM

doh! no, with invariants

Harbormaster completed remote builds in B16762: Diff 42852.May 22 2018, 5:36 PM
imp added a reviewer: mav.May 22 2018, 5:38 PM
imp mentioned this in D15453: Make reference counting more robust.May 22 2018, 5:41 PM
mav accepted this revision.EditedMay 22 2018, 6:11 PM

Looks good to me. The only rough edge I see is that outstanding_cmds is used only for the newly added assertion, that is why it was so broken before this and nobody cared.

This revision is now accepted and ready to land.May 22 2018, 6:11 PM
zeising added a subscriber: zeising.May 22 2018, 6:33 PM

Running fine for me. Reduced amount of panics, it looks like. Haven't run it for very long though.

imp added a comment.May 22 2018, 8:45 PM
In D15517#327757, @mav wrote:

Looks good to me. The only rough edge I see is that outstanding_cmds is used only for the newly added assertion, that is why it was so broken before this and nobody cared.

Yes. I think ada has similar issue with its oustanding_cmds. It's where I got it from :)

However, you're right: this is pure bug detection code. I thought about putting something like
#ifdef INVARIANTS
#define I(x) x
#else
#define I(x)
#endif

then adding I( ) around this stuff, but I decided against that for the moment...

zeising added a comment.May 24 2018, 8:26 AM

I've been running this patch since Tuseday evening without any issues. Without it I get a panic almost immediately when my disk is read, sometimes even before hitting multi user. Is it possible to get it committed, or what is the holdup?

mav added a comment.May 24 2018, 2:01 PM

As I have told, I have no objections. I still don't like the global counters, since they mean we can never remove respective lock or at least atomics, but for now I can live with that. It is better then broken code.

Closed by commit rS334166: We can't release the refcount outside of the periph lock. (authored by imp). · Explain WhyMay 24 2018, 4:31 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
head/
sys/
cam/
nvme/
11 lines

Diff 42934

View Options

head/sys/cam/nvme/nvme_da.c

Loading...