[PATCH 0/2] target: Fixes for AllRegistrants reservation handling

[PATCH 0/2] target: Fixes for AllRegistrants reservation handling

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi all,

This series addresses two issues raised recently by Ilias wrt
AllRegistrants reservation handling in target code that does not
adhere to SPC-4 specification requirements.

This first is a informational change to PR-IN READ_FULL_STATUS,
that when an AllRegistrants reservation is in place, all active
registrations should be setting R_HOLDER=1 within their respective
descriptors.

The second is a functional change to PR-OUT REGISTER w/ SARK=0
operation, to avoid dropping the AllRegistrants reservation until
the last registered I_T nexus has been released.  It also ensures
that the correct reservation type + scope is retained when the
new reservation is set within __core_scsi3_complete_pro_release()
for this AllRegistrants special case.

Also, thanks to James for the extra SPC-4 clarifications.

Please review.

--nab

Nicholas Bellinger (2):
  target: Fix R_HOLDER bit usage for AllRegistrants
  target: Avoid dropping AllRegistrants reservation during unregister

 drivers/target/target_core_pr.c | 113 +++++++++++++++++++++++++++++++---------
 1 file changed, 88 insertions(+), 25 deletions(-)

-- 
1.9.1

[PATCH 1/2] target: Fix R_HOLDER bit usage for AllRegistrants

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch fixes the usage of R_HOLDER bit for an All Registrants
reservation in READ_FULL_STATUS, where only the registration who
issued RESERVE was being reported as having an active reservation.

It changes core_scsi3_pri_read_full_status() to check ahead of the
list walk of active registrations to see if All Registrants is active,
and if so set R_HOLDER bit and scope/type fields for all active
registrations.

Reported-by: Ilias Tsitsimpis <i.tsitsimpis@gmail.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_pr.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index f91b6a1..c4a8da5 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3834,7 +3834,8 @@ core_scsi3_pri_read_full_status(struct se_cmd *cmd)
 	unsigned char *buf;
 	u32 add_desc_len = 0, add_len = 0, desc_len, exp_desc_len;
 	u32 off = 8; /* off into first Full Status descriptor */
