Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix the stpg command error for multiple port group #475

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix the stpg command error for multiple port group #475

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 15 additions & 20 deletions alua.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ int tcmu_emulate_set_tgt_port_grps(struct tcmu_device *dev,
uint32_t off = 4, param_list_len = tcmu_get_xfer_length(cmd->cdb);
uint16_t id, tmp_id;
char *buf, new_state;
int found, ret = TCMU_STS_OK;
int ret = TCMU_STS_OK;

port = tcmu_get_enabled_port(group_list);
if (!port)
Expand All @@ -721,44 +721,39 @@ int tcmu_emulate_set_tgt_port_grps(struct tcmu_device *dev,
id = be16toh(tmp_id);
off += 2;

found = 0;
list_for_each(group_list, group, entry) {
if (group->id != id)
continue;

if (group != port->grp) {
ret = TCMU_STS_HW_ERR;
tcmu_dev_err(dev, "Failing STPG for group %d. Unable to transition remote groups.\n",
id);
goto free_buf;
tcmu_dev_dbg(dev, "Port of group %d is disable.\n", id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you want this behavior? We will be reporting success even though we did not transition the state. It could result in a initiator thinking failover worked when it didn't or in some loop where STPG reports success but then a RTPG reports states that do not match what the STPG was requesting, so the initiator retries over and over.

I think I could go either way, because I think we only see this type of command in testing, so I do not really know what the initiator expects and I do not see anything in SPC about this partial case.

Copy link
Collaborator

@mikechristie mikechristie Sep 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want this behavior, just make the message above tcmu_dev_warn() in case we find out we were wrong and then this issue will not be silently dropped.

tcmu_dev_warn(dev, "Port is disabled. Not able to change ALUA group %u state to state %u.\n", 
              group->id, new_state);

Copy link
Contributor Author

@tangwenji tangwenji Sep 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like my example，the stpg command is successful，because the second loop meet the criteria。
In the actual environment, the TPG corresponding to the Initiator sd device is enable.So we don't need to log the message with warn level， and if no TPG match，there is error message following。
If there is no enabled TPG， it will be returned before this function.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like my example，the stpg command is successful，because the second loop meet the criteria。

What about users/apps that expect all transitions requested in the STPG to be successful or the command to fail?

  1. Are you saying SPC is ok with partial completions like in your patch? I did not see anything in SPC. If you see something then send the section number.

  2. Or, are you saying apps/users are ok with it? My concern was that apps/users expect all transitions in the STPG to succeed if the STPG is returned with a successful status. Is there a specific app that works like how you are testing with sg_stpg or is this just a protocol type of test to check the behavior?

I get what you are saying that for your example we matched one of the group transitions in the STPG, but I was saying the user needs some sort of notification that the entire command is not succeeding for some reason.

continue;
}

tcmu_dev_dbg(dev, "Got STPG for group %u\n", id);
ret = tcmu_explicit_transition(group_list, group,
new_state,
ALUA_STAT_ALTERED_BY_EXPLICIT_STPG,
cmd->sense_buf);
/* Only one group is enabled, so after matching the previous one,
* whether it is successful or not, we should exit the loop
* without continuing to process the content behind the command.
*/
if (ret != TCMU_STS_OK) {
tcmu_dev_err(dev, "Failing STPG for group %d\n",
id);
goto free_buf;
}
found = 1;
break;
}

if (!found) {
/*
* Could not find what error code to return in SCSI
* spec.
*/
tcmu_dev_err(dev, "Could not find group for %u for STPG\n",
id);
ret = TCMU_STS_EXPL_TRANSITION_ERR;
break;
goto free_buf;
}
}

/*
* Could not find what error code to return in SCSI
* spec.
*/
tcmu_dev_err(dev, "Could not find enable group for STPG\n");
ret = TCMU_STS_EXPL_TRANSITION_ERR;

free_buf:
free(buf);
return ret;
Expand Down