From nobody Thu Dec 18 17:56:31 2025 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=pass(p=quarantine dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1744117387; cv=none; d=zohomail.com; s=zohoarc; b=kfzWYiZFuKjkMfNxr0MHQDsxYndmBc1aYpSG64DfQ7N1tVOvyRLOvxWtBuDIuj038LRVBTieWoohQwLyFwes1b2CHOpbW5EKs5KcZ4Y7ejUXbqKQOHAzhS6mjQXN1O9yiW/uIQcdm0UyoJ/Uu2gl/6o1ZM3eAoobkc1YSCINW7o= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1744117387; h=Content-Transfer-Encoding:Cc:Cc:Date:Date:From:From:In-Reply-To:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:References:Sender:Subject:Subject:To:To:Message-Id:Reply-To; bh=h7O1+jeMjpt4PGSabZHa12EThqfaHw3Rj3mPvJFT2p0=; b=iHvcdPbO45VqBDZ7GiHy30tsBqDyZnzppQ4X4/k91OeLOgiXz5D7IKL2zsWSA7gKPuYPA6Iff1ZbpQ6CdAwXM3KjaOwiQpvpKxmn8MJxcp+7jVNadC0+VsL288YPs9S9w7+mkm6xkwy50cn4640H5powiEoMRimq9upJ1J9YnCk= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=pass header.from= (p=quarantine dis=none) Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1744117387367499.82680076919996; Tue, 8 Apr 2025 06:03:07 -0700 (PDT) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1u28at-0007Ag-Oc; Tue, 08 Apr 2025 09:02:23 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1u28Ze-0006ly-Qg for qemu-devel@nongnu.org; Tue, 08 Apr 2025 09:01:10 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1u28Zc-0005cU-5x for qemu-devel@nongnu.org; Tue, 08 Apr 2025 09:01:06 -0400 Received: from mx-prod-mc-06.mail-002.prod.us-west-2.aws.redhat.com (ec2-35-165-154-97.us-west-2.compute.amazonaws.com [35.165.154.97]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-116-89pWzGi1O6udpRPD99MjZA-1; Tue, 08 Apr 2025 09:01:00 -0400 Received: from mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.40]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-06.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 1B1FA180AF65; Tue, 8 Apr 2025 13:00:59 +0000 (UTC) Received: from merkur.redhat.com (unknown [10.44.33.56]) by mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id E87C71955BEF; Tue, 8 Apr 2025 13:00:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1744117263; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=h7O1+jeMjpt4PGSabZHa12EThqfaHw3Rj3mPvJFT2p0=; b=KHUr2YCD3RJMIHMEOCpLeRatiMaBL1nBEh+0Z1+/jI9CHZXtxJfQd4mzJzKrLLpD2ckMfY IkTHlOMCu+Ylqho+6WnF2QCjRedjkY5uR5XgRadj0EiYRJBdF5rIY/ETTaduYUd2lsMpYq nBHRrOzP9eoRRjKldzz+4A4SWNautKk= X-MC-Unique: 89pWzGi1O6udpRPD99MjZA-1 X-Mimecast-MFC-AGG-ID: 89pWzGi1O6udpRPD99MjZA_1744117259 From: Kevin Wolf To: qemu-block@nongnu.org Cc: kwolf@redhat.com, qemu-devel@nongnu.org Subject: [PULL 3/4] scsi-disk: Apply error policy for host_status errors again Date: Tue, 8 Apr 2025 15:00:47 +0200 Message-ID: <20250408130048.283364-4-kwolf@redhat.com> In-Reply-To: <20250408130048.283364-1-kwolf@redhat.com> References: <20250408130048.283364-1-kwolf@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Scanned-By: MIMEDefang 3.0 on 10.30.177.40 Received-SPF: pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Received-SPF: pass client-ip=170.10.133.124; envelope-from=kwolf@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -28 X-Spam_score: -2.9 X-Spam_bar: -- X-Spam_report: (-2.9 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.845, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H5=0.001, RCVD_IN_MSPIKE_WL=0.001, RCVD_IN_VALIDITY_CERTIFIED_BLOCKED=0.001, RCVD_IN_VALIDITY_RPBL_BLOCKED=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: qemu-devel-bounces+importer=patchew.org@nongnu.org X-ZohoMail-DKIM: pass (identity @redhat.com) X-ZM-MESSAGEID: 1744117390692019100 Content-Type: text/plain; charset="utf-8" Originally, all failed SG_IO requests called scsi_handle_rw_error() to apply the configured error policy. However, commit f3126d65, which was supposed to be a mere refactoring for scsi-disk.c, broke this and accidentally completed the SCSI request without considering the error policy any more if the error was signalled in the host_status field. Apart from the commit message not describing the change as intended, errors indicated in host_status are also obviously backend errors and not something the guest must deal with independently of the error policy. This behaviour means that some recoverable errors (such as a path error in multipath configurations) were reported to the guest anyway, which might not expect it and might consider its disk broken. Make sure that we apply the error policy again for host_status errors, too. This addresses an existing FIXME comment and allows us to remove some comments warning that callbacks weren't always called. With this fix, they are called in all cases again. The return value passed to the request callback doesn't have more free values that could be used to indicate host_status errors as well as SAM status codes and negative errno. Store the value in the host_status field of the SCSIRequest instead and use -ENODEV as the return value (if a path hasn't been reachable for a while, blk_aio_ioctl() will return -ENODEV instead of just setting host_status, so just reuse it here - it's not necessarily entirely accurate, but it's as good as any errno). Cc: qemu-stable@nongnu.org Fixes: f3126d65b393 ('scsi: move host_status handling into SCSI drivers') Signed-off-by: Kevin Wolf Message-ID: <20250407155949.44736-1-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Reviewed-by: Hanna Czenczek Signed-off-by: Kevin Wolf --- hw/scsi/scsi-disk.c | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 8da1d5a77c..e59632e9b1 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -68,10 +68,9 @@ struct SCSIDiskClass { SCSIDeviceClass parent_class; /* * Callbacks receive ret =3D=3D 0 for success. Errors are represented = either as - * negative errno values, or as positive SAM status codes. - * - * Beware: For errors returned in host_status, the function may direct= ly - * complete the request and never call the callback. + * negative errno values, or as positive SAM status codes. For host_st= atus + * errors, the function passes ret =3D=3D -ENODEV and sets the host_st= atus field + * of the SCSIRequest. */ DMAIOFunc *dma_readv; DMAIOFunc *dma_writev; @@ -225,11 +224,26 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int = ret, bool acct_failed) SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, r->req.dev); SCSIDiskClass *sdc =3D (SCSIDiskClass *) object_get_class(OBJECT(s)); SCSISense sense =3D SENSE_CODE(NO_SENSE); + int16_t host_status; int error; bool req_has_sense =3D false; BlockErrorAction action; int status; =20 + /* + * host_status should only be set for SG_IO requests that came back wi= th a + * host_status error in scsi_block_sgio_complete(). This error path pa= sses + * -ENODEV as the return value. + * + * Reset host_status in the request because we may still want to compl= ete + * the request successfully with the 'stop' or 'ignore' error policy. + */ + host_status =3D r->req.host_status; + if (host_status !=3D -1) { + assert(ret =3D=3D -ENODEV); + r->req.host_status =3D -1; + } + if (ret < 0) { status =3D scsi_sense_from_errno(-ret, &sense); error =3D -ret; @@ -289,6 +303,10 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int r= et, bool acct_failed) if (acct_failed) { block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct); } + if (host_status !=3D -1) { + scsi_req_complete_failed(&r->req, host_status); + return true; + } if (req_has_sense) { sdc->update_sense(&r->req); } else if (status =3D=3D CHECK_CONDITION) { @@ -409,7 +427,6 @@ done: scsi_req_unref(&r->req); } =20 -/* May not be called in all error cases, don't rely on cleanup here */ static void scsi_dma_complete(void *opaque, int ret) { SCSIDiskReq *r =3D (SCSIDiskReq *)opaque; @@ -448,7 +465,6 @@ done: scsi_req_unref(&r->req); } =20 -/* May not be called in all error cases, don't rely on cleanup here */ static void scsi_read_complete(void *opaque, int ret) { SCSIDiskReq *r =3D (SCSIDiskReq *)opaque; @@ -585,7 +601,6 @@ done: scsi_req_unref(&r->req); } =20 -/* May not be called in all error cases, don't rely on cleanup here */ static void scsi_write_complete(void * opaque, int ret) { SCSIDiskReq *r =3D (SCSIDiskReq *)opaque; @@ -2846,14 +2861,10 @@ static void scsi_block_sgio_complete(void *opaque, = int ret) sg_io_hdr_t *io_hdr =3D &req->io_header; =20 if (ret =3D=3D 0) { - /* FIXME This skips calling req->cb() and any cleanup in it */ if (io_hdr->host_status !=3D SCSI_HOST_OK) { - scsi_req_complete_failed(&r->req, io_hdr->host_status); - scsi_req_unref(&r->req); - return; - } - - if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) { + r->req.host_status =3D io_hdr->host_status; + ret =3D -ENODEV; + } else if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) { ret =3D BUSY; } else { ret =3D io_hdr->status; --=20 2.49.0