[PATCH 1/2] RAID5: check_reshape() shouldn't call mddev_suspend

[PATCH 1/2] RAID5: check_reshape() shouldn't call mddev_suspend

[Shaohua Li]

check_reshape() is called from raid5d thread. raid5d thread shouldn't
call mddev_suspend(), because mddev_suspend() waits for all IO finish
but IO is handled in raid5d thread, we could easily deadlock here.

Artur,
It would be great if you can verify this works for your test.

Reported-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
Cc: NeilBrown <neilb@suse.com>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 drivers/md/raid5.c | 18 ++++++++++++++++++
 drivers/md/raid5.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 7f770b0..c7fd070 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2089,6 +2089,14 @@ static int resize_chunks(struct r5conf *conf, int new_disks, int new_sectors)
 	unsigned long cpu;
 	int err = 0;
 
+	/*
+	 * Never shrink. And mddev_suspend() could deadlock if this is called
+	 * from raid5d. In that case, scribble_disks and scribble_sectors
+	 * should equal to new_disks and new_sectors
+	 */
+	if (conf->scribble_disks >= new_disks &&
+	    conf->scribble_sectors >= new_sectors)
+		return 0;
 	mddev_suspend(conf->mddev);
 	get_online_cpus();
 	for_each_present_cpu(cpu) {
@@ -2110,6 +2118,10 @@ static int resize_chunks(struct r5conf *conf, int new_disks, int new_sectors)
 	}
 	put_online_cpus();
 	mddev_resume(conf->mddev);
+	if (!err) {
+		conf->scribble_disks = new_disks;
+		conf->scribble_sectors = new_sectors;
+	}
 	return err;
 }
 
@@ -6413,6 +6425,12 @@ static int raid5_alloc_percpu(struct r5conf *conf)
 	}
 	put_online_cpus();
 
+	if (!err) {
+		conf->scribble_disks = max(conf->raid_disks,
+			conf->previous_raid_disks);
+		conf->scribble_sectors = max(conf->chunk_sectors,
+			conf->prev_chunk_sectors) / STRIPE_SECTORS;
+	}
 	return err;
 }
 
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index a415e1c..ae6068d 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -510,6 +510,8 @@ struct r5conf {
 					      * conversions
 					      */
 	} __percpu *percpu;
+	int scribble_disks;
+	int scribble_sectors;
 #ifdef CONFIG_HOTPLUG_CPU
 	struct notifier_block	cpu_notify;
 #endif
-- 
2.6.5

[PATCH 2/2] MD: warn for potential deadlock

[Shaohua Li]

The personality thread shouldn't call mddev_suspend(). Because
mddev_suspend() will for all IO finish, but IO is handled in personality
thread, so this could cause deadlock. To trigger this early, add a
warning if mddev_suspend() is called from personality thread.

