07 Feb, 2012
2 commits
-
Retval not very useful, and may even be harmful. Once submitted, fabrics
should expect a sense error if anything goes wrong. All fabrics checking
of this retval are useless or broken:fc checks it just to emit more debug output.
ib_srpt trickles retval up, then it is ignored.
qla2xxx trickles it up, which then causes a bug because the abort goto
in qla_target.c thinks cmd hasn't been sent to target.Just returning nothing is best.
Signed-off-by: Andy Grover
Signed-off-by: Nicholas Bellinger -
WindowsXP+BOT issues a MODE_SENSE request with page 0x1c which is not
suppoerted by target. Target rejects that command with
TCM_INVALID_CDB_FIELD, so far so good. On BOT I can't send the SENSE
response back, instead I can only reply that an error occured. The next
thing happens is a REQUEST_SENSE request with 18 bytes length. Since the
check here is more than 18 bytes I have to NACK that request as well.
This is not really required: We check for some additional room, but we
never use it. The additional length is set to 0xa so the total length is
0xa + 8 = 18 which is fine with my 18 bytes.Signed-off-by: Sebastian Andrzej Siewior
Signed-off-by: Nicholas Bellinger
18 Jan, 2012
17 commits
-
My draft of SPC-4 says:
If the PAGE CODE field is not set to zero when the EVPD bit is set
to zero, the command shall be terminated with CHECK CONDITION
status, with the sense key set to ILLEGAL REQUEST, and the
additional sense code set to INVALID FIELD IN CDB.Signed-off-by: Roland Dreier
Cc:
Signed-off-by: Nicholas Bellinger -
My draft of SPC-4 says:
If the device server does not implement the requested vital product
data page, then the command shall be terminated with CHECK CONDITION
status, with the sense key set to ILLEGAL REQUEST, and the
additional sense code set to INVALID FIELD IN CDB.Signed-off-by: Roland Dreier
Cc:
Signed-off-by: Nicholas Bellinger -
This patch addresses a bug with sendtargets discovery where INADDR_ANY (0.0.0.0)
+ IN6ADDR_ANY_INIT ([0:0:0:0:0:0:0:0]) network portals where incorrectly being
reported back to initiators instead of the address of the connecting interface.
To address this, save local socket ->getname() output during iscsi login setup,
and makes iscsit_build_sendtargets_response() return these TargetAddress keys
when INADDR_ANY or IN6ADDR_ANY_INIT portals are in use.Reported-by: Dax Kelson
Reported-by: Andy Grover
Cc: David S. Miller
Cc:
Signed-off-by: Nicholas Bellinger -
We need to handle >1 page control cdbs, so extend the code to do a vmap
if bigger than 1 page. It seems like kmap() is still preferable if just
a page, fewer TLB shootdowns(?), so keep using that when possible.Rename function pair for their new scope.
Signed-off-by: Andy Grover
Cc:
Signed-off-by: Nicholas Bellinger -
A statement such as
struct iscsi_node_attrib *na = na = iscsit_tpg_get_node_attrib(sess);
has undefined behaviour since there are two assignments to 'na', strictly
speaking (the order in which side-effects from the assignments take place
is undefined since there's no intervening sequence point), and it looks
unintentional in any case.Signed-off-by: Jesper Juhl
Signed-off-by: Nicholas Bellinger -
Signed bitfields are a problem because instead of being 1 or 0 like
you'd expect they are 0 and -1. It doesn't cause a problem in this case
but sparse complains:drivers/target/iscsi/iscsi_target_core.h:564:56: error: dubious one-bit
signed bitfieldSigned-off-by: Dan Carpenter
Signed-off-by: Nicholas Bellinger -
This patch fixes a bug where the iscsit_add_reject_from_cmd() call
from a failure to iscsit_alloc_buffs() was incorrectly passing
add_to_conn=1 and causing a double list_add after iscsi_cmd->i_list
had already been added in iscsit_handle_scsi_cmd().Cc:
Signed-off-by: Nicholas Bellinger -
This patch addresses a bug where iscsit_free_cmd() was incorrectly calling
iscsit_release_cmd() for ISCSI_OP_REJECT because iscsi_add_reject*() will
overwrite the original iscsi_cmd->iscsi_opcode assignment. This bug was
introduced with the following commit:commit 0be67f2ed8f577d2c72d917928394c5885fa9134
Author: Nicholas Bellinger
Date: Sun Oct 9 01:48:14 2011 -0700iscsi-target: Remove SCF_SE_LUN_CMD flag abuses
and was manifesting itself as list corruption with the following:
[ 131.191092] ------------[ cut here ]------------
[ 131.191092] WARNING: at lib/list_debug.c:53 __list_del_entry+0x8d/0x98()
[ 131.191092] Hardware name: VMware Virtual Platform
[ 131.191092] list_del corruption. prev->next should be ffff880022d3c100, but was 6b6b6b6b6b6b6b6b
[ 131.191092] Modules linked in: tcm_vhost ib_srpt ib_cm ib_sa ib_mad ib_core tcm_qla2xxx qla2xxx tcm_loop tcm_fc libfc scsi_transport_fc crc32c iscsi_target_mod target_core_stgt scsi_tgt target_core_pscsi target_core_file target_core_iblock target_core_mod configfs ipv6 iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi sr_mod cdrom sd_mod e1000 ata_piix libata mptspi mptscsih mptbase [last unloaded: scsi_wait_scan]
[ 131.191092] Pid: 2250, comm: iscsi_ttx Tainted: G W 3.2.0-rc4+ #42
[ 131.191092] Call Trace:
[ 131.191092] [] warn_slowpath_common+0x80/0x98
[ 131.191092] [] warn_slowpath_fmt+0x41/0x43
[ 131.191092] [] __list_del_entry+0x8d/0x98
[ 131.191092] [] transport_lun_remove_cmd+0x9b/0xb7 [target_core_mod]
[ 131.191092] [] transport_generic_free_cmd+0x5d/0x71 [target_core_mod]
[ 131.191092] [] iscsit_free_cmd+0x1e/0x27 [iscsi_target_mod]
[ 131.191092] [] iscsit_close_connection+0x14d/0x5b2 [iscsi_target_mod]
[ 131.191092] [] iscsit_take_action_for_connection_exit+0xdb/0xe0 [iscsi_target_mod]
[ 131.191092] [] iscsi_target_tx_thread+0x15cb/0x1608 [iscsi_target_mod]
[ 131.191092] [] ? check_preempt_wakeup+0x121/0x185
[ 131.191092] [] ? __dequeue_entity+0x2e/0x33
[ 131.191092] [] ? iscsit_send_text_rsp+0x25f/0x25f [iscsi_target_mod]
[ 131.191092] [] ? iscsit_send_text_rsp+0x25f/0x25f [iscsi_target_mod]
[ 131.191092] [] ? schedule+0x55/0x57
[ 131.191092] [] kthread+0x7d/0x85
[ 131.191092] [] kernel_thread_helper+0x4/0x10
[ 131.191092] [] ? kthread_worker_fn+0x16d/0x16d
[ 131.191092] [] ? gs_change+0x13/0x13Reported-by:
Cc:
Signed-off-by: Nicholas Bellinger -
- core_tpg_pre_addlun()
returns always ERR_PTR() or the pointer, never NULL. The additional
check for NULL in core_dev_add_lun() is not required.- core_tpg_pre_dellun()
returns always ERR_PTR() or the pointer, never NULL. The check for NULL
in core_dev_del_lun() is wrong. The third argument (int *) is never
used, remove it.- core_dev_add_lun()
returns always NULL or the pointer, never ERR_PTR. The check for
IS_ERR() is not required.(nab: Convert core_dev_add_lun() use err.h macros for failure
handling to be consistent with the rest of target_core_fabric_configfs.c
callers)Signed-off-by: Sebastian Andrzej Siewior
Cc:
Signed-off-by: Nicholas Bellinger -
It may happen that uasp will free the request in irq conntext, the
callchain:uasp_cmd_release() -> transport_generic_free_cmd() -> core_dec_lacl_count()
where the last function enables the IRQ. Those irqs are re-disabled
later (due to the spin.*irq_restore) but in between we could get hurt.Signed-off-by: Sebastian Andrzej Siewior
Signed-off-by: Nicholas Bellinger -
The multiple calls to pr_debug() each with one letter results in a new
line. This patch merges the multiple requests into one call per line
so we don't have the multiple line cuts.Signed-off-by: Sebastian Andrzej Siewior
Signed-off-by: Nicholas Bellinger -
This patch adds a work-around for handling zero allocation length
control CDBs (type SCF_SCSI_CONTROL_SG_IO_CDB) that was causing an
OOPs with the following raw calls:# sg_raw -v /dev/sdd 3 0 0 0 0 0
# sg_raw -v /dev/sdd 0x1a 0 1 0 0 0This patch will follow existing zero-length handling for data I/O
and silently return with GOOD status. This addresses the zero length
issue, but the proper long-term resolution for handling arbitary
allocation lengths will be to refactor out data-phase handling in
individual CDB emulation logic within target_core_cdb.cReported-by: Roland Dreier
Cc:
Signed-off-by: Nicholas Bellinger -
According to SPC-4, the sense key for commands that are failed with
INVALID FIELD IN PARAMETER LIST and INVALID FIELD IN CDB should be
ILLEGAL REQUEST (5h) rather than ABORTED COMMAND (Bh). Without this
patch, a tcm_loop LUN incorrectly gives:# sg_raw -r 1 -v /dev/sda 3 1 0 0 ff 0
Sense Information:
Fixed format, current; Sense key: Aborted Command
Additional sense: Invalid field in cdb
Raw sense data (in hex):
70 00 0b 00 00 00 00 0a 00 00 00 00 24 00 00 00
00 00While a real SCSI disk gives:
Sense Information:
Fixed format, current; Sense key: Illegal Request
Additional sense: Invalid field in cdb
Raw sense data (in hex):
70 00 05 00 00 00 00 18 00 00 00 00 24 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00with the main point being that the real disk gives a sense key of
ILLEGAL REQUEST (5h).Signed-off-by: Roland Dreier
Cc:
Signed-off-by: Nicholas Bellinger -
Doing alloc_page(GFP_KERNEL | __GFP_ZERO) to get pages used for data
buffers wastes a lot of CPU clearing pages that will be quickly be
overwritten by the actual data. However, for emulated control
commands such as INQUIRY and so on, the code does assume that the
buffer is zeroed.To avoid this CPU overhead, skip the __GFP_ZERO for commands that are
actually moving data, ie cmds that have SCF_SCSI_DATA_SG_IO_CDB set.Signed-off-by: Roland Dreier
Signed-off-by: Nicholas Bellinger -
Initiators that aren't the active reservation holder should be able to
do a PERSISTENT RESERVE IN command in all cases, so add it to the list
of allowed CDBs in core_scsi3_pr_seq_non_holder().Signed-off-by: Marco Sanvido
Signed-off-by: Roland Dreier
Cc:
Signed-off-by: Nicholas Bellinger -
The comments quote the right parts of the spec:
* d) Establish a unit attention condition for the
* initiator port associated with every I_T nexus
* that lost its registration other than the I_T
* nexus on which the PERSISTENT RESERVE OUT command
* was received, with the additional sense code set
* to REGISTRATIONS PREEMPTED.and
* e) Establish a unit attention condition for the initiator
* port associated with every I_T nexus that lost its
* persistent reservation and/or registration, with the
* additional sense code set to REGISTRATIONS PREEMPTED;but the actual code accidentally uses ASCQ_2AH_RESERVATIONS_PREEMPTED
instead of ASCQ_2AH_REGISTRATIONS_PREEMPTED. Fix this.Signed-off-by: Marco Sanvido
Signed-off-by: Roland Dreier
Cc:
Signed-off-by: Nicholas Bellinger -
We never embedd the bio into a structure, so there is no need to allocate
64 bytes of headroom per bio.Signed-off-by: Christoph Hellwig
Signed-off-by: Nicholas Bellinger
16 Dec, 2011
1 commit
-
The target code was not setting the additional sense length field in the
sense data it returned, which meant that at least the Linux stack
ignored the ASC/ASCQ fields. For example, without this patch, on a
tcm_loop device:# sg_raw -v /dev/sda 2 0 0 0 0 0
gives
cdb to send: 02 00 00 00 00 00
SCSI Status: Check ConditionSense Information:
Fixed format, current; Sense key: Illegal Request
Raw sense data (in hex):
70 00 05 00 00 00 00 00while after the patch we correctly get the following (which matches what
a regular disk returns):cdb to send: 02 00 00 00 00 00
SCSI Status: Check ConditionSense Information:
Fixed format, current; Sense key: Illegal Request
Additional sense: Invalid command operation code
Raw sense data (in hex):
70 00 05 00 00 00 00 0a 00 00 00 00 20 00 00 00
00 00Signed-off-by: Roland Dreier
Cc: stable@kernel.org
Signed-off-by: Nicholas Bellinger
14 Dec, 2011
20 commits
-
This patch removes a legacy se_dev_check_online() check from within
transport_execute_tasks() that should no longer be necessary as
transport_lookup_cmd_lun() is already making this call.Using transport_cmd_check_stop() from transport_execute_tasks() should
already be checking per se_cmd context for each descriptor upon active
I/O shutdown, so no need to acquire dev->dev_status_lock again while
executing se_task submission.Cc: Christoph Hellwig
Cc: Roland Dreier
Cc: Joern Engel
Signed-off-by: Nicholas Bellinger -
This patch removes the original usage of __transport_execute_tasks() ahead
of every transport_get_cmd_from_queue() call in transport_processing_thread().
This helps reduce se_device->execute_task_lock contention between qla2xxx wq
with target_submit_cmd() for READs and transport_processing_thread()
context servicing WRITEs with full payloads for I/O submission.It also adds a __transport_execute_tasks() to kick the task queue again
without a *se_cmd descriptor with existing queue full logic, but this may
end up not being necessary.Cc: Christoph Hellwig
Cc: Roland Dreier
Cc: Joern Engel
Signed-off-by: Nicholas Bellinger -
This patch makes __transport_execute_tasks() perform the addition of
tasks to dev->execute_task_list via __transport_add_tasks_from_cmd()
while holding dev->execute_task_lock during normal I/O fast path
submission.It effectively removes the unnecessary re-acquire of dev->execute_task_lock
during transport_execute_tasks() -> transport_add_tasks_from_cmd() ahead
of calling __transport_execute_tasks() to queue tasks for the passed
*se_cmd descriptor.(v2: Re-add goto check_depth usage for multi-task submission for now..)
Cc: Christoph Hellwig
Cc: Roland Dreier
Cc: Joern Engel
Signed-off-by: Nicholas Bellinger -
Historically, pSCSI devices have been the ones that required target-core
to enforce a per se_device->depth_left. This patch changes target-core
to no longer (by default) enforce a per se_device->depth_left or sleep in
transport_tcq_window_closed() when we out of queue slots for all backend
export cases.Cc: Christoph Hellwig
Cc: Roland Dreier
Cc: Joern Engel
Signed-off-by: Nicholas Bellinger -
This patch makes __transport_execute_tasks() use a local *se_dev
reference to prevent direct se_cmd->se_dev access after
transport_cmd_check_stop() -> transport_add_tasks_from_cmd()
has been called, as in the current implementation we can expect
__transport_execute_tasks() may be called from another context
that may have already completed the I/O.Cc: Christoph Hellwig
Signed-off-by: Nicholas Bellinger -
This patch converts the main ft_send_work() I/O path to use
target_submit_cmd() with a single se_cmd->cmd_kref reference
that is released via the existing ft_check_stop_free() response
path callback.It also makes ft_send_tm() use transport_init_se_cmd() and
target_get_sess_cmd() to also use single se_cmd->cmd_kref
reference.Cc: Christoph Hellwig
Cc: Kiran Patil
Signed-off-by: Nicholas Bellinger -
This patch adds a target_submit_cmd() caller that can be used by fabrics
to submit an uninitialized se_cmd descriptor to an struct se_session +
unpacked_lun from workqueue process context. This call will invoke the
following steps:- transport_init_se_cmd() to setup se_cmd specific pointers
- Obtain se_cmd->cmd_kref references with target_get_sess_cmd()
- set se_cmd->t_tasks_bidi
- transport_lookup_cmd_lun() to setup struct se_cmd->se_lun from
the passed unpacked_lun
- transport_generic_allocate_tasks() to setup the passed *cdb, and
- transport_handle_cdb_direct() handle READ dispatch or WRITE
ready-to-transfer callback to fabricv2 changes from hch feedback:
*) Add target_sc_flags_table for target_submit_cmd flags
*) Rename bidi parameter to flags, add TARGET_SCF_BIDI_OP
*) Convert checks to BUG_ON
*) Add out_check_cond for transport_send_check_condition_and_sense
usagev3 changes:
*) Add TARGET_SCF_ACK_KREF for target_submit_cmd into
target_get_sess_cmd to determine when the fabric caller is expecting
a second kref_put() from fabric packet acknowledgement.Cc: Christoph Hellwig
Cc: Roland Dreier
Signed-off-by: Nicholas Bellinger -
This patch moves target_put_sess_cmd() to use a se_cmd->cmd_kref
callback target_release_cmd_kref when performing driver release of
fabric->se_cmd descriptor memory. It sets the default cmd_kref
count value to '2' within target_get_sess_cmd() setup, and
currently assumes TFO->check_stop_free() usage.It drops se_tfo->check_release_cmd() usage in the main
transport_release_cmd codepath.Signed-off-by: Nicholas Bellinger
-
Current SCSI specs say that the "response format" field in the standard
INQUIRY response should be set to 2, and all the real SCSI devices I
have do put 2 here. So let's do that too.Signed-off-by: Roland Dreier
Cc: stable@kernel.org
Signed-off-by: Nicholas Bellinger -
There is not reason to artifically limit max_sectors in tcm_loop, set
it to UINT_MAX to allow stressing the large I/O handling in the target
core using the loopback driver. Also remove various superflous defines
hiding the values set in the host template.Signed-off-by: Christoph Hellwig
Signed-off-by: Nicholas Bellinger -
This patch strips the trailing newline from backend device udev_path and
alias attributes.Signed-off-by: Sebastian Andrzej Siewior
Signed-off-by: Nicholas Bellinger -
This patch makes chap_server_compute_md5() use proper unsigned long
usage for the CHAP_I (identifier) and check for values beyond 255 as
per RFC-1994.Reported-by: Joern Engel
Signed-off-by: Nicholas Bellinger -
A reader should spend an extra moment whenever noticing a cast,
because either something special is going on that deserves extra
attention or, as is all too often the case, the code is wrong.These casts, afaics, have all been useless. They cast a foo* to a
foo*, cast a void* to the assigned type, cast a foo* to void*, before
assigning it to a void* variable, etc.In a few cases I also removed an additional &...[0], which is equally
useless.Lastly I added three FIXMEs where, to the best of my judgement, the
code appears to have a bug. It would be good if someone could check
these.Signed-off-by: Joern Engel
Signed-off-by: Nicholas Bellinger -
- rename to target_check_cdb_and_preempt
- use non-safe list_for_each_entry
- move common check into callee (simplifying callers)Signed-off-by: Joern Engel
Signed-off-by: Nicholas Bellinger -
And make it static afterwards.
Signed-off-by: Joern Engel
Signed-off-by: Nicholas Bellinger -
The command
| echo rd_pages=32768 > ramdisk/controlDoes not work because it writes "rd_pages=32768\n" and the parser which
matches for "rd_pages=%d" does not recognize it due to the \n. One way
of fixing this would be using "echo -n" instead.
This patch adds \n to the list of separators so we don't have to use the
-n argument which I find is more convinient.Signed-off-by: Sebastian Andrzej Siewior
Signed-off-by: Nicholas Bellinger -
There is no need to make task_state_active an atomic_t given that it is
always set under the execute_task_lock so we can make it a simple bool.
Also rename it to t_state_active to be closer to the list it guards,
and make sure all checks before the list addion/removal actually happen
under execute_task_lock.Signed-off-by: Christoph Hellwig
Signed-off-by: Nicholas Bellinger -
We only reach transport_complete_task once per task, so the test and set on
task_error_status is never going to have an effect.Signed-off-by: Christoph Hellwig
Signed-off-by: Nicholas Bellinger -
Signed-off-by: Christoph Hellwig
Signed-off-by: Nicholas Bellinger -
This reorganized the headers under include/target into:
- target_core_base.h stays as is with all target-wide data stuctures and defines
- target_core_backend.h contains the whole interface to I/O backends
- target_core_fabric.h contains the whole interface to fabric modulesExcept for those only the various configfs macro headers stay around.
Signed-off-by: Christoph Hellwig
Signed-off-by: Nicholas Bellinger
