Hi Ilias,
Thanks for your patch. Comments are inline below.
On Fri, 2015-03-20 at 13:39 +0200, 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(
No need for an explicit inline. The compiler will do the right thing.
> + 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"
So doing the extra ALL_REG check here is wrong..
APTPL metadata must have one res_holder=1 registration in order to
determine which *pr_reg to set for dev->dev_pr_res_holder when
rebuilding PR state.
This is because dev->dev_pr_res_holder is a single *pr_reg as per our
previous discussions and not a list, and for active registrations when a
ALL_REG reservation is set, we just pretend it's the reservation holder
too but don't actually change dev->dev_pr_res_holder.
That said, please drop this part.
> @@ -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);
For register_and_move code this check is redundant btw, because an
ALL_REG check already exists immediately after this one. I'd prefer to
keep the existing comment + check for this special case. Please drop
this part too.
Rest of the patch looks fine.
Care to address these three comments, and respin a -v2..?
--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
