Break immediately if raid5_get_active_stripe() returns NULL and deindent
the rest of the loop. Annotate this check with an unlikely().
This makes the code easier to read and reduces the indentation level.
No functional changes intended.
Signed-off-by: Logan Gunthorpe <[email protected]>
---
drivers/md/raid5.c | 109 +++++++++++++++++++++++----------------------
1 file changed, 55 insertions(+), 54 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 97b23c18402b..cda6857e6207 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5906,68 +5906,69 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
sh = raid5_get_active_stripe(conf, new_sector, previous,
(bi->bi_opf & REQ_RAHEAD), 0);
- if (sh) {
- if (unlikely(previous)) {
- /* expansion might have moved on while waiting for a
- * stripe, so we must do the range check again.
- * Expansion could still move past after this
- * test, but as we are holding a reference to
- * 'sh', we know that if that happens,
- * STRIPE_EXPANDING will get set and the expansion
- * won't proceed until we finish with the stripe.
- */
- int must_retry = 0;
- spin_lock_irq(&conf->device_lock);
- if (!ahead_of_reshape(mddev, logical_sector,
- conf->reshape_progress))
- /* mismatch, need to try again */
- must_retry = 1;
- spin_unlock_irq(&conf->device_lock);
- if (must_retry) {
- raid5_release_stripe(sh);
- schedule();
- do_prepare = true;
- goto retry;
- }
- }
- if (read_seqcount_retry(&conf->gen_lock, seq)) {
- /* Might have got the wrong stripe_head
- * by accident
- */
- raid5_release_stripe(sh);
- goto retry;
- }
+ if (unlikely(!sh)) {
+ /* cannot get stripe, just give-up */
+ bi->bi_status = BLK_STS_IOERR;
+ break;
+ }
- if (test_bit(STRIPE_EXPANDING, &sh->state) ||
- !add_stripe_bio(sh, bi, dd_idx, rw, previous)) {
- /* Stripe is busy expanding or
- * add failed due to overlap. Flush everything
- * and wait a while
- */
- md_wakeup_thread(mddev->thread);
+ if (unlikely(previous)) {
+ /* expansion might have moved on while waiting for a
+ * stripe, so we must do the range check again.
+ * Expansion could still move past after this
+ * test, but as we are holding a reference to
+ * 'sh', we know that if that happens,
+ * STRIPE_EXPANDING will get set and the expansion
+ * won't proceed until we finish with the stripe.
+ */
+ int must_retry = 0;
+ spin_lock_irq(&conf->device_lock);
+ if (!ahead_of_reshape(mddev, logical_sector,
+ conf->reshape_progress))
+ /* mismatch, need to try again */
+ must_retry = 1;
+ spin_unlock_irq(&conf->device_lock);
+ if (must_retry) {
raid5_release_stripe(sh);
schedule();
do_prepare = true;
goto retry;
}
- if (do_flush) {
- set_bit(STRIPE_R5C_PREFLUSH, &sh->state);
- /* we only need flush for one stripe */
- do_flush = false;
- }
+ }
- set_bit(STRIPE_HANDLE, &sh->state);
- clear_bit(STRIPE_DELAYED, &sh->state);
- if ((!sh->batch_head || sh == sh->batch_head) &&
- (bi->bi_opf & REQ_SYNC) &&
- !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
- atomic_inc(&conf->preread_active_stripes);
- release_stripe_plug(mddev, sh);
- } else {
- /* cannot get stripe for read-ahead, just give-up */
- bi->bi_status = BLK_STS_IOERR;
- break;
+ if (read_seqcount_retry(&conf->gen_lock, seq)) {
+ /* Might have got the wrong stripe_head by accident */
+ raid5_release_stripe(sh);
+ goto retry;
+ }
+
+ if (test_bit(STRIPE_EXPANDING, &sh->state) ||
+ !add_stripe_bio(sh, bi, dd_idx, rw, previous)) {
+ /*
+ * Stripe is busy expanding or add failed due to
+ * overlap. Flush everything and wait a while.
+ */
+ md_wakeup_thread(mddev->thread);
+ raid5_release_stripe(sh);
+ schedule();
+ do_prepare = true;
+ goto retry;
}
+
+ if (do_flush) {
+ set_bit(STRIPE_R5C_PREFLUSH, &sh->state);
+ /* we only need flush for one stripe */
+ do_flush = false;
+ }
+
+ set_bit(STRIPE_HANDLE, &sh->state);
+ clear_bit(STRIPE_DELAYED, &sh->state);
+ if ((!sh->batch_head || sh == sh->batch_head) &&
+ (bi->bi_opf & REQ_SYNC) &&
+ !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+ atomic_inc(&conf->preread_active_stripes);
+
+ release_stripe_plug(mddev, sh);
}
finish_wait(&conf->wait_for_overlap, &w);
--
2.30.2
On Wed, Apr 20, 2022 at 01:54:15PM -0600, Logan Gunthorpe wrote:
> Break immediately if raid5_get_active_stripe() returns NULL and deindent
> the rest of the loop. Annotate this check with an unlikely().
>
> This makes the code easier to read and reduces the indentation level.
>
> No functional changes intended.
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
On 4/21/22 3:54 AM, Logan Gunthorpe wrote:
> Break immediately if raid5_get_active_stripe() returns NULL and deindent
> the rest of the loop. Annotate this check with an unlikely().
>
> This makes the code easier to read and reduces the indentation level.
>
> No functional changes intended.
>
> Signed-off-by: Logan Gunthorpe<[email protected]>
> ---
> drivers/md/raid5.c | 109 +++++++++++++++++++++++----------------------
> 1 file changed, 55 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 97b23c18402b..cda6857e6207 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -5906,68 +5906,69 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
...
> + if (unlikely(!sh)) {
> + /* cannot get stripe, just give-up */
> + bi->bi_status = BLK_STS_IOERR;
> + break;
> + }
Nit, I would suggest to keep below original comment.
> - /* cannot get stripe for read-ahead, just give-up */
> - bi->bi_status = BLK_STS_IOERR;
> - break;
Anyway. Reviewed-by: Guoqing Jiang <[email protected]>
Thanks,
Guoqing
On 2022-04-26 19:32, Guoqing Jiang wrote:
>
>
> On 4/21/22 3:54 AM, Logan Gunthorpe wrote:
>> Break immediately if raid5_get_active_stripe() returns NULL and deindent
>> the rest of the loop. Annotate this check with an unlikely().
>>
>> This makes the code easier to read and reduces the indentation level.
>>
>> No functional changes intended.
>>
>> Signed-off-by: Logan Gunthorpe<[email protected]>
>> ---
>> drivers/md/raid5.c | 109 +++++++++++++++++++++++----------------------
>> 1 file changed, 55 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index 97b23c18402b..cda6857e6207 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -5906,68 +5906,69 @@ static bool raid5_make_request(struct mddev
>> *mddev, struct bio * bi)
>
> ...
>
>> + if (unlikely(!sh)) {
>> + /* cannot get stripe, just give-up */
>> + bi->bi_status = BLK_STS_IOERR;
>> + break;
>> + }
>
>
> Nit, I would suggest to keep below original comment.
But the original comment was plainly wrong...
Logan
On 4/28/22 12:08 AM, Logan Gunthorpe wrote:
>
>>> + if (unlikely(!sh)) {
>>> + /* cannot get stripe, just give-up */
>>> + bi->bi_status = BLK_STS_IOERR;
>>> + break;
>>> + }
>>
>> Nit, I would suggest to keep below original comment.
> But the original comment was plainly wrong...
I think it was for get stripe for read-ahead.
https://elixir.bootlin.com/linux/v5.18-rc4/source/drivers/md/raid5.c#L5869
And it deserves a separate change if you think the comment is wrong, but
just my $0.02.
Thanks,
Guoqing