Hi Steven,
On Fri, 2014-10-24 at 15:18 -0700, Steven Allen wrote:
> PREEMPT (and PREEMPT AND ABORT) should return CONFLICT iff a specified
> SERVICE ACTION RESERVATION KEY is specified and matches no existing
> persistent reservation.
>
> Without this patch, a PREEMPT will return CONFLICT if either all
> reservations are held by the initiator (self preemption) or there is
> nothing to preempt. According to the spec, both of these cases should
> succeed.
Just curious which wording in the spec explicitly mentions this part..?
> ---
> drivers/target/target_core_pr.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 3013287..8607802 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -2758,7 +2758,8 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
> struct t10_pr_registration *pr_reg, *pr_reg_tmp, *pr_reg_n, *pr_res_holder;
> struct t10_reservation *pr_tmpl = &dev->t10_pr;
> u32 pr_res_mapped_lun = 0;
> - int all_reg = 0, calling_it_nexus = 0, released_regs = 0;
> + int all_reg = 0, calling_it_nexus = 0;
> + bool sa_res_key_unmatched = sa_res_key != 0;
> int prh_type = 0, prh_scope = 0;
>
> if (!se_sess)
> @@ -2833,6 +2834,7 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
> if (!all_reg) {
> if (pr_reg->pr_res_key != sa_res_key)
> continue;
> + sa_res_key_unmatched = false;
>
> calling_it_nexus = (pr_reg_n == pr_reg) ? 1 : 0;
> pr_reg_nacl = pr_reg->pr_reg_nacl;
> @@ -2840,7 +2842,6 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
> __core_scsi3_free_registration(dev, pr_reg,
> (preempt_type == PREEMPT_AND_ABORT) ? &preempt_and_abort_list :
> NULL, calling_it_nexus);
> - released_regs++;
> } else {
> /*
> * Case for any existing all registrants type
> @@ -2858,6 +2859,7 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
> if ((sa_res_key) &&
> (pr_reg->pr_res_key != sa_res_key))
> continue;
> + sa_res_key_unmatched = false;
>
> calling_it_nexus = (pr_reg_n == pr_reg) ? 1 : 0;
> if (calling_it_nexus)
Mmmm, I think the sa_res_key_unmatched = false assignment might need to
be after the if (calling_it_nexus) conditional here..
Otherwise, a calling_it_nexus = true might incorrectly reach the block
of code beyond the if (sa_res_key_unmatched) check below..
> @@ -2868,7 +2870,6 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
> __core_scsi3_free_registration(dev, pr_reg,
> (preempt_type == PREEMPT_AND_ABORT) ? &preempt_and_abort_list :
> NULL, 0);
> - released_regs++;
> }
> if (!calling_it_nexus)
> core_scsi3_ua_allocate(pr_reg_nacl,
> @@ -2883,7 +2884,7 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
> * registered reservation key, then the device server shall
> * complete the command with RESERVATION CONFLICT status.
> */
> - if (!released_regs) {
> + if (sa_res_key_unmatched) {
> spin_unlock(&dev->dev_reservation_lock);
> core_scsi3_put_pr_reg(pr_reg_n);
> return TCM_RESERVATION_CONFLICT;
Rest of the patch looks reasonable to me.
Please elaborate a bit more on the spec reference + fix the issue above,
and then I'll apply to target-pending/master.
Thanks!
--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
