lkml.org 
[lkml]   [2022]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates
From
Date
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.
>

\
 
 \ /
  Last update: 2022-08-17 03:31    [W:0.051 / U:1.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site