linux-kernel - Re: [PATCH v2 1/2] workqueue: new struct io

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 1 Jul 2024 23:53:01 -0400
From: Kent Overstreet <kent.overstreet@...ux.dev>
To: Tejun Heo <tj@...nel.org>
Cc: Yi Sun <yi.sun@...soc.com>, sunyibuaa@...il.com, 
	jiangshanlai@...il.com, jaegeuk@...nel.org, chao@...nel.org, ebiggers@...gle.com, 
	linux-f2fs-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org, niuzhiguo84@...il.com, 
	Hao_hao.Wang@...soc.com, yunlongxing23@...il.com
Subject: Re: [PATCH v2 1/2] workqueue: new struct io_work

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"