kern.cam.do_dynamic_iosched is really a bool, so change its type to
bool. While I'm here, also use the CTLFLAG_TUN flag instead of a
separate tunable line for it and kern.cam.iosched_alpha_bits.
MFC After: 1 week
Sponsored by: Netflix
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
If MMC_SIM_CAM_REQUEST() is successful the ccb could be running or being
completed as the method returns. Modifying the ccb status could override
whatever status was already set by a MMC driver.
I am not sure what was the purpose of setting the status to CAM_REQ_INVALID
in the success path. I assume that it was to catch a possibility that the
ccb could be completed without its status explicitly set. So, I am keeping
the code, it's just moved to before the MMC_SIM_CAM_REQUEST call.
Without this change I was getting random and phantom EIO errors on Rock64
running off an SD card (dwmmc driver) plus occasional panics like:
Memory modified after free 0xffffa00003985800(2040) val=6 @ 0xffffa00003985854
panic: Most recently used by CAM CCB
MFC after: 1 week
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
No functional changes. Do not MFC this, it changes kernel ABI.
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D30698
Before UMA CCBs, all CCBs were of the same size, and could
be trivially copied using bcopy(9). Now we have to preserve
alloc_flags, otherwise we might end up attempting to free
stack-allocated CCB to UMA; we also need to take CCB size
into account.
This fixes kernel panic which would occur when trying to access
a stopped (as in, SCSI START STOP, also "ctladm stop") SCSI device.
Reported By: Gary Jennejohn <gljennjohn@gmail.com>
Tested By: Gary Jennejohn <gljennjohn@gmail.com>
Reviewed By: imp
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D31054
Fix forgotten argument and type error. MMCCAM isn't enabled by default,
and I'd mistakenly thought it was, so these went undetected precommit.
Sponsored by: Netflix
Its use was removed in 227d67aa54 by mav when locking was revamped.
Reviewed by: scottl@, mav@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30890
xpt_clone_path originally returned a cam_status, but it doesn't do I/O
and should return an errno instead. I added it last year and it's only
used in one place. It's not yet documented, so no doc changes are
nneeded.
Reviewed by: scottl@, mav@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30884
These were in the original CAM commit in 3.0, but were not used there,
nor have they been used since then. They also duplicate the now-standard
bool type. Remove them.
Reviewed by: scottl@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30879
xpt_bus_register and xpt_bus_deregister returns a hybrid error that's
neither a cam_status, nor an errno, but a mix of both. Update
xpt_bus_register and xpt_bus_deregister to return an errno. The vast
majority of current users compare against zero, which can also be
spelled CAM_SUCCESS. Nobody uses CAM_FAILURE, so remove that symbol
to prevent comfusion (nothing returns it either).
Where the return value is saved, ensure that the variable 'error' is
used to store an errno and 'status' is used to store a cam_status where
it makes the code clearer (usually just in functions that already mix
and match). Where the return value isn't used at all, avoid storing it
at all.
Reviewed by: scottl@, mav@ (earlier version)
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30860
cam_sim_alloc_dev was only used internally by the MMC system. That has
been convered to using xpt_path_device() and has stopped using this
interface, so this can be retired.
Reviewed by: scottl@, mav@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30858
Use the vanilla flavor of cam_sim_alloc. Now that sdiob has been
converted to get the device_t from the cam_path, this data is no longer
necessary.
Reviewed by: scottl@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30856
Return the device associated with the sim's bus when it called
xpt_bus_register, if any. Most real SIMs in the tree set this device,
but some virtual ones do not have a device_t assocaited with them.
Reviewed by: scottl@, mav@ (earlier version)
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30853
The trick of subtracting one from the poitner returned from malloc
results in undefined behavior:
>>C89: 3.3.6 Unless both the pointer operand and the result point to a
>>member of the same array object, or one past the last member of the
>>array object, the behavior is undefined.
Instead, allocate 1 extra element and stop adjusting the pointer. While
a little wasteful, the extra is in the noise on today's systems.
Reviewed by: scottl@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30847
When the elapsed time of the operation is complete and stored in the QOS
field, set the CAM_QOS_VALID bit. In iosched, test to make sure it's
set before using it.
Sponsored by: Netflix
Since periph_runccb now handles all the polling stuff, and
xpt_polled_action is now unused and can be removed.
Sponsored by: Netflix
Reviewed by: mav@
Differential Revision: https://reviews.freebsd.org/D30394
Nothing is using the sim callout to unfreeze the queue. Remove it to
simplify the SIM. This was introduced in the original CAM commit in 1998
but setting the CAM_SIM_REL_TIMEOUT_PENDING flag was removed in 1999 in
commit 87cfaf0e1fbd which reworked how bus reset worked. That work was
merged just after 3.2R was released. Remove the unused residuals.
Sponsored by: Netflix
Reviewed by: scottl@, mav@
Differential Revision: https://reviews.freebsd.org/D30383
For the discovery phase of SD/eMMC we need to do some transaction in a async
way.
The classic CAM XPT_{GET,SET}_TRAN_SETTING cannot be used in a async way.
This also allow us to split the discovery phase into a more complete state
machine and we don't mtx_sleep with a random number to wait for completion
of the tasks.
For mmc_sim we now do the SET_TRAN_SETTING in a taskqueue so we can call
the needed function for regulators/clocks without the cam lock(s). This part is
still needed to be done for sdhci.
We also now save the host OCR in the discovery phase as it wasn't done before and
only worked because the same ccb was reused.
Reviewed by: imp, kibab, bz
Differential Revision: https://reviews.freebsd.org/D30038
This fixes a few bugs in iSCSI backends where the backends were using
the limits they advertised initially during the login phase as the
final values instead of the values negotiated with the other end.
Reported by: Jithesh Arakkan @ Chelsio
Reviewed by: mav
Differential Revision: https://reviews.freebsd.org/D30271
cxgbei stores state about a target transfer in the ctl_private[] array
of a ctl_io that is freed when a target transfer (represented by the
cdw) is freed. As such, freeing a ctl_io before a cdw that references
it can result in a use after free in cxgbei. Two of the four places
freed the cdw first, and the other two freed the ctl_io first. Fix
the latter two places to free the cdw first.
Reported by: Jithesh Arakkan @ Chelsio
Reviewed by: mav
Differential Revision: https://reviews.freebsd.org/D30270
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
Previously, daregister() could have been called before dainit()
initialized the UMA zone. This would trip a KASSERT.
Reported By: pho
Tested By: pho
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
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
Add description for what each of the parameters are to the cam_sim_alloc
call. Add some additional context for the mtx and queue parameters to
explain what special values passed in mean.
MFC After: 3 days
Reviewed by: mav@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30115
From my understanding this could happen with iSCSI LUNs with
unusually long names. The bug would make CAM fail to retrieve
the full inquiry data. Instead of bumping the size of the local
variable, just use a macro.
Reviewed By: imp, mav
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
X-NetApp-PR: #50
Differential Revision: https://reviews.freebsd.org/D29991
This adds a generic sim that abstract a lot of what needs to be implemented
in a driver for mmccam support.
A new interface with three methods is added :
- mmc_sim_get_tran_settings: Use to get what the controller supports in term
of capabilities, freq etc ...
- mmc_sim_set_tran_settings: Use to change the speed/freq/etc of the
sdcard host controller
- mmc_sim_cam_request: Used for MMCIO requests
Differential Revision: https://reviews.freebsd.org/D27485
Reviewed by: kibab
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
With new ZFS prefetcher improvements it is no longer needed to fully
serialize reads to reach decent prediction hit rate. Softer variant
only creates small time window to reduce races instead of completely
blocking following reads while previous is running. It much less
hurts the performance in case of prediction miss.
MFC after: 1 month
This is required for small CCBs support, where we need to track
whether the CCB was allocated from an UMA zone or not. There are
no (intended) functional changes with the current source.
Reviewed By: imp
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D29484
Queue all XPT_ASYNC ccb's and run those in a new cam async thread. This thread
is allowed to sleep for things like memory. This should allow us to make all the
registration routines for cam periph drivers simpler since they can assume they
can always allocate memory. This is a separate thread so that any I/O that's
completed in xpt_done_td isn't held up.
This should fix the panics for WAITOK alloations that are elsewhere in the
storage stack that aren't so easy to convert to NOWAIT. Additional future work
will convert other allocations in the registration path to WAITOK should
detailed analysis show it to be safe.
Reviewed by: chs@, rpokala@
Differential Revision: https://reviews.freebsd.org/D29210
Return while there are any I/Os in a queue may result in them stuck
indefinitely, since there is only one taskqueue task for all of them.
I think I've reproduced this by switching ha_role to secondary under
heavy load.
MFC after: 3 days
There were two definitions for the SCSI VPD Block Device Characteristics (page
0xb1): struct scsi_vpd_block_characteristics and struct
scsi_vpd_block_device_characteristics. The latter is more complete and more
widely used. Convert uses of the former to the latter by tweaking the da driver
and removing sturct scsi_vpd_block_characteristics.
- Move ctl_get_cmd_entry() calls from every OOA traversal to when
the requests first inserted, storing seridx in struct ctl_scsiio.
- Move some checks out of the loop in ctl_check_ooa().
- Replace checks for errors that can not happen with asserts.
- Transpose ctl_serialize_table, so that any OOA traversal accessed
only one row (cache line). Compact it from enum to uint8_t.
- Optimize static branch predictions in hottest places.
Due to O(n) nature on deep LUN queues this can be the hottest code
path in CTL, and additional 20% of IOPS I see in some 4KB I/O tests
are good to have in reserve. About 50% of CPU time here according
to the profiles is now spent in two memory accesses per traversed
request in OOA.
Sponsored by: iXsystems, Inc.
MFC after: 2 weeks
