Re: superfluous md_wakeup_thread()

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3456 bytes --]

On Thu, 08 Jan 2015 17:53:15 -0500 Jes Sorensen <Jes.Sorensen@redhat.com>
wrote:

> Neil,
> 
> I was looking over some md patches, and in
> commit 67f455486d2ea20b2d94d6adf5b9b783d079e321
> Author: NeilBrown <neilb@suse.de>
> Date:   Wed May 28 13:39:22 2014 +1000
> 
>     md/raid56: Don't perform reads to support writes until stripe is ready.
>     
> You add the following:
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index ad1b9be..c1e8607 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -292,9 +292,12 @@ static void do_release_stripe(struct r5conf *conf, struct stripe_head *sh,
>         BUG_ON(atomic_read(&conf->active_stripes)==0);
>         if (test_bit(STRIPE_HANDLE, &sh->state)) {
>                 if (test_bit(STRIPE_DELAYED, &sh->state) &&
> -                   !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
> +                   !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) {
>                         list_add_tail(&sh->lru, &conf->delayed_list);
> -               else if (test_bit(STRIPE_BIT_DELAY, &sh->state) &&
> +                       if (atomic_read(&conf->preread_active_stripes)
> +                           < IO_THRESHOLD)
> +                               md_wakeup_thread(conf->mddev->thread);
> +               } else if (test_bit(STRIPE_BIT_DELAY, &sh->state) &&
>                            sh->bm_seq - conf->seq_write > 0)
>                         list_add_tail(&sh->lru, &conf->bitmap_list);
>                 else {
> 
> However the additional md_wakeup_thread() seems unecessary as the
> resulting code now reads (pasted from current upstream):
> 
> static void do_release_stripe(struct r5conf *conf, struct stripe_head *sh,
>                               struct list_head *temp_inactive_list)
> {
>         BUG_ON(!list_empty(&sh->lru));
>         BUG_ON(atomic_read(&conf->active_stripes)==0);
>         if (test_bit(STRIPE_HANDLE, &sh->state)) {
>                 if (test_bit(STRIPE_DELAYED, &sh->state) &&
>                     !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) {
>                         list_add_tail(&sh->lru, &conf->delayed_list);
>                         if (atomic_read(&conf->preread_active_stripes)
>                             < IO_THRESHOLD)
>                                 md_wakeup_thread(conf->mddev->thread);
>                 } else if (test_bit(STRIPE_BIT_DELAY, &sh->state) &&
>                            sh->bm_seq - conf->seq_write > 0)
>                         list_add_tail(&sh->lru, &conf->bitmap_list);
>                 else {
>                         clear_bit(STRIPE_DELAYED, &sh->state);
>                         clear_bit(STRIPE_BIT_DELAY, &sh->state);
>                         if (conf->worker_cnt_per_group == 0) {
>                                 list_add_tail(&sh->lru, &conf->handle_list);
>                         } else {
>                                 raid5_wakeup_stripe_thread(sh);
>                                 return;
>                         }
>                 }
>                 md_wakeup_thread(conf->mddev->thread);
> 
> Is there a reason to wake the thread twice?

Nope.  No reason at all.  Clearly I need to find a way to get people to
review my patches *before* I commit them....
Maybe I should try posting them to the list more :-)

Would you like to send a patch to revert that pointless change?

Thanks,
NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]