[PATCH v2 0/3] md/raid1: don't change mempool if in-flight r1bio exists

[PATCH v2 0/3] md/raid1: don't change mempool if in-flight r1bio exists

[Xueshi Hu]

From: Xueshi Hu <xueshi.hu@smartx.com>

All the r1bio should be freed before raid1_reshape() changes the
mempool. However, freeze_array() doesn't guarantee there's no r1bio,
as some r1bio maybe queued. Also, in raid1_write_request() and
handle_read_error(), kernel shall not allow_barrier() before the r1bio is
properly handled.

Changes in v2:
	- fix the problem one by one instead of calling
	blk_mq_freeze_queue() as suggested by Yu Kuai

Xueshi Hu (3):
  md/raid1: gave up reshape in case there's queued io
  md/raid1: free old r1bio before retry write
  md/raid1: keep holding the barrier in handle_read_error()

 drivers/md/raid1.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

-- 
2.40.1

[PATCH v2 1/3] md/raid1: gave up reshape in case there's queued io

[Xueshi Hu]

From: Xueshi Hu <xueshi.hu@smartx.com>

When an IO error happens, reschedule_retry() will increase
r1conf::nr_queued, which makes freeze_array() unblocked. However, before
all r1bio in the memory pool are released, the memory pool should not be
modified.

Signed-off-by: Xueshi Hu <xueshi.hu@smartx.com>
---
 drivers/md/raid1.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index dd25832eb045..ce48cb3af301 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -3298,6 +3298,13 @@ static int raid1_reshape(struct mddev *mddev)
 
 	freeze_array(conf, 0);
 
+	if (unlikely(atomic_read(conf->nr_queued))) {
+		kfree(newpoolinfo);
+		mempool_exit(&newpool);
+		unfreeze_array(conf);
+		return -EBUSY;
+	}
+
 	/* ok, everything is stopped */
 	oldpool = conf->r1bio_pool;
 	conf->r1bio_pool = newpool;
-- 
2.40.1

Re: [PATCH v2 1/3] md/raid1: gave up reshape in case there's queued io

[Yu Kuai]

Hi,

在 2023/07/09 18:29, Xueshi Hu 写道:
> From: Xueshi Hu <xueshi.hu@smartx.com>
> 
> When an IO error happens, reschedule_retry() will increase
> r1conf::nr_queued, which makes freeze_array() unblocked. However, before
> all r1bio in the memory pool are released, the memory pool should not be
> modified.
> 

I didn't recieve these emails directly, I just notice this patchse...

It's right this is a problem, but I'm not sure about the fix, this fix
will work fine for the case that new reshape is started, however, for
the case that reshape is interrupted, this might cause that
md_check_recovery() can't continue to reshape atomically.

Perhaps raid1_reshape() can be improved that if oldpool and newpool is
the same, don't reallocate it.

Thanks,
Kuai
> Signed-off-by: Xueshi Hu <xueshi.hu@smartx.com>
> ---
>   drivers/md/raid1.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index dd25832eb045..ce48cb3af301 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -3298,6 +3298,13 @@ static int raid1_reshape(struct mddev *mddev)
>   
>   	freeze_array(conf, 0);
>   
> +	if (unlikely(atomic_read(conf->nr_queued))) {
> +		kfree(newpoolinfo);
> +		mempool_exit(&newpool);
> +		unfreeze_array(conf);
> +		return -EBUSY;
> +	}
> +
>   	/* ok, everything is stopped */
>   	oldpool = conf->r1bio_pool;
>   	conf->r1bio_pool = newpool;
> 

[PATCH v2 2/3] md/raid1: free old r1bio before retry write

[Xueshi Hu]

From: Xueshi Hu <xueshi.hu@smartx.com>

allow_barrier() make reshape possible. Raid reshape changes the
r1conf::raid_disks and mempool, so free the old r1bio and alloc a new one
when retry write.

Signed-off-by: Xueshi Hu <xueshi.hu@smartx.com>
---
 drivers/md/raid1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index ce48cb3af301..6c54786067b4 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1373,6 +1373,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 		return;
 	}
 
+ retry_write:
 	r1_bio = alloc_r1bio(mddev, bio);
 	r1_bio->sectors = max_write_sectors;
 
@@ -1388,7 +1389,6 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 	 */
 
 	disks = conf->raid_disks * 2;
- retry_write:
 	blocked_rdev = NULL;
 	rcu_read_lock();
 	max_sectors = r1_bio->sectors;
@@ -1468,7 +1468,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 		for (j = 0; j < i; j++)
 			if (r1_bio->bios[j])
 				rdev_dec_pending(conf->mirrors[j].rdev, mddev);
