On Wed, 2012-10-10 at 17:37 -0400, Christoph Hellwig wrote:
> plain text document attachment (target-simplify-reservations)
> We do not support host-level reservations for the pscsi backend, and all
> virtual backends are newere than SCSI-2, so just make the combined
> SPC-3 + SCSI-2 support the only supported variant and kill the switches
> for the different implementations, given that this code handles the no-op
> version just fine.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
>
> ---
> drivers/target/target_core_configfs.c | 155 ++++++++-----------------
> drivers/target/target_core_device.c | 3
> drivers/target/target_core_pr.c | 197 +++++++++++----------------------
> drivers/target/target_core_pr.h | 2
> drivers/target/target_core_spc.c | 19 ---
> drivers/target/target_core_transport.c | 22 +--
> include/target/target_core_base.h | 29 ----
> 7 files changed, 134 insertions(+), 293 deletions(-)
>
> Index: lio-core/drivers/target/target_core_pr.c
> ===================================================================
> --- lio-core.orig/drivers/target/target_core_pr.c 2012-10-07 11:18:12.019183329 -0700
> +++ lio-core/drivers/target/target_core_pr.c 2012-10-08 04:07:59.802306474 -0700
> @@ -68,48 +68,39 @@ int core_pr_dump_initiator_port(
> static void __core_scsi3_complete_pro_release(struct se_device *, struct se_node_acl *,
> struct t10_pr_registration *, int);
>
> -static int core_scsi2_reservation_seq_non_holder(
> - struct se_cmd *cmd,
> - unsigned char *cdb,
> - u32 pr_reg_type)
> +static int target_scsi2_reservation_check(struct se_cmd *cmd)
> {
> - switch (cdb[0]) {
> + struct se_device *dev = cmd->se_dev;
> + struct se_session *sess = cmd->se_sess;
> + int ret = 0;
> +
> + switch (cmd->t_task_cdb[0]) {
> case INQUIRY:
> case RELEASE:
> case RELEASE_10:
> return 0;
> default:
> - return 1;
> + break;
> }
>
> - return 1;
> -}
> -
> -static int core_scsi2_reservation_check(struct se_cmd *cmd, u32 *pr_reg_type)
> -{
> - struct se_device *dev = cmd->se_dev;
> - struct se_session *sess = cmd->se_sess;
> - int ret;
> -
> - if (!sess)
> - return 0;
> -
> spin_lock(&dev->dev_reservation_lock);
> - if (!dev->dev_reserved_node_acl || !sess) {
> - spin_unlock(&dev->dev_reservation_lock);
> - return 0;
> - }
> + if (!dev->dev_reserved_node_acl || !sess)
> + goto out_unlock;
> +
> if (dev->dev_reserved_node_acl != sess->se_node_acl) {
> - spin_unlock(&dev->dev_reservation_lock);
> - return -EINVAL;
> + ret = -EBUSY;
> + goto out_unlock;
> }
> - if (!(dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS_WITH_ISID)) {
> - spin_unlock(&dev->dev_reservation_lock);
> - return 0;
> +
> + if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS_WITH_ISID) {
> + if (dev->dev_res_bin_isid != sess->sess_bin_isid) {
> + ret = -EBUSY;
> + goto out_unlock;
> + }
> }
> - ret = (dev->dev_res_bin_isid == sess->sess_bin_isid) ? 0 : -EINVAL;
> - spin_unlock(&dev->dev_reservation_lock);
>
> +out_unlock:
> + spin_unlock(&dev->dev_reservation_lock);
> return ret;
> }
>
> @@ -123,12 +114,8 @@ static int target_check_scsi2_reservatio
> struct se_device *dev = cmd->se_dev;
> struct t10_pr_registration *pr_reg;
> struct t10_reservation *pr_tmpl = &dev->t10_pr;
> - int crh = (dev->t10_pr.res_type == SPC3_PERSISTENT_RESERVATIONS);
> int conflict = 0;
>
> - if (!crh)
> - return -EINVAL;
> -
> pr_reg = core_scsi3_locate_pr_reg(cmd->se_dev, se_sess->se_node_acl,
> se_sess);
> if (pr_reg) {
> @@ -319,9 +306,9 @@ out:
> */
> static int core_scsi3_pr_seq_non_holder(
> struct se_cmd *cmd,
> - unsigned char *cdb,
> u32 pr_reg_type)
> {
> + unsigned char *cdb = cmd->t_task_cdb;
> struct se_dev_entry *se_deve;
> struct se_session *se_sess = cmd->se_sess;
> int other_cdb = 0, ignore_reg;
> @@ -330,17 +317,11 @@ static int core_scsi3_pr_seq_non_holder(
> int we = 0; /* Write Exclusive */
> int legacy = 0; /* Act like a legacy device and return
> * RESERVATION CONFLICT on some CDBs */
> - /*
> - * A legacy SPC-2 reservation is being held.
> - */
> - if (cmd->se_dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
> - return core_scsi2_reservation_seq_non_holder(cmd,
> - cdb, pr_reg_type);
>
> se_deve = se_sess->se_node_acl->device_list[cmd->orig_fe_lun];
> /*
> * Determine if the registration should be ignored due to
> - * non-matching ISIDs in core_scsi3_pr_reservation_check().
> + * non-matching ISIDs in target_scsi3_pr_reservation_check().
> */
> ignore_reg = (pr_reg_type & 0x80000000);
> if (ignore_reg)
> @@ -563,6 +544,40 @@ static int core_scsi3_pr_seq_non_holder(
> return 1; /* Conflict by default */
> }
>
> +static int target_scsi3_pr_reservation_check(struct se_cmd *cmd)
> +{
> + struct se_device *dev = cmd->se_dev;
> + struct se_session *sess = cmd->se_sess;
> + u32 pr_reg_type;
> +
> + spin_lock(&dev->dev_reservation_lock);
> + if (!dev->dev_pr_res_holder)
> + goto out_unlock;
> +
> + pr_reg_type = dev->dev_pr_res_holder->pr_res_type;
> + cmd->pr_res_key = dev->dev_pr_res_holder->pr_res_key;
> + if (dev->dev_pr_res_holder->pr_reg_nacl != sess->se_node_acl)
> + goto check_nonholder;
> +
> + if (dev->dev_pr_res_holder->isid_present_at_reg) {
> + if (dev->dev_pr_res_holder->pr_reg_bin_isid !=
> + sess->sess_bin_isid) {
> + pr_reg_type |= 0x80000000;
> + goto check_nonholder;
> + }
> + }
> +
> +out_unlock:
> + spin_unlock(&dev->dev_reservation_lock);
> + return 0;
> +
> +check_nonholder:
> + spin_unlock(&dev->dev_reservation_lock);
> + if (core_scsi3_pr_seq_non_holder(cmd, pr_reg_type))
> + return -EBUSY;
> + return 0;
> +}
> +
> static u32 core_scsi3_pr_generation(struct se_device *dev)
> {
> u32 prg;
> @@ -583,50 +598,6 @@ static u32 core_scsi3_pr_generation(stru
> return prg;
> }
>
> -static int core_scsi3_pr_reservation_check(
> - struct se_cmd *cmd,
> - u32 *pr_reg_type)
> -{
> - struct se_device *dev = cmd->se_dev;
> - struct se_session *sess = cmd->se_sess;
> - int ret;
> -
> - if (!sess)
> - return 0;
> - /*
> - * A legacy SPC-2 reservation is being held.
> - */
> - if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
> - return core_scsi2_reservation_check(cmd, pr_reg_type);
> -
> - spin_lock(&dev->dev_reservation_lock);
> - if (!dev->dev_pr_res_holder) {
> - spin_unlock(&dev->dev_reservation_lock);
> - return 0;
> - }
> - *pr_reg_type = dev->dev_pr_res_holder->pr_res_type;
> - cmd->pr_res_key = dev->dev_pr_res_holder->pr_res_key;
> - if (dev->dev_pr_res_holder->pr_reg_nacl != sess->se_node_acl) {
> - spin_unlock(&dev->dev_reservation_lock);
> - return -EINVAL;
> - }
> - if (!dev->dev_pr_res_holder->isid_present_at_reg) {
> - spin_unlock(&dev->dev_reservation_lock);
> - return 0;
> - }
> - ret = (dev->dev_pr_res_holder->pr_reg_bin_isid ==
> - sess->sess_bin_isid) ? 0 : -EINVAL;
> - /*
> - * Use bit in *pr_reg_type to notify ISID mismatch in
> - * core_scsi3_pr_seq_non_holder().
> - */
> - if (ret != 0)
> - *pr_reg_type |= 0x80000000;
> - spin_unlock(&dev->dev_reservation_lock);
> -
> - return ret;
> -}
> -
> static struct t10_pr_registration *__core_scsi3_do_alloc_registration(
> struct se_device *dev,
> struct se_node_acl *nacl,
> @@ -998,7 +969,7 @@ int core_scsi3_check_aptpl_registration(
> struct se_node_acl *nacl = lun_acl->se_lun_nacl;
> struct se_dev_entry *deve = nacl->device_list[lun_acl->mapped_lun];
>
> - if (dev->t10_pr.res_type != SPC3_PERSISTENT_RESERVATIONS)
> + if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
> return 0;
>
> return __core_scsi3_check_aptpl_registration(dev, tpg, lun,
> @@ -4343,49 +4314,19 @@ int target_scsi3_emulate_pr_in(struct se
> return ret;
> }
>
> -static int core_pt_reservation_check(struct se_cmd *cmd, u32 *pr_res_type)
> +int target_check_reservation(struct se_cmd *cmd)
> {
> - return 0;
> -}
> -
> -static int core_pt_seq_non_holder(
> - struct se_cmd *cmd,
> - unsigned char *cdb,
> - u32 pr_reg_type)
> -{
> - return 0;
> -}
> + struct se_device *dev = cmd->se_dev;
>
> -void core_setup_reservations(struct se_device *dev)
> -{
> - struct t10_reservation *rest = &dev->t10_pr;
> + if (!cmd->se_sess)
> + return 0;
> + if (dev->se_hba->hba_flags & HBA_FLAGS_INTERNAL_USE)
> + return 0;
> + if (dev->transport->transport_type == TRANSPORT_PLUGIN_PHBA_PDEV)
> + return 0;
>
> - /*
> - * If this device is from Target_Core_Mod/pSCSI, use the reservations
> - * of the Underlying SCSI hardware. In Linux/SCSI terms, this can
> - * cause a problem because libata and some SATA RAID HBAs appear
> - * under Linux/SCSI, but to emulate reservations themselves.
> - */
> - if ((dev->se_hba->hba_flags & HBA_FLAGS_INTERNAL_USE) ||
> - (dev->transport->transport_type == TRANSPORT_PLUGIN_PHBA_PDEV &&
> - !dev->dev_attrib.emulate_reservations)) {
> - rest->res_type = SPC_PASSTHROUGH;
> - rest->pr_ops.t10_reservation_check = &core_pt_reservation_check;
> - rest->pr_ops.t10_seq_non_holder = &core_pt_seq_non_holder;
> - pr_debug("%s: Using SPC_PASSTHROUGH, no reservation"
> - " emulation\n", dev->transport->name);
> - } else if (dev->transport->get_device_rev(dev) >= SCSI_3) {
> - rest->res_type = SPC3_PERSISTENT_RESERVATIONS;
> - rest->pr_ops.t10_reservation_check = &core_scsi3_pr_reservation_check;
> - rest->pr_ops.t10_seq_non_holder = &core_scsi3_pr_seq_non_holder;
> - pr_debug("%s: Using SPC3_PERSISTENT_RESERVATIONS"
> - " emulation\n", dev->transport->name);
> - } else {
> - rest->res_type = SPC2_RESERVATIONS;
> - rest->pr_ops.t10_reservation_check = &core_scsi2_reservation_check;
> - rest->pr_ops.t10_seq_non_holder =
> - &core_scsi2_reservation_seq_non_holder;
> - pr_debug("%s: Using SPC2_RESERVATIONS emulation\n",
> - dev->transport->name);
> - }
> + if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
> + return target_scsi2_reservation_check(cmd);
> + else
> + return target_scsi3_pr_reservation_check(cmd);
> }
Mmmm, the unprotected use of ->dev_reservation_flags to check for legacy
reservations within target_check_reservation looks wrong to me..
Both target_scsi2_reservation_[reserve,release] already hold
->dev_reservation_lock, so we'd also need to obtain the same lock here
with your patch to prevent different process context from issuing
RESERVE/RELEASE and potentially hitting the wrong reservations code-path
when multiple initiators try to switch between the two modes.
I certainly like the cleanups + simplifications in this patch, but this
will need to be addressed before merging. So NACK on this for the
moment.
--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
