On Thu, 2012-10-11 at 11:39 -0400, Christoph Hellwig wrote:
> On Thu, Oct 11, 2012 at 09:26:22AM -0400, Christoph Hellwig wrote:
> > It is racy, but the race is not in any way new in this patch, the check
> > just moved from core_scsi3_pr_reservation_check to target_check_reservation
> > as part of the cleanup.
> >
> > I'll send you a patch ontop of the series to fix it up, which will be
> > a lot easier than respinning it and also keep unrelated changes apart.
>
> FYI here is the patch for the easy spots where we can extended the lock
> coverage.
So for the moment, folding this into WIP #2 and applying to for-next
along with the two other patches from your original series.
> I don't really think it's all that useful as basically all
> code in target_core_pr.c needs this lock, and for the various sleeping
> operations the existing spinlock won't be useable.
>
Indeed. Still thinking on how to best address the fallout this all
introduces, along with fixing the other unprotected uses of
DRF_SPC2_RESERVATIONS..
With modern target_submit_cmd() cmwq usage, there are sections of PR-OUT
CDB emulation that need to be under the device reservations lock as
well. Converting to a sleeping per-device mutex in the end is probably
going to make the most sense here..
In any event, I'll try out a couple of difference approaches and post a
patch over the next days.
Thanks hch!
--nab
> Index: lio-core/drivers/target/target_core_configfs.c
> ===================================================================
> --- lio-core.orig/drivers/target/target_core_configfs.c 2012-10-10 14:21:37.176134866 -0700
> +++ lio-core/drivers/target/target_core_configfs.c 2012-10-11 07:40:37.068164938 -0700
> @@ -977,28 +977,20 @@ static ssize_t target_core_dev_pr_show_s
> struct t10_pr_registration *pr_reg;
> char i_buf[PR_REG_ISID_ID_LEN];
> int prf_isid;
> - ssize_t len;
>
> memset(i_buf, 0, PR_REG_ISID_ID_LEN);
>
> - spin_lock(&dev->dev_reservation_lock);
> pr_reg = dev->dev_pr_res_holder;
> - if (!pr_reg) {
> - len = sprintf(page, "No SPC-3 Reservation holder\n");
> - goto out_unlock;
> - }
> + if (!pr_reg)
> + return sprintf(page, "No SPC-3 Reservation holder\n");
>
> se_nacl = pr_reg->pr_reg_nacl;
> prf_isid = core_pr_dump_initiator_port(pr_reg, &i_buf[0],
> PR_REG_ISID_ID_LEN);
>
> - len = sprintf(page, "SPC-3 Reservation: %s Initiator: %s%s\n",
> + return sprintf(page, "SPC-3 Reservation: %s Initiator: %s%s\n",
> se_nacl->se_tpg->se_tpg_tfo->get_fabric_name(),
> se_nacl->initiatorname, (prf_isid) ? &i_buf[0] : "");
> -
> -out_unlock:
> - spin_unlock(&dev->dev_reservation_lock);
> - return len;
> }
>
> static ssize_t target_core_dev_pr_show_spc2_res(struct se_device *dev,
> @@ -1007,7 +999,6 @@ static ssize_t target_core_dev_pr_show_s
> struct se_node_acl *se_nacl;
> ssize_t len;
>
> - spin_lock(&dev->dev_reservation_lock);
> se_nacl = dev->dev_reserved_node_acl;
> if (se_nacl) {
> len = sprintf(page,
> @@ -1017,20 +1008,24 @@ static ssize_t target_core_dev_pr_show_s
> } else {
> len = sprintf(page, "No SPC-2 Reservation holder\n");
> }
> -
> - spin_unlock(&dev->dev_reservation_lock);
> return len;
> }
>
> static ssize_t target_core_dev_pr_show_attr_res_holder(struct se_device *dev,
> char *page)
> {
> + int ret;
> +
> if (dev->transport->transport_type == TRANSPORT_PLUGIN_PHBA_PDEV)
> return sprintf(page, "Passthrough\n");
> - else if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
> - return target_core_dev_pr_show_spc2_res(dev, page);
> +
> + spin_lock(&dev->dev_reservation_lock);
> + if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
> + ret = target_core_dev_pr_show_spc2_res(dev, page);
> else
> - return target_core_dev_pr_show_spc3_res(dev, page);
> + ret = target_core_dev_pr_show_spc3_res(dev, page);
> + spin_unlock(&dev->dev_reservation_lock);
> + return ret;
> }
>
> SE_DEV_PR_ATTR_RO(res_holder);
> Index: lio-core/drivers/target/target_core_pr.c
> ===================================================================
> --- lio-core.orig/drivers/target/target_core_pr.c 2012-10-10 14:21:36.772134855 -0700
> +++ lio-core/drivers/target/target_core_pr.c 2012-10-11 07:39:35.896163373 -0700
> @@ -72,7 +72,6 @@ static int target_scsi2_reservation_chec
> {
> 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:
> @@ -83,25 +82,18 @@ static int target_scsi2_reservation_chec
> break;
> }
>
> - spin_lock(&dev->dev_reservation_lock);
> if (!dev->dev_reserved_node_acl || !sess)
> - goto out_unlock;
> + return 0;
>
> - if (dev->dev_reserved_node_acl != sess->se_node_acl) {
> - ret = -EBUSY;
> - goto out_unlock;
> - }
> + if (dev->dev_reserved_node_acl != sess->se_node_acl)
> + return -EBUSY;
>
> 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;
> - }
> + if (dev->dev_res_bin_isid != sess->sess_bin_isid)
> + return -EBUSY;
> }
>
> -out_unlock:
> - spin_unlock(&dev->dev_reservation_lock);
> - return ret;
> + return 0;
> }
>
> static struct t10_pr_registration *core_scsi3_locate_pr_reg(struct se_device *,
> @@ -550,9 +542,8 @@ static int target_scsi3_pr_reservation_c
> 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;
> + 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;
> @@ -567,12 +558,9 @@ static int target_scsi3_pr_reservation_c
> }
> }
>
> -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;
> @@ -4317,6 +4305,7 @@ int target_scsi3_emulate_pr_in(struct se
> int target_check_reservation(struct se_cmd *cmd)
> {
> struct se_device *dev = cmd->se_dev;
> + int ret;
>
> if (!cmd->se_sess)
> return 0;
> @@ -4325,8 +4314,12 @@ int target_check_reservation(struct se_c
> if (dev->transport->transport_type == TRANSPORT_PLUGIN_PHBA_PDEV)
> return 0;
>
> + spin_lock(&dev->dev_reservation_lock);
> if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
> - return target_scsi2_reservation_check(cmd);
> + ret = target_scsi2_reservation_check(cmd);
> else
> - return target_scsi3_pr_reservation_check(cmd);
> + ret = target_scsi3_pr_reservation_check(cmd);
> + spin_unlock(&dev->dev_reservation_lock);
> +
> + return ret;
> }
> --
> 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
--
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
