| Message ID | 20240122110722.690223-2-yi.sun@unisoc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix requests loss during virtio-blk device suspend | expand |
Hi Yi, kernel test robot noticed the following build warnings: [auto build test WARNING on axboe-block/for-next] [also build test WARNING on linus/master v6.8-rc1 next-20240123] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Yi-Sun/blk-mq-introduce-blk_mq_tagset_wait_request_completed/20240122-192222 base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next patch link: https://lore.kernel.org/r/20240122110722.690223-2-yi.sun%40unisoc.com patch subject: [PATCH 1/2] blk-mq: introduce blk_mq_tagset_wait_request_completed() config: i386-buildonly-randconfig-002-20240123 (https://download.01.org/0day-ci/archive/20240124/202401240242.k3K7SkiZ-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240124/202401240242.k3K7SkiZ-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202401240242.k3K7SkiZ-lkp@intel.com/ All warnings (new ones prefixed by >>): >> block/blk-mq-tag.c:498: warning: Function parameter or struct member 'tagset' not described in 'blk_mq_tagset_wait_request_completed' vim +498 block/blk-mq-tag.c 490 491 /** 492 * blk_mq_tagset_wait_request_completed - Wait for all inflight requests 493 * to become completed. 494 * 495 * Note: This function has to be run after all IO queues are shutdown. 496 */ 497 void blk_mq_tagset_wait_request_completed(struct blk_mq_tag_set *tagset) > 498 { 499 while (true) { 500 unsigned int count = 0; 501 502 blk_mq_tagset_busy_iter(tagset, 503 blk_mq_tagset_count_inflight_rqs, &count); 504 if (!count) 505 break; 506 msleep(20); 507 } 508 } 509 EXPORT_SYMBOL(blk_mq_tagset_wait_request_completed); 510
On Mon, Jan 22, 2024 at 07:07:21PM +0800, Yi Sun wrote: > In some cases, it is necessary to wait for all requests to become complete > status before performing other operations. Otherwise, these requests will never > be processed successfully. > > For example, when the virtio device is in hibernation, the virtqueues > will be deleted. It must be ensured that virtqueue is not in use before being deleted. > Otherwise the requests in the virtqueue will be lost. This function can ensure > that all requests have been taken out of the virtqueues. > > Prepare for fixing this kind of issue by introducing > blk_mq_tagset_wait_request_completed(). Does blk_mq_freeze_queue() not work for your use case? I think that should work unless you have some driver specific requests entered that don't ever get released. > +static bool blk_mq_tagset_count_inflight_rqs(struct request *rq, void *data) > +{ > + unsigned int *count = data; > + > + if (blk_mq_request_started(rq) && !blk_mq_request_completed(rq)) > + (*count)++; > + return true; > +} > + > +/** > + * blk_mq_tagset_wait_request_completed - Wait for all inflight requests > + * to become completed. > + * > + * Note: This function has to be run after all IO queues are shutdown. > + */ > +void blk_mq_tagset_wait_request_completed(struct blk_mq_tag_set *tagset) > +{ > + while (true) { > + unsigned int count = 0; > + > + blk_mq_tagset_busy_iter(tagset, > + blk_mq_tagset_count_inflight_rqs, &count); If the tagset is shared, then one active user can prevent this from ever completing. It sounds like your use case cares about just one specific request_queue, not all of them. > + if (!count) > + break; > + msleep(20); > + } > +} > +EXPORT_SYMBOL(blk_mq_tagset_wait_request_completed);
In my case, I want all hw queues owned by this device to be clean. Because in the virtio device, each hw queue corresponds to a virtqueue, and all virtqueues will be deleted when vdev suspends. The blk_mq_tagset_wait_request_completed() function can ensure that the device has processed all in_flight requests , and these requests have become the complete state. I don’t understand the blk_mq_freeze_queue() function very well. Can the function ensure that all requests have become complete status? How should I use the function to achieve the same effect? ps: requests become in_flight status in virtio_queue_rq() and become complete status in virtblk_done(). Yi On Wed, Jan 24, 2024 at 3:14 AM Keith Busch <kbusch@kernel.org> wrote: > > On Mon, Jan 22, 2024 at 07:07:21PM +0800, Yi Sun wrote: > > In some cases, it is necessary to wait for all requests to become complete > > status before performing other operations. Otherwise, these requests will never > > be processed successfully. > > > > For example, when the virtio device is in hibernation, the virtqueues > > will be deleted. It must be ensured that virtqueue is not in use before being deleted. > > Otherwise the requests in the virtqueue will be lost. This function can ensure > > that all requests have been taken out of the virtqueues. > > > > Prepare for fixing this kind of issue by introducing > > blk_mq_tagset_wait_request_completed(). > > Does blk_mq_freeze_queue() not work for your use case? I think that > should work unless you have some driver specific requests entered that > don't ever get released. > > > +static bool blk_mq_tagset_count_inflight_rqs(struct request *rq, void *data) > > +{ > > + unsigned int *count = data; > > + > > + if (blk_mq_request_started(rq) && !blk_mq_request_completed(rq)) > > + (*count)++; > > + return true; > > +} > > + > > +/** > > + * blk_mq_tagset_wait_request_completed - Wait for all inflight requests > > + * to become completed. > > + * > > + * Note: This function has to be run after all IO queues are shutdown. > > + */ > > +void blk_mq_tagset_wait_request_completed(struct blk_mq_tag_set *tagset) > > +{ > > + while (true) { > > + unsigned int count = 0; > > + > > + blk_mq_tagset_busy_iter(tagset, > > + blk_mq_tagset_count_inflight_rqs, &count); > > If the tagset is shared, then one active user can prevent this from ever > completing. It sounds like your use case cares about just one specific > request_queue, not all of them. > > > + if (!count) > > + break; > > + msleep(20); > > + } > > +} > > +EXPORT_SYMBOL(blk_mq_tagset_wait_request_completed);
On Wed, Jan 24, 2024 at 07:22:21PM +0800, yi sun wrote: > In my case, I want all hw queues owned by this device to be clean. > Because in the virtio device, each hw queue corresponds to a virtqueue, > and all virtqueues will be deleted when vdev suspends. > > The blk_mq_tagset_wait_request_completed() function can ensure that > the device has processed all in_flight requests , and these requests have > become the complete state. I don´t understand the blk_mq_freeze_queue() > function very well. Can the function ensure that all requests have become > complete status? How should I use the function to achieve the same effect? Yeah, just do something like this: --- diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 5bf98fd6a651a..2f69675abdf29 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -1593,14 +1593,14 @@ static int virtblk_freeze(struct virtio_device *vdev) { struct virtio_blk *vblk = vdev->priv; + blk_mq_freeze_queue(vblk->disk->queue); + /* Ensure we don't receive any more interrupts */ virtio_reset_device(vdev); /* Make sure no work handler is accessing the device. */ flush_work(&vblk->config_work); - blk_mq_quiesce_queue(vblk->disk->queue); - vdev->config->del_vqs(vdev); kfree(vblk->vqs); --
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c index cc57e2dd9a0b..cb0ef5f66c61 100644 --- a/block/blk-mq-tag.c +++ b/block/blk-mq-tag.c @@ -479,6 +479,35 @@ void blk_mq_tagset_wait_completed_request(struct blk_mq_tag_set *tagset) } EXPORT_SYMBOL(blk_mq_tagset_wait_completed_request); +static bool blk_mq_tagset_count_inflight_rqs(struct request *rq, void *data) +{ + unsigned int *count = data; + + if (blk_mq_request_started(rq) && !blk_mq_request_completed(rq)) + (*count)++; + return true; +} + +/** + * blk_mq_tagset_wait_request_completed - Wait for all inflight requests + * to become completed. + * + * Note: This function has to be run after all IO queues are shutdown. + */ +void blk_mq_tagset_wait_request_completed(struct blk_mq_tag_set *tagset) +{ + while (true) { + unsigned int count = 0; + + blk_mq_tagset_busy_iter(tagset, + blk_mq_tagset_count_inflight_rqs, &count); + if (!count) + break; + msleep(20); + } +} +EXPORT_SYMBOL(blk_mq_tagset_wait_request_completed); + /** * blk_mq_queue_tag_busy_iter - iterate over all requests with a driver tag * @q: Request queue to examine. diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h index a676e116085f..17768e154193 100644 --- a/include/linux/blk-mq.h +++ b/include/linux/blk-mq.h @@ -891,6 +891,7 @@ void blk_mq_run_hw_queues(struct request_queue *q, bool async); void blk_mq_delay_run_hw_queues(struct request_queue *q, unsigned long msecs); void blk_mq_tagset_busy_iter(struct blk_mq_tag_set *tagset, busy_tag_iter_fn *fn, void *priv); +void blk_mq_tagset_wait_request_completed(struct blk_mq_tag_set *tagset); void blk_mq_tagset_wait_completed_request(struct blk_mq_tag_set *tagset); void blk_mq_freeze_queue(struct request_queue *q); void blk_mq_unfreeze_queue(struct request_queue *q);
In some cases, it is necessary to wait for all requests to become complete status before performing other operations. Otherwise, these requests will never be processed successfully. For example, when the virtio device is in hibernation, the virtqueues will be deleted. It must be ensured that virtqueue is not in use before being deleted. Otherwise the requests in the virtqueue will be lost. This function can ensure that all requests have been taken out of the virtqueues. Prepare for fixing this kind of issue by introducing blk_mq_tagset_wait_request_completed(). Signed-off-by: Yi Sun <yi.sun@unisoc.com> --- block/blk-mq-tag.c | 29 +++++++++++++++++++++++++++++ include/linux/blk-mq.h | 1 + 2 files changed, 30 insertions(+)