Re: [PATCH V2] block: avoid io timeout in case of sync polled dio

[Ming Lei <ming.lei@redhat.com>]

On Tue, Apr 19, 2022 at 07:39:24AM +0200, Christoph Hellwig wrote:
> On Mon, Apr 18, 2022 at 04:19:09PM +0800, Ming Lei wrote:
> > But there isn't any such users from module now. Maybe never, since sync
> > polled dio becomes legacy after io_uring is invented.
> 
> I thought about that a bit, but if we decided synchronous polled I/O
> is not in major user anymore (which I think) and we think it is enough
> of a burden to support (which I'm not sure of, but this patch points to)
> then it might be time to remove it.

I agree to remove it because it is legacy poll interface, and very likely
no one use it since the problem to be addressed can easily be exposed by
sync polled dio sanity test or 'blktests block/007'.

I'd suggest to switch sync polled dio into non-polled silently, just with
logging sort of 'sync polled io is obsolete, please use io_uring for io
polling', or any other idea?

> 
> > Do we have such potential use case in which explicit flush plug is
> > needed except for polled io in __blkdev_direct_IO_simple() and swap_readpage()?
> > 
> > If there is, I am happy to add one flag for bypassing plug in blk core
> > code.
> 
> I think the point is that we need to offer sensible APIs for I/O
> methods we want to support.
> 
> > > 
> > > > iomap is one good example to show this point, since it does flush the plug
> > > > before call bio_poll(), see __iomap_dio_rw().
> > > 
> > > iomap does not do a manual plug flush anywhere.
> > > 
> > > iomap does finish the plug before polling, which makes sense.
> > > 
> > > Now of course __blkdev_direct_IO_simple doesn't even use a plug
> > > to start with, so I'm wondering what plug this patch even tries
> > > to flush?
> >  
> > At least blkdev_write_iter(), and __swap_writepage() might call
> > into ->direct_IO with one plug too.
> > 
> > Not mention loop driver can call into ->direct_IO directly, and we
> > should have applied plug for batching submission in loop_process_work().
> 
> The loop driver still calls through the read/write iter method, and
> the swap code ->direct_IO path is not used for block devices (and
> completley broken right now, see the patch series from Neil).
> 
> But the loop driver is a good point, even for the iomap case as the
> blk_finish_plug would only flush the plug that is on-stack in
> __iomap_dio_rw, it would not help you with any plug holder by caller
> higher in the stack.

Right, good catch!


thanks,
Ming