On Mon, Jul 29, 2024 at 11:47 AM Kevin Wolf <kw...@redhat.com> wrote:
> RESERVATION_CONFLICT is not a backend error, but indicates that the
> guest tried to make a request that it isn't allowed to execute. Pass the
> error to the guest so that it can decide what to do with it.
This is only true of scsi-block (though your patch is okay here -
scsi-disk would see an EBADE and go down the ret < 0 path).
In general, for scsi-block I'd expect people to use report instead of
stop. I agree that this is the best behavior for the case where you
have a pr-manager, but it may also be better to stop the VM if a
pr-manager has not been set up. That's probably a bit hackish, so I
guess it's okay to add a FIXME or TODO comment instead?
> - if (status == CHECK_CONDITION) {
> + switch (status) {
> + case CHECK_CONDITION:
> req_has_sense = true;
> error = scsi_sense_buf_to_errno(r->req.sense,
> sizeof(r->req.sense));
> - } else {
> + break;
> + case RESERVATION_CONFLICT:
> + /* Don't apply the error policy, always report to the guest */
This is the only case where you get error == 0. Maybe remove it from
the initializer, and set it here?
Paolo
On Mon, Jul 29, 2024 at 11:47 AM Kevin Wolf <kw...@redhat.com> wrote:
>
> RESERVATION_CONFLICT is not a backend error, but indicates that the
> guest tried to make a request that it isn't allowed to execute. Pass the
> error to the guest so that it can decide what to do with it.
>
> Without this, if we stop the VM in response to a RESERVATION_CONFLICT,
> it can happen that the VM cannot be resumed any more because every
> attempt to resume it immediately runs into the same error and stops the
> VM again.
>
> One case that expects RESERVATION_CONFLICT errors to be visible in the
> guest is running the validation tests in Windows 2019's Failover Cluster
> Manager, which intentionally tries to execute invalid requests to see if
> they are properly rejected.
>
> Buglink: https://issues.redhat.com/browse/RHEL-50000
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
> hw/scsi/scsi-disk.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 69a195177e..e173b238de 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -235,11 +235,17 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int
> ret, bool acct_failed)
> } else {
> /* A passthrough command has completed with nonzero status. */
> status = ret;
> - if (status == CHECK_CONDITION) {
> + switch (status) {
> + case CHECK_CONDITION:
> req_has_sense = true;
> error = scsi_sense_buf_to_errno(r->req.sense,
> sizeof(r->req.sense));
> - } else {
> + break;
> + case RESERVATION_CONFLICT:
> + /* Don't apply the error policy, always report to the guest */
> + break;
> + default:
> error = EINVAL;
> + break;
> }
> }
>
> @@ -249,8 +255,9 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int ret,
> bool acct_failed)
> * are usually retried immediately, so do not post them to QMP and
> * do not account them as failed I/O.
> */
> - if (req_has_sense &&
> - scsi_sense_buf_is_guest_recoverable(r->req.sense,
> sizeof(r->req.sense))) {
> + if (!error || (req_has_sense &&
> + scsi_sense_buf_is_guest_recoverable(r->req.sense,
> +
> sizeof(r->req.sense)))) {
> action = BLOCK_ERROR_ACTION_REPORT;
> acct_failed = false;
> } else {
> --
> 2.45.2
>
