[stgt] SCSI Persistent Group Reservations: possible bugs found
Lee Duncan
lduncan at suse.com
Mon Dec 17 03:45:16 CET 2012
On 12/16/2012 12:27 PM, Alexander Nezhinsky wrote:
> Hi,
>
> Great to hear about the test suite. This reminds me of something...
>
> I've been looking into SPC support and specifically Persistent Group
> Reservations recently and saw that stgt does not support "preempt_and_abort"
> reserve_out action.
>
> What are the practical implications? Which packages should break?
> I guess vmware can't care less. Does Microsoft cluster require it - i know
> they do use PGR, but do they actually need preempt and abort?
> RedHat Clustering? Solaris?
I'm afraid my clustering requirements knowledge is less than full. I
have only worked on two clustering systems, and only one of them used
Group Reservations for cluster management, and that was Microsoft
Clustering. It has been a few years since I modified Lefthand
Networks' product to support Microsoft Clustering, but if I remember
correctly MS only uses a fraction of the PGR commands. Sorry I don't
remember which ones.
So I do not have a matrix listing which clusters need which PGR
commands, but I'd love to build and maintain such a list, if I could
get others to contribute the data (or send me the clustering software,
to evaluate). I already have a SCSI-3 Group Reservations Tutorial
I've created that I'm need to find a home for.
As far as PREEMPT AND ABORT, I can imagine that might be used when a
cluster owner has gone missing, and another cluster member wants to
fence the original owner out of the cluster.
>
> Lee, can your test suite group the tests according to the minimal needs
> of various clustering clients' requirements?
The suite right now is mainly meant to test the various reservation
types, but the plan is for it to grow. For example, the current suite
doesn't test any check conditions that are supposed to be generated
when a reservation is preempted.
Never the less, some or all of the tests can be ran in any order with
a little bit of work, so all I would need is a list of which
clustering software needed what.
I currently have about 94 test cases, and the target code currently
fails 11 of these. Here's a patch that allows it to pass all 94 of
them. This is against the latest "master" branch:
----------------------------------------------------------------
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(®->registration_siblings,
+ ®->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;
--
Lee Duncan
SUSE Labs
--
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