Suggested-by: NeilBrown <neilb@suse.com>
Cc: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 drivers/md/md.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 464627b..c068f17 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -305,6 +305,7 @@ static blk_qc_t md_make_request(struct request_queue *q, struct bio *bio)
  */
 void mddev_suspend(struct mddev *mddev)
 {
+	WARN_ON_ONCE(current == mddev->thread->tsk);
 	if (mddev->suspended++)
 		return;
 	synchronize_rcu();
-- 
2.6.5

Re: [PATCH 1/2] RAID5: check_reshape() shouldn't call mddev_suspend

[NeilBrown]

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

On Fri, Feb 26 2016, Shaohua Li wrote:

> check_reshape() is called from raid5d thread. raid5d thread shouldn't
> call mddev_suspend(), because mddev_suspend() waits for all IO finish
> but IO is handled in raid5d thread, we could easily deadlock here.
>
> Artur,
> It would be great if you can verify this works for your test.
>
> Reported-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> Cc: NeilBrown <neilb@suse.com>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  drivers/md/raid5.c | 18 ++++++++++++++++++
>  drivers/md/raid5.h |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 7f770b0..c7fd070 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -2089,6 +2089,14 @@ static int resize_chunks(struct r5conf *conf, int new_disks, int new_sectors)
>  	unsigned long cpu;
>  	int err = 0;
>  
> +	/*
> +	 * Never shrink. And mddev_suspend() could deadlock if this is called
> +	 * from raid5d. In that case, scribble_disks and scribble_sectors
> +	 * should equal to new_disks and new_sectors
> +	 */
> +	if (conf->scribble_disks >= new_disks &&
> +	    conf->scribble_sectors >= new_sectors)
> +		return 0;
>  	mddev_suspend(conf->mddev);
>  	get_online_cpus();
>  	for_each_present_cpu(cpu) {
> @@ -2110,6 +2118,10 @@ static int resize_chunks(struct r5conf *conf, int new_disks, int new_sectors)
>  	}
>  	put_online_cpus();
>  	mddev_resume(conf->mddev);
> +	if (!err) {
> +		conf->scribble_disks = new_disks;
> +		conf->scribble_sectors = new_sectors;
> +	}
>  	return err;
>  }
>  
> @@ -6413,6 +6425,12 @@ static int raid5_alloc_percpu(struct r5conf *conf)
>  	}
>  	put_online_cpus();
>  
> +	if (!err) {
> +		conf->scribble_disks = max(conf->raid_disks,
> +			conf->previous_raid_disks);
> +		conf->scribble_sectors = max(conf->chunk_sectors,
> +			conf->prev_chunk_sectors) / STRIPE_SECTORS;

Here we set ->scribble_sectors to a number of stripes rather than a
number of sectors.  I think you need to remove the "/ STRIPE_SECTORS".

Otherwise:
  Reviewed-by: NeilBrown <neilb@suse.com>

Thanks,
NeilBrown


> +	}
>  	return err;
>  }
>  
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index a415e1c..ae6068d 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -510,6 +510,8 @@ struct r5conf {
>  					      * conversions
>  					      */
>  	} __percpu *percpu;
> +	int scribble_disks;
> +	int scribble_sectors;
>  #ifdef CONFIG_HOTPLUG_CPU
>  	struct notifier_block	cpu_notify;
>  #endif
> -- 
> 2.6.5

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

Re: [PATCH 1/2] RAID5: check_reshape() shouldn't call mddev_suspend

[Artur Paszkiewicz]

On 02/25/2016 11:08 PM, NeilBrown wrote:
> On Fri, Feb 26 2016, Shaohua Li wrote:
> 
>> check_reshape() is called from raid5d thread. raid5d thread shouldn't
>> call mddev_suspend(), because mddev_suspend() waits for all IO finish
>> but IO is handled in raid5d thread, we could easily deadlock here.
>>
>> Artur,
>> It would be great if you can verify this works for your test.
>>
>> Reported-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
>> Cc: NeilBrown <neilb@suse.com>
>> Signed-off-by: Shaohua Li <shli@fb.com>
>> ---
>>  drivers/md/raid5.c | 18 ++++++++++++++++++
>>  drivers/md/raid5.h |  2 ++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index 7f770b0..c7fd070 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -2089,6 +2089,14 @@ static int resize_chunks(struct r5conf *conf, int new_disks, int new_sectors)
>>  	unsigned long cpu;
>>  	int err = 0;
>>  
>> +	/*
>> +	 * Never shrink. And mddev_suspend() could deadlock if this is called
>> +	 * from raid5d. In that case, scribble_disks and scribble_sectors
>> +	 * should equal to new_disks and new_sectors
>> +	 */
>> +	if (conf->scribble_disks >= new_disks &&
>> +	    conf->scribble_sectors >= new_sectors)
>> +		return 0;
>>  	mddev_suspend(conf->mddev);
>>  	get_online_cpus();
>>  	for_each_present_cpu(cpu) {
>> @@ -2110,6 +2118,10 @@ static int resize_chunks(struct r5conf *conf, int new_disks, int new_sectors)
>>  	}
>>  	put_online_cpus();
>>  	mddev_resume(conf->mddev);
>> +	if (!err) {
>> +		conf->scribble_disks = new_disks;
>> +		conf->scribble_sectors = new_sectors;
>> +	}
>>  	return err;
>>  }
>>  
>> @@ -6413,6 +6425,12 @@ static int raid5_alloc_percpu(struct r5conf *conf)
>>  	}
>>  	put_online_cpus();
>>  
>> +	if (!err) {
>> +		conf->scribble_disks = max(conf->raid_disks,
>> +			conf->previous_raid_disks);
>> +		conf->scribble_sectors = max(conf->chunk_sectors,
>> +			conf->prev_chunk_sectors) / STRIPE_SECTORS;
> 
> Here we set ->scribble_sectors to a number of stripes rather than a
> number of sectors.  I think you need to remove the "/ STRIPE_SECTORS".

That's correct. I've tested the patch and it works well but only without
the "/ STRIPE_SECTORS". 

Thanks,
Artur