Re: [PATCH V3 2/2] RAID1: avoid unnecessary spin locks in I/O barrier code

[NeilBrown <neilb@suse.de>]

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

On Thu, Feb 16 2017, colyli@suse.de wrote:

> @@ -2393,6 +2455,11 @@ static void handle_write_finished(struct r1conf *conf, struct r1bio *r1_bio)
>  		idx = sector_to_idx(r1_bio->sector);
>  		conf->nr_queued[idx]++;
>  		spin_unlock_irq(&conf->device_lock);
> +		/*
> +		 * In case freeze_array() is waiting for condition
> +		 * get_unqueued_pending() == extra to be true.
> +		 */
> +		wake_up(&conf->wait_barrier);
>  		md_wakeup_thread(conf->mddev->thread);
>  	} else {
>  		if (test_bit(R1BIO_WriteError, &r1_bio->state))
> @@ -2529,9 +2596,7 @@ static void raid1d(struct md_thread *thread)
>  						  retry_list);
>  			list_del(&r1_bio->retry_list);
>  			idx = sector_to_idx(r1_bio->sector);
> -			spin_lock_irqsave(&conf->device_lock, flags);
>  			conf->nr_queued[idx]--;
> -			spin_unlock_irqrestore(&conf->device_lock, flags);

Why do you think it is safe to decrement nr_queued without holding the
lock?
Surely this could race with handle_write_finished, and an update could
be lost.

Otherwise, looks good.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]