Re: [PATCH 10/26] xen-blkfront: don't disable cache flushes when they fail
From: Damien Le Moal <dlemoal@kernel.org>
Date: 2024-06-11 07:30:46
Also in:
ceph-devel, dm-devel, linux-bcache, linux-block, linux-m68k, linux-mmc, linux-nvme, linux-raid, linux-s390, linux-scsi, linux-um, nvdimm, virtualization, xen-devel
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 <hch@lst.de>
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 <dlemoal@kernel.org>
quoted hunk ↗ jump to hunk
--- 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