On Fri, Sep 06, 2024 at 06:15:32PM +0100, Pavel Begunkov wrote: > On 8/29/24 05:29, Ming Lei wrote: > ... > > > > + if (WARN_ON_ONCE(lead->grp_refs <= 0)) > > > > + return false; > > > > + > > > > + req->flags &= ~REQ_F_SQE_GROUP; > > > > > > I'm getting completely lost when and why it clears and sets > > > back REQ_F_SQE_GROUP and REQ_F_SQE_GROUP_LEADER. Is there any > > > rule? > > > > My fault, it should have been documented somewhere. > > > > REQ_F_SQE_GROUP is cleared when the request is completed, but it is > > reused as flag for marking the last request in this group, so we can > > free the group leader when observing the 'last' member request. > > Maybe it'd be cleaner to use a second flag? I will add one new flag with same value, since the two's lifetime is non-overlapping. > > > The only other difference about the two flags is that both are cleared > > when the group leader becomes the last one in the group, then > > this leader degenerates as normal request, which way can simplify > > group leader freeing. > > > > > > > > > + /* > > > > + * Set linked leader as failed if any member is failed, so > > > > + * the remained link chain can be terminated > > > > + */ > > > > + if (unlikely((req->flags & REQ_F_FAIL) && > > > > + ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link))) > > > > + req_set_fail(lead); > > > > > > if (req->flags & REQ_F_FAIL) > > > req_set_fail(lead); > > > > > > REQ_F_FAIL is not specific to links, if a request fails we need > > > to mark it as such. > > > > It is for handling group failure. > > > > The following condition > > > > ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link)) > > > > means that this group is in one link-chain. > > > > If any member in this group is failed, we need to fail this group(lead), > > then the remained requests in this chain can be failed. > > > > Otherwise, it isn't necessary to fail group leader in case of any member > > io failure. > > What bad would happen if you do it like this? > > if (req->flags & REQ_F_FAIL) > req_set_fail(lead); > > I'm asking because if you rely on some particular combination > of F_FAIL and F_LINK somewhere, it's likely wrong, but otherwise > we F_FAIL a larger set of requests, which should never be an > issue.
