Re: [PATCH v2 1/2] workqueue: new struct io_work

[Kent Overstreet]

On Mon, Jul 01, 2024 at 07:32:23AM GMT, Tejun Heo wrote:
> Hello,
> 
> On Mon, Jul 01, 2024 at 03:51:37PM +0800, Yi Sun wrote:
> > +/*
> > + * If a work may do disk IO, it is recommended to use struct io_work
> > + * instead of struct work_struct.
> > + */
> > +struct io_work {
> > +	struct work_struct work;
> > +
> > +	/* If the work does submit_bio, io priority may be needed. */
> > +	unsigned short ioprio;
> > +	/* Record kworker's original io priority. */
> > +	unsigned short ori_ioprio;
> > +	/* Whether the work has set io priority? */
> > +	long ioprio_flag;
> > +};
> 
> There are fundamental limitations to this approach in terms of
> prioritization. If you tag each work items with priority and then send them
> to the same workqueue, there's nothing preventing a low priority issuer from
> flooding the workqueue and causing a priority inversion. ie. To solve this
> properly, you need per-issuer-class workqueue so that the concurrency limit
> is not shared across different priorities.
> 
> Now, this limited implementation, while incomplete and easy to defeat, may
> still be useful. After all, ioprio itself, I think, is flawed in the same
> way. If f2fs wants to implement this internally, that's okay, I suppose, but
> as a generic mechanism, I don't think this makes a lot of sense.

And I wonder if the reason for submitting from a workqueue isn't also
priority inversion?

I haven't looked at the f2fs code, but that comes up in bcachefs; we
have IOs that we submit from worqueue context because they're submitted
in contexts where we _really_ cannot block - they're metadata IOs, and
thus also high priority IOs. But if the queue is already full with lower
priority IOs...

perhaps what we need is a bio flag to say "do not block in the
submission path, queue is allowed to exceed normal limits for this (high
priority) IO"