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. 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.
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
