On Tue, 2012-10-30 at 11:07 -0400, Christoph Hellwig wrote:
> Pass the sense reason as an explicit return value from the I/O submission
> path instead of storing it in struct se_cmd and using negative return
> values. This cleans up a lot of the code pathes, and with the sparse
> annotations for the new sense_reason_t type allows for much better
> error checking.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
>
Hey Christoph,
Just getting back to this for-3.8 code.. Comments below.
> ---
> drivers/infiniband/ulp/srpt/ib_srpt.c | 43 -
> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 4
> drivers/target/iscsi/iscsi_target.c | 80 --
> drivers/target/iscsi/iscsi_target_core.h | 2
> drivers/target/iscsi/iscsi_target_erl1.c | 11
> drivers/target/target_core_alua.c | 98 +--
> drivers/target/target_core_alua.h | 6
> drivers/target/target_core_device.c | 31 -
> drivers/target/target_core_file.c | 22
> drivers/target/target_core_iblock.c | 55 -
> drivers/target/target_core_internal.h | 2
> drivers/target/target_core_pr.c | 918 +++++++++++++------------------
> drivers/target/target_core_pr.h | 10
> drivers/target/target_core_pscsi.c | 51 -
> drivers/target/target_core_rd.c | 10
> drivers/target/target_core_sbc.c | 87 +-
> drivers/target/target_core_spc.c | 106 +--
> drivers/target/target_core_transport.c | 252 +++-----
> drivers/target/target_core_ua.c | 11
> drivers/target/target_core_ua.h | 2
> drivers/vhost/tcm_vhost.c | 4
> include/target/target_core_backend.h | 16
> include/target/target_core_base.h | 46 -
> include/target/target_core_fabric.h | 13
> 24 files changed, 833 insertions(+), 1047 deletions(-)
>
Btw, any chance to sync up the git stat output for future patches..?
That would make things easier to read + review. ;)
<SNIP>
>
> -static int iblock_execute_unmap(struct se_cmd *cmd)
> +static sense_reason_t
> +iblock_execute_unmap(struct se_cmd *cmd)
> {
> struct se_device *dev = cmd->se_dev;
> struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
> @@ -307,17 +306,18 @@ static int iblock_execute_unmap(struct s
> sector_t lba;
> int size;
> u32 range;
> - int ret = 0;
> - int dl, bd_dl;
> + sense_reason_t ret = 0;
> + int dl, bd_dl, err;
>
> if (cmd->data_length < 8) {commit 14150a6bb
> pr_warn("UNMAP parameter list length %u too small\n",
> cmd->data_length);
> - cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
> - return -EINVAL;
> + return TCM_INVALID_PARAMETER_LIST;
> }
>
> buf = transport_kmap_data_sg(cmd);
> + if (!buf)
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>
Checking the return value here of transport_kmap_data_sg() looks like a
separate bugfix that needs to go-to stable as well, yes..?
> @@ -1052,24 +1053,21 @@ static int pscsi_execute_cmd(struct se_c
> if (!req || IS_ERR(req)) {
> pr_err("PSCSI: blk_get_request() failed: %ld\n",
> req ? IS_ERR(req) : -ENOMEM);
> - cmd->scsi_sense_reason =
> - TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> + ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> goto fail;
> }
> } else {
> BUG_ON(!cmd->data_length);
>
> ret = pscsi_map_sg(cmd, sgl, sgl_nents, data_direction, &hbio);
> - if (ret < 0) {
> - cmd->scsi_sense_reason =
> - TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> + if (ret)
> goto fail;
> - }
>
> req = blk_make_request(pdv->pdv_sd->request_queue, hbio,
> GFP_KERNEL);
> if (IS_ERR(req)) {
> pr_err("pSCSI: blk_make_request() failed\n");
> + ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> goto fail_free_bio;
> }
> }
> @@ -1100,10 +1098,10 @@ fail_free_bio:
> bio->bi_next = NULL;
> bio_endio(bio, 0); /* XXX: should be error */
> }
> - cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> + ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> fail:
> kfree(pt);
> - return -ENOMEM;
> + return ret;
> }
>
> /* pscsi_get_device_type():
> @@ -1152,7 +1150,6 @@ static void pscsi_req_done(struct reques
> pr_debug("PSCSI Host Byte exception at cmd: %p CDB:"
> " 0x%02x Result: 0x%08x\n", cmd, pt->pscsi_cdb[0],
> pt->pscsi_result);
> - cmd->scsi_sense_reason = TCM_UNSUPPORTED_SCSI_OPCODE;
> target_complete_cmd(cmd, SAM_STAT_CHECK_CONDITION);
> break;
> }
Mmmm, this special case results in a completion of an pSCSI
CHECK_CONDITION w/o a sense reason, as TCM_UNSUPPORTED_SCSI_OPCODE.
Changing this to use transport_generic_request_failure() directly, and
including a separate incremental patch for this..
> Index: lio-core/drivers/target/target_core_sbc.c
> ===================================================================
> --- lio-core.orig/drivers/target/target_core_sbc.c 2012-10-30 14:14:53.593253429 +0100
> +++ lio-core/drivers/target/target_core_sbc.c 2012-10-30 14:15:09.893253330 +0100
> @@ -37,7 +37,8 @@
> #include "target_core_ua.h"
>
>
> -static int sbc_emulate_readcapacity(struct se_cmd *cmd)
> +static sense_reason_t
> +sbc_emulate_readcapacity(struct se_cmd *cmd)
> {
> struct se_device *dev = cmd->se_dev;
> unsigned long long blocks_long = dev->transport->get_blocks(dev);
> @@ -60,16 +61,18 @@ static int sbc_emulate_readcapacity(stru
> buf[7] = dev->dev_attrib.block_size & 0xff;
>
> rbuf = transport_kmap_data_sg(cmd);
> - if (rbuf) {
> - memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length));
> - transport_kunmap_data_sg(cmd);
> - }
> + if (!rbuf)
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> +
> + memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length));
> + transport_kunmap_data_sg(cmd);
>
> target_complete_cmd(cmd, GOOD);
> return 0;
> }
>
The proper handling of transport_kmap_data_sg() needs to be sent to
stable as well. (Paolo CC'ed)
> -static int sbc_emulate_readcapacity_16(struct se_cmd *cmd)
> +static sense_reason_t
> +sbc_emulate_readcapacity_16(struct se_cmd *cmd)
> {
> struct se_device *dev = cmd->se_dev;
> unsigned char *rbuf;
> @@ -97,10 +100,11 @@ static int sbc_emulate_readcapacity_16(s
> buf[14] = 0x80;
>
> rbuf = transport_kmap_data_sg(cmd);
> - if (rbuf) {
> - memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length));
> - transport_kunmap_data_sg(cmd);
> - }
> + if (!rbuf)
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> +
> + memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length));
> + transport_kunmap_data_sg(cmd);
>
> target_complete_cmd(cmd, GOOD);
> return 0;
Ditto here..
<SNIP>
> +sense_reason_t
> +target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
> {
> struct se_device *dev = cmd->se_dev;
> unsigned long flags;
> - int ret;
> + sense_reason_t ret;
>
> /*
> * Ensure that the received CDB is less than the max (252 + 8) bytes
> @@ -1113,9 +1106,7 @@ int target_setup_cmd_from_cdb(
> pr_err("Received SCSI CDB with command_size: %d that"
> " exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
> scsi_command_size(cdb), SCSI_MAX_VARLEN_CDB_SIZE);
> - cmd->se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> - cmd->scsi_sense_reason = TCM_INVALID_CDB_FIELD;
> - return -EINVAL;
> + return TCM_INVALID_CDB_FIELD;
> }
> /*
> * If the received CDB is larger than TCM_MAX_COMMAND_SIZE,
> @@ -1130,10 +1121,7 @@ int target_setup_cmd_from_cdb(
> " %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
> scsi_command_size(cdb),
> (unsigned long)sizeof(cmd->__t_task_cdb));
> - cmd->se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> - cmd->scsi_sense_reason =
> - TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> - return -ENOMEM;
> + return TCM_OUT_OF_RESOURCES;
> }
> } else
> cmd->t_task_cdb = &cmd->__t_task_cdb[0];
> @@ -1145,50 +1133,30 @@ int target_setup_cmd_from_cdb(
> /*
> * Check for an existing UNIT ATTENTION condition
> */
> - if (core_scsi3_ua_check(cmd, cdb) < 0) {
> - cmd->se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> - cmd->scsi_sense_reason = TCM_CHECK_CONDITION_UNIT_ATTENTION;
> - return -EINVAL;
> - }
> + ret = target_scsi3_ua_check(cmd);
> + if (ret)
> + return ret;
>
> ret = target_alua_state_check(cmd);
> - if (ret) {
> - cmd->se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> - if (ret > 0)
> - cmd->scsi_sense_reason = TCM_CHECK_CONDITION_NOT_READY;
> - else
> - cmd->scsi_sense_reason = TCM_INVALID_CDB_FIELD;
> - return -EINVAL;
> - }
> + if (ret)
> + return ret;
>
> - /*
> - * Check status for SPC-3 Persistent Reservations
> - */
> ret = target_check_reservation(cmd);
> - if (ret) {
> - cmd->se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> - cmd->se_cmd_flags |= SCF_SCSI_RESERVATION_CONFLICT;
> - cmd->scsi_status = SAM_STAT_RESERVATION_CONFLICT;
> - cmd->scsi_sense_reason = TCM_RESERVATION_CONFLICT;
> + if (ret)
> return ret;
> - }
>
> ret = dev->transport->parse_cdb(cmd);
> - if (ret < 0)
> + if (ret)
> + return ret;
> +
> + ret = transport_check_alloc_task_attr(cmd);
> + if (ret)
> return ret;
>
Nice work to get these all cleaned up in target_setup_cmd_from_cdb()
> spin_lock_irqsave(&cmd->t_state_lock, flags);
> cmd->se_cmd_flags |= SCF_SUPPORTED_SAM_OPCODE;
> spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>
> - /*
> - * Check for SAM Task Attribute Emulation
> - */
> - if (transport_check_alloc_task_attr(cmd) < 0) {
> - cmd->se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> - cmd->scsi_sense_reason = TCM_INVALID_CDB_FIELD;
> - return -EINVAL;
> - }
> spin_lock(&cmd->se_lun->lun_sep_lock);
> if (cmd->se_lun->lun_sep)
> cmd->se_lun->lun_sep->sep_stats.cmd_pdus++;
<SNIP>
> /*
> * target_submit_cmd_map_sgls - lookup unpacked lun and submit uninitialized
> * se_cmd + use pre-allocated SGL memory.
> @@ -1273,7 +1269,8 @@ int target_submit_cmd_map_sgls(struct se
> struct scatterlist *sgl_bidi, u32 sgl_bidi_count)
> {
> struct se_portal_group *se_tpg;
> - int rc;
> + sense_reason_t rc;
> + int ret;
>
> se_tpg = se_sess->se_tpg;
> BUG_ON(!se_tpg);
> @@ -1294,9 +1291,9 @@ int target_submit_cmd_map_sgls(struct se
> * for fabrics using TARGET_SCF_ACK_KREF that expect a second
> * kref_put() to happen during fabric packet acknowledgement.
> */
> - rc = target_get_sess_cmd(se_sess, se_cmd, (flags & TARGET_SCF_ACK_KREF));
> - if (rc)
> - return rc;
> + ret = target_get_sess_cmd(se_sess, se_cmd, (flags & TARGET_SCF_ACK_KREF));
> + if (ret)
> + return ret;
> /*
> * Signal bidirectional data payloads to target-core
> */
> @@ -1305,16 +1302,16 @@ int target_submit_cmd_map_sgls(struct se
> /*
> * Locate se_lun pointer and attach it to struct se_cmd
> */
> - if (transport_lookup_cmd_lun(se_cmd, unpacked_lun) < 0) {
> - transport_send_check_condition_and_sense(se_cmd,
> - se_cmd->scsi_sense_reason, 0);
> + rc = transport_lookup_cmd_lun(se_cmd, unpacked_lun);
> + if (rc) {
> + transport_send_check_condition_and_sense(se_cmd, rc, 0);
> target_put_sess_cmd(se_sess, se_cmd);
> return 0;
> }
>
> rc = target_setup_cmd_from_cdb(se_cmd, cdb);
> if (rc != 0) {
> - transport_generic_request_failure(se_cmd);
> + transport_generic_request_failure(se_cmd, rc);
> return 0;
> }
> /*
> @@ -1349,7 +1346,7 @@ int target_submit_cmd_map_sgls(struct se
> rc = transport_generic_map_mem_to_cmd(se_cmd, sgl, sgl_count,
> sgl_bidi, sgl_bidi_count);
> if (rc != 0) {
> - transport_generic_request_failure(se_cmd);
> + transport_generic_request_failure(se_cmd, rc);
> return 0;
> }
> }
So these changes look clean enough for main target_submit_cmd() I/O
path.
> @@ -2136,7 +2085,8 @@ out:
> * might not have the payload yet, so notify the fabric via a call to
> * ->write_pending instead. Otherwise place it on the execution queue.
> */
> -int transport_generic_new_cmd(struct se_cmd *cmd)
> +sense_reason_t
> +transport_generic_new_cmd(struct se_cmd *cmd)
> {
> int ret = 0;
>
> @@ -2149,7 +2099,7 @@ int transport_generic_new_cmd(struct se_
> cmd->data_length) {
> ret = transport_generic_get_mem(cmd);
> if (ret < 0)
> - goto out_fail;
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> }
>
> atomic_inc(&cmd->t_fe_count);
> @@ -2175,14 +2125,11 @@ int transport_generic_new_cmd(struct se_
> if (ret == -EAGAIN || ret == -ENOMEM)
> goto queue_full;
>
> - if (ret < 0)
> - return ret;
> - return 1;
> + /* fabric drivers should only return -EAGAIN or -ENOMEM as error */
> + WARN_ON(ret);
> +
> + return 0;
>
Returning zero here for a failed WRITE_PENDING is definitely incorrect,
and would end up causing bad things to happen. (Roland CC'ed)
Sending out a separate incremental patch to address this one.
> -out_fail:
> - cmd->se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> - cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> - return -EINVAL;
> queue_full:
> pr_debug("Handling write_pending QUEUE__FULL: se_cmd: %p\n", cmd);
> cmd->t_state = TRANSPORT_COMPLETE_QF_WP;
> @@ -2626,10 +2573,9 @@ static int transport_get_sense_codes(
> return 0;
> }
<SNIP>
> -static int spc_emulate_modesense(struct se_cmd *cmd)
> +static sense_reason_t
> +spc_emulate_modesense(struct se_cmd *cmd)
> {
> struct se_device *dev = cmd->se_dev;
> char *cdb = cmd->t_task_cdb;
> @@ -798,8 +809,7 @@ static int spc_emulate_modesense(struct
> default:
> pr_err("MODE SENSE: unimplemented page/subpage: 0x%02x/0x%02x\n",
> cdb[2] & 0x3f, cdb[3]);
> - cmd->scsi_sense_reason = TCM_UNKNOWN_MODE_PAGE;
> - return -EINVAL;
> + return TCM_UNKNOWN_MODE_PAGE;
> }
> offset += length;
>
> @@ -833,16 +843,18 @@ static int spc_emulate_modesense(struct
> }
>
> rbuf = transport_kmap_data_sg(cmd);
> - if (rbuf) {
> - memcpy(rbuf, buf, min(offset, cmd->data_length));
> - transport_kunmap_data_sg(cmd);
> - }
> + if (!rbuf)
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> +
> + memcpy(rbuf, buf, min(offset, cmd->data_length));
> + transport_kunmap_data_sg(cmd);
>
> target_complete_cmd(cmd, GOOD);
> return 0;
> }
>
Ok, this part conflicts with Roland's for-3.8 patch:
commit 717e44956bc230bca54a4e044ec4b1a4a8438e09
Author: Roland Dreier <roland@xxxxxxxxxxxxxxx>
Date: Wed Oct 31 09:16:50 2012 -0700
target: Add emulation for MODE SELECT
Fixing up the minor breakage here, and folding into your original patch.
<SNIP>
>
> Index: lio-core/drivers/infiniband/ulp/srpt/ib_srpt.c
> ===================================================================
> --- lio-core.orig/drivers/infiniband/ulp/srpt/ib_srpt.c 2012-10-30 14:14:53.593253429 +0100
> +++ lio-core/drivers/infiniband/ulp/srpt/ib_srpt.c 2012-10-30 14:43:46.626576409 +0100
> @@ -1730,7 +1730,7 @@ static int srpt_handle_cmd(struct srpt_r
> uint64_t unpacked_lun;
> u64 data_len;
> enum dma_data_direction dir;
> - int ret;
> + sense_reason_t ret;
>
> BUG_ON(!send_ioctx);
>
> @@ -1755,12 +1755,10 @@ static int srpt_handle_cmd(struct srpt_r
> break;
> }
>
> - ret = srpt_get_desc_tbl(send_ioctx, srp_cmd, &dir, &data_len);
> - if (ret) {
> + if (srpt_get_desc_tbl(send_ioctx, srp_cmd, &dir, &data_len)) {
> printk(KERN_ERR "0x%llx: parsing SRP descriptor table failed.\n",
> srp_cmd->tag);
> - cmd->se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> - cmd->scsi_sense_reason = TCM_INVALID_CDB_FIELD;
> + ret = TCM_INVALID_CDB_FIELD;
> kref_put(&send_ioctx->kref, srpt_put_send_ioctx_kref);
> goto send_sense;
> }
> @@ -1769,26 +1767,26 @@ static int srpt_handle_cmd(struct srpt_r
> cmd->data_direction = dir;
> unpacked_lun = srpt_unpack_lun((uint8_t *)&srp_cmd->lun,
> sizeof(srp_cmd->lun));
> - if (transport_lookup_cmd_lun(cmd, unpacked_lun) < 0) {
> + ret = transport_lookup_cmd_lun(cmd, unpacked_lun);
> + if (ret) {
> kref_put(&send_ioctx->kref, srpt_put_send_ioctx_kref);
> goto send_sense;
> }
> ret = target_setup_cmd_from_cdb(cmd, srp_cmd->cdb);
> - if (ret < 0) {
> + if (ret) {
> kref_put(&send_ioctx->kref, srpt_put_send_ioctx_kref);
> - if (cmd->se_cmd_flags & SCF_SCSI_RESERVATION_CONFLICT) {
> + if (ret == TCM_RESERVATION_CONFLICT) {
> srpt_queue_status(cmd);
> return 0;
> - } else
> - goto send_sense;
> + }
> + goto send_sense;
> }
>
> transport_handle_cdb_direct(cmd);
> return 0;
>
> send_sense:
> - transport_send_check_condition_and_sense(cmd, cmd->scsi_sense_reason,
> - 0);
> + transport_send_check_condition_and_sense(cmd, ret, 0);
> return -1;
> }
>
So this looks reasonable to apply now to for-next ahead of finally
getting the target_submit_cmd() parts dusted off for ib_srpt.
> @@ -1882,16 +1880,14 @@ static void srpt_handle_tsk_mgmt(struct
> send_ioctx->tag = srp_tsk->tag;
> tcm_tmr = srp_tmr_to_tcm(srp_tsk->tsk_mgmt_func);
> if (tcm_tmr < 0) {
> - send_ioctx->cmd.se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> send_ioctx->cmd.se_tmr_req->response =
> TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
> - goto process_tmr;
> + goto fail;
> }
> res = core_tmr_alloc_req(cmd, NULL, tcm_tmr, GFP_KERNEL);
> if (res < 0) {
> - send_ioctx->cmd.se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> send_ioctx->cmd.se_tmr_req->response = TMR_FUNCTION_REJECTED;
> - goto process_tmr;
> + goto fail;
> }
>
> unpacked_lun = srpt_unpack_lun((uint8_t *)&srp_tsk->lun,
> @@ -1899,22 +1895,19 @@ static void srpt_handle_tsk_mgmt(struct
> res = transport_lookup_tmr_lun(&send_ioctx->cmd, unpacked_lun);
> if (res) {
> pr_debug("rejecting TMR for LUN %lld\n", unpacked_lun);
> - send_ioctx->cmd.se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> send_ioctx->cmd.se_tmr_req->response = TMR_LUN_DOES_NOT_EXIST;
> - goto process_tmr;
> + goto fail;
> }
>
> if (srp_tsk->tsk_mgmt_func == SRP_TSK_ABORT_TASK)
> srpt_rx_mgmt_fn_tag(send_ioctx, srp_tsk->task_tag);
>
> -process_tmr:
> kref_get(&send_ioctx->kref);
> - if (!(send_ioctx->cmd.se_cmd_flags & SCF_SCSI_CDB_EXCEPTION))
> - transport_generic_handle_tmr(&send_ioctx->cmd);
> - else
> - transport_send_check_condition_and_sense(cmd,
> - cmd->scsi_sense_reason, 0);
> -
> + transport_generic_handle_tmr(&send_ioctx->cmd);
> + return;
> +fail:
> + kref_get(&send_ioctx->kref);
> + transport_send_check_condition_and_sense(cmd, 0, 0); // XXX:
> }
Mmmmm, the use of transport_send_check_condition_and_sense() is because
ib_srpt uses srpt_queue_response() for both TFO->queue_data_in() +
TFO->queue_tm_rsp(). This also needs to be de-talged at some point for
target_submit_tmr() usage.
>
> Index: lio-core/drivers/target/target_core_alua.c
> ===================================================================
> --- lio-core.orig/drivers/target/target_core_alua.c 2012-10-30 14:14:53.593253429 +0100
> +++ lio-core/drivers/target/target_core_alua.c 2012-10-30 14:15:09.903253331 +0100
<SNIP>
> @@ -200,7 +203,8 @@ int target_emulate_report_target_port_gr
> *
> * See spc4r17 section 6.35
> */
> -int target_emulate_set_target_port_groups(struct se_cmd *cmd)
> +sense_reason_t
> +target_emulate_set_target_port_groups(struct se_cmd *cmd)
> {
> struct se_device *dev = cmd->se_dev;
> struct se_port *port, *l_port = cmd->se_lun->lun_sep;
> @@ -209,22 +213,23 @@ int target_emulate_set_target_port_group
> struct t10_alua_tg_pt_gp_member *tg_pt_gp_mem, *l_tg_pt_gp_mem;
> unsigned char *buf;
> unsigned char *ptr;
> + sense_reason_t rc;
> u32 len = 4; /* Skip over RESERVED area in header */
> - int alua_access_state, primary = 0, rc;
> + int alua_access_state, primary = 0;
> u16 tg_pt_id, rtpi;
>
> - if (!l_port) {
> - cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> - return -EINVAL;
> - }
> + if (!l_port)
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> +
> if (cmd->data_length < 4) {
> pr_warn("SET TARGET PORT GROUPS parameter list length %u too"
> " small\n", cmd->data_length);
> - cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
> - return -EINVAL;
> + return TCM_INVALID_PARAMETER_LIST;
> }
>
> buf = transport_kmap_data_sg(cmd);
> + if (!buf)
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>
> /*
> * Determine if explict ALUA via SET_TARGET_PORT_GROUPS is allowed
> @@ -233,8 +238,7 @@ int target_emulate_set_target_port_group
> l_tg_pt_gp_mem = l_port->sep_alua_tg_pt_gp_mem;
> if (!l_tg_pt_gp_mem) {
> pr_err("Unable to access l_port->sep_alua_tg_pt_gp_mem\n");
> - cmd->scsi_sense_reason = TCM_UNSUPPORTED_SCSI_OPCODE;
> - rc = -EINVAL;
> + rc = TCM_UNSUPPORTED_SCSI_OPCODE;
> goto out;
> }
> spin_lock(&l_tg_pt_gp_mem->tg_pt_gp_mem_lock);
> @@ -242,24 +246,22 @@ int target_emulate_set_target_port_group
> if (!l_tg_pt_gp) {
> spin_unlock(&l_tg_pt_gp_mem->tg_pt_gp_mem_lock);
> pr_err("Unable to access *l_tg_pt_gp_mem->tg_pt_gp\n");
> - cmd->scsi_sense_reason = TCM_UNSUPPORTED_SCSI_OPCODE;
> - rc = -EINVAL;
> + rc = TCM_UNSUPPORTED_SCSI_OPCODE;
> goto out;
> }
> - rc = (l_tg_pt_gp->tg_pt_gp_alua_access_type & TPGS_EXPLICT_ALUA);
> spin_unlock(&l_tg_pt_gp_mem->tg_pt_gp_mem_lock);
>
> - if (!rc) {
> + if (l_tg_pt_gp->tg_pt_gp_alua_access_type & TPGS_EXPLICT_ALUA) {
> pr_debug("Unable to process SET_TARGET_PORT_GROUPS"
> " while TPGS_EXPLICT_ALUA is disabled\n");
This change is wrong.
Fixing up the incorrect inversion check here for TPGS_EXPLICT_ALUA with
a separate patch.
> - cmd->scsi_sense_reason = TCM_UNSUPPORTED_SCSI_OPCODE;
> - rc = -EINVAL;
> + rc = TCM_UNSUPPORTED_SCSI_OPCODE;
> goto out;
> }
>
> ptr = &buf[4]; /* Skip over RESERVED area in header */
>
> while (len < cmd->data_length) {
> + bool found = false;
> alua_access_state = (ptr[0] & 0x0f);
> /*
> * Check the received ALUA access state, and determine if
> @@ -267,7 +269,7 @@ int target_emulate_set_target_port_group
> * access state.
> */
> rc = core_alua_check_transition(alua_access_state, &primary);
> - if (rc != 0) {
> + if (rc) {
> /*
> * If the SET TARGET PORT GROUPS attempts to establish
> * an invalid combination of target port asymmetric
> @@ -278,11 +280,9 @@ int target_emulate_set_target_port_group
> * REQUEST, and the additional sense code set to INVALID
> * FIELD IN PARAMETER LIST.
> */
> - cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
> - rc = -EINVAL;
> goto out;
> }
> - rc = -1;
> +
> /*
> * If the ASYMMETRIC ACCESS STATE field (see table 267)
> * specifies a primary target port asymmetric access state,
> @@ -314,11 +314,13 @@ int target_emulate_set_target_port_group
>
> atomic_inc(&tg_pt_gp->tg_pt_gp_ref_cnt);
> smp_mb__after_atomic_inc();
> +
> spin_unlock(&dev->t10_alua.tg_pt_gps_lock);
>
> - rc = core_alua_do_port_transition(tg_pt_gp,
> + if (!core_alua_do_port_transition(tg_pt_gp,
> dev, l_port, nacl,
> - alua_access_state, 1);
> + alua_access_state, 1))
> + found = true;
>
> spin_lock(&dev->t10_alua.tg_pt_gps_lock);
> atomic_dec(&tg_pt_gp->tg_pt_gp_ref_cnt);
> @@ -326,15 +328,6 @@ int target_emulate_set_target_port_group
> break;
> }
> spin_unlock(&dev->t10_alua.tg_pt_gps_lock);
> - /*
> - * If not matching target port group ID can be located
> - * throw an exception with ASCQ: INVALID_PARAMETER_LIST
> - */
> - if (rc != 0) {
> - cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
> - rc = -EINVAL;
> - goto out;
> - }
> } else {
> /*
> * Extact the RELATIVE TARGET PORT IDENTIFIER to identify
> @@ -353,25 +346,22 @@ int target_emulate_set_target_port_group
> continue;
>
> tg_pt_gp_mem = port->sep_alua_tg_pt_gp_mem;
> +
> spin_unlock(&dev->se_port_lock);
>
> - rc = core_alua_set_tg_pt_secondary_state(
> - tg_pt_gp_mem, port, 1, 1);
> + if (!core_alua_set_tg_pt_secondary_state(
> + tg_pt_gp_mem, port, 1, 1))
> + found = true;
>
> spin_lock(&dev->se_port_lock);
> break;
> }
> spin_unlock(&dev->se_port_lock);
> - /*
> - * If not matching relative target port identifier can
> - * be located, throw an exception with ASCQ:
> - * INVALID_PARAMETER_LIST
> - */
> - if (rc != 0) {
> - cmd->scsi_sense_reason = TCM_INVALID_PARAMETER_LIST;
> - rc = -EINVAL;
> - goto out;
> - }
> + }
> +
> + if (!found) {
> + rc = TCM_INVALID_PARAMETER_LIST;
> + goto out;
> }
>
> ptr += 4;
<SNIP>
> Index: lio-core/drivers/target/target_core_device.c
> ===================================================================
> --- lio-core.orig/drivers/target/target_core_device.c 2012-10-30 14:14:53.593253429 +0100
> +++ lio-core/drivers/target/target_core_device.c 2012-10-30 14:15:09.903253331 +0100
> @@ -54,18 +54,16 @@ static struct se_hba *lun0_hba;
> /* not static, needed by tpg.c */
> struct se_device *g_lun0_dev;
>
> -int transport_lookup_cmd_lun(struct se_cmd *se_cmd, u32 unpacked_lun)
> +sense_reason_t
> +transport_lookup_cmd_lun(struct se_cmd *se_cmd, u32 unpacked_lun)
> {
> struct se_lun *se_lun = NULL;
> struct se_session *se_sess = se_cmd->se_sess;
> struct se_device *dev;
> unsigned long flags;
>
> - if (unpacked_lun >= TRANSPORT_MAX_LUNS_PER_TPG) {
> - se_cmd->scsi_sense_reason = TCM_NON_EXISTENT_LUN;
> - se_cmd->se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> - return -ENODEV;
> - }
> + if (unpacked_lun >= TRANSPORT_MAX_LUNS_PER_TPG)
> + return TCM_NON_EXISTENT_LUN;
>
> spin_lock_irqsave(&se_sess->se_node_acl->device_list_lock, flags);
> se_cmd->se_deve = se_sess->se_node_acl->device_list[unpacked_lun];
> @@ -77,14 +75,12 @@ int transport_lookup_cmd_lun(struct se_c
>
> if ((se_cmd->data_direction == DMA_TO_DEVICE) &&
> (deve->lun_flags & TRANSPORT_LUNFLAGS_READ_ONLY)) {
> - se_cmd->scsi_sense_reason = TCM_WRITE_PROTECTED;
> - se_cmd->se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> pr_err("TARGET_CORE[%s]: Detected WRITE_PROTECTED LUN"
> " Access for 0x%08x\n",
> se_cmd->se_tfo->get_fabric_name(),
> unpacked_lun);
> spin_unlock_irqrestore(&se_sess->se_node_acl->device_list_lock, flags);
> - return -EACCES;
> + return TCM_WRITE_PROTECTED;
> }
>
> if (se_cmd->data_direction == DMA_TO_DEVICE)
> @@ -109,23 +105,18 @@ int transport_lookup_cmd_lun(struct se_c
> * MappedLUN=0 exists for this Initiator Port.
> */
> if (unpacked_lun != 0) {
> - se_cmd->scsi_sense_reason = TCM_NON_EXISTENT_LUN;
> - se_cmd->se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> pr_err("TARGET_CORE[%s]: Detected NON_EXISTENT_LUN"
> " Access for 0x%08x\n",
> se_cmd->se_tfo->get_fabric_name(),
> unpacked_lun);
> - return -ENODEV;
> + return TCM_NON_EXISTENT_LUN;
> }
> /*
> * Force WRITE PROTECT for virtual LUN 0
> */
> if ((se_cmd->data_direction != DMA_FROM_DEVICE) &&
> - (se_cmd->data_direction != DMA_NONE)) {
> - se_cmd->scsi_sense_reason = TCM_WRITE_PROTECTED;
> - se_cmd->se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
> - return -EACCES;
> - }
> + (se_cmd->data_direction != DMA_NONE))
> + return TCM_WRITE_PROTECTED;
>
The transport_lookup_cmd_lun changes look reasonable enough..
> Index: lio-core/drivers/target/target_core_pr.c
> ===================================================================
> --- lio-core.orig/drivers/target/target_core_pr.c 2012-10-30 14:14:53.593253429 +0100
> +++ lio-core/drivers/target/target_core_pr.c 2012-10-30 14:15:09.909919998 +0100
> -static int core_scsi3_emulate_pro_register(
> - struct se_cmd *cmd,
> - u64 res_key,
> - u64 sa_res_key,
> - int aptpl,
> - int all_tg_pt,
> - int spec_i_pt,
> - int ignore_key)
> +static sense_reason_t
> +core_scsi3_emulate_pro_register(struct se_cmd *cmd, u64 res_key, u64 sa_res_key,
> + int aptpl, int all_tg_pt, int spec_i_pt, int ignore_key)
> {
> struct se_session *se_sess = cmd->se_sess;
> struct se_device *dev = cmd->se_dev;
> @@ -2079,12 +2054,12 @@ static int core_scsi3_emulate_pro_regist
> /* Used for APTPL metadata w/ UNREGISTER */
> unsigned char *pr_aptpl_buf = NULL;
> unsigned char isid_buf[PR_REG_ISID_LEN], *isid_ptr = NULL;
> - int pr_holder = 0, ret = 0, type;
> + sense_reason_t ret;
> + int pr_holder = 0, type;
>
<SNIP>
> +static sense_reason_t
> +core_scsi3_emulate_pro_release(struct se_cmd *cmd, int type, int scope,
> + u64 res_key)
> {
> struct se_device *dev = cmd->se_dev;
> struct se_session *se_sess = cmd->se_sess;
> struct se_lun *se_lun = cmd->se_lun;
> struct t10_pr_registration *pr_reg, *pr_reg_p, *pr_res_holder;
> struct t10_reservation *pr_tmpl = &dev->t10_pr;
> - int ret, all_reg = 0;
> + int all_reg = 0;
> + sense_reason_t ret = 0;
>
> if (!se_sess || !se_lun) {
> pr_err("SPC-3 PR: se_sess || struct se_lun is NULL!\n");
> - cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> - return -EINVAL;
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> }
> /*
> * Locate the existing *pr_reg via struct se_node_acl pointers
> @@ -2637,8 +2583,7 @@ static int core_scsi3_emulate_pro_releas
> if (!pr_reg) {
> pr_err("SPC-3 PR: Unable to locate"
> " PR_REGISTERED *pr_reg for RELEASE\n");
> - cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> - return -EINVAL;
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> }
> /*
> * From spc4r17 Section 5.7.11.2 Releasing:
> @@ -2659,7 +2604,6 @@ static int core_scsi3_emulate_pro_releas
> * No persistent reservation, return GOOD status.
> */
> spin_unlock(&dev->dev_reservation_lock);
> - core_scsi3_put_pr_reg(pr_reg);
> return 0;
> }
This is a bug to not put the newly allocated pr_reg before returning.
<SNIP>
>
> /*
> - * Used by transport_send_check_condition_and_sense() and se_cmd->scsi_sense_reason
> + * Used by transport_send_check_condition_and_sense()
> * to signal which ASC/ASCQ sense payload should be built.
> */
> +typedef unsigned __bitwise__ sense_reason_t;
> +
> enum tcm_sense_reason_table {
> - TCM_NON_EXISTENT_LUN = 0x01,
> - TCM_UNSUPPORTED_SCSI_OPCODE = 0x02,
> - TCM_INCORRECT_AMOUNT_OF_DATA = 0x03,
> - TCM_UNEXPECTED_UNSOLICITED_DATA = 0x04,
> - TCM_SERVICE_CRC_ERROR = 0x05,
> - TCM_SNACK_REJECTED = 0x06,
> - TCM_SECTOR_COUNT_TOO_MANY = 0x07,
> - TCM_INVALID_CDB_FIELD = 0x08,
> - TCM_INVALID_PARAMETER_LIST = 0x09,
> - TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE = 0x0a,
> - TCM_UNKNOWN_MODE_PAGE = 0x0b,
> - TCM_WRITE_PROTECTED = 0x0c,
> - TCM_CHECK_CONDITION_ABORT_CMD = 0x0d,
> - TCM_CHECK_CONDITION_UNIT_ATTENTION = 0x0e,
> - TCM_CHECK_CONDITION_NOT_READY = 0x0f,
> - TCM_RESERVATION_CONFLICT = 0x10,
> - TCM_ADDRESS_OUT_OF_RANGE = 0x11,
> +#define R(x) (__force sense_reason_t )(x)
> + TCM_NON_EXISTENT_LUN = R(0x01),
> + TCM_UNSUPPORTED_SCSI_OPCODE = R(0x02),
> + TCM_INCORRECT_AMOUNT_OF_DATA = R(0x03),
> + TCM_UNEXPECTED_UNSOLICITED_DATA = R(0x04),
> + TCM_SERVICE_CRC_ERROR = R(0x05),
> + TCM_SNACK_REJECTED = R(0x06),
> + TCM_SECTOR_COUNT_TOO_MANY = R(0x07),
> + TCM_INVALID_CDB_FIELD = R(0x08),
> + TCM_INVALID_PARAMETER_LIST = R(0x09),
> + TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE = R(0x0a),
> + TCM_UNKNOWN_MODE_PAGE = R(0x0b),
> + TCM_WRITE_PROTECTED = R(0x0c),
> + TCM_CHECK_CONDITION_ABORT_CMD = R(0x0d),
> + TCM_CHECK_CONDITION_UNIT_ATTENTION = R(0x0e),
> + TCM_CHECK_CONDITION_NOT_READY = R(0x0f),
> + TCM_RESERVATION_CONFLICT = R(0x10),
> + TCM_ADDRESS_OUT_OF_RANGE = R(0x11),
> + TCM_OUT_OF_RESOURCES = R(0x12),
> +#undef R
> };
>
> Index: lio-core/drivers/target/iscsi/iscsi_target.c
> ===================================================================
> --- lio-core.orig/drivers/target/iscsi/iscsi_target.c 2012-10-30 14:14:53.593253429 +0100
> +++ lio-core/drivers/target/iscsi/iscsi_target.c 2012-10-30 15:00:48.453236971 +0100
> @@ -767,9 +767,8 @@ static int iscsit_handle_scsi_cmd(
> struct iscsi_conn *conn,
> unsigned char *buf)
> {
<SNIP>
> @@ -1050,19 +1035,17 @@ attach_cmd:
> * thread. They are processed in CmdSN order by
> * iscsit_check_received_cmdsn() below.
> */
> - if (send_check_condition) {
> + if (cmd->sense_reason) {
> immed_ret = IMMEDIATE_DATA_NORMAL_OPERATION;
> - dump_immediate_data = 1;
> goto after_immediate_data;
> }
> /*
> * Call directly into transport_generic_new_cmd() to perform
> * the backend memory allocation.
> */
> - ret = transport_generic_new_cmd(&cmd->se_cmd);
> - if (ret < 0) {
> + cmd->sense_reason = transport_generic_new_cmd(&cmd->se_cmd);
> + if (cmd->sense_reason) {
> immed_ret = IMMEDIATE_DATA_NORMAL_OPERATION;
> - dump_immediate_data = 1;
> goto after_immediate_data;
> }
>
> @@ -1079,7 +1062,7 @@ after_immediate_data:
> * Special case for Unsupported SAM WRITE Opcodes
> * and ImmediateData=Yes.
> */
> - if (dump_immediate_data) {
> + if (cmd->sense_reason) {
> if (iscsit_dump_data_payload(conn, payload_length, 1) < 0)
> return -1;
> } else if (cmd->unsolicited_data) {
Nice work to get rid of send_check_condition and dump_immediate_data !
> @@ -1272,8 +1255,7 @@ static int iscsit_handle_data_out(struct
> spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
>
> spin_lock_irqsave(&se_cmd->t_state_lock, flags);
> - if (!(se_cmd->se_cmd_flags & SCF_SUPPORTED_SAM_OPCODE) ||
> - (se_cmd->se_cmd_flags & SCF_SCSI_CDB_EXCEPTION))
> + if (!(se_cmd->se_cmd_flags & SCF_SUPPORTED_SAM_OPCODE))
> dump_unsolicited_data = 1;
> spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
>
Should not this include a cmd->sense_reason check as well, if it was
previously checking SCF_SCSI_CDB_EXCEPTION..?
Adding this a separate increment patch going out shortly as well.
This large of a change obviously will require alot more testing from the
fabrics exception paths, but overall for target core I think it's worth
making the change within for-3.8 code..
Nice work Christoph !
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
