[stgt] [PATCH] fix bugs in persistent group reservations

Lee Duncan lduncan at suse.com
Sat Dec 22 19:10:49 CET 2012


I created a small Persistent Group Reservations test suite, available
at git://github.com/gonzoleeman/open-iscsi-pgr-validate. It uncovered
several bugs in the SCSI target's PGR implementation. Here is a list
of the tests that failed or could not run:

Reserve Exclusive Access:
  - Can release reservation:				ERROR
  - Unregister Releases Reservation:			FAIL

Reserve Write Exclusive:
  - Resvn Holder Unregister Releases Resvn:		FAIL
  - Non-Registrant Does Not Have Read Access:		FAIL
  - Non-Resvn-Holder Has Read Access:			FAIL

Reserve Exclusive Access Registrants Only:
  - Unregister Releases Reservation:			FAIL

Reserve Write Exclusive Registrants Only:
  - Resvn Holder Unregister releases resvn:		FAIL

Reserve Exclusive Access All Registrants:
  - Alternative Resvn Holder Can Release Resvn:		FAIL
  - Main Resvn Unregister Does Not Release Resvn:	FAIL

Reserve Write Exclusive All Registrants:
  - Alt Resvn Holder Can Release Resvn:			FAIL
  - Main Resvn Holder Unregister does not
	 release resvn:					FAIL

This patch enables all PGR tests is said suite to pass.

Signed-off-by: Lee Duncan <lduncan at suse.com>
---
 usr/list.h |    6 ++++
 usr/spc.c  |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 75 insertions(+), 11 deletions(-)

diff --git a/usr/list.h b/usr/list.h
index 2f80a56..819bba9 100644
--- a/usr/list.h
+++ b/usr/list.h
@@ -37,6 +37,12 @@ static inline int list_empty(const struct list_head *head)
 	return head->next == head;
 }
 
+static inline int list_is_last(const struct list_head *list,
+			       const struct list_head *head)
+{
+	return list->next == head;
+}
+
 #define list_entry(ptr, type, member) \
 	container_of(ptr, type, member)
 
diff --git a/usr/spc.c b/usr/spc.c
index 9fb5a6d..24d449e 100644
--- a/usr/spc.c
+++ b/usr/spc.c
@@ -109,6 +109,15 @@
 #define DESG_MD5 7
 #define DESG_SCSI 8
 
