Re: [PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Yu Kuai]

Hi, Tejun!

在 2022/08/17 4:01, Tejun Heo 写道:
> On Tue, Aug 02, 2022 at 10:04:10PM +0800, Yu Kuai wrote:
> ...
> > +static void __tg_update_skipped(struct throtl_grp *tg, bool rw)
> > +{
> > +	unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw];
> > +	u64 bps_limit = tg_bps_limit(tg, rw);
> > +	u32 iops_limit = tg_iops_limit(tg, rw);
> > +
> > +	/*
> > +	 * If config is updated while bios are still throttled, calculate and
> > +	 * accumulate how many bytes/io are waited across changes. And
> > +	 * bytes/io_skipped will be used to calculate new wait time under new
> > +	 * configuration.
> > +	 *
> > +	 * Following calculation won't overflow as long as bios that are
> > +	 * dispatched later won't preempt already throttled bios. Even if such
> > +	 * overflow do happen, there should be no problem because unsigned is
> > +	 * used here, and bytes_skipped/io_skipped will be updated correctly.
> > +	 */
>
> Would it be easier if the fields were signed? It's fragile and odd to
> explain "these are unsigned but if they underflow they behave just like
> signed when added" when they can just be signed. Also, I have a hard time
> understand what "preempt" means above.

I think preempt shound never happen based on current FIFO
implementation, perhaps
> > +	if (bps_limit != U64_MAX)
> > +		tg->bytes_skipped[rw] +=
> > +			calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
> > +			tg->bytes_disp[rw];
> > +	if (iops_limit != UINT_MAX)
> > +		tg->io_skipped[rw] +=
> > +			calculate_io_allowed(iops_limit, jiffy_elapsed) -
> > +			tg->io_disp[rw];
>
> So, this is calculating the budgets to carry over. Can we name them
> accordingly? I don't know what "skipped" means.

Yeah, thanks for you advice, art of naming is a little hard for me...
How do you think about these name: extended_bytes/io_budget?
> > @@ -115,6 +115,17 @@ struct throtl_grp {
> >   	uint64_t bytes_disp[2];
> >   	/* Number of bio's dispatched in current slice */
> >   	unsigned int io_disp[2];
> > +	/*
> > +	 * The following two fields are updated when new configuration is
> > +	 * submitted while some bios are still throttled, they record how many
> > +	 * bytes/io are waited already in previous configuration, and they will
> > +	 * be used to calculate wait time under new configuration.
> > +	 *
> > +	 * Number of bytes will be skipped in current slice
> > +	 */
> > +	uint64_t bytes_skipped[2];
> > +	/* Number of bio will be skipped in current slice */
> > +	unsigned int io_skipped[2];
>
> So, the code seems to make sense but the field names and comments don't
> really, at least to me. I can't find an intuitive understanding of what's
> being skipped. Can you please take another stab at making this more
> understandable?
>
> Thanks.