When biofinish calls g_io_deliver with an error of ENOMEM, that kicks
off the slowdown protocol, forcing I/O to go through g_down rather than
be directly dispatch. One of the side effects is that the I/O is
resubmitted, so the start routines get called recursively, leading to a
recursive lock panic. Rather than make the periph lock recursive, drop
and reacquire the lock around such calls to biofinish.
For nda, this happens only when we can't allocate space to construct a
TRIM. For ada and da, this is only for certain ZONE operations.
Sponsored by: Netflix
Reviewed by: gallatin
Differential Revision: https://reviews.freebsd.org/D45310
NCQ TRIM for Samsung 860/870 SSDs results in data corruption on systems
with some SATA controllers.
This can be easily reproduced using ZFS which uses TRIM and is able to
detect block content changes.
Linux bug report for this issue:
https://bugzilla.kernel.org/show_bug.cgi?id=201693
Since at present we can not limit a quirk based on the contorller / SIM,
apply the quirk in all cases.
Reviewed by: imp
MFC after: 2 weeks
Differential Revision: https://reviews.freebsd.org/D43961
sys/cam/cam.h includes opt_cam.h, so none of the clients need to do
this. cam.h does all the right dancing to conditionally include
opt_cam.h only when it makes sense. It generally only matters when
cam_debug.h is included (it must be included before that). Many of the
stray opt_cam.h includes were after cam_debug.h which would be a problem
were it not included in cam/cam.h. The other users of CAM options that
aren't debug all already include cam/cam.h.
Also trim unneeded sys/cdefs.h files from the files touched.
Sponsored by: Netflix
Prefer UNMAPPEDIO and ROTATING from flags sysctl. See
1. aeab0812e6 (Add flags sysctl to ada)
2. cf3ff63e55 (Convert unmappedio over to a flag)
3. 96eb32bf0f (Convert rotating to a flag bit)
Reviewed by: imp, ken, #cam
MFC after: immediately (we want this in 14.0)
Differential Revision: https://reviews.freebsd.org/D42402
The SPDX folks have obsoleted the BSD-2-Clause-FreeBSD identifier. Catch
up to that fact and revert to their recommended match of BSD-2-Clause.
Discussed with: pfg
MFC After: 3 days
Sponsored by: Netflix
ccb_h.status has two parts: the actual status and some addition bits to
indicate additional information. It must be masked before comparing
against completion codes. Add new inline function cam_ccb_success to
simplify this to test whether or not the request succeeded. Most of the
code already does this, but a few places don't (the rest likely should
be converted to use cam_ccb_status and/or cam_ccb_success, but that's
for another day). This caused at least one bug in recognizing devices
behind a SATA port multiplexer, though some of these checks were
fine with the special knowledge of the code paths involved.
PR: 270459
Sponsored by: Netflix
MFC After: 1 week (and maybe a EN requst)
Reviewed by: ken, mav
Differential Revision: https://reviews.freebsd.org/D39572
Before this patch CAM periph drivers called both disk_alloc() and
disk_create() same time on periph creation. But then prevented disks
from opening until the periph probe completion with cam_periph_hold().
As result, especially if disk misbehaves during the probe, GEOM event
thread, triggered to taste the disk, got blocked on open attempt,
potentially for a long time, unable to process other events.
This patch moves disk_create() call from periph creation to the end of
the probe. To allow disk_create() calls from non-sleepable CAM contexts
some of its duties requiring memory allocations are moved either back
to disk_alloc() or forward to g_disk_create(), so now disk_alloc() and
disk_add_alias() are the only disk methods that require sleeping. If
disk fails during the probe disk_create() may just be skipped, going
directly to disk_destroy(). Other method calls during that time are
just ignored. Since GEOM may now see the disks after CAM bus scan is
already completed, introduce per-periph boot hold functions. Enclosure
driver already had such mechanism, so just generalize it.
Reviewed by: imp
MFC after: 1 month
Sponsored by: iXsystems, Inc.
Differential Revision: https://reviews.freebsd.org/D35784
The physical address argument is essentially ignored by every dumper
method. In addition, the dump routines don't actually pass a real
address; every call to dump_append() passes a value of zero for
physical.
Reviewed by: markj
MFC after: 2 weeks
Differential Revision: https://reviews.freebsd.org/D35173
The AHCI and ATA SIMs will return CAM_SEL_TIMEOUT when an underlying
device has stopped responding. This is usually seen after a timeouted
out command and can be a transient event. Rather than fail the
peripheral immediately after seeing this, queue a retry. For transient
events, this allows drives to continue to provide data, though with some
added latency, just like we do when we have some other kind of retriable
error. If the error isn't transient (the drive is truly gone), then
we'll discover that eventually and fail the transaction and invalidate
the drive like we do today.
This helps us avoid a panic at the end of camperiphfree when
CAM_PERIPH_NEW_DEV_FOUND is set. However, the deferred callback should
be queued to xpt_async_td instead of being made inline there. This issue
will be solved in a different patch that does that. PR 263703.
This also helps us avoid another bug where we can drop all references to
the device (causing us to go through camperiphfree and destroy the path)
while we have an I/O pending in the ata_da state machine (usually in
state ADA_STATE_RAHEAD with ATA_SETFEATURES ATA_SF_ENAB_RCACHE
command). It's not clear why the reference that we take out to do the
reprobe isn't effective at blocking this. By retrying this condition,
though we avoid this bug (at least more often, I don't have a good
reproduction test case, I just see this panic a few times a month at
work on systems that have transient disk errors on ahci connected SATA
SSDs). PR 263704. It's too soon to know how much this helps us avoid
this bug.
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D34977
We should call cam_periph_async() always, like SCSI does. This routine
is supposed to be more of a catch-all.
cam_periph_async() only does actions for AC_LOST_DEVICE. It ignores all
other events (today), but this may not always be true. So this is a nop
change.
Drop in a 'break' so we don't fall through unnecessarily.
Sponsored by: Netflix
Reviewed by: mav
Differential Revision: https://reviews.freebsd.org/D35057
We never use the cgd that we get from the XPT_GDEV_TYPE call. Prior to
9a6844d55f we used it to determine if READ AHEAD or WRITE CACHING was
supported. However, all that information was moved into adasetflags so
we no longer need to this since it's cached in the softc and updated
with the IDENTIFY data changes automatically.
Sponsored by: Netflix
Reviewed by: mav
Differential Revision: https://reviews.freebsd.org/D35039
Use a switch rather than a nested if to simplify the async event
processing code. No functional changes intended.
Sponsored by: Netflix
Reviewed by: mav
Differential Revision: https://reviews.freebsd.org/D35038
Since both scsi_xpt and ata_xpt use the same name for the softc, this
can lead to problems in gdb. Avoid the issue by renaming the ata
probe_softc to aprobe_softc as has been done for the aprobe in
0f280cbd0a. This was overlooked at the time.
Sponsored by: Netflix
MFC After: 2 weeks
Those can be returned by CHECK POWER MODE command (0xe5).
Note that some of the definitions duplicate definitions for Extended
Power Conditions.
MFC after: 2 weeks
Differential Revision: https://reviews.freebsd.org/D33646
On large systems even relatively rare callouts may fire many times
per second. This should allow them to aggregate better, since we do
not require any precision when polling for media change, etc.
MFC after: 2 weeks
If a disk is already in STANDBY mode, then setting IDLE mode can
actually spin it up.
Reviewed by: mav
MFC after: 4 weeks
Differential Revision: https://reviews.freebsd.org/D33588
In the ATA/ATAPI spec these are space-padded fixed-length strings with
no NUL-terminator (and byte swapped). When performing the identify we
call ata_param_fixup to swap the bytes back to be in order, strip any
leading/trailing spaces and coalesce consecutive spaces, padding with
NULs. However, if the input has no padding spaces, the fixed-up strings
are still not NUL-terminated. This causes two issues. The first is that
strlcpy will truncate the string by replacing the final byte with a NUL.
The second is that strlcpy will keep reading src until it finds a NUL in
order to calculate the return value, which is defined as the length of
src (so that callers can then compare it with the dsize input to see if
the input string was truncated), thereby reading past the end of the
buffer and into whatever adjacent fields are in the structure. In
practice there's a NUL byte somewhere in the structure, but on CHERI
with subobject bounds enabled in the compiler this overread will be
detected and trap as a bounds violation.
Note this matches ata_xpt's aprobedone, which does a bcopy to a
malloc'ed buffer and manually NUL-terminates it for the CAM path's
device's serial_num.
Found by: CHERI
Reviewed by: imp, scottl
Differential Revision: https://reviews.freebsd.org/D32567
This turns debugging printf() into a KASSERT().
It's for ATA for now; SCSI will came later.
Reviewed By: imp
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D31380
In ata_dev_advinfo() and nvme_dev_advinfo(), if the physical path is
being stored and there is a malloc failure (malloc(9) is called with
M_NOWAIT), we could wind up in a situation where the device's
physpath_len is set to the length the user provided, but the physpath
itself is NULL.
If another context then comes in to fetch the physical path value, we
would wind up trying to memcpy a NULL pointer into the caller's buffer.
So, set the physpath_len to 0 when we free the physpath on entry into
the store case for the physical path. Reset the length to a non-zero
value only after we've successfully malloced a buffer to hold it.
This code mirrors scsi_xpt.c does already as well.
Signed-off-by: Young Xiao <92siuyang@gmail.com>
Reviewed by: imp
PR: 238014
This makes the ada(4) driver use UMA for its CCBs. While it's
da(4) counterpart needs some more testing, this one seems to be
safe now.
Please let me know via email if you notice any suspicious kernel
messages,
Reviewed By: imp
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D30567
It looks like I've missed a couple of places where we don't clear
stack-allocated CCBs. Don't panic when that happens, just print
a warning.
This is a temporary measure until I get those cases fixed.
Reviewed By: markj
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D30296
This patch makes it possible for CAM to use small CCBs allocated
from an periph-specific UMA zone instead of the usual, huge ones.
The end result is that CCBs issued via da(4) take 544B (size of
ccb_scsiio) instead of the usual 2kB (size of 'union ccb', ~1.5kB,
rounded up by malloc(9)). For ATA it's 272B. We waste less
memory, we avoid zeroing the unused 1kB, and it should be easier
to allocate those CCBs in low memory conditions. It should also
be possible to use uma_zone_reserve(9) to improve behaviour
in low memory conditions even further.
Note that this does not change the size, or the layout, of CCBs
as such. CCBs get allocated in various different ways, in particular
on the stack, and I don't want to redo all that. Instead, this
provides an opt-in mechanism for the periph to declare "my start()
callback is fine with receiving a CCB allocated from this UMA zone".
In other words, most of the code works exactly as it used to; the
change only happens to IOs issued by xpt_run_allockq(), which
is - conveniently - pretty much all that matters for performance.
The reason for doing it this way is that it's pretty small, localized
change, and can be implemented gradually and iteratively: take a
periph, make sure its start() callback only casts the CCBs it takes
to a particular type of CCB, for example ccb_scsiio, and that it only
casts CCBs returned by cam_periph_getccb() to that type, then add UMA
zone for that size, and declare it safe to XPT.
This is disabled by default. Set 'kern.cam.ada.enable_uma_ccbs=1'
and 'kern.cam.da.enable_uma_ccbs=1' tunables to enable it. Testing
is welcome; I will flip the default to enable in two weeks from now.
Reviewed By: imp
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D28674
This is my second pass, this time over all of CAM except
for the SCSI target bits. There should be no functional
changes.
Reviewed By: imp
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D29549
If a disk's SIM doesn't support polling, then it can't be used to
store crashdumps. Leave d_dump NULL in that case so that dumpon(8)
fails gracefully rather than having dumps fail at crash time.
Reviewed by: scottl, mav, imp
MFC after: 2 weeks
Sponsored by: Chelsio
Differential Revision: https://reviews.freebsd.org/D28454
Replace MAXPHYS by runtime variable maxphys. It is initialized from
MAXPHYS by default, but can be also adjusted with the tunable kern.maxphys.
Make b_pages[] array in struct buf flexible. Size b_pages[] for buffer
cache buffers exactly to atop(maxbcachebuf) (currently it is sized to
atop(MAXPHYS)), and b_pages[] for pbufs is sized to atop(maxphys) + 1.
The +1 for pbufs allow several pbuf consumers, among them vmapbuf(),
to use unaligned buffers still sized to maxphys, esp. when such
buffers come from userspace (*). Overall, we save significant amount
of otherwise wasted memory in b_pages[] for buffer cache buffers,
while bumping MAXPHYS to desired high value.
Eliminate all direct uses of the MAXPHYS constant in kernel and driver
sources, except a place which initialize maxphys. Some random (and
arguably weird) uses of MAXPHYS, e.g. in linuxolator, are converted
straight. Some drivers, which use MAXPHYS to size embeded structures,
get private MAXPHYS-like constant; their convertion is out of scope
for this work.
Changes to cam/, dev/ahci, dev/ata, dev/mpr, dev/mpt, dev/mvs,
dev/siis, where either submitted by, or based on changes by mav.
Suggested by: mav (*)
Reviewed by: imp, mav, imp, mckusick, scottl (intermediate versions)
Tested by: pho
Sponsored by: The FreeBSD Foundation
Differential revision: https://reviews.freebsd.org/D27225
Often, in traiging core files, one only has a traceback of where a
panic occurred. We have probe* and xpt* routines that live in both the
scsi and ata layers with identical names. To make one or the other
stand out, prefix all the probe and xpt routines in ata with an
'a'. I've left the scsi ones alone since they were there first and are
more numerous. I also rejected using #define to do this as being too
confusing. I chose this method because the CAM name for the probe
device was already 'aprobe'.
Normally, this doesn't matter because file scope protects one from
interfering with the other. However, due to the indirect nature of
CAM's state machine, you don't know if the following traceback is
SCSI or ATA:
xpt_done
probedone
xpt_done_process
xpt_done_td
fork_exit
nvme and mmc already have unique names.
MFC: 1 week
Differential revision: https://reviews.freebsd.org/D24825
