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:4.424 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site