From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mail19.linbit.com (LINBIT Mail Daemon) with ESMTP id 0E9F542089E for ; Tue, 11 Jun 2024 09:30:46 +0200 (CEST) Message-ID: Date: Tue, 11 Jun 2024 16:30:39 +0900 MIME-Version: 1.0 Subject: Re: [PATCH 10/26] xen-blkfront: don't disable cache flushes when they fail To: Christoph Hellwig , Jens Axboe References: <20240611051929.513387-1-hch@lst.de> <20240611051929.513387-11-hch@lst.de> Content-Language: en-US From: Damien Le Moal In-Reply-To: <20240611051929.513387-11-hch@lst.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: nvdimm@lists.linux.dev, "Michael S. Tsirkin" , Jason Wang , linux-nvme@lists.infradead.org, Song Liu , linux-mtd@lists.infradead.org, Vineeth Vijayan , 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 , nbd@other.debian.org, linux-raid@vger.kernel.org, linux-m68k@lists.linux-m68k.org, Mikulas Patocka , xen-devel@lists.xenproject.org, ceph-devel@vger.kernel.org, Ming Lei , linux-bcache@vger.kernel.org, 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, =?UTF-8?Q?Roger_Pau_Monn=C3=A9?= 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 6/11/24 2:19 PM, Christoph Hellwig wrote: > blkfront always had a robust negotiation protocol for detecting a write > cache. Stop simply disabling cache flushes when they fail as that is > a grave error. > > Signed-off-by: Christoph Hellwig Looks good to me but maybe mention that removal of xlvbd_flush() as well ? And it feels like the "stop disabling cache flushes when they fail" part should be a fix patch sent separately... Anyway, for both parts, feel free to add: Reviewed-by: Damien Le Moal > --- > drivers/block/xen-blkfront.c | 29 +++++++++-------------------- > 1 file changed, 9 insertions(+), 20 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index 9b4ec3e4908cce..9794ac2d3299d1 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -982,18 +982,6 @@ static const char *flush_info(struct blkfront_info *info) > return "barrier or flush: disabled;"; > } > > -static void xlvbd_flush(struct blkfront_info *info) > -{ > - blk_queue_write_cache(info->rq, info->feature_flush ? true : false, > - info->feature_fua ? true : false); > - pr_info("blkfront: %s: %s %s %s %s %s %s %s\n", > - info->gd->disk_name, flush_info(info), > - "persistent grants:", info->feature_persistent ? > - "enabled;" : "disabled;", "indirect descriptors:", > - info->max_indirect_segments ? "enabled;" : "disabled;", > - "bounce buffer:", info->bounce ? "enabled" : "disabled;"); > -} > - > static int xen_translate_vdev(int vdevice, int *minor, unsigned int *offset) > { > int major; > @@ -1162,7 +1150,15 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity, > info->sector_size = sector_size; > info->physical_sector_size = physical_sector_size; > > - xlvbd_flush(info); > + blk_queue_write_cache(info->rq, info->feature_flush ? true : false, > + info->feature_fua ? true : false); > + > + pr_info("blkfront: %s: %s %s %s %s %s %s %s\n", > + info->gd->disk_name, flush_info(info), > + "persistent grants:", info->feature_persistent ? > + "enabled;" : "disabled;", "indirect descriptors:", > + info->max_indirect_segments ? "enabled;" : "disabled;", > + "bounce buffer:", info->bounce ? "enabled" : "disabled;"); > > if (info->vdisk_info & VDISK_READONLY) > set_disk_ro(gd, 1); > @@ -1622,13 +1618,6 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id) > info->gd->disk_name, op_name(bret.operation)); > blkif_req(req)->error = BLK_STS_NOTSUPP; > } > - if (unlikely(blkif_req(req)->error)) { > - if (blkif_req(req)->error == BLK_STS_NOTSUPP) > - blkif_req(req)->error = BLK_STS_OK; > - info->feature_fua = 0; > - info->feature_flush = 0; > - xlvbd_flush(info); > - } > fallthrough; > case BLKIF_OP_READ: > case BLKIF_OP_WRITE: -- Damien Le Moal Western Digital Research