Re: [PATCH V6 6/8] io_uring: support providing sqe group buffer

[Ming Lei <ming.lei@redhat.com>]

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