[PATCH -next 0/2] md: fix potential hang for mddev_suspend()

[PATCH -next 0/2] md: fix potential hang for mddev_suspend()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Yu Kuai (2):
  md: factor out helpers to grab and put 'active_io'
  md: fix potential hang for mddev_suspend()

 drivers/md/md.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

-- 
2.39.2

[PATCH -next 1/2] md: factor out helpers to grab and put 'active_io'

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

There are no functional changes, prepare to fix a problem that 'sb_wait'
is not woke up while 'active_io' is decreased to 0.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/md.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 0fe7ab6e8ab9..0d69b1a2e2d5 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -368,16 +368,18 @@ static bool is_suspended(struct mddev *mddev, struct bio *bio)
 	return true;
 }
 
-void md_handle_request(struct mddev *mddev, struct bio *bio)
+static bool md_array_enter(struct mddev *mddev, struct bio *bio)
 {
 check_suspended:
 	if (is_suspended(mddev, bio)) {
 		DEFINE_WAIT(__wait);
+
 		/* Bail out if REQ_NOWAIT is set for the bio */
 		if (bio->bi_opf & REQ_NOWAIT) {
 			bio_wouldblock_error(bio);
-			return;
+			return false;
 		}
+
 		for (;;) {
 			prepare_to_wait(&mddev->sb_wait, &__wait,
 					TASK_UNINTERRUPTIBLE);
@@ -387,15 +389,34 @@ void md_handle_request(struct mddev *mddev, struct bio *bio)
 		}
 		finish_wait(&mddev->sb_wait, &__wait);
 	}
+
 	if (!percpu_ref_tryget_live(&mddev->active_io))
 		goto check_suspended;
 
+	return true;
+}
+
+static void md_array_exit(struct mddev *mddev)
+{
+	percpu_ref_put(&mddev->active_io);
+}
+
+void md_handle_request(struct mddev *mddev, struct bio *bio)
+{
+retry:
+	if (!md_array_enter(mddev, bio))
+		return;
+
 	if (!mddev->pers->make_request(mddev, bio)) {
-		percpu_ref_put(&mddev->active_io);
-		goto check_suspended;
+		md_array_exit(mddev);
+		goto retry;
 	}
 
-	percpu_ref_put(&mddev->active_io);
+	/*
+	 * pers->make_request() will grab additional reference until bio is
+	 * done.
+	 */
+	md_array_exit(mddev);
 }
 EXPORT_SYMBOL(md_handle_request);
 
@@ -8667,7 +8688,7 @@ static void md_end_clone_io(struct bio *bio)
 
 	bio_put(bio);
 	bio_endio(orig_bio);
-	percpu_ref_put(&mddev->active_io);
+	md_array_exit(mddev);
 }
 
 static void md_clone_bio(struct mddev *mddev, struct bio **bio)
-- 
2.39.2

[PATCH -next 2/2] md: fix potential hang for mddev_suspend()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Commit 72adae23a72c ("md: Change active_io to percpu") drop that if
'active_io' is decreased to 0 and array is suspended, 'sb_wait' will be
woke up. This is wrong, however, there is no regression reported and I
think this is probably because 'sb_wait' is used in many scenarios and
it's woke up from other context.

Anyway, fix this potential problem by waking up 'sb_wait' in this case.

Fixes: 72adae23a72c ("md: Change active_io to percpu")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/md.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 0d69b1a2e2d5..1c7eb3cfadb4 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -399,6 +399,9 @@ static bool md_array_enter(struct mddev *mddev, struct bio *bio)
 static void md_array_exit(struct mddev *mddev)
 {
 	percpu_ref_put(&mddev->active_io);
+	if (percpu_ref_is_zero(&mddev->active_io) &&
+	    wq_has_sleeper(&mddev->sb_wait))
+		wake_up(&mddev->sb_wait);
 }
 
 void md_handle_request(struct mddev *mddev, struct bio *bio)
-- 
2.39.2

Re: [PATCH -next 0/2] md: fix potential hang for mddev_suspend()

[Song Liu]

On Wed, Aug 30, 2023 at 2:33 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> Yu Kuai (2):
>   md: factor out helpers to grab and put 'active_io'
>   md: fix potential hang for mddev_suspend()

Applied to md-next. Thanks!

Song

>
>  drivers/md/md.c | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
>
> --
> 2.39.2
>

Re: [PATCH -next 0/2] md: fix potential hang for mddev_suspend()

[Xiao Ni]

On Sat, Sep 23, 2023 at 5:33 AM Song Liu <song@kernel.org> wrote:
>
> On Wed, Aug 30, 2023 at 2:33 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
> >
> > From: Yu Kuai <yukuai3@huawei.com>
> >
> > Yu Kuai (2):
> >   md: factor out helpers to grab and put 'active_io'
> >   md: fix potential hang for mddev_suspend()
>
> Applied to md-next. Thanks!
>
> Song
>
> >
> >  drivers/md/md.c | 36 ++++++++++++++++++++++++++++++------
> >  1 file changed, 30 insertions(+), 6 deletions(-)
> >
> > --
> > 2.39.2
> >
>

Hi all

For the second patch, active_io_release does this job. So it doesn't
need to do this in md_array_exit again.

Best Regards
Xiao

Re: [PATCH -next 0/2] md: fix potential hang for mddev_suspend()

[Yu Kuai]

Hi,

在 2023/09/25 15:58, Xiao Ni 写道:
> On Sat, Sep 23, 2023 at 5:33 AM Song Liu <song@kernel.org> wrote:
>>
>> On Wed, Aug 30, 2023 at 2:33 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>>
>>> From: Yu Kuai <yukuai3@huawei.com>
>>>
>>> Yu Kuai (2):
>>>    md: factor out helpers to grab and put 'active_io'
>>>    md: fix potential hang for mddev_suspend()
>>
>> Applied to md-next. Thanks!
>>
>> Song
>>
>>>
>>>   drivers/md/md.c | 36 ++++++++++++++++++++++++++++++------
>>>   1 file changed, 30 insertions(+), 6 deletions(-)
>>>
>>> --
>>> 2.39.2
>>>
>>
> 
> Hi all
> 
> For the second patch, active_io_release does this job. So it doesn't
> need to do this in md_array_exit again.

Yes, this is correct, I missed this while reviewing related code.

Song, can you revert this patchset for now? Sorry for the trouble.

Thanks,
Kuai

> 
> Best Regards
> Xiao
> 
> .
>