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.

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

Conversation

tangwenji
Copy link
Contributor

When there are multiple target port group,the stpg comman send by the SCSI Initiator carries all target port group information via sg_stpg in Linux。
For example:there are 2 TPGs(id:4,5),the TPG 4 is disable and the TPG 5 is enable. Generate an sdb device via the TPG 5 on the Linux Initiator. The information contained in cdb of sg_stpg /dev/sdb command as follows: 02 00 00 04 00 00 00 05. In the original code, when parsing the first TPG descriptor, it will exit abnormally because group 4 not equal enable port group 5.

Signed-off-by: tangwenji [email protected]

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

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

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.

@lxbsz lxbsz changed the base branch from master to main August 10, 2022 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants