[PATCH v3 0/5] md/raid10: reduce lock contention for io

[PATCH v3 0/5] md/raid10: reduce lock contention for io

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Changes in v3:
 - split a patch from patch 1
 - only modify hot path in patch 3
 - wake up barrier if 'nr_pending' is decreased to 0 in
 wait_barrier_nolock(), otherwise raise_barrier() might hung.

Changes in v2:
 - add patch 1, as suggested by Logan Gunthorpe.
 - in patch 4, instead of use spin_lock/unlock in wait_event, which will
 confuse lockdep, use write_seqlock/unlock instead.
 - in patch 4, use read_seqbegin() to get seqcount instead of unusual
 usage of raw_read_seqcount().
 - test result is different from v1 in aarch64 due to retest from different
 environment.

This patchset tries to avoid that two locks are held unconditionally
in hot path.

Test environment:

Architecture:
aarch64 Huawei KUNPENG 920
x86 Intel(R) Xeon(R) Platinum 8380

Raid10 initialize:
mdadm --create /dev/md0 --level 10 --bitmap none --raid-devices 4 /dev/nvme0n1 /dev/nvme1n1 /dev/nvme2n1 /dev/nvme3n1

Test cmd:
(task set -c 0-15) fio -name=0 -ioengine=libaio -direct=1 -group_reporting=1 -randseed=2022 -rwmixread=70 -refill_buffers -filename=/dev/md0 -numjobs=16 -runtime=60s -bs=4k -iodepth=256 -rw=randread

Test result:

aarch64:
before this patchset:		3.2 GiB/s
bind node before this patchset: 6.9 Gib/s
after this patchset:		7.9 Gib/s
bind node after this patchset:	8.0 Gib/s

x86:(bind node is not tested yet)
before this patchset: 7.0 GiB/s
after this patchset : 9.3 GiB/s

Please noted that in the test machine, memory access latency is very bad
across nodes compare to local node in aarch64, which is why bandwidth
while bind node is much better.

Yu Kuai (5):
  md/raid10: Factor out code from wait_barrier() to
    stop_waiting_barrier()
  md/raid10: don't modify 'nr_waitng' in wait_barrier() for the case
    nowait
  md/raid10: prevent unnecessary calls to wake_up() in fast path
  md/raid10: fix improper BUG_ON() in raise_barrier()
  md/raid10: convert resync_lock to use seqlock

 drivers/md/raid10.c | 149 ++++++++++++++++++++++++++++----------------
 drivers/md/raid10.h |   2 +-
 2 files changed, 96 insertions(+), 55 deletions(-)

-- 
2.31.1

[PATCH v3 1/5] md/raid10: factor out code from wait_barrier() to stop_waiting_barrier()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Currently the nasty condition in wait_barrier() is hard to read. This
patch factors out the condition into a function.

There are no functional changes.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 drivers/md/raid10.c | 50 +++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 64d6e4cd8a3a..37fd9284054e 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -957,41 +957,47 @@ static void lower_barrier(struct r10conf *conf)
 	wake_up(&conf->wait_barrier);
 }
 
+static bool stop_waiting_barrier(struct r10conf *conf)
+{
+	struct bio_list *bio_list = current->bio_list;
+
+	/* barrier is dropped */
+	if (!conf->barrier)
+		return true;
+
+	/*
+	 * If there are already pending requests (preventing the barrier from
+	 * rising completely), and the pre-process bio queue isn't empty, then
+	 * don't wait, as we need to empty that queue to get the nr_pending
+	 * count down.
+	 */
+	if (atomic_read(&conf->nr_pending) && bio_list &&
+	    (!bio_list_empty(&bio_list[0]) || !bio_list_empty(&bio_list[1])))
+		return true;
+
+	/* move on if recovery thread is blocked by us */
+	if (conf->mddev->thread->tsk == current &&
+	    test_bit(MD_RECOVERY_RUNNING, &conf->mddev->recovery) &&
+	    conf->nr_queued > 0)
+		return true;
+
+	return false;
+}
+
 static bool wait_barrier(struct r10conf *conf, bool nowait)
 {
 	bool ret = true;
 
 	spin_lock_irq(&conf->resync_lock);
 	if (conf->barrier) {
-		struct bio_list *bio_list = current->bio_list;
 		conf->nr_waiting++;
-		/* Wait for the barrier to drop.
-		 * However if there are already pending
-		 * requests (preventing the barrier from
-		 * rising completely), and the
-		 * pre-process bio queue isn't empty,
-		 * then don't wait, as we need to empty
-		 * that queue to get the nr_pending
-		 * count down.
-		 */
 		/* Return false when nowait flag is set */
 		if (nowait) {
 			ret = false;
 		} else {
 			raid10_log(conf->mddev, "wait barrier");
 			wait_event_lock_irq(conf->wait_barrier,
-					    !conf->barrier ||
-					    (atomic_read(&conf->nr_pending) &&
-					     bio_list &&
-					     (!bio_list_empty(&bio_list[0]) ||
-					      !bio_list_empty(&bio_list[1]))) ||
-					     /* move on if recovery thread is
-					      * blocked by us
-					      */
-					     (conf->mddev->thread->tsk == current &&
-					      test_bit(MD_RECOVERY_RUNNING,
-						       &conf->mddev->recovery) &&
-					      conf->nr_queued > 0),
+					    stop_waiting_barrier(conf),
 					    conf->resync_lock);
 		}
 		conf->nr_waiting--;
-- 
2.31.1

[PATCH v3 2/5] md/raid10: don't modify 'nr_waitng' in wait_barrier() for the case nowait

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

For the case nowait in wait_barrier(), there is no point to increase
nr_waiting and then decrease it.

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

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 37fd9284054e..df435d693637 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -990,17 +990,17 @@ static bool wait_barrier(struct r10conf *conf, bool nowait)
 
 	spin_lock_irq(&conf->resync_lock);
 	if (conf->barrier) {
-		conf->nr_waiting++;
 		/* Return false when nowait flag is set */
 		if (nowait) {
 			ret = false;
 		} else {
+			conf->nr_waiting++;
 			raid10_log(conf->mddev, "wait barrier");
 			wait_event_lock_irq(conf->wait_barrier,
 					    stop_waiting_barrier(conf),
 					    conf->resync_lock);
+			conf->nr_waiting--;
 		}
-		conf->nr_waiting--;
 		if (!conf->nr_waiting)
 			wake_up(&conf->wait_barrier);
 	}
-- 
2.31.1

[PATCH v3 3/5] md/raid10: prevent unnecessary calls to wake_up() in fast path

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Currently, wake_up() is called unconditionally in fast path such as
raid10_make_request(), which will cause lock contention under high
concurrency:

raid10_make_request
 wake_up
  __wake_up_common_lock
   spin_lock_irqsave

Improve performance by only call wake_up() if waitqueue is not empty
in allow_barrier() and raid10_make_request().

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

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index df435d693637..cf452c25e1e5 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -274,6 +274,12 @@ static void put_buf(struct r10bio *r10_bio)
 	lower_barrier(conf);
 }
 
+static void wake_up_barrier(struct r10conf *conf)
+{
+	if (wq_has_sleeper(&conf->wait_barrier))
+		wake_up(&conf->wait_barrier);
+}
+
 static void reschedule_retry(struct r10bio *r10_bio)
 {
 	unsigned long flags;
@@ -1015,7 +1021,7 @@ static void allow_barrier(struct r10conf *conf)
 {
 	if ((atomic_dec_and_test(&conf->nr_pending)) ||
 			(conf->array_freeze_pending))
-		wake_up(&conf->wait_barrier);
+		wake_up_barrier(conf);
 }
 
 static void freeze_array(struct r10conf *conf, int extra)
@@ -1891,7 +1897,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
 	__make_request(mddev, bio, sectors);
 
 	/* In case raid10d snuck in to freeze_array */
-	wake_up(&conf->wait_barrier);
+	wake_up_barrier(conf);
 	return true;
 }
 
-- 
2.31.1

[PATCH v3 4/5] md/raid10: fix improper BUG_ON() in raise_barrier()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

'conf->barrier' is protected by 'conf->resync_lock', reading
'conf->barrier' without holding the lock is wrong.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/raid10.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index cf452c25e1e5..9a28abd19709 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -936,8 +936,8 @@ static void flush_pending_writes(struct r10conf *conf)
 
 static void raise_barrier(struct r10conf *conf, int force)
 {
-	BUG_ON(force && !conf->barrier);
 	spin_lock_irq(&conf->resync_lock);
+	BUG_ON(force && !conf->barrier);
 
 	/* Wait until no block IO is waiting (unless 'force') */
 	wait_event_lock_irq(conf->wait_barrier, force || !conf->nr_waiting,
-- 
2.31.1

[PATCH v3 5/5] md/raid10: convert resync_lock to use seqlock

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Currently, wait_barrier() will hold 'resync_lock' to read 'conf->barrier',
and io can't be dispatched until 'barrier' is dropped.

Since holding the 'barrier' is not common, convert 'resync_lock' to use
seqlock so that holding lock can be avoided in fast path.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/raid10.c | 87 ++++++++++++++++++++++++++++++---------------
 drivers/md/raid10.h |  2 +-
 2 files changed, 59 insertions(+), 30 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 9a28abd19709..2daa7d57034c 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -79,6 +79,21 @@ static void end_reshape(struct r10conf *conf);
 
 #include "raid1-10.c"
 
+#define NULL_CMD
+#define cmd_before(conf, cmd) \
+	do { \
+		write_sequnlock_irq(&(conf)->resync_lock); \
+		cmd; \
+	} while (0)
+#define cmd_after(conf) write_seqlock_irq(&(conf)->resync_lock)
+
+#define wait_event_barrier_cmd(conf, cond, cmd) \
+	wait_event_cmd((conf)->wait_barrier, cond, cmd_before(conf, cmd), \
+		       cmd_after(conf))
+
+#define wait_event_barrier(conf, cond) \
+	wait_event_barrier_cmd(conf, cond, NULL_CMD)
+
 /*
  * for resync bio, r10bio pointer can be retrieved from the per-bio
  * 'struct resync_pages'.
@@ -936,30 +951,29 @@ static void flush_pending_writes(struct r10conf *conf)
 
 static void raise_barrier(struct r10conf *conf, int force)
 {
-	spin_lock_irq(&conf->resync_lock);
+	write_seqlock_irq(&conf->resync_lock);
 	BUG_ON(force && !conf->barrier);
 
 	/* Wait until no block IO is waiting (unless 'force') */
-	wait_event_lock_irq(conf->wait_barrier, force || !conf->nr_waiting,
-			    conf->resync_lock);
+	wait_event_barrier(conf, force || !conf->nr_waiting);
 
 	/* block any new IO from starting */
-	conf->barrier++;
+	WRITE_ONCE(conf->barrier, conf->barrier + 1);
 
 	/* Now wait for all pending IO to complete */
-	wait_event_lock_irq(conf->wait_barrier,
-			    !atomic_read(&conf->nr_pending) && conf->barrier < RESYNC_DEPTH,
-			    conf->resync_lock);
+	wait_event_barrier(conf, !atomic_read(&conf->nr_pending) &&
+				 conf->barrier < RESYNC_DEPTH);
 
-	spin_unlock_irq(&conf->resync_lock);
+	write_sequnlock_irq(&conf->resync_lock);
 }
 
 static void lower_barrier(struct r10conf *conf)
 {
 	unsigned long flags;
-	spin_lock_irqsave(&conf->resync_lock, flags);
-	conf->barrier--;
-	spin_unlock_irqrestore(&conf->resync_lock, flags);
+
+	write_seqlock_irqsave(&conf->resync_lock, flags);
+	WRITE_ONCE(conf->barrier, conf->barrier - 1);
+	write_sequnlock_irqrestore(&conf->resync_lock, flags);
 	wake_up(&conf->wait_barrier);
 }
 
@@ -990,11 +1004,31 @@ static bool stop_waiting_barrier(struct r10conf *conf)
 	return false;
 }
 
+static bool wait_barrier_nolock(struct r10conf *conf)
+{
+	unsigned int seq = read_seqbegin(&conf->resync_lock);
+
+	if (READ_ONCE(conf->barrier))
+		return false;
+
+	atomic_inc(&conf->nr_pending);
+	if (!read_seqretry(&conf->resync_lock, seq))
+		return true;
+
+	if (atomic_dec_and_test(&conf->nr_pending))
+		wake_up_barrier(conf);
+
+	return false;
+}
+
 static bool wait_barrier(struct r10conf *conf, bool nowait)
 {
 	bool ret = true;
 
-	spin_lock_irq(&conf->resync_lock);
+	if (wait_barrier_nolock(conf))
+		return true;
+
+	write_seqlock_irq(&conf->resync_lock);
 	if (conf->barrier) {
 		/* Return false when nowait flag is set */
 		if (nowait) {
@@ -1002,9 +1036,7 @@ static bool wait_barrier(struct r10conf *conf, bool nowait)
 		} else {
 			conf->nr_waiting++;
 			raid10_log(conf->mddev, "wait barrier");
-			wait_event_lock_irq(conf->wait_barrier,
-					    stop_waiting_barrier(conf),
-					    conf->resync_lock);
+			wait_event_barrier(conf, stop_waiting_barrier(conf));
 			conf->nr_waiting--;
 		}
 		if (!conf->nr_waiting)
@@ -1013,7 +1045,7 @@ static bool wait_barrier(struct r10conf *conf, bool nowait)
 	/* Only increment nr_pending when we wait */
 	if (ret)
 		atomic_inc(&conf->nr_pending);
-	spin_unlock_irq(&conf->resync_lock);
+	write_sequnlock_irq(&conf->resync_lock);
 	return ret;
 }
 
@@ -1038,27 +1070,24 @@ static void freeze_array(struct r10conf *conf, int extra)
 	 * must match the number of pending IOs (nr_pending) before
 	 * we continue.
 	 */
-	spin_lock_irq(&conf->resync_lock);
+	write_seqlock_irq(&conf->resync_lock);
 	conf->array_freeze_pending++;
-	conf->barrier++;
+	WRITE_ONCE(conf->barrier, conf->barrier + 1);
 	conf->nr_waiting++;
-	wait_event_lock_irq_cmd(conf->wait_barrier,
-				atomic_read(&conf->nr_pending) == conf->nr_queued+extra,
-				conf->resync_lock,
-				flush_pending_writes(conf));
-
+	wait_event_barrier_cmd(conf, atomic_read(&conf->nr_pending) ==
+			conf->nr_queued + extra, flush_pending_writes(conf));
 	conf->array_freeze_pending--;
-	spin_unlock_irq(&conf->resync_lock);
+	write_sequnlock_irq(&conf->resync_lock);
 }
 
 static void unfreeze_array(struct r10conf *conf)
 {
 	/* reverse the effect of the freeze */
-	spin_lock_irq(&conf->resync_lock);
-	conf->barrier--;
+	write_seqlock_irq(&conf->resync_lock);
+	WRITE_ONCE(conf->barrier, conf->barrier - 1);
 	conf->nr_waiting--;
 	wake_up(&conf->wait_barrier);
-	spin_unlock_irq(&conf->resync_lock);
+	write_sequnlock_irq(&conf->resync_lock);
 }
 
 static sector_t choose_data_offset(struct r10bio *r10_bio,
@@ -4044,7 +4073,7 @@ static struct r10conf *setup_conf(struct mddev *mddev)
 	INIT_LIST_HEAD(&conf->retry_list);
 	INIT_LIST_HEAD(&conf->bio_end_io_list);
 
-	spin_lock_init(&conf->resync_lock);
+	seqlock_init(&conf->resync_lock);
 	init_waitqueue_head(&conf->wait_barrier);
 	atomic_set(&conf->nr_pending, 0);
 
@@ -4363,7 +4392,7 @@ static void *raid10_takeover_raid0(struct mddev *mddev, sector_t size, int devs)
 				rdev->new_raid_disk = rdev->raid_disk * 2;
 				rdev->sectors = size;
 			}
-		conf->barrier = 1;
+		WRITE_ONCE(conf->barrier, 1);
 	}
 
 	return conf;
diff --git a/drivers/md/raid10.h b/drivers/md/raid10.h
index 5c0804d8bb1f..8c072ce0bc54 100644
--- a/drivers/md/raid10.h
+++ b/drivers/md/raid10.h
@@ -76,7 +76,7 @@ struct r10conf {
 	/* queue pending writes and submit them on unplug */
 	struct bio_list		pending_bio_list;
 
-	spinlock_t		resync_lock;
+	seqlock_t		resync_lock;
 	atomic_t		nr_pending;
 	int			nr_waiting;
 	int			nr_queued;
-- 
2.31.1

Re: [PATCH v3 1/5] md/raid10: factor out code from wait_barrier() to stop_waiting_barrier()

[Logan Gunthorpe]

On 2022-09-16 05:34, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Currently the nasty condition in wait_barrier() is hard to read. This
> patch factors out the condition into a function.
> 
> There are no functional changes.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

Re: [PATCH v3 2/5] md/raid10: don't modify 'nr_waitng' in wait_barrier() for the case nowait

[Logan Gunthorpe]

On 2022-09-16 05:34, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> For the case nowait in wait_barrier(), there is no point to increase
> nr_waiting and then decrease it.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

Re: [PATCH v3 3/5] md/raid10: prevent unnecessary calls to wake_up() in fast path

[Logan Gunthorpe]

On 2022-09-16 05:34, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Currently, wake_up() is called unconditionally in fast path such as
> raid10_make_request(), which will cause lock contention under high
> concurrency:
> 
> raid10_make_request
>  wake_up
>   __wake_up_common_lock
>    spin_lock_irqsave
> 
> Improve performance by only call wake_up() if waitqueue is not empty
> in allow_barrier() and raid10_make_request().
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

Re: [PATCH v3 4/5] md/raid10: fix improper BUG_ON() in raise_barrier()

[Logan Gunthorpe]

On 2022-09-16 05:34, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> 'conf->barrier' is protected by 'conf->resync_lock', reading
> 'conf->barrier' without holding the lock is wrong.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

Re: [PATCH v3 5/5] md/raid10: convert resync_lock to use seqlock

[Logan Gunthorpe]

On 2022-09-16 05:34, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Currently, wait_barrier() will hold 'resync_lock' to read 'conf->barrier',
> and io can't be dispatched until 'barrier' is dropped.
> 
> Since holding the 'barrier' is not common, convert 'resync_lock' to use
> seqlock so that holding lock can be avoided in fast path.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Reviewed-and-Tested-by: Logan Gunthorpe <logang@deltatee.com>

So far, I've run this series 3 times through the mdadm test suite and
haven't detected any issues.

Thanks,

Logan

Re: [PATCH v3 5/5] md/raid10: convert resync_lock to use seqlock

[Guoqing Jiang]

On 9/16/22 7:34 PM, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
>
> Currently, wait_barrier() will hold 'resync_lock' to read 'conf->barrier',
> and io can't be dispatched until 'barrier' is dropped.
>
> Since holding the 'barrier' is not common, convert 'resync_lock' to use
> seqlock so that holding lock can be avoided in fast path.
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>   drivers/md/raid10.c | 87 ++++++++++++++++++++++++++++++---------------
>   drivers/md/raid10.h |  2 +-
>   2 files changed, 59 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 9a28abd19709..2daa7d57034c 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -79,6 +79,21 @@ static void end_reshape(struct r10conf *conf);
>   
>   #include "raid1-10.c"
>   
> +#define NULL_CMD
> +#define cmd_before(conf, cmd) \
> +	do { \
> +		write_sequnlock_irq(&(conf)->resync_lock); \
> +		cmd; \
> +	} while (0)
> +#define cmd_after(conf) write_seqlock_irq(&(conf)->resync_lock)

The two is not paired well given only cmd_before needs the 'cmd'.

> +
> +#define wait_event_barrier_cmd(conf, cond, cmd) \
> +	wait_event_cmd((conf)->wait_barrier, cond, cmd_before(conf, cmd), \
> +		       cmd_after(conf))
> +
> +#define wait_event_barrier(conf, cond) \
> +	wait_event_barrier_cmd(conf, cond, NULL_CMD)

What is the issue without define NULL_CMD?

Thanks,
Guoqing

Re: [PATCH v3 4/5] md/raid10: fix improper BUG_ON() in raise_barrier()

[Guoqing Jiang]

On 9/16/22 7:34 PM, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
>
> 'conf->barrier' is protected by 'conf->resync_lock', reading
> 'conf->barrier' without holding the lock is wrong.
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>   drivers/md/raid10.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index cf452c25e1e5..9a28abd19709 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -936,8 +936,8 @@ static void flush_pending_writes(struct r10conf *conf)
>   
>   static void raise_barrier(struct r10conf *conf, int force)
>   {
> -	BUG_ON(force && !conf->barrier);
>   	spin_lock_irq(&conf->resync_lock);
> +	BUG_ON(force && !conf->barrier);
>   
>   	/* Wait until no block IO is waiting (unless 'force') */
>   	wait_event_lock_irq(conf->wait_barrier, force || !conf->nr_waiting,

Acked-by: Guoqing Jiang <guoqing.jiang@linux.dev>

Thanks,
Guoqing

Re: [PATCH v3 3/5] md/raid10: prevent unnecessary calls to wake_up() in fast path

[Guoqing Jiang]

On 9/16/22 7:34 PM, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
>
> Currently, wake_up() is called unconditionally in fast path such as
> raid10_make_request(), which will cause lock contention under high
> concurrency:
>
> raid10_make_request
>   wake_up
>    __wake_up_common_lock
>     spin_lock_irqsave
>
> Improve performance by only call wake_up() if waitqueue is not empty
> in allow_barrier() and raid10_make_request().
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>   drivers/md/raid10.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index df435d693637..cf452c25e1e5 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -274,6 +274,12 @@ static void put_buf(struct r10bio *r10_bio)
>   	lower_barrier(conf);
>   }
>   
> +static void wake_up_barrier(struct r10conf *conf)
> +{
> +	if (wq_has_sleeper(&conf->wait_barrier))
> +		wake_up(&conf->wait_barrier);
> +}
> +
>   static void reschedule_retry(struct r10bio *r10_bio)
>   {
>   	unsigned long flags;
> @@ -1015,7 +1021,7 @@ static void allow_barrier(struct r10conf *conf)
>   {
>   	if ((atomic_dec_and_test(&conf->nr_pending)) ||
>   			(conf->array_freeze_pending))
> -		wake_up(&conf->wait_barrier);
> +		wake_up_barrier(conf);
>   }
>   
>   static void freeze_array(struct r10conf *conf, int extra)
> @@ -1891,7 +1897,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
>   	__make_request(mddev, bio, sectors);
>   
>   	/* In case raid10d snuck in to freeze_array */
> -	wake_up(&conf->wait_barrier);
> +	wake_up_barrier(conf);
>   	return true;
>   }
>   

Acked-by: Guoqing Jiang <guoqing.jiang@linux.dev>

Thanks,
Guoqing

Re: [PATCH v3 2/5] md/raid10: don't modify 'nr_waitng' in wait_barrier() for the case nowait

[Guoqing Jiang]

On 9/16/22 7:34 PM, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
>
> For the case nowait in wait_barrier(), there is no point to increase
> nr_waiting and then decrease it.
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>   drivers/md/raid10.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 37fd9284054e..df435d693637 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -990,17 +990,17 @@ static bool wait_barrier(struct r10conf *conf, bool nowait)
>   
>   	spin_lock_irq(&conf->resync_lock);
>   	if (conf->barrier) {
> -		conf->nr_waiting++;
>   		/* Return false when nowait flag is set */
>   		if (nowait) {
>   			ret = false;
>   		} else {
> +			conf->nr_waiting++;
>   			raid10_log(conf->mddev, "wait barrier");
>   			wait_event_lock_irq(conf->wait_barrier,
>   					    stop_waiting_barrier(conf),
>   					    conf->resync_lock);
> +			conf->nr_waiting--;
>   		}
> -		conf->nr_waiting--;
>   		if (!conf->nr_waiting)
>   			wake_up(&conf->wait_barrier);
>   	}

Acked-by: Guoqing Jiang <guoqing.jiang@linux.dev>

Thanks,
Guoqing

Re: [PATCH v3 1/5] md/raid10: factor out code from wait_barrier() to stop_waiting_barrier()

[Guoqing Jiang]

On 9/16/22 7:34 PM, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
>
> Currently the nasty condition in wait_barrier() is hard to read. This
> patch factors out the condition into a function.
>
> There are no functional changes.
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
>   drivers/md/raid10.c | 50 +++++++++++++++++++++++++--------------------
>   1 file changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 64d6e4cd8a3a..37fd9284054e 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -957,41 +957,47 @@ static void lower_barrier(struct r10conf *conf)
>   	wake_up(&conf->wait_barrier);
>   }
>   
> +static bool stop_waiting_barrier(struct r10conf *conf)
> +{
> +	struct bio_list *bio_list = current->bio_list;
> +
> +	/* barrier is dropped */
> +	if (!conf->barrier)
> +		return true;
> +
> +	/*
> +	 * If there are already pending requests (preventing the barrier from
> +	 * rising completely), and the pre-process bio queue isn't empty, then
> +	 * don't wait, as we need to empty that queue to get the nr_pending
> +	 * count down.
> +	 */
> +	if (atomic_read(&conf->nr_pending) && bio_list &&
> +	    (!bio_list_empty(&bio_list[0]) || !bio_list_empty(&bio_list[1])))
> +		return true;
> +
> +	/* move on if recovery thread is blocked by us */
> +	if (conf->mddev->thread->tsk == current &&
> +	    test_bit(MD_RECOVERY_RUNNING, &conf->mddev->recovery) &&
> +	    conf->nr_queued > 0)
> +		return true;
> +
> +	return false;
> +}
> +
>   static bool wait_barrier(struct r10conf *conf, bool nowait)
>   {
>   	bool ret = true;
>   
>   	spin_lock_irq(&conf->resync_lock);
>   	if (conf->barrier) {
> -		struct bio_list *bio_list = current->bio_list;
>   		conf->nr_waiting++;
> -		/* Wait for the barrier to drop.
> -		 * However if there are already pending
> -		 * requests (preventing the barrier from
> -		 * rising completely), and the
> -		 * pre-process bio queue isn't empty,
> -		 * then don't wait, as we need to empty
> -		 * that queue to get the nr_pending
> -		 * count down.
> -		 */
>   		/* Return false when nowait flag is set */
>   		if (nowait) {
>   			ret = false;
>   		} else {
>   			raid10_log(conf->mddev, "wait barrier");
>   			wait_event_lock_irq(conf->wait_barrier,
> -					    !conf->barrier ||
> -					    (atomic_read(&conf->nr_pending) &&
> -					     bio_list &&
> -					     (!bio_list_empty(&bio_list[0]) ||
> -					      !bio_list_empty(&bio_list[1]))) ||
> -					     /* move on if recovery thread is
> -					      * blocked by us
> -					      */
> -					     (conf->mddev->thread->tsk == current &&
> -					      test_bit(MD_RECOVERY_RUNNING,
> -						       &conf->mddev->recovery) &&
> -					      conf->nr_queued > 0),
> +					    stop_waiting_barrier(conf),
>   					    conf->resync_lock);
>   		}
>   		conf->nr_waiting--;

Acked-by: Guoqing Jiang <guoqing.jiang@linux.dev>

Thanks,
Guoqing

Re: [PATCH v3 5/5] md/raid10: convert resync_lock to use seqlock

[Yu Kuai]

Hi,

在 2022/09/18 19:36, Guoqing Jiang 写道:
> 
> 
> On 9/16/22 7:34 PM, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Currently, wait_barrier() will hold 'resync_lock' to read 
>> 'conf->barrier',
>> and io can't be dispatched until 'barrier' is dropped.
>>
>> Since holding the 'barrier' is not common, convert 'resync_lock' to use
>> seqlock so that holding lock can be avoided in fast path.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   drivers/md/raid10.c | 87 ++++++++++++++++++++++++++++++---------------
>>   drivers/md/raid10.h |  2 +-
>>   2 files changed, 59 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index 9a28abd19709..2daa7d57034c 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -79,6 +79,21 @@ static void end_reshape(struct r10conf *conf);
>>   #include "raid1-10.c"
>> +#define NULL_CMD
>> +#define cmd_before(conf, cmd) \
>> +    do { \
>> +        write_sequnlock_irq(&(conf)->resync_lock); \
>> +        cmd; \
>> +    } while (0)
>> +#define cmd_after(conf) write_seqlock_irq(&(conf)->resync_lock)
> 
> The two is not paired well given only cmd_before needs the 'cmd'.

May be should I just remove cmd_after?
> 
>> +
>> +#define wait_event_barrier_cmd(conf, cond, cmd) \
>> +    wait_event_cmd((conf)->wait_barrier, cond, cmd_before(conf, cmd), \
>> +               cmd_after(conf))
>> +
>> +#define wait_event_barrier(conf, cond) \
>> +    wait_event_barrier_cmd(conf, cond, NULL_CMD)
> 
> What is the issue without define NULL_CMD?
> 

Checkpatch will complain this:

ERROR: space prohibited before that close parenthesis ')'
#38: FILE: drivers/md/raid10.c:94:
+       wait_event_barrier_cmd(conf, cond, )

total: 1 errors, 0 warnings, 169 lines checked

Thanks,
Kuai
> Thanks,
> Guoqing
> .
> 

Re: [PATCH v3 5/5] md/raid10: convert resync_lock to use seqlock

[Guoqing Jiang]

On 9/19/22 9:08 AM, Yu Kuai wrote:
> Hi,
>
> 在 2022/09/18 19:36, Guoqing Jiang 写道:
>>
>>
>> On 9/16/22 7:34 PM, Yu Kuai wrote:
>>> From: Yu Kuai <yukuai3@huawei.com>
>>>
>>> Currently, wait_barrier() will hold 'resync_lock' to read 
>>> 'conf->barrier',
>>> and io can't be dispatched until 'barrier' is dropped.
>>>
>>> Since holding the 'barrier' is not common, convert 'resync_lock' to use
>>> seqlock so that holding lock can be avoided in fast path.
>>>
>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>>> ---
>>>   drivers/md/raid10.c | 87 
>>> ++++++++++++++++++++++++++++++---------------
>>>   drivers/md/raid10.h |  2 +-
>>>   2 files changed, 59 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>>> index 9a28abd19709..2daa7d57034c 100644
>>> --- a/drivers/md/raid10.c
>>> +++ b/drivers/md/raid10.c
>>> @@ -79,6 +79,21 @@ static void end_reshape(struct r10conf *conf);
>>>   #include "raid1-10.c"
>>> +#define NULL_CMD
>>> +#define cmd_before(conf, cmd) \
>>> +    do { \
>>> +        write_sequnlock_irq(&(conf)->resync_lock); \
>>> +        cmd; \
>>> +    } while (0)
>>> +#define cmd_after(conf) write_seqlock_irq(&(conf)->resync_lock)
>>
>> The two is not paired well given only cmd_before needs the 'cmd'.
>
> May be should I just remove cmd_after?

I'd remove it but just my personal flavor.

>>
>>> +
>>> +#define wait_event_barrier_cmd(conf, cond, cmd) \
>>> +    wait_event_cmd((conf)->wait_barrier, cond, cmd_before(conf, 
>>> cmd), \
>>> +               cmd_after(conf))
>>> +
>>> +#define wait_event_barrier(conf, cond) \
>>> +    wait_event_barrier_cmd(conf, cond, NULL_CMD)
>>
>> What is the issue without define NULL_CMD?
>>
>
> Checkpatch will complain this:
>
> ERROR: space prohibited before that close parenthesis ')'
> #38: FILE: drivers/md/raid10.c:94:
> +       wait_event_barrier_cmd(conf, cond, )
>
> total: 1 errors, 0 warnings, 169 lines checked

Hmm, freeze_array has a different usage for it, so two cmds before sleep
and one cmd after sleep, perhaps it is the best way for now.

Thanks,
Guoqing

Re: [PATCH v3 5/5] md/raid10: convert resync_lock to use seqlock

[Song Liu]

On Mon, Sep 19, 2022 at 3:28 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
>
>
> On 9/19/22 9:08 AM, Yu Kuai wrote:
> > Hi,
> >
> > 在 2022/09/18 19:36, Guoqing Jiang 写道:
> >>
> >>
> >> On 9/16/22 7:34 PM, Yu Kuai wrote:
> >>> From: Yu Kuai <yukuai3@huawei.com>
> >>>
> >>> Currently, wait_barrier() will hold 'resync_lock' to read
> >>> 'conf->barrier',
> >>> and io can't be dispatched until 'barrier' is dropped.
> >>>
> >>> Since holding the 'barrier' is not common, convert 'resync_lock' to use
> >>> seqlock so that holding lock can be avoided in fast path.
> >>>
> >>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> >>> ---
> >>>   drivers/md/raid10.c | 87
> >>> ++++++++++++++++++++++++++++++---------------
> >>>   drivers/md/raid10.h |  2 +-
> >>>   2 files changed, 59 insertions(+), 30 deletions(-)
> >>>
> >>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> >>> index 9a28abd19709..2daa7d57034c 100644
> >>> --- a/drivers/md/raid10.c
> >>> +++ b/drivers/md/raid10.c
> >>> @@ -79,6 +79,21 @@ static void end_reshape(struct r10conf *conf);
> >>>   #include "raid1-10.c"
> >>> +#define NULL_CMD
> >>> +#define cmd_before(conf, cmd) \
> >>> +    do { \
> >>> +        write_sequnlock_irq(&(conf)->resync_lock); \
> >>> +        cmd; \
> >>> +    } while (0)
> >>> +#define cmd_after(conf) write_seqlock_irq(&(conf)->resync_lock)
> >>
> >> The two is not paired well given only cmd_before needs the 'cmd'.
> >
> > May be should I just remove cmd_after?
>
> I'd remove it but just my personal flavor.
>
> >>
> >>> +
> >>> +#define wait_event_barrier_cmd(conf, cond, cmd) \
> >>> +    wait_event_cmd((conf)->wait_barrier, cond, cmd_before(conf,
> >>> cmd), \
> >>> +               cmd_after(conf))
> >>> +
> >>> +#define wait_event_barrier(conf, cond) \
> >>> +    wait_event_barrier_cmd(conf, cond, NULL_CMD)
> >>
> >> What is the issue without define NULL_CMD?
> >>
> >
> > Checkpatch will complain this:
> >
> > ERROR: space prohibited before that close parenthesis ')'
> > #38: FILE: drivers/md/raid10.c:94:
> > +       wait_event_barrier_cmd(conf, cond, )
> >
> > total: 1 errors, 0 warnings, 169 lines checked
>
> Hmm, freeze_array has a different usage for it, so two cmds before sleep
> and one cmd after sleep, perhaps it is the best way for now.

Current version looks good to me. Please let me know if there is more
concern.

Applied the set to md-next. Thanks!

Song