-	int format_code = 0;
+	int format_code = 0, pr_res_type = 0, pr_res_scope = 0;
+	bool all_reg = false;
 
 	if (cmd->data_length < 8) {
 		pr_err("PRIN SA READ_FULL_STATUS SCSI Data Length: %u"
@@ -3851,6 +3852,19 @@ core_scsi3_pri_read_full_status(struct se_cmd *cmd)
 	buf[2] = ((dev->t10_pr.pr_generation >> 8) & 0xff);
 	buf[3] = (dev->t10_pr.pr_generation & 0xff);
 
+	spin_lock(&dev->dev_reservation_lock);
+	if (dev->dev_pr_res_holder) {
+		struct t10_pr_registration *pr_holder = dev->dev_pr_res_holder;
+
+		if (pr_holder->pr_res_type == PR_TYPE_WRITE_EXCLUSIVE_ALLREG ||
+		    pr_holder->pr_res_type == PR_TYPE_EXCLUSIVE_ACCESS_ALLREG) {
+			all_reg = true;
+			pr_res_type = pr_holder->pr_res_type;
+			pr_res_scope = pr_holder->pr_res_scope;
+		}
+	}
+	spin_unlock(&dev->dev_reservation_lock);
+
 	spin_lock(&pr_tmpl->registration_lock);
 	list_for_each_entry_safe(pr_reg, pr_reg_tmp,
 			&pr_tmpl->registration_list, pr_reg_list) {
@@ -3898,14 +3912,20 @@ core_scsi3_pri_read_full_status(struct se_cmd *cmd)
 		 * reservation holder for PR_HOLDER bit.
 		 *
 		 * Also, if this registration is the reservation
-		 * holder, fill in SCOPE and TYPE in the next byte.
+		 * holder or there is an All Registrants reservation
+		 * active, fill in SCOPE and TYPE in the next byte.
 		 */
 		if (pr_reg->pr_res_holder) {
 			buf[off++] |= 0x01;
 			buf[off++] = (pr_reg->pr_res_scope & 0xf0) |
 				     (pr_reg->pr_res_type & 0x0f);
-		} else
+		} else if (all_reg) {
+			buf[off++] |= 0x01;
+			buf[off++] = (pr_res_scope & 0xf0) |
+				     (pr_res_type & 0x0f);
+		} else {
 			off += 2;
+		}
 
 		off += 4; /* Skip over reserved area */
 		/*
-- 
1.9.1

[PATCH 2/2] target: Avoid dropping AllRegistrants reservation during unregister

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch fixes an issue with AllRegistrants reservations where
an unregister operation by the I_T nexus reservation holder would
incorrectly drop the reservation, instead of waiting until the
last active I_T nexus is unregistered as per SPC-4.

This includes updating __core_scsi3_complete_pro_release() to reset
dev->dev_pr_res_holder with another pr_reg for this special case,
as well as a new 'unreg' parameter to determine when the release
is occuring from an implicit unregister, vs. explicit RELEASE.

It also adds special handling in core_scsi3_free_pr_reg_from_nacl()
to release the left-over pr_res_holder, now that pr_reg is deleted
from pr_reg_list within __core_scsi3_complete_pro_release().

Reported-by: Ilias Tsitsimpis <i.tsitsimpis@gmail.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_pr.c | 87 ++++++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 22 deletions(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index c4a8da5..703890c 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -76,7 +76,7 @@ enum preempt_type {
 };
 
 static void __core_scsi3_complete_pro_release(struct se_device *, struct se_node_acl *,
-			struct t10_pr_registration *, int);
+					      struct t10_pr_registration *, int, int);
 
 static sense_reason_t
 target_scsi2_reservation_check(struct se_cmd *cmd)
@@ -1177,7 +1177,7 @@ static int core_scsi3_check_implicit_release(
 		 *    service action with the SERVICE ACTION RESERVATION KEY
 		 *    field set to zero (see 5.7.11.3).
 		 */
-		__core_scsi3_complete_pro_release(dev, nacl, pr_reg, 0);
+		__core_scsi3_complete_pro_release(dev, nacl, pr_reg, 0, 1);
 		ret = 1;
 		/*
 		 * For 'All Registrants' reservation types, all existing
@@ -1219,7 +1219,8 @@ static void __core_scsi3_free_registration(
 
 	pr_reg->pr_reg_deve->def_pr_registered = 0;
 	pr_reg->pr_reg_deve->pr_res_key = 0;
-	list_del(&pr_reg->pr_reg_list);
+	if (!list_empty(&pr_reg->pr_reg_list))
+		list_del(&pr_reg->pr_reg_list);
 	/*
 	 * Caller accessing *pr_reg using core_scsi3_locate_pr_reg(),
 	 * so call core_scsi3_put_pr_reg() to decrement our reference.
@@ -1271,6 +1272,7 @@ void core_scsi3_free_pr_reg_from_nacl(
 {
 	struct t10_reservation *pr_tmpl = &dev->t10_pr;
 	struct t10_pr_registration *pr_reg, *pr_reg_tmp, *pr_res_holder;
+	bool free_reg = false;
 	/*
 	 * If the passed se_node_acl matches the reservation holder,
 	 * release the reservation.
@@ -1278,13 +1280,18 @@ void core_scsi3_free_pr_reg_from_nacl(
 	spin_lock(&dev->dev_reservation_lock);
 	pr_res_holder = dev->dev_pr_res_holder;
 	if ((pr_res_holder != NULL) &&
-	    (pr_res_holder->pr_reg_nacl == nacl))
-		__core_scsi3_complete_pro_release(dev, nacl, pr_res_holder, 0);
+	    (pr_res_holder->pr_reg_nacl == nacl)) {
+		__core_scsi3_complete_pro_release(dev, nacl, pr_res_holder, 0, 1);
+		free_reg = true;
+	}
 	spin_unlock(&dev->dev_reservation_lock);
 	/*
 	 * Release any registration associated with the struct se_node_acl.
 	 */
 	spin_lock(&pr_tmpl->registration_lock);
+	if (pr_res_holder && free_reg)
+		__core_scsi3_free_registration(dev, pr_res_holder, NULL, 0);
+
 	list_for_each_entry_safe(pr_reg, pr_reg_tmp,
 			&pr_tmpl->registration_list, pr_reg_list) {
 
@@ -1307,7 +1314,7 @@ void core_scsi3_free_all_registrations(
 	if (pr_res_holder != NULL) {
 		struct se_node_acl *pr_res_nacl = pr_res_holder->pr_reg_nacl;
 		__core_scsi3_complete_pro_release(dev, pr_res_nacl,
-				pr_res_holder, 0);
+						  pr_res_holder, 0, 0);
 	}
 	spin_unlock(&dev->dev_reservation_lock);
 
@@ -2103,13 +2110,13 @@ core_scsi3_emulate_pro_register(struct se_cmd *cmd, u64 res_key, u64 sa_res_key,
 		/*
 		 * sa_res_key=0 Unregister Reservation Key for registered I_T Nexus.
 		 */
-		pr_holder = core_scsi3_check_implicit_release(
-				cmd->se_dev, pr_reg);
+		type = pr_reg->pr_res_type;
+		pr_holder = core_scsi3_check_implicit_release(cmd->se_dev,
+							      pr_reg);
 		if (pr_holder < 0) {
 			ret = TCM_RESERVATION_CONFLICT;
 			goto out;
 		}
-		type = pr_reg->pr_res_type;
 
 		spin_lock(&pr_tmpl->registration_lock);
 		/*
@@ -2383,23 +2390,59 @@ static void __core_scsi3_complete_pro_release(
 	struct se_device *dev,
 	struct se_node_acl *se_nacl,
 	struct t10_pr_registration *pr_reg,
-	int explicit)
+	int explicit,
+	int unreg)
 {
 	struct target_core_fabric_ops *tfo = se_nacl->se_tpg->se_tpg_tfo;
 	char i_buf[PR_REG_ISID_ID_LEN];
+	int pr_res_type = 0, pr_res_scope = 0;
 
 	memset(i_buf, 0, PR_REG_ISID_ID_LEN);
 	core_pr_dump_initiator_port(pr_reg, i_buf, PR_REG_ISID_ID_LEN);
 	/*
 	 * Go ahead and release the current PR reservation holder.
+	 * If an All Registrants reservation is currently active and
+	 * a unregister operation is requested, replace the current
+	 * dev_pr_res_holder with another active registration.
 	 */
-	dev->dev_pr_res_holder = NULL;
+	if (dev->dev_pr_res_holder) {
+		pr_res_type = dev->dev_pr_res_holder->pr_res_type;
+		pr_res_scope = dev->dev_pr_res_holder->pr_res_scope;
+		dev->dev_pr_res_holder->pr_res_type = 0;
+		dev->dev_pr_res_holder->pr_res_scope = 0;
+		dev->dev_pr_res_holder->pr_res_holder = 0;
+		dev->dev_pr_res_holder = NULL;
+	}
+	if (!unreg)
+		goto out;
 
-	pr_debug("SPC-3 PR [%s] Service Action: %s RELEASE cleared"
-		" reservation holder TYPE: %s ALL_TG_PT: %d\n",
-		tfo->get_fabric_name(), (explicit) ? "explicit" : "implicit",
-		core_scsi3_pr_dump_type(pr_reg->pr_res_type),
-		(pr_reg->pr_reg_all_tg_pt) ? 1 : 0);
+	spin_lock(&dev->t10_pr.registration_lock);
+	list_del_init(&pr_reg->pr_reg_list);
+	/*
+	 * If the I_T nexus is a reservation holder, the persistent reservation
+	 * is of an all registrants type, and the I_T nexus is the last remaining
+	 * registered I_T nexus, then the device server shall also release the
+	 * persistent reservation.
+	 */
+	if (!list_empty(&dev->t10_pr.registration_list) &&
+	    ((pr_res_type == PR_TYPE_WRITE_EXCLUSIVE_ALLREG) ||
+	     (pr_res_type == PR_TYPE_EXCLUSIVE_ACCESS_ALLREG))) {
+		dev->dev_pr_res_holder =
+			list_entry(dev->t10_pr.registration_list.next,
+				   struct t10_pr_registration, pr_reg_list);
+		dev->dev_pr_res_holder->pr_res_type = pr_res_type;
+		dev->dev_pr_res_holder->pr_res_scope = pr_res_scope;
+		dev->dev_pr_res_holder->pr_res_holder = 1;
+	}
+	spin_unlock(&dev->t10_pr.registration_lock);
+out:
+	if (!dev->dev_pr_res_holder) {
+		pr_debug("SPC-3 PR [%s] Service Action: %s RELEASE cleared"
+			" reservation holder TYPE: %s ALL_TG_PT: %d\n",
+			tfo->get_fabric_name(), (explicit) ? "explicit" :
+			"implicit", core_scsi3_pr_dump_type(pr_res_type),
+			(pr_reg->pr_reg_all_tg_pt) ? 1 : 0);
+	}
 	pr_debug("SPC-3 PR [%s] RELEASE Node: %s%s\n",
 		tfo->get_fabric_name(), se_nacl->initiatorname,
 		i_buf);
@@ -2530,7 +2573,7 @@ core_scsi3_emulate_pro_release(struct se_cmd *cmd, int type, int scope,
 	 *    server shall not establish a unit attention condition.
 	 */
 	__core_scsi3_complete_pro_release(dev, se_sess->se_node_acl,
-			pr_reg, 1);
+					  pr_reg, 1, 0);
 
 	spin_unlock(&dev->dev_reservation_lock);
 
@@ -2618,7 +2661,7 @@ core_scsi3_emulate_pro_clear(struct se_cmd *cmd, u64 res_key)
 	if (pr_res_holder) {
 		struct se_node_acl *pr_res_nacl = pr_res_holder->pr_reg_nacl;
 		__core_scsi3_complete_pro_release(dev, pr_res_nacl,
-			pr_res_holder, 0);
+						  pr_res_holder, 0, 0);
 	}
 	spin_unlock(&dev->dev_reservation_lock);
 	/*
@@ -2677,7 +2720,7 @@ static void __core_scsi3_complete_pro_preempt(
 	 */
 	if (dev->dev_pr_res_holder)
 		__core_scsi3_complete_pro_release(dev, nacl,
-				dev->dev_pr_res_holder, 0);
+						  dev->dev_pr_res_holder, 0, 0);
 
 	dev->dev_pr_res_holder = pr_reg;
 	pr_reg->pr_res_holder = 1;
@@ -2922,8 +2965,8 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
 	 */
 	if (pr_reg_n != pr_res_holder)
 		__core_scsi3_complete_pro_release(dev,
-				pr_res_holder->pr_reg_nacl,
-				dev->dev_pr_res_holder, 0);
+						  pr_res_holder->pr_reg_nacl,
+						  dev->dev_pr_res_holder, 0, 0);
 	/*
 	 * b) Remove the registrations for all I_T nexuses identified
 	 *    by the SERVICE ACTION RESERVATION KEY field, except the
@@ -3386,7 +3429,7 @@ after_iport_check:
 	 *    holder (i.e., the I_T nexus on which the
 	 */
 	__core_scsi3_complete_pro_release(dev, pr_res_nacl,
-			dev->dev_pr_res_holder, 0);
+					  dev->dev_pr_res_holder, 0, 0);
 	/*
 	 * g) Move the persistent reservation to the specified I_T nexus using
 	 *    the same scope and type as the persistent reservation released in
-- 
1.9.1

Re: [PATCH 1/2] target: Fix R_HOLDER bit usage for AllRegistrants

[Lee Duncan]

On 12/15/2014 04:09 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch fixes the usage of R_HOLDER bit for an All Registrants
> reservation in READ_FULL_STATUS, where only the registration who
> issued RESERVE was being reported as having an active reservation.
> 
> It changes core_scsi3_pri_read_full_status() to check ahead of the
> list walk of active registrations to see if All Registrants is active,
> and if so set R_HOLDER bit and scope/type fields for all active
> registrations.
> 
> Reported-by: Ilias Tsitsimpis <i.tsitsimpis@gmail.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/target/target_core_pr.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index f91b6a1..c4a8da5 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -3834,7 +3834,8 @@ core_scsi3_pri_read_full_status(struct se_cmd *cmd)
>  	unsigned char *buf;
>  	u32 add_desc_len = 0, add_len = 0, desc_len, exp_desc_len;
>  	u32 off = 8; /* off into first Full Status descriptor */
> -	int format_code = 0;
> +	int format_code = 0, pr_res_type = 0, pr_res_scope = 0;
> +	bool all_reg = false;
>  
>  	if (cmd->data_length < 8) {
>  		pr_err("PRIN SA READ_FULL_STATUS SCSI Data Length: %u"
> @@ -3851,6 +3852,19 @@ core_scsi3_pri_read_full_status(struct se_cmd *cmd)
>  	buf[2] = ((dev->t10_pr.pr_generation >> 8) & 0xff);
>  	buf[3] = (dev->t10_pr.pr_generation & 0xff);
>  
> +	spin_lock(&dev->dev_reservation_lock);
> +	if (dev->dev_pr_res_holder) {
> +		struct t10_pr_registration *pr_holder = dev->dev_pr_res_holder;
> +
> +		if (pr_holder->pr_res_type == PR_TYPE_WRITE_EXCLUSIVE_ALLREG ||
> +		    pr_holder->pr_res_type == PR_TYPE_EXCLUSIVE_ACCESS_ALLREG) {
> +			all_reg = true;
> +			pr_res_type = pr_holder->pr_res_type;
> +			pr_res_scope = pr_holder->pr_res_scope;
> +		}
> +	}
> +	spin_unlock(&dev->dev_reservation_lock);
> +
>  	spin_lock(&pr_tmpl->registration_lock);
>  	list_for_each_entry_safe(pr_reg, pr_reg_tmp,
>  			&pr_tmpl->registration_list, pr_reg_list) {
> @@ -3898,14 +3912,20 @@ core_scsi3_pri_read_full_status(struct se_cmd *cmd)
>  		 * reservation holder for PR_HOLDER bit.
>  		 *
>  		 * Also, if this registration is the reservation
> -		 * holder, fill in SCOPE and TYPE in the next byte.
> +		 * holder or there is an All Registrants reservation
> +		 * active, fill in SCOPE and TYPE in the next byte.
>  		 */
>  		if (pr_reg->pr_res_holder) {
>  			buf[off++] |= 0x01;
>  			buf[off++] = (pr_reg->pr_res_scope & 0xf0) |
>  				     (pr_reg->pr_res_type & 0x0f);
> -		} else
> +		} else if (all_reg) {
> +			buf[off++] |= 0x01;
> +			buf[off++] = (pr_res_scope & 0xf0) |
> +				     (pr_res_type & 0x0f);
> +		} else {
>  			off += 2;
> +		}
>  
>  		off += 4; /* Skip over reserved area */
>  		/*
> 
Passes my test suite now for All Registrants.

Tested-by: Lee Duncan <lduncan@suse.com>

Re: [PATCH 2/2] target: Avoid dropping AllRegistrants reservation during unregister

[Lee Duncan]

On 12/15/2014 04:09 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch fixes an issue with AllRegistrants reservations where
> an unregister operation by the I_T nexus reservation holder would
> incorrectly drop the reservation, instead of waiting until the
> last active I_T nexus is unregistered as per SPC-4.
> 
> This includes updating __core_scsi3_complete_pro_release() to reset
> dev->dev_pr_res_holder with another pr_reg for this special case,
> as well as a new 'unreg' parameter to determine when the release
> is occuring from an implicit unregister, vs. explicit RELEASE.
> 
> It also adds special handling in core_scsi3_free_pr_reg_from_nacl()
> to release the left-over pr_res_holder, now that pr_reg is deleted
> from pr_reg_list within __core_scsi3_complete_pro_release().
> 
> Reported-by: Ilias Tsitsimpis <i.tsitsimpis@gmail.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/target/target_core_pr.c | 87 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 65 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index c4a8da5..703890c 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -76,7 +76,7 @@ enum preempt_type {
>  };
>  
>  static void __core_scsi3_complete_pro_release(struct se_device *, struct se_node_acl *,
> -			struct t10_pr_registration *, int);
> +					      struct t10_pr_registration *, int, int);
>  
>  static sense_reason_t
>  target_scsi2_reservation_check(struct se_cmd *cmd)
> @@ -1177,7 +1177,7 @@ static int core_scsi3_check_implicit_release(
>  		 *    service action with the SERVICE ACTION RESERVATION KEY
>  		 *    field set to zero (see 5.7.11.3).
>  		 */
> -		__core_scsi3_complete_pro_release(dev, nacl, pr_reg, 0);
> +		__core_scsi3_complete_pro_release(dev, nacl, pr_reg, 0, 1);
>  		ret = 1;
>  		/*
>  		 * For 'All Registrants' reservation types, all existing
> @@ -1219,7 +1219,8 @@ static void __core_scsi3_free_registration(
>  
>  	pr_reg->pr_reg_deve->def_pr_registered = 0;
>  	pr_reg->pr_reg_deve->pr_res_key = 0;
> -	list_del(&pr_reg->pr_reg_list);
> +	if (!list_empty(&pr_reg->pr_reg_list))
> +		list_del(&pr_reg->pr_reg_list);
>  	/*
>  	 * Caller accessing *pr_reg using core_scsi3_locate_pr_reg(),
>  	 * so call core_scsi3_put_pr_reg() to decrement our reference.
> @@ -1271,6 +1272,7 @@ void core_scsi3_free_pr_reg_from_nacl(
>  {
>  	struct t10_reservation *pr_tmpl = &dev->t10_pr;
>  	struct t10_pr_registration *pr_reg, *pr_reg_tmp, *pr_res_holder;
> +	bool free_reg = false;
>  	/*
>  	 * If the passed se_node_acl matches the reservation holder,
>  	 * release the reservation.
> @@ -1278,13 +1280,18 @@ void core_scsi3_free_pr_reg_from_nacl(
>  	spin_lock(&dev->dev_reservation_lock);
>  	pr_res_holder = dev->dev_pr_res_holder;
>  	if ((pr_res_holder != NULL) &&
> -	    (pr_res_holder->pr_reg_nacl == nacl))
> -		__core_scsi3_complete_pro_release(dev, nacl, pr_res_holder, 0);
> +	    (pr_res_holder->pr_reg_nacl == nacl)) {
> +		__core_scsi3_complete_pro_release(dev, nacl, pr_res_holder, 0, 1);
> +		free_reg = true;
> +	}
>  	spin_unlock(&dev->dev_reservation_lock);
>  	/*
>  	 * Release any registration associated with the struct se_node_acl.
>  	 */
>  	spin_lock(&pr_tmpl->registration_lock);
> +	if (pr_res_holder && free_reg)
> +		__core_scsi3_free_registration(dev, pr_res_holder, NULL, 0);
> +
>  	list_for_each_entry_safe(pr_reg, pr_reg_tmp,
>  			&pr_tmpl->registration_list, pr_reg_list) {
>  
> @@ -1307,7 +1314,7 @@ void core_scsi3_free_all_registrations(
>  	if (pr_res_holder != NULL) {
>  		struct se_node_acl *pr_res_nacl = pr_res_holder->pr_reg_nacl;
>  		__core_scsi3_complete_pro_release(dev, pr_res_nacl,
> -				pr_res_holder, 0);
> +						  pr_res_holder, 0, 0);
>  	}
>  	spin_unlock(&dev->dev_reservation_lock);
>  
> @@ -2103,13 +2110,13 @@ core_scsi3_emulate_pro_register(struct se_cmd *cmd, u64 res_key, u64 sa_res_key,
>  		/*
>  		 * sa_res_key=0 Unregister Reservation Key for registered I_T Nexus.
>  		 */
> -		pr_holder = core_scsi3_check_implicit_release(
> -				cmd->se_dev, pr_reg);
> +		type = pr_reg->pr_res_type;
> +		pr_holder = core_scsi3_check_implicit_release(cmd->se_dev,
> +							      pr_reg);
>  		if (pr_holder < 0) {
>  			ret = TCM_RESERVATION_CONFLICT;
>  			goto out;
>  		}
> -		type = pr_reg->pr_res_type;
>  
>  		spin_lock(&pr_tmpl->registration_lock);
>  		/*
> @@ -2383,23 +2390,59 @@ static void __core_scsi3_complete_pro_release(
>  	struct se_device *dev,
>  	struct se_node_acl *se_nacl,
>  	struct t10_pr_registration *pr_reg,
> -	int explicit)
> +	int explicit,
> +	int unreg)
>  {
>  	struct target_core_fabric_ops *tfo = se_nacl->se_tpg->se_tpg_tfo;
>  	char i_buf[PR_REG_ISID_ID_LEN];
> +	int pr_res_type = 0, pr_res_scope = 0;
>  
>  	memset(i_buf, 0, PR_REG_ISID_ID_LEN);
>  	core_pr_dump_initiator_port(pr_reg, i_buf, PR_REG_ISID_ID_LEN);
>  	/*
>  	 * Go ahead and release the current PR reservation holder.
> +	 * If an All Registrants reservation is currently active and
> +	 * a unregister operation is requested, replace the current
> +	 * dev_pr_res_holder with another active registration.
>  	 */
> -	dev->dev_pr_res_holder = NULL;
> +	if (dev->dev_pr_res_holder) {
> +		pr_res_type = dev->dev_pr_res_holder->pr_res_type;
> +		pr_res_scope = dev->dev_pr_res_holder->pr_res_scope;
> +		dev->dev_pr_res_holder->pr_res_type = 0;
> +		dev->dev_pr_res_holder->pr_res_scope = 0;
> +		dev->dev_pr_res_holder->pr_res_holder = 0;
> +		dev->dev_pr_res_holder = NULL;
> +	}
> +	if (!unreg)
> +		goto out;
>  
> -	pr_debug("SPC-3 PR [%s] Service Action: %s RELEASE cleared"
> -		" reservation holder TYPE: %s ALL_TG_PT: %d\n",
> -		tfo->get_fabric_name(), (explicit) ? "explicit" : "implicit",
> -		core_scsi3_pr_dump_type(pr_reg->pr_res_type),
> -		(pr_reg->pr_reg_all_tg_pt) ? 1 : 0);
> +	spin_lock(&dev->t10_pr.registration_lock);
> +	list_del_init(&pr_reg->pr_reg_list);
> +	/*
> +	 * If the I_T nexus is a reservation holder, the persistent reservation
> +	 * is of an all registrants type, and the I_T nexus is the last remaining
> +	 * registered I_T nexus, then the device server shall also release the
> +	 * persistent reservation.
> +	 */
> +	if (!list_empty(&dev->t10_pr.registration_list) &&
> +	    ((pr_res_type == PR_TYPE_WRITE_EXCLUSIVE_ALLREG) ||
> +	     (pr_res_type == PR_TYPE_EXCLUSIVE_ACCESS_ALLREG))) {
> +		dev->dev_pr_res_holder =
> +			list_entry(dev->t10_pr.registration_list.next,
> +				   struct t10_pr_registration, pr_reg_list);
> +		dev->dev_pr_res_holder->pr_res_type = pr_res_type;
> +		dev->dev_pr_res_holder->pr_res_scope = pr_res_scope;
> +		dev->dev_pr_res_holder->pr_res_holder = 1;
> +	}
> +	spin_unlock(&dev->t10_pr.registration_lock);
> +out:
> +	if (!dev->dev_pr_res_holder) {
> +		pr_debug("SPC-3 PR [%s] Service Action: %s RELEASE cleared"
> +			" reservation holder TYPE: %s ALL_TG_PT: %d\n",
> +			tfo->get_fabric_name(), (explicit) ? "explicit" :
> +			"implicit", core_scsi3_pr_dump_type(pr_res_type),
> +			(pr_reg->pr_reg_all_tg_pt) ? 1 : 0);
> +	}
>  	pr_debug("SPC-3 PR [%s] RELEASE Node: %s%s\n",
>  		tfo->get_fabric_name(), se_nacl->initiatorname,
>  		i_buf);
> @@ -2530,7 +2573,7 @@ core_scsi3_emulate_pro_release(struct se_cmd *cmd, int type, int scope,
>  	 *    server shall not establish a unit attention condition.
>  	 */
>  	__core_scsi3_complete_pro_release(dev, se_sess->se_node_acl,
> -			pr_reg, 1);
> +					  pr_reg, 1, 0);
>  
>  	spin_unlock(&dev->dev_reservation_lock);
>  
> @@ -2618,7 +2661,7 @@ core_scsi3_emulate_pro_clear(struct se_cmd *cmd, u64 res_key)
>  	if (pr_res_holder) {
>  		struct se_node_acl *pr_res_nacl = pr_res_holder->pr_reg_nacl;
>  		__core_scsi3_complete_pro_release(dev, pr_res_nacl,
> -			pr_res_holder, 0);
> +						  pr_res_holder, 0, 0);
>  	}
>  	spin_unlock(&dev->dev_reservation_lock);
>  	/*
> @@ -2677,7 +2720,7 @@ static void __core_scsi3_complete_pro_preempt(
>  	 */
>  	if (dev->dev_pr_res_holder)
>  		__core_scsi3_complete_pro_release(dev, nacl,
> -				dev->dev_pr_res_holder, 0);
> +						  dev->dev_pr_res_holder, 0, 0);
>  
>  	dev->dev_pr_res_holder = pr_reg;
>  	pr_reg->pr_res_holder = 1;
> @@ -2922,8 +2965,8 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
>  	 */
>  	if (pr_reg_n != pr_res_holder)
>  		__core_scsi3_complete_pro_release(dev,
> -				pr_res_holder->pr_reg_nacl,
> -				dev->dev_pr_res_holder, 0);
> +						  pr_res_holder->pr_reg_nacl,
> +						  dev->dev_pr_res_holder, 0, 0);
>  	/*
>  	 * b) Remove the registrations for all I_T nexuses identified
>  	 *    by the SERVICE ACTION RESERVATION KEY field, except the
> @@ -3386,7 +3429,7 @@ after_iport_check:
>  	 *    holder (i.e., the I_T nexus on which the
>  	 */
>  	__core_scsi3_complete_pro_release(dev, pr_res_nacl,
> -			dev->dev_pr_res_holder, 0);
> +					  dev->dev_pr_res_holder, 0, 0);
>  	/*
>  	 * g) Move the persistent reservation to the specified I_T nexus using
>  	 *    the same scope and type as the persistent reservation released in
> 


Tested-by: Lee Duncan <lduncan@suse.com>

Re: [PATCH 0/2] target: Fixes for AllRegistrants reservation handling

[Ilias Tsitsimpis]

[-- Attachment #1: Type: text/plain, Size: 6233 bytes --]

On Tue, Dec 16, 2014 at 12:09AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Hi all,
> 
> This series addresses two issues raised recently by Ilias wrt
> AllRegistrants reservation handling in target code that does not
> adhere to SPC-4 specification requirements.
> 
> This first is a informational change to PR-IN READ_FULL_STATUS,
> that when an AllRegistrants reservation is in place, all active
> registrations should be setting R_HOLDER=1 within their respective
> descriptors.
> 
> The second is a functional change to PR-OUT REGISTER w/ SARK=0
> operation, to avoid dropping the AllRegistrants reservation until
> the last registered I_T nexus has been released.  It also ensures
> that the correct reservation type + scope is retained when the
> new reservation is set within __core_scsi3_complete_pro_release()
> for this AllRegistrants special case.
> 
> Also, thanks to James for the extra SPC-4 clarifications.
> 
> Please review.

Hi Nicholas,

Thanks for the quick fix. These patches address the two issues that I
raised in my previous email but they don't seem to fix all the problems
with AllRegistrants reservation handling in target code. Please see my
example below.

I have set up an iSCSI target, using the Linux SCSI Target
implementation with your patches applied, and I am connecting to it from
two different nodes using the open-iscsi project.

    root@node1:~# sg_inq /dev/sdc
    standard INQUIRY:
      PQual=0  Device_type=0  RMB=0  version=0x05  [SPC-3]
      [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=0  Resp_data_format=2
      SCCS=1  ACC=0  TPGS=3  3PC=1  Protect=0  BQue=0
      EncServ=0  MultiP=0  [MChngr=0]  [ACKREQQ=0]  Addr16=0
      [RelAdr=0]  WBus16=0  Sync=0  Linked=0  [TranDis=0]  CmdQue=1
        length=36 (0x24)   Peripheral device type: disk
     Vendor identification: LIO-ORG 
     Product identification: FILEIO          
     Product revision level: 4.0
     Unit serial number: 853ea439-7fcf-4aaa-9de2-1399e06b45be

    root@node2:~# sg_inq /dev/sdc
    standard INQUIRY:
      PQual=0  Device_type=0  RMB=0  version=0x05  [SPC-3]
      [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=0  Resp_data_format=2
      SCCS=1  ACC=0  TPGS=3  3PC=1  Protect=0  BQue=0
      EncServ=0  MultiP=0  [MChngr=0]  [ACKREQQ=0]  Addr16=0
      [RelAdr=0]  WBus16=0  Sync=0  Linked=0  [TranDis=0]  CmdQue=1
        length=36 (0x24)   Peripheral device type: disk
     Vendor identification: LIO-ORG 
     Product identification: FILEIO          
     Product revision level: 4.0
     Unit serial number: 853ea439-7fcf-4aaa-9de2-1399e06b45be

I register both of the I_T nexuses with the target.

    root@node1:~# sg_persist --out --register --param-sark 0x1 /dev/sdc

    root@node2:~# sg_persist --out --register --param-sark 0x2 /dev/sdc

    root@node1:~# sg_persist --in --read-full-status /dev/sdc
      LIO-ORG   FILEIO            4.0
      Peripheral device type: disk
      PR generation=0x17
        Key=0x1
          All target ports bit clear
          Relative port address: 0x5
          not reservation holder
          Transport Id of initiator:
            iSCSI name and session id: iqn.1993-08.org.debian:01:node1
        Key=0x2
          All target ports bit clear
          Relative port address: 0x5
          not reservation holder
          Transport Id of initiator:
            iSCSI name and session id: iqn.1993-08.org.debian:01:node2

Then I get a persistent reservation with type 'Exclusive Access - All
Registrants' from node1.

    root@node1:~# sg_persist --out --reserve -T 8 --param-rk 0x1 /dev/sdc
    root@node1:~# sg_persist --in --read-full-status /dev/sdc
      LIO-ORG   FILEIO            4.0
      Peripheral device type: disk
      PR generation=0x17
        Key=0x1
          All target ports bit clear
          Relative port address: 0x5
          << Reservation holder >>
          scope: LU_SCOPE,  type: Exclusive Access, all registrants
          Transport Id of initiator:
            iSCSI name and session id: iqn.1993-08.org.debian:01:node1
        Key=0x2
          All target ports bit clear
          Relative port address: 0x5
          << Reservation holder >>
          scope: LU_SCOPE,  type: Exclusive Access, all registrants
          Transport Id of initiator:
            iSCSI name and session id: iqn.1993-08.org.debian:01:node2

As you can see both registrants are reported as reservations holders,
which is the right thing to do.

Now let's try to get a persistent reservation with the same type from
node2. SPC4r17 (par. 5.7.9 Reserving) states that:

    If the device server receives a PERSISTENT RESERVE OUT command with
    RESERVE service action where the TYPE field and the SCOPE field
    contain the same values as the existing type and scope from a
    persistent reservation holder, it shall not make any change to the
    existing persistent reservation and shall complete the command with
    GOOD status.

Node2 *is* a persistent reservation holder based on the wording of
SPC4r17 (par. 5.7.10 Persistent reservation holder).
But instead:

    root@node2:~# sg_persist --out --reserve -T 8 --param-rk 0x2 /dev/sdc
      LIO-ORG   FILEIO            4.0
      Peripheral device type: disk
    persistent reserve out: scsi status: Reservation Conflict
    PR out: command failed

and on the target machine I get:

    [95713.681066] SPC-3 PR: Attempted RESERVE from [iSCSI]:
    iqn.1993-08.org.debian:01:node2 while reservation already held by
    [iSCSI]: iqn.1993-08.org.debian:01:node1, returning
    RESERVATION_CONFLICT

If I try the same from node1, the target completes the command with GOOD
status.


It is clear that the target code still considers node1 as the (only)
reservation holder and only *reports* node2 as being a reservation
holder.

I believe we should fix the above errors by changing the
'pr_res_holder' inside the 't10_reservation' struct to be a list of
holders rather than trying to handle this implicitly. Having a single
holder in the code seems prone to more errors similar to this one.

Thanks,
Ilias

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/2] target: Fixes for AllRegistrants reservation handling

[Douglas Gilbert]

On 14-12-18 09:54 AM, Ilias Tsitsimpis wrote:
> On Tue, Dec 16, 2014 at 12:09AM, Nicholas A. Bellinger wrote:
>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>
>> Hi all,
>>
>> This series addresses two issues raised recently by Ilias wrt
>> AllRegistrants reservation handling in target code that does not
>> adhere to SPC-4 specification requirements.
>>
>> This first is a informational change to PR-IN READ_FULL_STATUS,
>> that when an AllRegistrants reservation is in place, all active
>> registrations should be setting R_HOLDER=1 within their respective
>> descriptors.
>>
>> The second is a functional change to PR-OUT REGISTER w/ SARK=0
>> operation, to avoid dropping the AllRegistrants reservation until
>> the last registered I_T nexus has been released.  It also ensures
>> that the correct reservation type + scope is retained when the
>> new reservation is set within __core_scsi3_complete_pro_release()
>> for this AllRegistrants special case.
>>
>> Also, thanks to James for the extra SPC-4 clarifications.
>>
>> Please review.
>
> Hi Nicholas,
>
> Thanks for the quick fix. These patches address the two issues that I
> raised in my previous email but they don't seem to fix all the problems
> with AllRegistrants reservation handling in target code. Please see my
> example below.
>
> I have set up an iSCSI target, using the Linux SCSI Target
> implementation with your patches applied, and I am connecting to it from
> two different nodes using the open-iscsi project.
>
>      root@node1:~# sg_inq /dev/sdc
>      standard INQUIRY:
>        PQual=0  Device_type=0  RMB=0  version=0x05  [SPC-3]
>        [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=0  Resp_data_format=2
>        SCCS=1  ACC=0  TPGS=3  3PC=1  Protect=0  BQue=0
>        EncServ=0  MultiP=0  [MChngr=0]  [ACKREQQ=0]  Addr16=0
>        [RelAdr=0]  WBus16=0  Sync=0  Linked=0  [TranDis=0]  CmdQue=1
>          length=36 (0x24)   Peripheral device type: disk
>       Vendor identification: LIO-ORG
>       Product identification: FILEIO
>       Product revision level: 4.0
>       Unit serial number: 853ea439-7fcf-4aaa-9de2-1399e06b45be

Here is an alternate open source implementation of an
iSCSI target/server:

root@node1:~# sg_inq /dev/sdc
standard INQUIRY:
   PQual=0  Device_type=0  RMB=0  LU_CONG=0  version=0x06  [SPC-4]
   [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=1  Resp_data_format=2
   SCCS=0  ACC=0  TPGS=1  3PC=1  Protect=0  [BQue=0]
   EncServ=0  MultiP=1 (VS=0)  [MChngr=0]  [ACKREQQ=0]  Addr16=0
   [RelAdr=0]  WBus16=0  Sync=0  [Linked=0]  [TranDis=0]  CmdQue=1
   [SPI: Clocking=0x0  QAS=0  IUS=0]
     length=96 (0x60)   Peripheral device type: disk
  Vendor identification: FreeBSD
  Product identification: iSCSI Disk
  Product revision level: 0123
  Unit serial number: 0015f2d0d8b600

>      root@node2:~# sg_inq /dev/sdc
>      standard INQUIRY:
>        PQual=0  Device_type=0  RMB=0  version=0x05  [SPC-3]
>        [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=0  Resp_data_format=2
>        SCCS=1  ACC=0  TPGS=3  3PC=1  Protect=0  BQue=0
>        EncServ=0  MultiP=0  [MChngr=0]  [ACKREQQ=0]  Addr16=0
>        [RelAdr=0]  WBus16=0  Sync=0  Linked=0  [TranDis=0]  CmdQue=1
>          length=36 (0x24)   Peripheral device type: disk
>       Vendor identification: LIO-ORG
>       Product identification: FILEIO
>       Product revision level: 4.0
>       Unit serial number: 853ea439-7fcf-4aaa-9de2-1399e06b45be

root@node2:~# sg_inq /dev/sdb
standard INQUIRY:
   PQual=0  Device_type=0  RMB=0  LU_CONG=0  version=0x06  [SPC-4]
   [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=1  Resp_data_format=2
   SCCS=0  ACC=0  TPGS=1  3PC=1  Protect=0  [BQue=0]
   EncServ=0  MultiP=1 (VS=0)  [MChngr=0]  [ACKREQQ=0]  Addr16=0
   [RelAdr=0]  WBus16=0  Sync=0  [Linked=0]  [TranDis=0]  CmdQue=1
   [SPI: Clocking=0x0  QAS=0  IUS=0]
     length=96 (0x60)   Peripheral device type: disk
  Vendor identification: FreeBSD
  Product identification: iSCSI Disk
  Product revision level: 0123
  Unit serial number: 0015f2d0d8b600


Now playing along with the same test sequence and noting when
there are significant differences.

> I register both of the I_T nexuses with the target.
>
>      root@node1:~# sg_persist --out --register --param-sark 0x1 /dev/sdc
>
>      root@node2:~# sg_persist --out --register --param-sark 0x2 /dev/sdc
>
>      root@node1:~# sg_persist --in --read-full-status /dev/sdc
>        LIO-ORG   FILEIO            4.0
>        Peripheral device type: disk
>        PR generation=0x17
>          Key=0x1
>            All target ports bit clear
>            Relative port address: 0x5
>            not reservation holder
>            Transport Id of initiator:
>              iSCSI name and session id: iqn.1993-08.org.debian:01:node1
>          Key=0x2
>            All target ports bit clear
>            Relative port address: 0x5
>            not reservation holder
>            Transport Id of initiator:
>              iSCSI name and session id: iqn.1993-08.org.debian:01:node2
>
> Then I get a persistent reservation with type 'Exclusive Access - All
> Registrants' from node1.
>
>      root@node1:~# sg_persist --out --reserve -T 8 --param-rk 0x1 /dev/sdc
>      root@node1:~# sg_persist --in --read-full-status /dev/sdc
>        LIO-ORG   FILEIO            4.0
>        Peripheral device type: disk
>        PR generation=0x17
>          Key=0x1
>            All target ports bit clear
>            Relative port address: 0x5
>            << Reservation holder >>
>            scope: LU_SCOPE,  type: Exclusive Access, all registrants
>            Transport Id of initiator:
>              iSCSI name and session id: iqn.1993-08.org.debian:01:node1
>          Key=0x2
>            All target ports bit clear
>            Relative port address: 0x5
>            << Reservation holder >>
>            scope: LU_SCOPE,  type: Exclusive Access, all registrants
>            Transport Id of initiator:
>              iSCSI name and session id: iqn.1993-08.org.debian:01:node2
>
> As you can see both registrants are reported as reservations holders,
> which is the right thing to do.
>
> Now let's try to get a persistent reservation with the same type from
> node2. SPC4r17 (par. 5.7.9 Reserving) states that:
>
>      If the device server receives a PERSISTENT RESERVE OUT command with
>      RESERVE service action where the TYPE field and the SCOPE field
>      contain the same values as the existing type and scope from a
>      persistent reservation holder, it shall not make any change to the
>      existing persistent reservation and shall complete the command with
>      GOOD status.
>
> Node2 *is* a persistent reservation holder based on the wording of
> SPC4r17 (par. 5.7.10 Persistent reservation holder).
> But instead:
>
>      root@node2:~# sg_persist --out --reserve -T 8 --param-rk 0x2 /dev/sdc
>        LIO-ORG   FILEIO            4.0
>        Peripheral device type: disk
>      persistent reserve out: scsi status: Reservation Conflict
>      PR out: command failed
>
> and on the target machine I get:
>
>      [95713.681066] SPC-3 PR: Attempted RESERVE from [iSCSI]:
>      iqn.1993-08.org.debian:01:node2 while reservation already held by
>      [iSCSI]: iqn.1993-08.org.debian:01:node1, returning
>      RESERVATION_CONFLICT

With FreeNAS as the iSCSI target (server), the above command on
node2 succeeds.

> If I try the same from node1, the target completes the command with GOOD
> status.

And back on node1 I see:

root@node1:~# sg_persist --out --reserve -T 8 --param-rk 0x2 /dev/sdc
   FreeBSD   iSCSI Disk        0123
   Peripheral device type: disk
PR out (Reserve): Reservation conflict

which I am guessing is what was expected.

> It is clear that the target code still considers node1 as the (only)
> reservation holder and only *reports* node2 as being a reservation
> holder.
>
> I believe we should fix the above errors by changing the
> 'pr_res_holder' inside the 't10_reservation' struct to be a list of
> holders rather than trying to handle this implicitly. Having a single
> holder in the code seems prone to more errors similar to this one.


I think Alexander Motin has done a pretty impressive
job over at FreeBSD in the SCSI target area. FreeNAS
(9.3 recently released) is a convenient packaging for
turning a box (or just an old disk in my case) into an
iSCSI target.


FreeNAS's iSCSI target also implements ODX which is one
reason why I have been looking at it (hint to NAB). With
ODX it does cut one significant corner in only supporting
"access upon reference" RODs. Its ODX implementation found
several bugs and holes in my ddpt ODX client code. Perhaps
NAB might check how FreeBSD's code handles the AllRegistrants
reservation handling case.

Doug Gilbert

Re: [PATCH 0/2] target: Fixes for AllRegistrants reservation handling

[Nicholas A. Bellinger]

On Thu, 2014-12-18 at 16:54 +0200, Ilias Tsitsimpis wrote:
> On Tue, Dec 16, 2014 at 12:09AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > Hi all,
> > 
> > This series addresses two issues raised recently by Ilias wrt
> > AllRegistrants reservation handling in target code that does not
> > adhere to SPC-4 specification requirements.
> > 
> > This first is a informational change to PR-IN READ_FULL_STATUS,
> > that when an AllRegistrants reservation is in place, all active
> > registrations should be setting R_HOLDER=1 within their respective
> > descriptors.
> > 
> > The second is a functional change to PR-OUT REGISTER w/ SARK=0
> > operation, to avoid dropping the AllRegistrants reservation until
> > the last registered I_T nexus has been released.  It also ensures
> > that the correct reservation type + scope is retained when the
> > new reservation is set within __core_scsi3_complete_pro_release()
> > for this AllRegistrants special case.
> > 
> > Also, thanks to James for the extra SPC-4 clarifications.
> > 
> > Please review.
> 
> Hi Nicholas,
> 
> Thanks for the quick fix. These patches address the two issues that I
> raised in my previous email but they don't seem to fix all the problems
> with AllRegistrants reservation handling in target code. Please see my
> example below.
> 

<SNIP>

> Now let's try to get a persistent reservation with the same type from
> node2. SPC4r17 (par. 5.7.9 Reserving) states that:
> 
>     If the device server receives a PERSISTENT RESERVE OUT command with
>     RESERVE service action where the TYPE field and the SCOPE field
>     contain the same values as the existing type and scope from a
>     persistent reservation holder, it shall not make any change to the
>     existing persistent reservation and shall complete the command with
>     GOOD status.
> 
> Node2 *is* a persistent reservation holder based on the wording of
> SPC4r17 (par. 5.7.10 Persistent reservation holder).
> But instead:
> 
>     root@node2:~# sg_persist --out --reserve -T 8 --param-rk 0x2 /dev/sdc
>       LIO-ORG   FILEIO            4.0
>       Peripheral device type: disk
>     persistent reserve out: scsi status: Reservation Conflict
>     PR out: command failed
> 
> and on the target machine I get:
> 
>     [95713.681066] SPC-3 PR: Attempted RESERVE from [iSCSI]:
>     iqn.1993-08.org.debian:01:node2 while reservation already held by
>     [iSCSI]: iqn.1993-08.org.debian:01:node1, returning
>     RESERVATION_CONFLICT
> 
> If I try the same from node1, the target completes the command with GOOD
> status.
> 
> 
> It is clear that the target code still considers node1 as the (only)
> reservation holder and only *reports* node2 as being a reservation
> holder.

That is a very small change.  Sending out a patch for this now.

> 
> I believe we should fix the above errors by changing the
> 'pr_res_holder' inside the 't10_reservation' struct to be a list of
> holders rather than trying to handle this implicitly. Having a single
> holder in the code seems prone to more errors similar to this one.
> 

Not exactly, changing all existing code for ALLREG related special cases
really isn't worth the extra pain here.  I'd much rather identify any
remaining ALLREG specific issues, and address them individually.

Thanks Ilias!

--nab

Re: [PATCH 0/2] target: Fixes for AllRegistrants reservation handling

[Nicholas A. Bellinger]

Hi Doug,

On Thu, 2014-12-18 at 11:53 -0500, Douglas Gilbert wrote:

<SNIP>

> 
> FreeNAS's iSCSI target also implements ODX which is one
> reason why I have been looking at it (hint to NAB). With
> ODX it does cut one significant corner in only supporting
> "access upon reference" RODs. Its ODX implementation found
> several bugs and holes in my ddpt ODX client code. 

So ODX support is still low on my current TODO list, but I'm happy to
assist anyone in the short-term who is willing to submit patches for
implementing the first steps to get the ball rolling.  IIRC, the CDB
emulation additions are really quite straight-forward, especially given
you've already got a working ddpt client.  ;)

Of course, the more interesting question is once EXTENDED_COPY(LID4)
emulation is in place, how is the interface for supporting tokenized
READ/WRITE through to struct block_device (and FILEIO too) going to
look..?

I haven't seen much discussion on this topic, and would be interested to
hear if anyone has thoughts on how that piece might work.

--nab