From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qt1-f182.google.com (mail-qt1-f182.google.com [209.85.160.182]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 98BA01822F8 for ; Fri, 14 Jun 2024 07:56:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.160.182 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718351809; cv=none; b=hBwiVKx1x9cpLZGif1RluSMtuR7ThoIc1qQzv2qfBeGJlITVT7k5yVfeXLCgghFwcnAUGe24Bi4qn/moh1pMDJvcgAjt0H45OK8pKSLlxg/5+bFKz0+RV93NfBahYsKUfzO6dTlbWbz+ZIXx74uLgqTi0Oh+SHN09fRGXGJbEV8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718351809; c=relaxed/simple; bh=cCTHejGQxMi6RzmM8k0/qqpdwyLsfEJT+eXWS8ocifI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Qsi4TGdLvlfEnA9FVItZC0MdOiCJFyeRrtVp5WYEJcmseTetT++MTXY8TN87Fu+BTER18P/JECyQw7flhnpo/mfGcjEJ0MG+IyrsWb3lhFQI6zJka4zu5/YP1jBf/u0sTy3V/SRojOOATKpUDYaEOUANEQ/NTEH3WczjXSEQXcg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=citrix.com; spf=pass smtp.mailfrom=cloud.com; dkim=pass (1024-bit key) header.d=citrix.com header.i=@citrix.com header.b=Fm1tcN3V; arc=none smtp.client-ip=209.85.160.182 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=citrix.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=cloud.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=citrix.com header.i=@citrix.com header.b="Fm1tcN3V" Received: by mail-qt1-f182.google.com with SMTP id d75a77b69052e-44116be80ecso11040631cf.2 for ; Fri, 14 Jun 2024 00:56:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=citrix.com; s=google; t=1718351805; x=1718956605; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=BedO1x12znmji4xt7UpC8XDe1FvgdzsEI86mvlWGudA=; b=Fm1tcN3Vo3gJO6/SCGnqGesXhAtXTa/SULB8j+OCYKi91pBbnkv9madB1toyUbEmqU n1PWVnReUBEajRAGUKRwA/n4y6aByM5MFtRcCVNryNdcI08bph4xYoVR37w9nTKZYbm2 Ey9Gr1/er1ywu6fbx5njWRb6cyHvBMvDWfQuU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718351805; x=1718956605; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=BedO1x12znmji4xt7UpC8XDe1FvgdzsEI86mvlWGudA=; b=Ke/cHmqnFkPugOstjAqbnBEHVzPHwL2niG0Frr0Nouzpm6npnTIaHWwTqt8XQReH42 ec29f0VlagNOZSrqoDox7vCdklCWEsaOcVZz07WZb/QKixWVBXohZGf5wnQ1myWDWnwL mcRoHUqdNS6ETxRaILlxjKk0fa1oWiaIEnJGe8vZKFYqA8PfX8o1rHgXXIHV6FwwnejY rht+htEOLvXaaSio5G+L8E6iKq1CmNX0qNhriLwpQU1ogtTkcnUL0kPXZjGgoKxa42PB K90JH6gtXSZJB4xUD+Z0C+H6QIanpH6TEtTrso6D2QLeQuHxaglowpAltfhi6/bTQ+Zg t5fw== X-Forwarded-Encrypted: i=1; AJvYcCVXmvgLn58IVWkpGvJDeNIU7iYlz2wiIoG2QfU45qYb0XxcN8TfqUEtVyY9SAH8OhiTRMrkyspD7Bkk3DwhZUrp2StGZXZmWaZ/0w== X-Gm-Message-State: AOJu0YyzSHTMZGHXGOmiXgRkzPXI8AKayKzcQPqrXwXb4B08PfRynXaZ AgV6dHuwTB+QTe7x4wBafwBA7PmqYEeySiXL9X1Jitkj6dxGIcATpOHozzMDxdQ= X-Google-Smtp-Source: AGHT+IEDi8BhVjLu2vbVbmBxB83hP6gyIibQXt+H84ODwaVrhmTedMPZYFZeMGGsfo/+7hGiDnXdRg== X-Received: by 2002:a05:622a:1822:b0:43e:2639:a987 with SMTP id d75a77b69052e-44216b3a874mr27744421cf.59.1718351804940; Fri, 14 Jun 2024 00:56:44 -0700 (PDT) Received: from localhost ([213.195.124.163]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-441f2ff9ef8sm13823221cf.89.2024.06.14.00.56.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 14 Jun 2024 00:56:44 -0700 (PDT) Date: Fri, 14 Jun 2024 09:56:42 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Christoph Hellwig Cc: Jens Axboe , Geert Uytterhoeven , Richard Weinberger , Philipp Reisner , Lars Ellenberg , Christoph =?utf-8?Q?B=C3=B6hmwalder?= , Josef Bacik , Ming Lei , "Michael S. Tsirkin" , Jason Wang , Alasdair Kergon , Mike Snitzer , Mikulas Patocka , Song Liu , Yu Kuai , Vineeth Vijayan , "Martin K. Petersen" , linux-m68k@lists.linux-m68k.org, linux-um@lists.infradead.org, drbd-dev@lists.linbit.com, nbd@other.debian.org, linuxppc-dev@lists.ozlabs.org, ceph-devel@vger.kernel.org, virtualization@lists.linux.dev, xen-devel@lists.xenproject.org, linux-bcache@vger.kernel.org, dm-devel@lists.linux.dev, linux-raid@vger.kernel.org, linux-mmc@vger.kernel.org, linux-mtd@lists.infradead.org, nvdimm@lists.linux.dev, linux-nvme@lists.infradead.org, linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org, linux-block@vger.kernel.org Subject: Re: [PATCH 10/26] xen-blkfront: don't disable cache flushes when they fail Message-ID: References: <20240611051929.513387-1-hch@lst.de> <20240611051929.513387-11-hch@lst.de> <20240612150030.GA29188@lst.de> <20240613140508.GA16529@lst.de> Precedence: bulk X-Mailing-List: ceph-devel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240613140508.GA16529@lst.de> On Thu, Jun 13, 2024 at 04:05:08PM +0200, Christoph Hellwig wrote: > On Wed, Jun 12, 2024 at 05:56:15PM +0200, Roger Pau Monné wrote: > > Right. AFAICT advertising "feature-barrier" and/or > > "feature-flush-cache" could be done based on whether blkback > > understand those commands, not on whether the underlying storage > > supports the equivalent of them. > > > > Worst case we can print a warning message once about the underlying > > storage failing to complete flush/barrier requests, and that data > > integrity might not be guaranteed going forward, and not propagate the > > error to the upper layer? > > > > What would be the consequence of propagating a flush error to the > > upper layers? > > If you propage the error to the upper layer you will generate an > I/O error there, which usually leads to a file system shutdown. > > > Given the description of the feature in the blkif header, I'm afraid > > we cannot guarantee that seeing the feature exposed implies barrier or > > flush support, since the request could fail at any time (or even from > > the start of the disk attachment) and it would still sadly be a correct > > implementation given the description of the options. > > Well, then we could do something like the patch below, which keeps > the existing behavior, but insolates the block layer from it and > removes the only user of blk_queue_write_cache from interrupt > context: LGTM, I'm not sure there's much else we can do. > --- > From e6e82c769ab209a77302994c3829cf6ff7a595b8 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig > Date: Thu, 30 May 2024 08:58:52 +0200 > Subject: xen-blkfront: don't disable cache flushes when they fail > > blkfront always had a robust negotiation protocol for detecting a write > cache. Stop simply disabling cache flushes in the block layer as the > flags handling is moving to the atomic queue limits API that needs > user context to freeze the queue for that. Instead handle the case > of the feature flags cleared inside of blkfront. This removes old > debug code to check for such a mismatch which was previously impossible > to hit, including the check for passthrough requests that blkfront > never used to start with. > > Signed-off-by: Christoph Hellwig > --- > drivers/block/xen-blkfront.c | 44 +++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 21 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index 9b4ec3e4908cce..e2c92d5095ff17 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -788,6 +788,14 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri > * A barrier request a superset of FUA, so we can > * implement it the same way. (It's also a FLUSH+FUA, > * since it is guaranteed ordered WRT previous writes.) > + * > + * Note that can end up here with a FUA write and the > + * flags cleared. This happens when the flag was > + * run-time disabled and raced with I/O submission in > + * the block layer. We submit it as a normal write Since blkfront no longer signals that FUA is no longer available for the device, getting a request with FUA is not actually a race I think? > + * here. A pure flush should never end up here with > + * the flags cleared as they are completed earlier for > + * the !feature_flush case. > */ > if (info->feature_flush && info->feature_fua) > ring_req->operation = > @@ -795,8 +803,6 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri > else if (info->feature_flush) > ring_req->operation = > BLKIF_OP_FLUSH_DISKCACHE; > - else > - ring_req->operation = 0; > } > ring_req->u.rw.nr_segments = num_grant; > if (unlikely(require_extra_req)) { > @@ -887,16 +893,6 @@ static inline void flush_requests(struct blkfront_ring_info *rinfo) > notify_remote_via_irq(rinfo->irq); > } > > -static inline bool blkif_request_flush_invalid(struct request *req, > - struct blkfront_info *info) > -{ > - return (blk_rq_is_passthrough(req) || > - ((req_op(req) == REQ_OP_FLUSH) && > - !info->feature_flush) || > - ((req->cmd_flags & REQ_FUA) && > - !info->feature_fua)); > -} > - > static blk_status_t blkif_queue_rq(struct blk_mq_hw_ctx *hctx, > const struct blk_mq_queue_data *qd) > { > @@ -908,23 +904,30 @@ static blk_status_t blkif_queue_rq(struct blk_mq_hw_ctx *hctx, > rinfo = get_rinfo(info, qid); > blk_mq_start_request(qd->rq); > spin_lock_irqsave(&rinfo->ring_lock, flags); > - if (RING_FULL(&rinfo->ring)) > - goto out_busy; > > - if (blkif_request_flush_invalid(qd->rq, rinfo->dev_info)) > - goto out_err; > + /* > + * Check if the backend actually supports flushes. > + * > + * While the block layer won't send us flushes if we don't claim to > + * support them, the Xen protocol allows the backend to revoke support > + * at any time. That is of course a really bad idea and dangerous, but > + * has been allowed for 10+ years. In that case we simply clear the > + * flags, and directly return here for an empty flush and ignore the > + * FUA flag later on. > + */ > + if (unlikely(req_op(qd->rq) == REQ_OP_FLUSH && !info->feature_flush)) > + goto out; Don't you need to complete the request here? Thanks, Roger. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3073FC27C6E for ; Fri, 14 Jun 2024 07:57:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=J35t5Wb8F/yrlC/PW9vTrIE+7CZxHjM+ZJ67L5xmy+o=; b=qi6NF0OuRbsIIj xl3/ojZCO4y+keHXruGgxUz3BTJo0ZZ9OXvCrCZHiNNjLKKStWkoX5CVt/LldYXOG878BQaOL7yBJ HD26gmij0CHAf+CLRmubteJy59Uhqq3unfPC2J9Q9sHmht6Xh3y2UFkJACalOYk/sE/yldweGvT6W urAyfT2llzUUUe27zEDPv1Jv24/xWhOd5gsooNGpXkBhfwkjq1KqVUbLcm9O/PCo932ZrP+CQug6h VjiLrWh6s7Iwk2+butO02/lk9cdZwaUzY5dWqZpTKgo13xxkgT1yk7KgqQlxWRnNxh8zVxHWBvQpU YMb/T7UJASJZI+q48LAA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sI1nr-00000001s0b-2Odz; Fri, 14 Jun 2024 07:56:55 +0000 Received: from mail-qt1-x831.google.com ([2607:f8b0:4864:20::831]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sI1nl-00000001ryW-3oj5 for linux-mtd@lists.infradead.org; Fri, 14 Jun 2024 07:56:52 +0000 Received: by mail-qt1-x831.google.com with SMTP id d75a77b69052e-43ffbc0927fso10911301cf.3 for ; Fri, 14 Jun 2024 00:56:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=citrix.com; s=google; t=1718351805; x=1718956605; darn=lists.infradead.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=BedO1x12znmji4xt7UpC8XDe1FvgdzsEI86mvlWGudA=; b=RaIl4x/adI/HuNOc3VwbvU4uzLYKtkqKRWQ80BHrpZC5GbHDiZnP4JzreUyoFVzgqt 9xw9ntMGnEtX/px4pV4l1WCY/aB/VN6z86GjIstb0stV2pbCIJKqw1tS0JdlPWeUHJGX AZ+F+C17IleGHh1XY9koLX+jaBCQVbz3P3FAM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718351805; x=1718956605; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=BedO1x12znmji4xt7UpC8XDe1FvgdzsEI86mvlWGudA=; b=T+1wfidHrA6Xxugza8i/4b35bncjY/+PXdbhQGWiv7zU807N84QA8nPO5o0Q527FFZ At/uwT1Sji8GmGkVHlyVzjWmV3iQVDRimORVZ88UgPboZCwntruE+8PXypyxvhOMAfkR 6EJSQIyUMqua6cqIoAIXqTeogan/X1GR75ef11k7aYRifz1AqQHWLxIbDZmt209T0qgf CSQ4btcDnyFDSsLaqbFWaivh2JOV2WY9Xrj4wGmySaxEut1lUpnM9hGqX6ahWJJe6QRb KnTp8cY15gCHnZvGbRcJX7L1QWYBuat82/CUVFqFd3yPtfuPlCZnSu7xvOcl+keLH2cq v5ew== X-Forwarded-Encrypted: i=1; AJvYcCUvYKRrU3SpJasMwCy/zl5DgIDpZQ9x3QidJTGjD8uhitigSJ+i5lRfRBnbdwzhKhKi5rNUHblmsQNuBvDddvpXLbVl1MWQtzdS2tdJ8w== X-Gm-Message-State: AOJu0Ywm/L1yINLivb5PprOPlIyYE9skPQOH05zuGN1lZOGwpH3eYiT2 RABl/19HmXBjchcShnlsPO0V7mR2uRhyYV6jPjBP12TQ7Y7RLJz3OXhgiKCW/Y0= X-Google-Smtp-Source: AGHT+IEDi8BhVjLu2vbVbmBxB83hP6gyIibQXt+H84ODwaVrhmTedMPZYFZeMGGsfo/+7hGiDnXdRg== X-Received: by 2002:a05:622a:1822:b0:43e:2639:a987 with SMTP id d75a77b69052e-44216b3a874mr27744421cf.59.1718351804940; Fri, 14 Jun 2024 00:56:44 -0700 (PDT) Received: from localhost ([213.195.124.163]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-441f2ff9ef8sm13823221cf.89.2024.06.14.00.56.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 14 Jun 2024 00:56:44 -0700 (PDT) Date: Fri, 14 Jun 2024 09:56:42 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Christoph Hellwig Cc: Jens Axboe , Geert Uytterhoeven , Richard Weinberger , Philipp Reisner , Lars Ellenberg , Christoph =?utf-8?Q?B=C3=B6hmwalder?= , Josef Bacik , Ming Lei , "Michael S. Tsirkin" , Jason Wang , Alasdair Kergon , Mike Snitzer , Mikulas Patocka , Song Liu , Yu Kuai , Vineeth Vijayan , "Martin K. Petersen" , linux-m68k@lists.linux-m68k.org, linux-um@lists.infradead.org, drbd-dev@lists.linbit.com, nbd@other.debian.org, linuxppc-dev@lists.ozlabs.org, ceph-devel@vger.kernel.org, virtualization@lists.linux.dev, xen-devel@lists.xenproject.org, linux-bcache@vger.kernel.org, dm-devel@lists.linux.dev, linux-raid@vger.kernel.org, linux-mmc@vger.kernel.org, linux-mtd@lists.infradead.org, nvdimm@lists.linux.dev, linux-nvme@lists.infradead.org, linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org, linux-block@vger.kernel.org Subject: Re: [PATCH 10/26] xen-blkfront: don't disable cache flushes when they fail Message-ID: References: <20240611051929.513387-1-hch@lst.de> <20240611051929.513387-11-hch@lst.de> <20240612150030.GA29188@lst.de> <20240613140508.GA16529@lst.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240613140508.GA16529@lst.de> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240614_005650_137144_D20B3168 X-CRM114-Status: GOOD ( 47.97 ) X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: "linux-mtd" Errors-To: linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org T24gVGh1LCBKdW4gMTMsIDIwMjQgYXQgMDQ6MDU6MDhQTSArMDIwMCwgQ2hyaXN0b3BoIEhlbGx3 aWcgd3JvdGU6Cj4gT24gV2VkLCBKdW4gMTIsIDIwMjQgYXQgMDU6NTY6MTVQTSArMDIwMCwgUm9n ZXIgUGF1IE1vbm7DqSB3cm90ZToKPiA+IFJpZ2h0LiAgQUZBSUNUIGFkdmVydGlzaW5nICJmZWF0 dXJlLWJhcnJpZXIiIGFuZC9vcgo+ID4gImZlYXR1cmUtZmx1c2gtY2FjaGUiIGNvdWxkIGJlIGRv bmUgYmFzZWQgb24gd2hldGhlciBibGtiYWNrCj4gPiB1bmRlcnN0YW5kIHRob3NlIGNvbW1hbmRz LCBub3Qgb24gd2hldGhlciB0aGUgdW5kZXJseWluZyBzdG9yYWdlCj4gPiBzdXBwb3J0cyB0aGUg ZXF1aXZhbGVudCBvZiB0aGVtLgo+ID4gCj4gPiBXb3JzdCBjYXNlIHdlIGNhbiBwcmludCBhIHdh cm5pbmcgbWVzc2FnZSBvbmNlIGFib3V0IHRoZSB1bmRlcmx5aW5nCj4gPiBzdG9yYWdlIGZhaWxp bmcgdG8gY29tcGxldGUgZmx1c2gvYmFycmllciByZXF1ZXN0cywgYW5kIHRoYXQgZGF0YQo+ID4g aW50ZWdyaXR5IG1pZ2h0IG5vdCBiZSBndWFyYW50ZWVkIGdvaW5nIGZvcndhcmQsIGFuZCBub3Qg cHJvcGFnYXRlIHRoZQo+ID4gZXJyb3IgdG8gdGhlIHVwcGVyIGxheWVyPwo+ID4gCj4gPiBXaGF0 IHdvdWxkIGJlIHRoZSBjb25zZXF1ZW5jZSBvZiBwcm9wYWdhdGluZyBhIGZsdXNoIGVycm9yIHRv IHRoZQo+ID4gdXBwZXIgbGF5ZXJzPwo+IAo+IElmIHlvdSBwcm9wYWdlIHRoZSBlcnJvciB0byB0 aGUgdXBwZXIgbGF5ZXIgeW91IHdpbGwgZ2VuZXJhdGUgYW4KPiBJL08gZXJyb3IgdGhlcmUsIHdo aWNoIHVzdWFsbHkgbGVhZHMgdG8gYSBmaWxlIHN5c3RlbSBzaHV0ZG93bi4KPiAKPiA+IEdpdmVu IHRoZSBkZXNjcmlwdGlvbiBvZiB0aGUgZmVhdHVyZSBpbiB0aGUgYmxraWYgaGVhZGVyLCBJJ20g YWZyYWlkCj4gPiB3ZSBjYW5ub3QgZ3VhcmFudGVlIHRoYXQgc2VlaW5nIHRoZSBmZWF0dXJlIGV4 cG9zZWQgaW1wbGllcyBiYXJyaWVyIG9yCj4gPiBmbHVzaCBzdXBwb3J0LCBzaW5jZSB0aGUgcmVx dWVzdCBjb3VsZCBmYWlsIGF0IGFueSB0aW1lIChvciBldmVuIGZyb20KPiA+IHRoZSBzdGFydCBv ZiB0aGUgZGlzayBhdHRhY2htZW50KSBhbmQgaXQgd291bGQgc3RpbGwgc2FkbHkgYmUgYSBjb3Jy ZWN0Cj4gPiBpbXBsZW1lbnRhdGlvbiBnaXZlbiB0aGUgZGVzY3JpcHRpb24gb2YgdGhlIG9wdGlv bnMuCj4gCj4gV2VsbCwgdGhlbiB3ZSBjb3VsZCBkbyBzb21ldGhpbmcgbGlrZSB0aGUgcGF0Y2gg YmVsb3csIHdoaWNoIGtlZXBzCj4gdGhlIGV4aXN0aW5nIGJlaGF2aW9yLCBidXQgaW5zb2xhdGVz IHRoZSBibG9jayBsYXllciBmcm9tIGl0IGFuZAo+IHJlbW92ZXMgdGhlIG9ubHkgdXNlciBvZiBi bGtfcXVldWVfd3JpdGVfY2FjaGUgZnJvbSBpbnRlcnJ1cHQKPiBjb250ZXh0OgoKTEdUTSwgSSdt IG5vdCBzdXJlIHRoZXJlJ3MgbXVjaCBlbHNlIHdlIGNhbiBkby4KCj4gLS0tCj4gRnJvbSBlNmU4 MmM3NjlhYjIwOWE3NzMwMjk5NGMzODI5Y2Y2ZmY3YTU5NWI4IE1vbiBTZXAgMTcgMDA6MDA6MDAg MjAwMQo+IEZyb206IENocmlzdG9waCBIZWxsd2lnIDxoY2hAbHN0LmRlPgo+IERhdGU6IFRodSwg MzAgTWF5IDIwMjQgMDg6NTg6NTIgKzAyMDAKPiBTdWJqZWN0OiB4ZW4tYmxrZnJvbnQ6IGRvbid0 IGRpc2FibGUgY2FjaGUgZmx1c2hlcyB3aGVuIHRoZXkgZmFpbAo+IAo+IGJsa2Zyb250IGFsd2F5 cyBoYWQgYSByb2J1c3QgbmVnb3RpYXRpb24gcHJvdG9jb2wgZm9yIGRldGVjdGluZyBhIHdyaXRl Cj4gY2FjaGUuICBTdG9wIHNpbXBseSBkaXNhYmxpbmcgY2FjaGUgZmx1c2hlcyBpbiB0aGUgYmxv Y2sgbGF5ZXIgYXMgdGhlCj4gZmxhZ3MgaGFuZGxpbmcgaXMgbW92aW5nIHRvIHRoZSBhdG9taWMg cXVldWUgbGltaXRzIEFQSSB0aGF0IG5lZWRzCj4gdXNlciBjb250ZXh0IHRvIGZyZWV6ZSB0aGUg cXVldWUgZm9yIHRoYXQuICBJbnN0ZWFkIGhhbmRsZSB0aGUgY2FzZQo+IG9mIHRoZSBmZWF0dXJl IGZsYWdzIGNsZWFyZWQgaW5zaWRlIG9mIGJsa2Zyb250LiAgVGhpcyByZW1vdmVzIG9sZAo+IGRl YnVnIGNvZGUgdG8gY2hlY2sgZm9yIHN1Y2ggYSBtaXNtYXRjaCB3aGljaCB3YXMgcHJldmlvdXNs eSBpbXBvc3NpYmxlCj4gdG8gaGl0LCBpbmNsdWRpbmcgdGhlIGNoZWNrIGZvciBwYXNzdGhyb3Vn aCByZXF1ZXN0cyB0aGF0IGJsa2Zyb250Cj4gbmV2ZXIgdXNlZCB0byBzdGFydCB3aXRoLgo+IAo+ IFNpZ25lZC1vZmYtYnk6IENocmlzdG9waCBIZWxsd2lnIDxoY2hAbHN0LmRlPgo+IC0tLQo+ICBk cml2ZXJzL2Jsb2NrL3hlbi1ibGtmcm9udC5jIHwgNDQgKysrKysrKysrKysrKysrKysrKy0tLS0t LS0tLS0tLS0tLS0tCj4gIDEgZmlsZSBjaGFuZ2VkLCAyMyBpbnNlcnRpb25zKCspLCAyMSBkZWxl dGlvbnMoLSkKPiAKPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ibG9jay94ZW4tYmxrZnJvbnQuYyBi L2RyaXZlcnMvYmxvY2sveGVuLWJsa2Zyb250LmMKPiBpbmRleCA5YjRlYzNlNDkwOGNjZS4uZTJj OTJkNTA5NWZmMTcgMTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy9ibG9jay94ZW4tYmxrZnJvbnQuYwo+ ICsrKyBiL2RyaXZlcnMvYmxvY2sveGVuLWJsa2Zyb250LmMKPiBAQCAtNzg4LDYgKzc4OCwxNCBA QCBzdGF0aWMgaW50IGJsa2lmX3F1ZXVlX3J3X3JlcShzdHJ1Y3QgcmVxdWVzdCAqcmVxLCBzdHJ1 Y3QgYmxrZnJvbnRfcmluZ19pbmZvICpyaQo+ICAJCQkgKiBBIGJhcnJpZXIgcmVxdWVzdCBhIHN1 cGVyc2V0IG9mIEZVQSwgc28gd2UgY2FuCj4gIAkJCSAqIGltcGxlbWVudCBpdCB0aGUgc2FtZSB3 YXkuICAoSXQncyBhbHNvIGEgRkxVU0grRlVBLAo+ICAJCQkgKiBzaW5jZSBpdCBpcyBndWFyYW50 ZWVkIG9yZGVyZWQgV1JUIHByZXZpb3VzIHdyaXRlcy4pCj4gKwkJCSAqCj4gKwkJCSAqIE5vdGUg dGhhdCBjYW4gZW5kIHVwIGhlcmUgd2l0aCBhIEZVQSB3cml0ZSBhbmQgdGhlCj4gKwkJCSAqIGZs YWdzIGNsZWFyZWQuICBUaGlzIGhhcHBlbnMgd2hlbiB0aGUgZmxhZyB3YXMKPiArCQkJICogcnVu LXRpbWUgZGlzYWJsZWQgYW5kIHJhY2VkIHdpdGggSS9PIHN1Ym1pc3Npb24gaW4KPiArCQkJICog dGhlIGJsb2NrIGxheWVyLiAgV2Ugc3VibWl0IGl0IGFzIGEgbm9ybWFsIHdyaXRlCgpTaW5jZSBi bGtmcm9udCBubyBsb25nZXIgc2lnbmFscyB0aGF0IEZVQSBpcyBubyBsb25nZXIgYXZhaWxhYmxl IGZvciB0aGUKZGV2aWNlLCBnZXR0aW5nIGEgcmVxdWVzdCB3aXRoIEZVQSBpcyBub3QgYWN0dWFs bHkgYSByYWNlIEkgdGhpbms/Cgo+ICsJCQkgKiBoZXJlLiAgQSBwdXJlIGZsdXNoIHNob3VsZCBu ZXZlciBlbmQgdXAgaGVyZSB3aXRoCj4gKwkJCSAqIHRoZSBmbGFncyBjbGVhcmVkIGFzIHRoZXkg YXJlIGNvbXBsZXRlZCBlYXJsaWVyIGZvcgo+ICsJCQkgKiB0aGUgIWZlYXR1cmVfZmx1c2ggY2Fz ZS4KPiAgCQkJICovCj4gIAkJCWlmIChpbmZvLT5mZWF0dXJlX2ZsdXNoICYmIGluZm8tPmZlYXR1 cmVfZnVhKQo+ICAJCQkJcmluZ19yZXEtPm9wZXJhdGlvbiA9Cj4gQEAgLTc5NSw4ICs4MDMsNiBA QCBzdGF0aWMgaW50IGJsa2lmX3F1ZXVlX3J3X3JlcShzdHJ1Y3QgcmVxdWVzdCAqcmVxLCBzdHJ1 Y3QgYmxrZnJvbnRfcmluZ19pbmZvICpyaQo+ICAJCQllbHNlIGlmIChpbmZvLT5mZWF0dXJlX2Zs dXNoKQo+ICAJCQkJcmluZ19yZXEtPm9wZXJhdGlvbiA9Cj4gIAkJCQkJQkxLSUZfT1BfRkxVU0hf RElTS0NBQ0hFOwo+IC0JCQllbHNlCj4gLQkJCQlyaW5nX3JlcS0+b3BlcmF0aW9uID0gMDsKPiAg CQl9Cj4gIAkJcmluZ19yZXEtPnUucncubnJfc2VnbWVudHMgPSBudW1fZ3JhbnQ7Cj4gIAkJaWYg KHVubGlrZWx5KHJlcXVpcmVfZXh0cmFfcmVxKSkgewo+IEBAIC04ODcsMTYgKzg5Myw2IEBAIHN0 YXRpYyBpbmxpbmUgdm9pZCBmbHVzaF9yZXF1ZXN0cyhzdHJ1Y3QgYmxrZnJvbnRfcmluZ19pbmZv ICpyaW5mbykKPiAgCQlub3RpZnlfcmVtb3RlX3ZpYV9pcnEocmluZm8tPmlycSk7Cj4gIH0KPiAg Cj4gLXN0YXRpYyBpbmxpbmUgYm9vbCBibGtpZl9yZXF1ZXN0X2ZsdXNoX2ludmFsaWQoc3RydWN0 IHJlcXVlc3QgKnJlcSwKPiAtCQkJCQkgICAgICAgc3RydWN0IGJsa2Zyb250X2luZm8gKmluZm8p Cj4gLXsKPiAtCXJldHVybiAoYmxrX3JxX2lzX3Bhc3N0aHJvdWdoKHJlcSkgfHwKPiAtCQkoKHJl cV9vcChyZXEpID09IFJFUV9PUF9GTFVTSCkgJiYKPiAtCQkgIWluZm8tPmZlYXR1cmVfZmx1c2gp IHx8Cj4gLQkJKChyZXEtPmNtZF9mbGFncyAmIFJFUV9GVUEpICYmCj4gLQkJICFpbmZvLT5mZWF0 dXJlX2Z1YSkpOwo+IC19Cj4gLQo+ICBzdGF0aWMgYmxrX3N0YXR1c190IGJsa2lmX3F1ZXVlX3Jx KHN0cnVjdCBibGtfbXFfaHdfY3R4ICpoY3R4LAo+ICAJCQkgIGNvbnN0IHN0cnVjdCBibGtfbXFf cXVldWVfZGF0YSAqcWQpCj4gIHsKPiBAQCAtOTA4LDIzICs5MDQsMzAgQEAgc3RhdGljIGJsa19z dGF0dXNfdCBibGtpZl9xdWV1ZV9ycShzdHJ1Y3QgYmxrX21xX2h3X2N0eCAqaGN0eCwKPiAgCXJp bmZvID0gZ2V0X3JpbmZvKGluZm8sIHFpZCk7Cj4gIAlibGtfbXFfc3RhcnRfcmVxdWVzdChxZC0+ cnEpOwo+ICAJc3Bpbl9sb2NrX2lycXNhdmUoJnJpbmZvLT5yaW5nX2xvY2ssIGZsYWdzKTsKPiAt CWlmIChSSU5HX0ZVTEwoJnJpbmZvLT5yaW5nKSkKPiAtCQlnb3RvIG91dF9idXN5Owo+ICAKPiAt CWlmIChibGtpZl9yZXF1ZXN0X2ZsdXNoX2ludmFsaWQocWQtPnJxLCByaW5mby0+ZGV2X2luZm8p KQo+IC0JCWdvdG8gb3V0X2VycjsKPiArCS8qCj4gKwkgKiBDaGVjayBpZiB0aGUgYmFja2VuZCBh Y3R1YWxseSBzdXBwb3J0cyBmbHVzaGVzLgo+ICsJICoKPiArCSAqIFdoaWxlIHRoZSBibG9jayBs YXllciB3b24ndCBzZW5kIHVzIGZsdXNoZXMgaWYgd2UgZG9uJ3QgY2xhaW0gdG8KPiArCSAqIHN1 cHBvcnQgdGhlbSwgdGhlIFhlbiBwcm90b2NvbCBhbGxvd3MgdGhlIGJhY2tlbmQgdG8gcmV2b2tl IHN1cHBvcnQKPiArCSAqIGF0IGFueSB0aW1lLiAgVGhhdCBpcyBvZiBjb3Vyc2UgYSByZWFsbHkg YmFkIGlkZWEgYW5kIGRhbmdlcm91cywgYnV0Cj4gKwkgKiBoYXMgYmVlbiBhbGxvd2VkIGZvciAx MCsgeWVhcnMuICBJbiB0aGF0IGNhc2Ugd2Ugc2ltcGx5IGNsZWFyIHRoZQo+ICsJICogZmxhZ3Ms IGFuZCBkaXJlY3RseSByZXR1cm4gaGVyZSBmb3IgYW4gZW1wdHkgZmx1c2ggYW5kIGlnbm9yZSB0 aGUKPiArCSAqIEZVQSBmbGFnIGxhdGVyIG9uLgo+ICsJICovCj4gKwlpZiAodW5saWtlbHkocmVx X29wKHFkLT5ycSkgPT0gUkVRX09QX0ZMVVNIICYmICFpbmZvLT5mZWF0dXJlX2ZsdXNoKSkKPiAr CQlnb3RvIG91dDsKCkRvbid0IHlvdSBuZWVkIHRvIGNvbXBsZXRlIHRoZSByZXF1ZXN0IGhlcmU/ CgpUaGFua3MsIFJvZ2VyLgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fCkxpbnV4IE1URCBkaXNjdXNzaW9uIG1haWxpbmcgbGlzdApodHRwOi8v bGlzdHMuaW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LW10ZC8K From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CA416C27C75 for ; Fri, 14 Jun 2024 07:57:34 +0000 (UTC) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=citrix.com header.i=@citrix.com header.a=rsa-sha256 header.s=google header.b=bxCYI+/0; dkim-atps=neutral Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4W0s7w4YRPz3cYb for ; Fri, 14 Jun 2024 17:57:32 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dmarc=pass (p=reject dis=none) header.from=citrix.com Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=citrix.com header.i=@citrix.com header.a=rsa-sha256 header.s=google header.b=bxCYI+/0; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=cloud.com (client-ip=2607:f8b0:4864:20::834; helo=mail-qt1-x834.google.com; envelope-from=roger.pau@cloud.com; receiver=lists.ozlabs.org) Received: from mail-qt1-x834.google.com (mail-qt1-x834.google.com [IPv6:2607:f8b0:4864:20::834]) (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 lists.ozlabs.org (Postfix) with ESMTPS id 4W0s7656SYz3bwL for ; Fri, 14 Jun 2024 17:56:50 +1000 (AEST) Received: by mail-qt1-x834.google.com with SMTP id d75a77b69052e-4405cf0cb1eso10021791cf.1 for ; Fri, 14 Jun 2024 00:56:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=citrix.com; s=google; t=1718351805; x=1718956605; darn=lists.ozlabs.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=BedO1x12znmji4xt7UpC8XDe1FvgdzsEI86mvlWGudA=; b=bxCYI+/0fLSNcmfMBJkuGGZOREGrcq2lBwf4YdPy3z5iK4FuEtS2xZHh4OfR2b0zvr N0AoVV8mfF9kru9IvVGgPlmDBLF/rN35wPz/XbSJj8uQvqFRtCzl0YP+ZmaiGz+zbaNZ hJaKlIabvabSMAdK2beJuuVFMZbM3Hh+Y4aGk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718351805; x=1718956605; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=BedO1x12znmji4xt7UpC8XDe1FvgdzsEI86mvlWGudA=; b=pTfgXGNM54/1uWT7LIOhVej0VZl5POA85byK2FKIgmg+vSqVtBDIvH+/VAN1FprJWD 0X13haKVVCN1+HqQ1wB1OP9/VISznYEQXtsz6X0rZNH8a3uFMV/GabOZcaa7u+C0pmxB cO0+6IZFshapf3dhBAhhsjrZl3BxPULa0vnfIdDt/bZwgySlMqDVPLEoPUn8BQQ9IYo4 qxpP+hjOR1dn+0Gtk1lrBex7/UZ/sSqI44AJ+dhSVnPEIBR+bKzYicL5YxDOg6CxFZp3 B9lU9Y9Z6EF+dcOTjmZGv/9eKjs38w2CndiOrPYQXMRvGDsxaNCdz1zEn7nNvA9BuE4A Rdww== X-Forwarded-Encrypted: i=1; AJvYcCU1TAwWE3gnkPSzcSbt0tSJwMbwPxBfiuZUpNmXm9MiAUI9DZeakmhWk3oqnO2SwEL4elkBnUKfUXSUtyFKirbjKqk/o87//217dPpGUQ== X-Gm-Message-State: AOJu0Yz+/9ZZBE+zJNPsfnJR578wQzUE2azXqqcl1eK7Vv2niKEH7ion JHUP3NYoTKLIm3c20tdIwrAv+P65Etn1Z1LBURDjXbbR3v+Nwuueoaji3VkSvto= X-Google-Smtp-Source: AGHT+IEDi8BhVjLu2vbVbmBxB83hP6gyIibQXt+H84ODwaVrhmTedMPZYFZeMGGsfo/+7hGiDnXdRg== X-Received: by 2002:a05:622a:1822:b0:43e:2639:a987 with SMTP id d75a77b69052e-44216b3a874mr27744421cf.59.1718351804940; Fri, 14 Jun 2024 00:56:44 -0700 (PDT) Received: from localhost ([213.195.124.163]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-441f2ff9ef8sm13823221cf.89.2024.06.14.00.56.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 14 Jun 2024 00:56:44 -0700 (PDT) Date: Fri, 14 Jun 2024 09:56:42 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Christoph Hellwig Subject: Re: [PATCH 10/26] xen-blkfront: don't disable cache flushes when they fail Message-ID: References: <20240611051929.513387-1-hch@lst.de> <20240611051929.513387-11-hch@lst.de> <20240612150030.GA29188@lst.de> <20240613140508.GA16529@lst.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240613140508.GA16529@lst.de> X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: nvdimm@lists.linux.dev, "Michael S. Tsirkin" , Jason Wang , linux-nvme@lists.infradead.org, Song Liu , linux-mtd@lists.infradead.org, Vineeth Vijayan , linux-bcache@vger.kernel.org, Alasdair Kergon , drbd-dev@lists.linbit.com, linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org, Richard Weinberger , Geert Uytterhoeven , Yu Kuai , dm-devel@lists.linux.dev, linux-um@lists.infradead.org, Mike Snitzer , Josef Bacik , Ming Lei , linux-raid@vger.kernel.org, linux-m68k@lists.linux-m68k.org, Mikulas Patocka , xen-devel@lists.xenproject.org, ceph-devel@vger.kernel.org, nbd@other.debian.org, Jens Axboe , linux-block@vger.kernel.org, "Martin K. Petersen" , linux-mmc@vger.kernel.org, Philipp Reisner , Christoph =?utf-8?Q?B=C3=B6hmwalder?= , virtualization@lists.linux.dev, Lars Ellenberg , linuxppc-dev@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Thu, Jun 13, 2024 at 04:05:08PM +0200, Christoph Hellwig wrote: > On Wed, Jun 12, 2024 at 05:56:15PM +0200, Roger Pau Monné wrote: > > Right. AFAICT advertising "feature-barrier" and/or > > "feature-flush-cache" could be done based on whether blkback > > understand those commands, not on whether the underlying storage > > supports the equivalent of them. > > > > Worst case we can print a warning message once about the underlying > > storage failing to complete flush/barrier requests, and that data > > integrity might not be guaranteed going forward, and not propagate the > > error to the upper layer? > > > > What would be the consequence of propagating a flush error to the > > upper layers? > > If you propage the error to the upper layer you will generate an > I/O error there, which usually leads to a file system shutdown. > > > Given the description of the feature in the blkif header, I'm afraid > > we cannot guarantee that seeing the feature exposed implies barrier or > > flush support, since the request could fail at any time (or even from > > the start of the disk attachment) and it would still sadly be a correct > > implementation given the description of the options. > > Well, then we could do something like the patch below, which keeps > the existing behavior, but insolates the block layer from it and > removes the only user of blk_queue_write_cache from interrupt > context: LGTM, I'm not sure there's much else we can do. > --- > From e6e82c769ab209a77302994c3829cf6ff7a595b8 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig > Date: Thu, 30 May 2024 08:58:52 +0200 > Subject: xen-blkfront: don't disable cache flushes when they fail > > blkfront always had a robust negotiation protocol for detecting a write > cache. Stop simply disabling cache flushes in the block layer as the > flags handling is moving to the atomic queue limits API that needs > user context to freeze the queue for that. Instead handle the case > of the feature flags cleared inside of blkfront. This removes old > debug code to check for such a mismatch which was previously impossible > to hit, including the check for passthrough requests that blkfront > never used to start with. > > Signed-off-by: Christoph Hellwig > --- > drivers/block/xen-blkfront.c | 44 +++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 21 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index 9b4ec3e4908cce..e2c92d5095ff17 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -788,6 +788,14 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri > * A barrier request a superset of FUA, so we can > * implement it the same way. (It's also a FLUSH+FUA, > * since it is guaranteed ordered WRT previous writes.) > + * > + * Note that can end up here with a FUA write and the > + * flags cleared. This happens when the flag was > + * run-time disabled and raced with I/O submission in > + * the block layer. We submit it as a normal write Since blkfront no longer signals that FUA is no longer available for the device, getting a request with FUA is not actually a race I think? > + * here. A pure flush should never end up here with > + * the flags cleared as they are completed earlier for > + * the !feature_flush case. > */ > if (info->feature_flush && info->feature_fua) > ring_req->operation = > @@ -795,8 +803,6 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri > else if (info->feature_flush) > ring_req->operation = > BLKIF_OP_FLUSH_DISKCACHE; > - else > - ring_req->operation = 0; > } > ring_req->u.rw.nr_segments = num_grant; > if (unlikely(require_extra_req)) { > @@ -887,16 +893,6 @@ static inline void flush_requests(struct blkfront_ring_info *rinfo) > notify_remote_via_irq(rinfo->irq); > } > > -static inline bool blkif_request_flush_invalid(struct request *req, > - struct blkfront_info *info) > -{ > - return (blk_rq_is_passthrough(req) || > - ((req_op(req) == REQ_OP_FLUSH) && > - !info->feature_flush) || > - ((req->cmd_flags & REQ_FUA) && > - !info->feature_fua)); > -} > - > static blk_status_t blkif_queue_rq(struct blk_mq_hw_ctx *hctx, > const struct blk_mq_queue_data *qd) > { > @@ -908,23 +904,30 @@ static blk_status_t blkif_queue_rq(struct blk_mq_hw_ctx *hctx, > rinfo = get_rinfo(info, qid); > blk_mq_start_request(qd->rq); > spin_lock_irqsave(&rinfo->ring_lock, flags); > - if (RING_FULL(&rinfo->ring)) > - goto out_busy; > > - if (blkif_request_flush_invalid(qd->rq, rinfo->dev_info)) > - goto out_err; > + /* > + * Check if the backend actually supports flushes. > + * > + * While the block layer won't send us flushes if we don't claim to > + * support them, the Xen protocol allows the backend to revoke support > + * at any time. That is of course a really bad idea and dangerous, but > + * has been allowed for 10+ years. In that case we simply clear the > + * flags, and directly return here for an empty flush and ignore the > + * FUA flag later on. > + */ > + if (unlikely(req_op(qd->rq) == REQ_OP_FLUSH && !info->feature_flush)) > + goto out; Don't you need to complete the request here? Thanks, Roger. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-f171.google.com (mail-qt1-f171.google.com [209.85.160.171]) by mail19.linbit.com (LINBIT Mail Daemon) with ESMTP id E664C4203D9 for ; Fri, 14 Jun 2024 09:56:45 +0200 (CEST) Received: by mail-qt1-f171.google.com with SMTP id d75a77b69052e-4405cf0cb1eso10021781cf.1 for ; Fri, 14 Jun 2024 00:56:45 -0700 (PDT) Date: Fri, 14 Jun 2024 09:56:42 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Christoph Hellwig Subject: Re: [PATCH 10/26] xen-blkfront: don't disable cache flushes when they fail Message-ID: References: <20240611051929.513387-1-hch@lst.de> <20240611051929.513387-11-hch@lst.de> <20240612150030.GA29188@lst.de> <20240613140508.GA16529@lst.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240613140508.GA16529@lst.de> Cc: nvdimm@lists.linux.dev, "Michael S. Tsirkin" , Jason Wang , linux-nvme@lists.infradead.org, Song Liu , linux-mtd@lists.infradead.org, Vineeth Vijayan , linux-bcache@vger.kernel.org, Alasdair Kergon , drbd-dev@lists.linbit.com, linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org, Richard Weinberger , Geert Uytterhoeven , Yu Kuai , dm-devel@lists.linux.dev, linux-um@lists.infradead.org, Mike Snitzer , Josef Bacik , Ming Lei , linux-raid@vger.kernel.org, linux-m68k@lists.linux-m68k.org, Mikulas Patocka , xen-devel@lists.xenproject.org, ceph-devel@vger.kernel.org, nbd@other.debian.org, Jens Axboe , linux-block@vger.kernel.org, "Martin K. Petersen" , linux-mmc@vger.kernel.org, Philipp Reisner , virtualization@lists.linux.dev, Lars Ellenberg , linuxppc-dev@lists.ozlabs.org List-Id: "*Coordination* of development, patches, contributions -- *Questions* \(even to developers\) go to drbd-user, please." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Thu, Jun 13, 2024 at 04:05:08PM +0200, Christoph Hellwig wrote: > On Wed, Jun 12, 2024 at 05:56:15PM +0200, Roger Pau Monné wrote: > > Right. AFAICT advertising "feature-barrier" and/or > > "feature-flush-cache" could be done based on whether blkback > > understand those commands, not on whether the underlying storage > > supports the equivalent of them. > > > > Worst case we can print a warning message once about the underlying > > storage failing to complete flush/barrier requests, and that data > > integrity might not be guaranteed going forward, and not propagate the > > error to the upper layer? > > > > What would be the consequence of propagating a flush error to the > > upper layers? > > If you propage the error to the upper layer you will generate an > I/O error there, which usually leads to a file system shutdown. > > > Given the description of the feature in the blkif header, I'm afraid > > we cannot guarantee that seeing the feature exposed implies barrier or > > flush support, since the request could fail at any time (or even from > > the start of the disk attachment) and it would still sadly be a correct > > implementation given the description of the options. > > Well, then we could do something like the patch below, which keeps > the existing behavior, but insolates the block layer from it and > removes the only user of blk_queue_write_cache from interrupt > context: LGTM, I'm not sure there's much else we can do. > --- > From e6e82c769ab209a77302994c3829cf6ff7a595b8 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig > Date: Thu, 30 May 2024 08:58:52 +0200 > Subject: xen-blkfront: don't disable cache flushes when they fail > > blkfront always had a robust negotiation protocol for detecting a write > cache. Stop simply disabling cache flushes in the block layer as the > flags handling is moving to the atomic queue limits API that needs > user context to freeze the queue for that. Instead handle the case > of the feature flags cleared inside of blkfront. This removes old > debug code to check for such a mismatch which was previously impossible > to hit, including the check for passthrough requests that blkfront > never used to start with. > > Signed-off-by: Christoph Hellwig > --- > drivers/block/xen-blkfront.c | 44 +++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 21 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index 9b4ec3e4908cce..e2c92d5095ff17 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -788,6 +788,14 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri > * A barrier request a superset of FUA, so we can > * implement it the same way. (It's also a FLUSH+FUA, > * since it is guaranteed ordered WRT previous writes.) > + * > + * Note that can end up here with a FUA write and the > + * flags cleared. This happens when the flag was > + * run-time disabled and raced with I/O submission in > + * the block layer. We submit it as a normal write Since blkfront no longer signals that FUA is no longer available for the device, getting a request with FUA is not actually a race I think? > + * here. A pure flush should never end up here with > + * the flags cleared as they are completed earlier for > + * the !feature_flush case. > */ > if (info->feature_flush && info->feature_fua) > ring_req->operation = > @@ -795,8 +803,6 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri > else if (info->feature_flush) > ring_req->operation = > BLKIF_OP_FLUSH_DISKCACHE; > - else > - ring_req->operation = 0; > } > ring_req->u.rw.nr_segments = num_grant; > if (unlikely(require_extra_req)) { > @@ -887,16 +893,6 @@ static inline void flush_requests(struct blkfront_ring_info *rinfo) > notify_remote_via_irq(rinfo->irq); > } > > -static inline bool blkif_request_flush_invalid(struct request *req, > - struct blkfront_info *info) > -{ > - return (blk_rq_is_passthrough(req) || > - ((req_op(req) == REQ_OP_FLUSH) && > - !info->feature_flush) || > - ((req->cmd_flags & REQ_FUA) && > - !info->feature_fua)); > -} > - > static blk_status_t blkif_queue_rq(struct blk_mq_hw_ctx *hctx, > const struct blk_mq_queue_data *qd) > { > @@ -908,23 +904,30 @@ static blk_status_t blkif_queue_rq(struct blk_mq_hw_ctx *hctx, > rinfo = get_rinfo(info, qid); > blk_mq_start_request(qd->rq); > spin_lock_irqsave(&rinfo->ring_lock, flags); > - if (RING_FULL(&rinfo->ring)) > - goto out_busy; > > - if (blkif_request_flush_invalid(qd->rq, rinfo->dev_info)) > - goto out_err; > + /* > + * Check if the backend actually supports flushes. > + * > + * While the block layer won't send us flushes if we don't claim to > + * support them, the Xen protocol allows the backend to revoke support > + * at any time. That is of course a really bad idea and dangerous, but > + * has been allowed for 10+ years. In that case we simply clear the > + * flags, and directly return here for an empty flush and ignore the > + * FUA flag later on. > + */ > + if (unlikely(req_op(qd->rq) == REQ_OP_FLUSH && !info->feature_flush)) > + goto out; Don't you need to complete the request here? Thanks, Roger.