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]

Message-ID: <CALpufv3bAsMey1DFb8HPbp8gPZWBU-6rrhvRWwVQsxXhPXOvdA@mail.gmail.com>
Date: Tue, 2 Jul 2024 17:27:19 +0800
From: yi sun <sunyibuaa@...il.com>
To: Kent Overstreet <kent.overstreet@...ux.dev>, Tejun Heo <tj@...nel.org>
Cc: Yi Sun <yi.sun@...soc.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

Yes, adding the io priority attribute to work will bring huge benefits in the
following scenarios:
The system has huge IO pressure (these IOs may all be low-priority IOs),
and a work (we hope to complete quickly) is also doing IO. If this work
does not set IO priority, it will be blocked because it is difficult to get IO
resources. And if this work sets a high IO priority and passes the IO priority
to kworker, then this work will be completed quickly (as we expect).

On Tue, Jul 2, 2024 at 11:53 AM Kent Overstreet
<kent.overstreet@...ux.dev> wrote:
>
> 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"