On Thu, 08 Jan 2015 17:53:15 -0500 Jes Sorensen <Jes.Sorensen@xxxxxxxxxx>
wrote:
> Neil,
>
> I was looking over some md patches, and in
> commit 67f455486d2ea20b2d94d6adf5b9b783d079e321
> Author: NeilBrown <neilb@xxxxxxx>
> 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:
pgpQNVEhwgKJq.pgp
Description: OpenPGP digital signature
