On 03/20/2015 04:39 AM, Ilias Tsitsimpis wrote:
> Fix two issues with AllRegistrants reservations where the code
> didn't handle all of the registered devices as reservation
> holders.
>
> At the same time, introduce a helper function named
> 'is_reservation_holder()' that properly checks if a device is a
> reservation holder, taking into account the reservation type. This
> function cleans up the code and improves readability.
>
> Signed-off-by: Ilias Tsitsimpis <iliastsi@xxxxxxxxxxx>
> Signed-off-by: Vangelis Koukis <vkoukis@xxxxxxxxxxx> ---
>
> Hi Nicholas,
>
> Continuing our conversation from December 19, at thread "[PATCH
> 0/2] target: Fixes for AllRegistrants reservation handling" I
> identified two more cases where the code didn't correctly handle
> ALLREG. Since you said that you would prefer identify any remaining
> ALLREG specific issues and address them individually, I have
> created this patch to address the above issues.
>
> At the same time this patch introduces the function
> 'is_reservation_holder()' which properly checks if a device is a
> reservation holder, correctly handling the ALLREG cases. This
> function cleans up the code and improves readability.
>
> Thanks, Ilias
>
>
> drivers/target/target_core_pr.c | 38
> ++++++++++++++++++++++++-------------- 1 file changed, 24
> insertions(+), 14 deletions(-)
>
> diff --git a/drivers/target/target_core_pr.c
> b/drivers/target/target_core_pr.c index 2de6fb8..ca9fa61 100644 ---
> a/drivers/target/target_core_pr.c +++
> b/drivers/target/target_core_pr.c @@ -78,6 +78,22 @@ enum
> preempt_type { static void __core_scsi3_complete_pro_release(struct
> se_device *, struct se_node_acl *, struct t10_pr_registration *,
> int, int);
>
> +static inline int is_reservation_holder( + struct
> t10_pr_registration *pr_res_holder, + struct t10_pr_registration
> *pr_reg) +{ + int pr_res_type; + + if (pr_res_holder) { +
> pr_res_type = pr_res_holder->pr_res_type; + + return pr_res_holder
> == pr_reg || + pr_res_type ==
> PR_TYPE_WRITE_EXCLUSIVE_ALLREG || + pr_res_type ==
> PR_TYPE_EXCLUSIVE_ACCESS_ALLREG; + } + return 0; +} + static
> sense_reason_t target_scsi2_reservation_check(struct se_cmd *cmd)
> { @@ -1825,6 +1841,7 @@ static int core_scsi3_update_aptpl_buf(
> ssize_t len = 0; int reg_count = 0; int ret = 0; + struct
> t10_pr_registration *pr_res_holder = dev->dev_pr_res_holder;
>
> spin_lock(&dev->dev_reservation_lock);
> spin_lock(&dev->t10_pr.registration_lock); @@ -1849,7 +1866,7 @@
> static int core_scsi3_update_aptpl_buf( * Include special metadata
> if the pr_reg matches the * reservation holder. */ - if
> (dev->dev_pr_res_holder == pr_reg) { + if
> (is_reservation_holder(pr_res_holder, pr_reg)) { snprintf(tmp, 512,
> "PR_REG_START: %d" "\ninitiator_fabric=%s\n"
> "initiator_node=%s\n%s" @@ -1859,8 +1876,9 @@ static int
> core_scsi3_update_aptpl_buf( "mapped_lun=%u\n", reg_count,
> tpg->se_tpg_tfo->get_fabric_name(),
> pr_reg->pr_reg_nacl->initiatorname, isid_buf, -
> pr_reg->pr_res_key, pr_reg->pr_res_type, - pr_reg->pr_res_scope,
> pr_reg->pr_reg_all_tg_pt, + pr_reg->pr_res_key,
> pr_res_holder->pr_res_type, + pr_res_holder->pr_res_scope, +
> pr_reg->pr_reg_all_tg_pt, pr_reg->pr_res_mapped_lun); } else {
> snprintf(tmp, 512, "PR_REG_START: %d\n" @@ -2287,7 +2305,6 @@
> core_scsi3_pro_reserve(struct se_cmd *cmd, int type, int scope, u64
> res_key) spin_lock(&dev->dev_reservation_lock); pr_res_holder =
> dev->dev_pr_res_holder; if (pr_res_holder) { - int pr_res_type =
> pr_res_holder->pr_res_type; /* * From spc4r17 Section 5.7.9:
> Reserving: * @@ -2298,9 +2315,7 @@ core_scsi3_pro_reserve(struct
> se_cmd *cmd, int type, int scope, u64 res_key) * the logical unit,
> then the command shall be completed with * RESERVATION CONFLICT
> status. */ - if ((pr_res_holder != pr_reg) && - (pr_res_type
> != PR_TYPE_WRITE_EXCLUSIVE_ALLREG) && - (pr_res_type !=
> PR_TYPE_EXCLUSIVE_ACCESS_ALLREG)) { + if
> (!is_reservation_holder(pr_res_holder, pr_reg)) { struct
> se_node_acl *pr_res_nacl = pr_res_holder->pr_reg_nacl;
> pr_err("SPC-3 PR: Attempted RESERVE from" " [%s]: %s while
> reservation already held by" @@ -2477,7 +2492,6 @@
> core_scsi3_emulate_pro_release(struct se_cmd *cmd, int type, int
> scope, 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 all_reg = 0;
> sense_reason_t ret = 0;
>
> if (!se_sess || !se_lun) { @@ -2514,13 +2528,9 @@
> core_scsi3_emulate_pro_release(struct se_cmd *cmd, int type, int
> scope, spin_unlock(&dev->dev_reservation_lock); goto
> out_put_pr_reg; } - if ((pr_res_holder->pr_res_type ==
> PR_TYPE_WRITE_EXCLUSIVE_ALLREG) || -
> (pr_res_holder->pr_res_type == PR_TYPE_EXCLUSIVE_ACCESS_ALLREG)) -
> all_reg = 1;
>
> - if ((all_reg == 0) && (pr_res_holder != pr_reg)) { + if
> (!is_reservation_holder(pr_res_holder, pr_reg)) { /* - * Non 'All
> Registrants' PR Type cases.. * Release request from a registered
> I_T nexus that is not a * persistent reservation holder. return
> GOOD status. */ @@ -3375,7 +3385,7 @@ after_iport_check: * From
> spc4r17 section 5.7.8 Table 50 -- * Register behaviors for a
> REGISTER AND MOVE service action */ - if (pr_res_holder != pr_reg)
> { + if (!is_reservation_holder(pr_res_holder, pr_reg)) {
> pr_warn("SPC-3 PR REGISTER_AND_MOVE: Calling I_T" " Nexus is not
> reservation holder\n"); spin_unlock(&dev->dev_reservation_lock);
>
Reviewed-by: Lee Duncan <lduncan@xxxxxxxx>
--
Lee
--
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
