On Tue, Jan 18, 2022 at 09:25:08AM +0100, Christoph Hellwig wrote:
> On Mon, Jan 17, 2022 at 05:08:23PM +0800, Ming Lei wrote:
> > > We need it to have proper life times in the block layer. Everything only
> > > needed for file system I/O and not blk-mq specific should slowly move
> > > from the request_queue to the gendisk and I have patches going in
> > > that direction. In the end only the SCSI discovery code and the case
> > > of /dev/sg without SCSI ULP will ever do passthrough I/O purely on the
> > > gendisk.
> > >
> > > So I think this series is moving in the wrong direction. If you care
> > > about no doing two freeze cycles the right thing to do is to record
> >
> > I just think that the extra draining point in del_gendisk() isn't useful,
> > can you share any use case with this change?
>
> SCSI disk detach for example is a place where we need it.
SCSI disk detach doesn't need it, since 8e141f9eb803 ("block: drain file
system I/O on del_gendisk") is added for fixing q->disk in Sept., 2021, but
sd detach has been there for dozens of years.
del_gendisk() starts to prevent new IO from being submitted to queue,
and sync and flush dirty pages, but other in-flight IOs are still fine,
since disk release will wait for all of them.
>
> > > if we ever did non-disk based passthrough I/O on a requeue_queue and
> > > if not simplify the request_queue cleanup. Doing this is on my TODO
> > > list but I haven't look into the details yet.
> > >
> > > > 1) queue freezing can't drain FS I/O for bio based driver
> > >
> > > This is something I've started looking into it.
> >
> > But that is one big problem, not sure you can solve it in short time,
> > also not sure if it is useful, cause FS already guaranteed that every
> > IO is drained before releasing disk, or IOs in the submission task are
> > drained when exiting the task.
>
> Think of a hot unplug. The device gets a removal even, but the file
> system still lives on.
No need the extra drain point, same with above.
>
> > Firstly, FS layer has already guaranteed that every FS IO is done before
> > releasing disk, so no need to take so much effort and make code more
> > fragile to add one extra FS IO draining point in del_gendisk().
>
> In the hot removal case the file system is still alive when del_gendisk
> is called.
Yeah, but draining IO in del_gendisk() doesn't make difference from FS
viewpoint, does it?
>
> > Also the above two things aren't trivial enough to solve in short time, so
> > can we delay the FS draining in del_gendisk() until the two are done?
>
> We already have the draining. What are you trying to fix by removing it?
It is just added months ago, and doesn't mean it is reasonable/necessary.
Now I am thinking of handling this stuff in the following approach:
1) move block cgroup and rqos allocation into allocating disk, and move their
destroying into disk release handler; and in long term, both two can be moved
into gendisk, as you mentioned.
2) just run io accounting on passthrough request from user space, such
as, sg io and nvme ns io, and one RQF_USER_IO flag may help to do that; for
other private kinds of command, we needn't to account them and q->disk may
not be available for them.
3) once the above two are done, we still can remove the added io draining in
del_gendisk().
Thanks,
Ming
