Thread (19 messages) 19 messages, 6 authors, 2020-04-23

Re: [PATCH] block: Limit number of items taken from the I/O scheduler in one go

From: Salman Qazi <hidden>
Date: 2020-02-07 20:38:10
Also in: lkml

On Fri, Feb 7, 2020 at 12:19 PM Bart Van Assche [off-list ref] wrote:
On 2/7/20 10:45 AM, Salman Qazi wrote:
quoted
If I were to write this as a for-loop, it will look like this:

for (i = 0; i == 0 || (run_again && i < 2); i++) {
/* another level of 8 character wide indentation */
     run_again = false;
    /* a bunch of code that possibly sets run_again to true
}

if (run_again)
     blk_mq_run_hw_queue(hctx, true);
That's not what I meant. What I meant is a loop that iterates at most
two times and also to break out of the loop if run_again == false.
I picked the most compact variant to demonstrate the problem.  Adding
breaks isn't
really helping the readability.

for (i = 0; i < 2; i++) {
  run_again = false;
/* bunch of code that possibly sets it to true */
...
 if (!run_again)
    break;
}
if (run_again)
    blk_mq_run_hw_queue(hctx, true);

When I read this, I initially assume that the loop in general runs
twice and that this is the common case.  It has the
same problem with conveying intent.  Perhaps, more importantly, the
point of using programming constructs is to shorten and simplify the
code.
There are still two if-statements in addition to the loop. We haven't
gained much by introducing the loop.
BTW, I share your concern about the additional indentation by eight
positions. How about avoiding deeper indentation by introducing a new
function?
If there was a benefit to introducing the loop, this would be a good
call.  But the way I see it, the introduction of another
function is yet another way in which the introduction of the loop
makes the code less readable.

This is not a hill I want to die on.  If the maintainer agrees with
you on this point, I will use a loop.
Thanks,

Bart.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help