Re: [PATCH v4 0/2] block, iomap: disable iopoll for split bio

[JeffleXu <jefflexu@linux.alibaba.com>]

On 11/17/20 3:56 PM, Jeffle Xu wrote:
> This patchset is to fix the potential hang occurred in sync polling.
>
> Please refer the following link for background info and the v1 patch:
> https://patchwork.kernel.org/project/linux-block/patch/20201013084051.27255-1-jefflexu@linux.alibaba.com/
>
> The first patch disables iopoll for split bio in block layer, which is
> suggested by Ming Lei.
>
>
> The second patch disables iopoll when one dio need to be split into
> multiple bios. As for this patch, Ming Lei had ever asked what's the
> expected behaviour of upper layers when simply clear IOCB_HIPRI in
> the direct routine of blkdev fs, iomap-based fs. Currently there are
> two parts concerning IOCB_HIPRI (or io polling). One is the sync
> polling logic embedded in the direct IO routine. In this case, sync
> polling won't be executed any more since IOCB_HIPRI flag has been
> cleared from iocb->ki_flags. Consider the following code snippet:
>
> fs/block_dev.c: __blkdev_direct_IO
> 	for (;;) {
> 		...
> 		if (!(iocb->ki_flags & IOCB_HIPRI) ||
> 		    !blk_poll(bdev_get_queue(bdev), qc, true))
> 			blk_io_schedule();
> 	}
>
> fs/iomap/direct-io.c: __iomap_dio_rw
> 	for (;;) {
> 		...
> 		if (!(iocb->ki_flags & IOCB_HIPRI) ||
> 		    !dio->submit.last_queue ||
> 		    !blk_poll(dio->submit.last_queue,
> 				 dio->submit.cookie, true))
> 			blk_io_schedule();
> 	}
>
>
> The other part is io_uring.
>
> fs/io_uring.c:
> io_iopoll_getevents
>    io_do_iopoll
>      list_for_each_entry_safe(...) {
>        ret = kiocb->ki_filp->f_op->iopoll(kiocb, spin);
>      }
>
> In this case, though the split bios have been enqueued into DEFAULT
> hw queues, io_uring will still poll POLL hw queues. When polling on
> the cookie returned by split bio, blk_poll() will return 0 immediately
> since the hw queue type check added in patch 1. If there's no other
> bio in the POLL hw queues, io_do_iopoll() will loop indefinitely
> until the split bio is completed by interrupt of DEFAULT queue. Indeed
> there may be a pulse of high CPU sys in this time window here, but it
> is no worse than before. After all io_do_iopoll() will still get stuck
> in this loop when there's only one bio (that we are polling on) in POLL
> hw queue, before this patch applied.
>
> The situation described above may be less impossible. As long as there
> are other bios in POLL hw queue, work of io_do_iopoll() is still
> meaningful as it *helps* reap these other bios in POLL hw queue, while
> the split bios are still completed by interrupt of DEFAULT hw queue.

ops, this design could still be problematic. Once the cookie of split 
bio is iterated in io_do_iopoll(),

io_do_iopoll() will get stuck in indefinite loop doing nothing until the 
split bio is completed by the interrupt of

DEFAULT hw queue, even when there may be other bios in POLL hw queue 
waiting to be reaped.


I need other design to fix this issue. By the way, do we need to fix the 
issue since currently this issue

won't be triggered, though by a relatively fragile constraint 
(BLKDEV_MIN_RQ/UIO_MAXIOV)?


-- 
Thanks,
Jeffle