-		r1_bio->state = 0;
+		free_r1bio(r1_bio);
 		allow_barrier(conf, bio->bi_iter.bi_sector);
 
 		if (bio->bi_opf & REQ_NOWAIT) {
-- 
2.40.1

Re: [PATCH v2 2/3] md/raid1: free old r1bio before retry write

[Yu Kuai]

在 2023/07/09 18:29, Xueshi Hu 写道:
> From: Xueshi Hu <xueshi.hu@smartx.com>
> 
> allow_barrier() make reshape possible. Raid reshape changes the
> r1conf::raid_disks and mempool, so free the old r1bio and alloc a new one
> when retry write.
> 
LGTM, feel free to add:

Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> Signed-off-by: Xueshi Hu <xueshi.hu@smartx.com>
> ---
>   drivers/md/raid1.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index ce48cb3af301..6c54786067b4 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1373,6 +1373,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>   		return;
>   	}
>   
> + retry_write:
>   	r1_bio = alloc_r1bio(mddev, bio);
>   	r1_bio->sectors = max_write_sectors;
>   
> @@ -1388,7 +1389,6 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>   	 */
>   
>   	disks = conf->raid_disks * 2;
> - retry_write:
>   	blocked_rdev = NULL;
>   	rcu_read_lock();
>   	max_sectors = r1_bio->sectors;
> @@ -1468,7 +1468,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>   		for (j = 0; j < i; j++)
>   			if (r1_bio->bios[j])
>   				rdev_dec_pending(conf->mirrors[j].rdev, mddev);
> -		r1_bio->state = 0;
> +		free_r1bio(r1_bio);
>   		allow_barrier(conf, bio->bi_iter.bi_sector);
>   
>   		if (bio->bi_opf & REQ_NOWAIT) {
> 

[PATCH v2 3/3] md/raid1: keep holding the barrier in handle_read_error()

[Xueshi Hu]

From: Xueshi Hu <xueshi.hu@smartx.com>

Raid reshape changes the r1conf::raid_disks and mempool. Keep holding the
barrier in handle_read_error() to avoid raid reshape.

Signed-off-by: Xueshi Hu <xueshi.hu@smartx.com>
---
 drivers/md/raid1.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 6c54786067b4..43a9c095f94d 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1240,20 +1240,20 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
 		rcu_read_unlock();
 	}
 
-	/*
-	 * Still need barrier for READ in case that whole
-	 * array is frozen.
-	 */
-	if (!wait_read_barrier(conf, bio->bi_iter.bi_sector,
-				bio->bi_opf & REQ_NOWAIT)) {
-		bio_wouldblock_error(bio);
-		return;
-	}
-
-	if (!r1_bio)
+	if (!r1_bio) {
+		/*
+		 * Still need barrier for READ in case that whole
+		 * array is frozen.
+		 */
+		if (!wait_read_barrier(conf, bio->bi_iter.bi_sector,
+					bio->bi_opf & REQ_NOWAIT)) {
+			bio_wouldblock_error(bio);
+			return;
+		}
 		r1_bio = alloc_r1bio(mddev, bio);
-	else
+	} else
 		init_r1bio(r1_bio, mddev, bio);
+
 	r1_bio->sectors = max_read_sectors;
 
 	/*
@@ -2527,7 +2527,6 @@ static void handle_read_error(struct r1conf *conf, struct r1bio *r1_bio)
 	}
 
 	rdev_dec_pending(rdev, conf->mddev);
-	allow_barrier(conf, r1_bio->sector);
 	bio = r1_bio->master_bio;
 
 	/* Reuse the old r1_bio so that the IO_BLOCKED settings are preserved */
-- 
2.40.1

Re: [PATCH v2 3/3] md/raid1: keep holding the barrier in handle_read_error()

[Yu Kuai]

Hi,

在 2023/07/09 18:29, Xueshi Hu 写道:
> From: Xueshi Hu <xueshi.hu@smartx.com>
> 
> Raid reshape changes the r1conf::raid_disks and mempool. Keep holding the
> barrier in handle_read_error() to avoid raid reshape.
> 
> Signed-off-by: Xueshi Hu <xueshi.hu@smartx.com>
> ---
>   drivers/md/raid1.c | 25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 6c54786067b4..43a9c095f94d 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1240,20 +1240,20 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
>   		rcu_read_unlock();
>   	}
>   
> -	/*
> -	 * Still need barrier for READ in case that whole
> -	 * array is frozen.
> -	 */
> -	if (!wait_read_barrier(conf, bio->bi_iter.bi_sector,
> -				bio->bi_opf & REQ_NOWAIT)) {
> -		bio_wouldblock_error(bio);
> -		return;
> -	}
> -
> -	if (!r1_bio)
> +	if (!r1_bio) {
> +		/*
> +		 * Still need barrier for READ in case that whole
> +		 * array is frozen.
> +		 */
> +		if (!wait_read_barrier(conf, bio->bi_iter.bi_sector,
> +					bio->bi_opf & REQ_NOWAIT)) {
> +			bio_wouldblock_error(bio);
> +			return;
> +		}
>   		r1_bio = alloc_r1bio(mddev, bio);
> -	else
> +	} else
>   		init_r1bio(r1_bio, mddev, bio);
> +
>   	r1_bio->sectors = max_read_sectors;
>   
>   	/*
> @@ -2527,7 +2527,6 @@ static void handle_read_error(struct r1conf *conf, struct r1bio *r1_bio)
>   	}
>   
>   	rdev_dec_pending(rdev, conf->mddev);
> -	allow_barrier(conf, r1_bio->sector);

I think it's better to just move allow_barrier() to after
raid1_read_request().

Thanks,
Kuai
>   	bio = r1_bio->master_bio;
>   
>   	/* Reuse the old r1_bio so that the IO_BLOCKED settings are preserved */
>