On 11/30/23 17:25, Stefan Hajnoczi wrote: > On Wed, Nov 29, 2023 at 11:01:33PM -0800, Chaitanya Kulkarni wrote: >> Improve block layer request handling by implementing a timeout handler. >> Current implementation assums that request will never timeout and will >> be completed by underlaying transport. However, this assumption can >> cause issues under heavy load especially when dealing with different >> subsystems and real hardware. >> >> To solve this, add a block layer request timeout handler that will >> complete timed-out requests in the same context if the virtio device >> has a VIRTIO_CONFIG_S_DRIVER_OK status. If the device has any other >> status, we'll stop the block layer request queue and proceed with the >> teardown sequence, allowing applications waiting for I/O to exit >> gracefully with appropriate error. >> >> Also, add two new module parameters that allows user to specify the >> I/O timeout for the tagset when allocating the disk and a teardown limit >> for the timed out requeets before we initiate device teardown from the >> timeout handler. These changes will improve the stability and >> reliability of our system under request timeout scenario. >> >> Signed-off-by: Chaitanya Kulkarni <kch@xxxxxxxxxx> >> --- >> drivers/block/virtio_blk.c | 122 ++++++++++++++++++++++++++++++++ >> include/uapi/linux/virtio_blk.h | 1 + >> 2 files changed, 123 insertions(+) > Hi, > This looks interesting. There was a discussion about implementing > ->timeout() in September: > https://lore.kernel.org/io-uring/20230926145526.GA387951@fedora/ Thanks for pointing that out ... >> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c >> index 4689ac2e0c0e..da26c2bf933b 100644 >> --- a/drivers/block/virtio_blk.c >> +++ b/drivers/block/virtio_blk.c >> @@ -16,6 +16,7 @@ >> #include <linux/blk-mq-virtio.h> >> #include <linux/numa.h> >> #include <linux/vmalloc.h> >> +#include <linux/xarray.h> >> #include <uapi/linux/virtio_ring.h> >> >> #define PART_BITS 4 >> @@ -31,6 +32,15 @@ >> #define VIRTIO_BLK_INLINE_SG_CNT 2 >> #endif >> >> +static unsigned int io_timeout = 20; >> +module_param(io_timeout, uint, 0644); >> +MODULE_PARM_DESC(io_timeout, "timeout in seconds for I/O requests. Default:20"); >> + >> +static unsigned int timeout_teardown_limit = 2; >> +module_param(timeout_teardown_limit, uint, 0644); >> +MODULE_PARM_DESC(timeout_teardown_limit, >> + "request timeout teardown limit for stable dev. Default:2"); >> + >> static unsigned int num_request_queues; >> module_param(num_request_queues, uint, 0644); >> MODULE_PARM_DESC(num_request_queues, >> @@ -84,6 +94,20 @@ struct virtio_blk { >> >> /* For zoned device */ >> unsigned int zone_sectors; >> + >> + /* >> + * Block layer Request timeout teardown limit when device is in the >> + * stable state, i.e. it has VIRTIO_CONFIG_S_DRIVER_OK value for its >> + * config status. Once this limit is reached issue >> + * virtblk_teardown_work to teardown the device in the block lyaer >> + * request timeout callback. >> + */ >> + atomic_t rq_timeout_count; >> + /* avoid tear down race between remove and teardown work */ >> + struct mutex teardown_mutex; >> + /* tear down work to be scheduled from block layer request handler */ >> + struct work_struct teardown_work; >> + >> }; >> >> struct virtblk_req { >> @@ -117,6 +141,8 @@ static inline blk_status_t virtblk_result(u8 status) >> case VIRTIO_BLK_S_OK: >> return BLK_STS_OK; >> case VIRTIO_BLK_S_UNSUPP: >> + case VIRTIO_BLK_S_TIMEOUT: >> + return BLK_STS_TIMEOUT; >> return BLK_STS_NOTSUPP; >> case VIRTIO_BLK_S_ZONE_OPEN_RESOURCE: >> return BLK_STS_ZONE_OPEN_RESOURCE; >> @@ -926,6 +952,7 @@ static void virtblk_free_disk(struct gendisk *disk) >> struct virtio_blk *vblk = disk->private_data; >> >> ida_free(&vd_index_ida, vblk->index); >> + mutex_destroy(&vblk->teardown_mutex); >> mutex_destroy(&vblk->vdev_mutex); >> kfree(vblk); >> } >> @@ -1287,6 +1314,86 @@ static int virtblk_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob) >> return found; >> } >> >> +static bool virtblk_cancel_request(struct request *rq, void *data) >> +{ >> + struct virtblk_req *vbr = blk_mq_rq_to_pdu(rq); >> + >> + vbr->in_hdr.status = VIRTIO_BLK_S_TIMEOUT; >> + if (blk_mq_request_started(rq) && !blk_mq_request_completed(rq)) >> + blk_mq_complete_request(rq); >> + >> + return true; >> +} >> + >> +static void virtblk_teardown_work(struct work_struct *w) >> +{ >> + struct virtio_blk *vblk = >> + container_of(w, struct virtio_blk, teardown_work); >> + struct request_queue *q = vblk->disk->queue; >> + struct virtio_device *vdev = vblk->vdev; >> + struct blk_mq_hw_ctx *hctx; >> + unsigned long idx; >> + >> + mutex_lock(&vblk->teardown_mutex); >> + if (!vblk->vdev) >> + goto unlock; >> + >> + blk_mq_quiesce_queue(q); >> + >> + /* Process any outstanding request from device. */ >> + xa_for_each(&q->hctx_table, idx, hctx) >> + virtblk_poll(hctx, NULL); >> + >> + blk_sync_queue(q); >> + blk_mq_tagset_busy_iter(&vblk->tag_set, virtblk_cancel_request, vblk); >> + blk_mq_tagset_wait_completed_request(&vblk->tag_set); >> + >> + /* >> + * Unblock any pending dispatch I/Os before we destroy device. From >> + * del_gendisk() -> __blk_mark_disk_dead(disk) will set GD_DEAD flag, >> + * that will make sure any new I/O from bio_queue_enter() to fail. >> + */ >> + blk_mq_unquiesce_queue(q); >> + del_gendisk(vblk->disk); >> + blk_mq_free_tag_set(&vblk->tag_set); >> + >> + mutex_lock(&vblk->vdev_mutex); >> + flush_work(&vblk->config_work); >> + >> + virtio_reset_device(vdev); >> + >> + vblk->vdev = NULL; >> + >> + vdev->config->del_vqs(vdev); >> + kfree(vblk->vqs); >> + >> + mutex_unlock(&vblk->vdev_mutex); >> + >> + put_disk(vblk->disk); >> + >> +unlock: >> + mutex_unlock(&vblk->teardown_mutex); >> +} >> + >> +static enum blk_eh_timer_return virtblk_timeout(struct request *req) >> +{ >> + struct virtio_blk *vblk = req->mq_hctx->queue->queuedata; >> + struct virtio_device *vdev = vblk->vdev; >> + bool ok = vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK; > Please check VIRTIO_CONFIG_S_NEEDS_RESET first. When > VIRTIO_CONFIG_S_NEEDS_RESET is set the device is not ok, regardless of > whether DRIVER_OK is set. See 2.1.1 Driver Requirements: Device Status > Field in the VIRTIO specification. okay will add that to next version ... > >> + >> + if ((atomic_dec_return(&vblk->rq_timeout_count) != 0) && ok) { >> + virtblk_cancel_request(req, NULL); > The driver cannot abandon the request here because the device still owns > the request: > > 1. I/O buffer memory is corrupted for READ requests and disk contents > are corrupted for WRITE requests when the device does finally process > the request. > > 2. After virtblk_timeout() -> virtblk_cancel_request() -> > blk_mq_complete_request(), a freed address is returned from > virtblk_done() -> virtqueue_get_buf() -> blk_mq_rq_from_pdu() when > the device finally completes the request. > > I suggest the following: > > (Optional) Add an ABORT/CANCEL request type to the VIRTIO specification > and use it to safely cancel requests when the device supports it. I really want to keep this simple without introducing a new command so we don't have to modify the underlying transport to handle that. > Otherwise reset the device so that all pending requests are canceled. will issue reset here and see if that works ... >> + return BLK_EH_DONE; >> + } >> + >> + dev_err(&vdev->dev, "%s:%s initiating teardown\n", __func__, >> + vblk->disk->disk_name); >> + >> + queue_work(virtblk_wq, &vblk->teardown_work); >> + >> + return BLK_EH_RESET_TIMER; >> +} >> + >> static const struct blk_mq_ops virtio_mq_ops = { >> .queue_rq = virtio_queue_rq, >> .queue_rqs = virtio_queue_rqs, >> @@ -1294,6 +1401,7 @@ static const struct blk_mq_ops virtio_mq_ops = { >> .complete = virtblk_request_done, >> .map_queues = virtblk_map_queues, >> .poll = virtblk_poll, >> + .timeout = virtblk_timeout, >> }; >> >> static unsigned int virtblk_queue_depth; >> @@ -1365,6 +1473,7 @@ static int virtblk_probe(struct virtio_device *vdev) >> memset(&vblk->tag_set, 0, sizeof(vblk->tag_set)); >> vblk->tag_set.ops = &virtio_mq_ops; >> vblk->tag_set.queue_depth = queue_depth; >> + vblk->tag_set.timeout = io_timeout * HZ; >> vblk->tag_set.numa_node = NUMA_NO_NODE; >> vblk->tag_set.flags = BLK_MQ_F_SHOULD_MERGE; >> vblk->tag_set.cmd_size = >> @@ -1387,6 +1496,10 @@ static int virtblk_probe(struct virtio_device *vdev) >> } >> q = vblk->disk->queue; >> >> + mutex_init(&vblk->teardown_mutex); >> + INIT_WORK(&vblk->teardown_work, virtblk_teardown_work); >> + atomic_set(&vblk->rq_timeout_count, timeout_teardown_limit); >> + >> virtblk_name_format("vd", index, vblk->disk->disk_name, DISK_NAME_LEN); >> >> vblk->disk->major = major; >> @@ -1598,6 +1711,12 @@ static void virtblk_remove(struct virtio_device *vdev) >> { >> struct virtio_blk *vblk = vdev->priv; >> >> + mutex_lock(&vblk->teardown_mutex); >> + >> + /* we did the cleanup in the timeout handler */ >> + if (!vblk->vdev) >> + goto unlock; >> + >> /* Make sure no work handler is accessing the device. */ >> flush_work(&vblk->config_work); >> >> @@ -1618,6 +1737,9 @@ static void virtblk_remove(struct virtio_device *vdev) >> mutex_unlock(&vblk->vdev_mutex); >> >> put_disk(vblk->disk); >> + >> +unlock: >> + mutex_unlock(&vblk->teardown_mutex); >> } >> >> #ifdef CONFIG_PM_SLEEP >> diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h >> index 3744e4da1b2a..ed864195ab26 100644 >> --- a/include/uapi/linux/virtio_blk.h >> +++ b/include/uapi/linux/virtio_blk.h >> @@ -317,6 +317,7 @@ struct virtio_scsi_inhdr { >> #define VIRTIO_BLK_S_OK 0 >> #define VIRTIO_BLK_S_IOERR 1 >> #define VIRTIO_BLK_S_UNSUPP 2 >> +#define VIRTIO_BLK_S_TIMEOUT 3 > The structs and constants in this header file come from the VIRTIO > specification. Anything changed in this file must first be accepted into > the VIRTIO spec because this is the hardware interface definition. > > VIRTIO_BLK_S_TIMEOUT seems to be synthetic value that is purely used by > software, not the device. Maybe there is no need to update the spec. > Just avoid using in_hdr.status to signal timeouts and use a separate > flag/field instead in a block layer or virtio_blk driver request struct. It is a specific error hence I've added that on the similar lines, do you have a specific field in mind that you would prefer ? Thanks for the reply, just got back from time off, looking forward to send V2 as soon as I get the reply. -ck
