On Wed, Oct 09, 2024 at 12:57:48PM +0100, Pavel Begunkov wrote:
> On 10/6/24 10:47, Ming Lei wrote:
> > On Fri, Oct 04, 2024 at 04:32:04PM +0100, Pavel Begunkov wrote:
> > > On 9/12/24 11:49, Ming Lei wrote:
> > > ...
> > ...
> > > > @@ -473,6 +494,7 @@ enum {
> > > > REQ_F_BUFFERS_COMMIT_BIT,
> > > > REQ_F_SQE_GROUP_LEADER_BIT,
> > > > REQ_F_SQE_GROUP_DEP_BIT,
> > > > + REQ_F_GROUP_KBUF_BIT,
> > > > /* not a real bit, just to check we're not overflowing the space */
> > > > __REQ_F_LAST_BIT,
> > > > @@ -557,6 +579,8 @@ enum {
> > > > REQ_F_SQE_GROUP_LEADER = IO_REQ_FLAG(REQ_F_SQE_GROUP_LEADER_BIT),
> > > > /* sqe group with members depending on leader */
> > > > REQ_F_SQE_GROUP_DEP = IO_REQ_FLAG(REQ_F_SQE_GROUP_DEP_BIT),
> > > > + /* group lead provides kbuf for members, set for both lead and member */
> > > > + REQ_F_GROUP_KBUF = IO_REQ_FLAG(REQ_F_GROUP_KBUF_BIT),
> > >
> > > We have a huge flag problem here. It's a 4th group flag, that gives
> > > me an idea that it's overabused. We're adding state machines based on
> > > them "set group, clear group, but if last set it again. And clear
> > > group lead if refs are of particular value". And it's not really
> > > clear what these two flags are here for or what they do.
> > >
> > > From what I see you need here just one flag to mark requests
> > > that provide a buffer, ala REQ_F_PROVIDING_KBUF. On the import
> > > side:
> > >
> > > if ((req->flags & GROUP) && (req->lead->flags & REQ_F_PROVIDING_KBUF))
> > > ...
> > >
> > > And when you kill the request:
> > >
> > > if (req->flags & REQ_F_PROVIDING_KBUF)
> > > io_group_kbuf_drop();
> >
> > REQ_F_PROVIDING_KBUF may be killed too, and the check helper can become:
> >
> > bool io_use_group_provided_buf(req)
> > {
> > return (req->flags & GROUP) && req->lead->grp_buf;
> > }
>
> ->grp_kbuf is unionised, so for that to work you need to ensure that
> only a buffer providing cmd / request could be a leader of a group,
> which doesn't sound right.
Yes, both 'req->lead->flags & REQ_F_PROVIDING_KBUF' and 'req->lead->grp_buf'
may not work because the helper may be called in ->prep(), when req->lead
isn't setup yet.
Another idea is to reuse one of the three unused flags(LINK, HARDLINK and DRAIN)
of members for marking GROUP_KBUF, then it is aligned with BUFFER_SELECT and
implementation can be cleaner, what do you think of this approach?
Thanks,
Ming