+static inline int is_pr_type_all_registrants(uint8_t pr_type)
+{
+	if ((pr_type == PR_TYPE_WRITE_EXCLUSIVE_ALLREG) ||
+	    (pr_type == PR_TYPE_EXCLUSIVE_ACCESS_ALLREG))
+		return 1;
+	return 0;
+}
+
+
 static void update_vpd_80(struct scsi_lu *lu, void *sn)
 {
 	struct vpd *vpd_pg = lu->attrs.lu_vpd[PCODE_OFFSET(0x80)];
@@ -867,8 +876,7 @@ int spc_service_action(int host_no, struct scsi_cmd *cmd)
 
 static int is_pr_holder(struct scsi_lu *lu, struct registration *reg)
 {
-	if (lu->pr_holder->pr_type == PR_TYPE_WRITE_EXCLUSIVE_ALLREG ||
-	    lu->pr_holder->pr_type == PR_TYPE_EXCLUSIVE_ACCESS_ALLREG)
+	if (is_pr_type_all_registrants(lu->pr_holder->pr_type))
 		return 1;
 
 	if (lu->pr_holder == reg)
@@ -956,8 +964,7 @@ static int spc_pr_read_reservation(int host_no, struct scsi_cmd *cmd)
 	if (reg) {
 		put_unaligned_be32(16, &buf[4]);
 
-		if (reg->pr_type == PR_TYPE_WRITE_EXCLUSIVE_ALLREG ||
-		    reg->pr_type == PR_TYPE_EXCLUSIVE_ACCESS_ALLREG)
+		if (is_pr_type_all_registrants(reg->pr_type))
 			res_key = 0;
 		else
 			res_key = reg->key;
@@ -1055,6 +1062,56 @@ static void __unregister(struct scsi_lu *lu, struct registration *reg)
 	free(reg);
 }
 
+static void __unregister_and_clean(struct scsi_cmd *cmd,
+				   struct registration *reg)
+{
+	struct registration *holder, *sibling;
+
+	/* if reservation owner goes away then so does reservation */
+	holder = cmd->dev->pr_holder;
+	if (!holder)
+		goto unregister_and_go;
+
+	if (!is_pr_holder(cmd->dev, reg))
+		goto unregister_and_go;
+
+	if (!is_pr_type_all_registrants(holder->pr_type) ||
+	     list_is_last(&reg->registration_siblings,
+		 &reg->registration_siblings)) {
+		holder->pr_scope = 0;
+		holder->pr_type = 0;
+		cmd->dev->pr_holder = NULL;
+
+		list_for_each_entry(sibling,
+		    &cmd->dev->registration_list,
+		    registration_siblings) {
+			/* do not send UA to self */
+			if (sibling == reg)
+				continue;
+			ua_sense_add_other_it_nexus(sibling->nexus_id,
+			    cmd->dev,
+			    ASC_RESERVATIONS_RELEASED);
+		}
+		cmd->dev->prgeneration++;
+	} else {
+		/* all-registrants, and not last registrant */
+		list_for_each_entry(sibling,
+		    &cmd->dev->registration_list,
+		    registration_siblings) {
+			if (sibling != reg) {
+				/* give resvn to any sibling */
+				cmd->dev->pr_holder = sibling;
+				sibling->pr_scope = holder->pr_scope;
+				sibling->pr_type = holder->pr_type;
+				break;
+			}
+		}
+	}
+
+unregister_and_go:
+	__unregister(cmd->dev, reg);
+}
+
 static int check_pr_out_basic_parameter(struct scsi_cmd *cmd)
 {
 	uint8_t spec_i_pt, all_tg_pt, aptpl;
@@ -1110,7 +1167,7 @@ static int spc_pr_register(int host_no, struct scsi_cmd *cmd)
 			if (sa_res_key)
 				reg->key = sa_res_key;
 			else
-				__unregister(cmd->dev, reg);
+				__unregister_and_clean(cmd, reg);
 		} else
 			return SAM_STAT_RESERVATION_CONFLICT;
 	} else {
@@ -1235,7 +1292,8 @@ static int spc_pr_release(int host_no, struct scsi_cmd *cmd)
 	if (res_key != reg->key)
 		return SAM_STAT_RESERVATION_CONFLICT;
 
-	if (reg->pr_scope != pr_scope || reg->pr_type != pr_type) {
+	if (holder->pr_scope != pr_scope ||
+	    holder->pr_type != pr_type) {
 		asc = ASC_INVALID_RELEASE_OF_PERSISTENT_RESERVATION;
 		goto sense;
 	}
@@ -1359,8 +1417,7 @@ static int spc_pr_preempt(int host_no, struct scsi_cmd *cmd)
 	holder = cmd->dev->pr_holder;
 
 	if (holder) {
-		if (holder->pr_type == PR_TYPE_WRITE_EXCLUSIVE_ALLREG ||
-			holder->pr_type == PR_TYPE_EXCLUSIVE_ACCESS_ALLREG) {
+		if (is_pr_type_all_registrants(holder->pr_type)) {
 
 			if (!sa_res_key) {
 				if (pr_type != holder->pr_type ||
@@ -1520,9 +1577,10 @@ int spc_access_check(struct scsi_cmd *cmd)
 	pr_type = cmd->dev->pr_holder->pr_type;
 	bits = cmd->dev->dev_type_template.ops[op].pr_conflict_bits;
 
-	if (pr_type == PR_TYPE_WRITE_EXCLUSIVE ||
-	    pr_type == PR_TYPE_EXCLUSIVE_ACCESS)
-		conflict = bits & (PR_WE_FA|PR_EA_FA);
+	if (pr_type == PR_TYPE_EXCLUSIVE_ACCESS)
+		conflict = bits & PR_EA_FA;
+	else if (pr_type == PR_TYPE_WRITE_EXCLUSIVE)
+		conflict = bits & PR_WE_FA;
 	else {
 		if (reg)
 			conflict = bits & PR_RR_FR;

--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



More information about the stgt mailing list