Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Yu Kuai]

Hi,

在 2022/08/18 9:23, Yu Kuai 写道:
> Hi, Tejun!
>
> 在 2022/08/18 1:50, Tejun Heo 写道:
> > Hello,
> >
> > On Wed, Aug 17, 2022 at 09:13:38AM +0800, Yu Kuai wrote:
> > > > So, as a fix for the immediate problem, I guess this might do but 
> > > > this feels
> > > > really fragile. How can we be certain that re-entering only happens 
> > > > because
> > > > of splitting? What if future core development changes that? It seems 
> > > > to be
> > > > solving the problem in the wrong place. Shouldn't we flag the bio 
> > > > indicating
> > > > that it's split when we're splitting the bio so that we only limit 
> > > > them for
> > > > iops in the first place?
> > >
> > > Splited bio is tracked in __bio_clone:
> >
> > As the word is used in commit messages and comments, the past perfect 
> > form
> > of the verb "split" is "split". It looks like "splitted" is used in rare
> > cases but dictionary says it's an archaic form.
>
> Ok, thanks for pointing it out, I'll change that in next iteration.
> > > if (bio_flagged(bio_src, BIO_THROTTLED))
> > >     bio_set_flag(bio, BIO_THROTTLED);

While implementing the new method, I found that there seems to be a
misunderstanding here, the code seems to try to add flag to split bio
so that it won't be throttled again for bps limit, however:

1) for blk throttle, split bio is issued directly and will never be
throttled again, while orignal bio will go through throttle path again.
2) if cloned bio is directed to a new disk, the flag is cleared anyway.
> > > And currenty, the iops limit and bps limit are treated differently,
> > > however there are only one flag 'BIO_THROTTLED' and they can't be
> > > distinguished.
> > >
> > > Perhaps I can use two flags, for example BIO_IOPS_THROTTLED and
> > > BIO_BPS_THROTTLED, this way only iops limit can be handled and bps
> > > limit can be skipped for splited bio.
> > >
> > > What do you think?
> >
> > I think the code would be a lot more intuitive and less fragile if we 
> > used
> > two flags but the bits in the bi_flags field are a scarce resource
> > unfortunately. Even then, I think the right thing to do here is using two
> > flags.
>
> Yes, the field 'bio->bi_flags' is unsigned short, and there are only two
> bits left. I'll use the new sulution which will acquire a new bit.
>
> Thanks,
> Kuai
> > Thanks.
>
> .