Re: [PATCH 4/4] scsi-disk: Always report RESERVATION_CONFLICT to guest

[Paolo Bonzini]

On Mon, Jul 29, 2024 at 11:47 AM Kevin Wolf <kwolf@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 <kwolf@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 <kwolf@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
>