[RFC PATCH] MD: fix lock contention for flush bios

[RFC PATCH] MD: fix lock contention for flush bios

[Xiao Ni]

There is a lock contention when there are many processes which send flush bios
to md device. eg. Create many lvs on one raid device and mkfs.xfs on each lv.

Now it just can handle flush request sequentially. It needs to wait mddev->flush_bio
to be NULL, otherwise get mddev->lock.

This patch remove mddev->flush_bio and allocat one bio for each flush bio.
I did a test with command dbench -s 128 -t 4800. This is the test result:

=================Before the patch============================

 Operation      Count    AvgLat    MaxLat
 ----------------------------------------
 NTCreateX     124271     5.128 12752.344
 Close          96922     0.645  6250.826
 Rename          5141     1.556  6747.033
 Unlink         20283     5.045 12752.294
 Qpathinfo     111872     0.012   250.069
 Qfileinfo      25684     0.004     0.293
 Qfsinfo        18535     0.007     0.646
 Sfileinfo      11763     0.014     0.571
 Find           41018     0.026     0.869
 WriteX         88616  6750.094 80740.875
 ReadX         179250     0.018  1749.302
 LockX            328     0.008     0.042
 UnlockX          328     0.004     0.023
 Flush           9650  1688.288 38998.814

Throughput 1.01668 MB/sec (sync open)  128 clients  128 procs  max_latency=80740.883 ms

========================after patch============================
 128  cleanup 600 sec
   0  cleanup 600 sec

 Operation      Count    AvgLat    MaxLat
 ----------------------------------------
 NTCreateX      52444     1.403  1499.920
 Close          45740     0.035  1000.301
 Rename          2090     0.073     1.591
 Unlink          5032     5.855  1251.375
 Qpathinfo      44493     0.167   501.498
 Qfileinfo      16261     0.004     0.577
 Qfsinfo         5887     0.011     1.133
 Sfileinfo       6303     0.022     1.277
 Find           15321     0.034     1.041
 WriteX         62730  1197.684  3247.813
 ReadX          60379     0.013     2.408
 LockX             48     0.010     0.039
 UnlockX           48     0.004     0.006
 Flush           4370   382.070   999.735

Throughput 5.06007 MB/sec (sync open)  128 clients  128 procs  max_latency=3247.823 ms

The test was did on one raid10 with 4 SSDs.

Suggested-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Xiao Ni <xni@redhat.com>
---
 drivers/md/md.c | 105 +++++++++++++++++++++++++-------------------------------
 drivers/md/md.h |  14 ++++----
 2 files changed, 54 insertions(+), 65 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 4e4dee0..1e562f5 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -409,33 +409,61 @@ static int md_congested(void *data, int bits)
 	return mddev_congested(mddev, bits);
 }
 
-/*
- * Generic flush handling for md
- */
+static void submit_flushes(struct work_struct *ws)
+{
+	struct flush_info *fi = container_of(ws, struct flush_info, flush_work);
+	struct mddev *mddev = fi->mddev;
+	struct bio *bio = fi->bio;
+
+	bio->bi_opf &= ~REQ_PREFLUSH;
+	md_handle_request(mddev, bio);
 
-static void md_end_flush(struct bio *bio)
+	kfree(fi);
+}
+
+
+static void md_end_flush(struct bio *flush_bio)
 {
-	struct md_rdev *rdev = bio->bi_private;
-	struct mddev *mddev = rdev->mddev;
+	struct flush_info *fi = flush_bio->bi_private;
+	struct bio *bio = fi->bio;
+	struct mddev *mddev = fi->mddev;
+	struct md_rdev *rdev;
 
-	rdev_dec_pending(rdev, mddev);
+	rcu_read_lock();
+		rdev_for_each_rcu(rdev, mddev)
+			rdev_dec_pending(rdev, mddev);
+	rcu_read_unlock();
 
-	if (atomic_dec_and_test(&mddev->flush_pending)) {
-		/* The pre-request flush has finished */
-		queue_work(md_wq, &mddev->flush_work);
+	if (bio->bi_iter.bi_size == 0)
+		/* an empty barrier - all done */
+		bio_endio(bio);
+	else {
+		INIT_WORK(&fi->flush_work, submit_flushes);
+		queue_work(md_wq, &fi->flush_work);
 	}
-	bio_put(bio);
 }
 
-static void md_submit_flush_data(struct work_struct *ws);
-
-static void submit_flushes(struct work_struct *ws)
+void md_flush_request(struct mddev *mddev, struct bio *bio)
 {
-	struct mddev *mddev = container_of(ws, struct mddev, flush_work);
 	struct md_rdev *rdev;
+	struct flush_info *fi;
+	struct bio *f_bio;
+
+	fi = kmalloc(sizeof(*fi), GFP_NOIO);
+	if (fi == NULL) {
+		pr_err("md: %s failed to alloc memory for flush bio\n",
+		       mdname(mddev));
+		bio->bi_status = BLK_STS_IOERR;
+		bio_endio(bio);
+	}
+
+	fi->bio = bio;
+	fi->mddev = mddev;
+	f_bio = &fi->flush_bio;
+	bio_init(f_bio, NULL, 0);
+	f_bio->bi_private = fi;
+	f_bio->bi_end_io = md_end_flush;
 
-	INIT_WORK(&mddev->flush_work, md_submit_flush_data);
-	atomic_set(&mddev->flush_pending, 1);
 	rcu_read_lock();
 	rdev_for_each_rcu(rdev, mddev)
 		if (rdev->raid_disk >= 0 &&
@@ -449,54 +477,16 @@ static void submit_flushes(struct work_struct *ws)
 			atomic_inc(&rdev->nr_pending);
 			rcu_read_unlock();
 			bi = bio_alloc_mddev(GFP_NOIO, 0, mddev);
-			bi->bi_end_io = md_end_flush;
-			bi->bi_private = rdev;
 			bio_set_dev(bi, rdev->bdev);
 			bi->bi_opf = REQ_OP_WRITE | REQ_PREFLUSH;
-			atomic_inc(&mddev->flush_pending);
+			bio_chain(bi, f_bio);
 			submit_bio(bi);
 			rcu_read_lock();
 			rdev_dec_pending(rdev, mddev);
 		}
 	rcu_read_unlock();
-	if (atomic_dec_and_test(&mddev->flush_pending))
-		queue_work(md_wq, &mddev->flush_work);
-}
-
-static void md_submit_flush_data(struct work_struct *ws)
-{
-	struct mddev *mddev = container_of(ws, struct mddev, flush_work);
-	struct bio *bio = mddev->flush_bio;
-
-	/*
-	 * must reset flush_bio before calling into md_handle_request to avoid a
-	 * deadlock, because other bios passed md_handle_request suspend check
-	 * could wait for this and below md_handle_request could wait for those
-	 * bios because of suspend check
-	 */
-	mddev->flush_bio = NULL;
-	wake_up(&mddev->sb_wait);
-
-	if (bio->bi_iter.bi_size == 0)
-		/* an empty barrier - all done */
-		bio_endio(bio);
-	else {
-		bio->bi_opf &= ~REQ_PREFLUSH;
-		md_handle_request(mddev, bio);
-	}
-}
-
-void md_flush_request(struct mddev *mddev, struct bio *bio)
-{
-	spin_lock_irq(&mddev->lock);
-	wait_event_lock_irq(mddev->sb_wait,
-			    !mddev->flush_bio,
-			    mddev->lock);
-	mddev->flush_bio = bio;
-	spin_unlock_irq(&mddev->lock);
 
-	INIT_WORK(&mddev->flush_work, submit_flushes);
-	queue_work(md_wq, &mddev->flush_work);
+	bio_endio(f_bio);
 }
 EXPORT_SYMBOL(md_flush_request);
 
@@ -555,7 +545,6 @@ void mddev_init(struct mddev *mddev)
 	atomic_set(&mddev->openers, 0);
 	atomic_set(&mddev->active_io, 0);
 	spin_lock_init(&mddev->lock);
-	atomic_set(&mddev->flush_pending, 0);
 	init_waitqueue_head(&mddev->sb_wait);
 	init_waitqueue_head(&mddev->recovery_wait);
 	mddev->reshape_position = MaxSector;
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 7d6bcf0..16e7f03 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -252,6 +252,13 @@ enum mddev_sb_flags {
 	MD_SB_NEED_REWRITE,	/* metadata write needs to be repeated */
 };
 
+struct flush_info {
+	struct bio *bio;
+	struct bio flush_bio;
+	struct mddev *mddev;
+	struct work_struct flush_work;
+};
+
 struct mddev {
 	void				*private;
 	struct md_personality		*pers;
@@ -457,13 +464,6 @@ struct mddev {
 						   * metadata and bitmap writes
 						   */
 
-	/* Generic flush handling.
-	 * The last to finish preflush schedules a worker to submit
-	 * the rest of the request (without the REQ_PREFLUSH flag).
-	 */
-	struct bio *flush_bio;
-	atomic_t flush_pending;
-	struct work_struct flush_work;
 	struct work_struct event_work;	/* used by dm to report failure event */
 	void (*sync_super)(struct mddev *mddev, struct md_rdev *rdev);
 	struct md_cluster_info		*cluster_info;
-- 
2.7.4

Re: [RFC PATCH] MD: fix lock contention for flush bios

[Guoqing Jiang]

On 01/24/2018 10:43 AM, Xiao Ni wrote:
> There is a lock contention when there are many processes which send flush bios
> to md device. eg. Create many lvs on one raid device and mkfs.xfs on each lv.
>
> Now it just can handle flush request sequentially. It needs to wait mddev->flush_bio
> to be NULL, otherwise get mddev->lock.

With the new approach, can we still keep the synchronization across all 
devices?
I found the previous commit a2826aa92e2e ("md: support barrier requests 
on all
personalities") did want to keep synchronization.

[snip]

> Suggested-by: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>   drivers/md/md.c | 105 +++++++++++++++++++++++++-------------------------------
>   drivers/md/md.h |  14 ++++----
>   2 files changed, 54 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 4e4dee0..1e562f5 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -409,33 +409,61 @@ static int md_congested(void *data, int bits)
>   	return mddev_congested(mddev, bits);
>   }
>   
> -/*
> - * Generic flush handling for md
> - */
> +static void submit_flushes(struct work_struct *ws)
> +{
> +	struct flush_info *fi = container_of(ws, struct flush_info, flush_work);
> +	struct mddev *mddev = fi->mddev;
> +	struct bio *bio = fi->bio;
> +
> +	bio->bi_opf &= ~REQ_PREFLUSH;
> +	md_handle_request(mddev, bio);
>   
> -static void md_end_flush(struct bio *bio)
> +	kfree(fi);
> +}
> +
> +

An extra blank line above.

> +static void md_end_flush(struct bio *flush_bio)
>   {
> -	struct md_rdev *rdev = bio->bi_private;
> -	struct mddev *mddev = rdev->mddev;
> +	struct flush_info *fi = flush_bio->bi_private;
> +	struct bio *bio = fi->bio;
> +	struct mddev *mddev = fi->mddev;
> +	struct md_rdev *rdev;
>   
> -	rdev_dec_pending(rdev, mddev);
> +	rcu_read_lock();
> +		rdev_for_each_rcu(rdev, mddev)
> +			rdev_dec_pending(rdev, mddev);

Unnecessary indentation.

> +	rcu_read_unlock();
>   
> -	if (atomic_dec_and_test(&mddev->flush_pending)) {
> -		/* The pre-request flush has finished */
> -		queue_work(md_wq, &mddev->flush_work);
> +	if (bio->bi_iter.bi_size == 0)
> +		/* an empty barrier - all done */
> +		bio_endio(bio);
> +	else {
> +		INIT_WORK(&fi->flush_work, submit_flushes);
> +		queue_work(md_wq, &fi->flush_work);
>   	}
> -	bio_put(bio);
>   }
>   
> -static void md_submit_flush_data(struct work_struct *ws);
> -
> -static void submit_flushes(struct work_struct *ws)
> +void md_flush_request(struct mddev *mddev, struct bio *bio)
>   {
> -	struct mddev *mddev = container_of(ws, struct mddev, flush_work);
>   	struct md_rdev *rdev;
> +	struct flush_info *fi;
> +	struct bio *f_bio;
> +
> +	fi = kmalloc(sizeof(*fi), GFP_NOIO);
> +	if (fi == NULL) {
> +		pr_err("md: %s failed to alloc memory for flush bio\n",
> +		       mdname(mddev));
> +		bio->bi_status = BLK_STS_IOERR;
> +		bio_endio(bio);

Maybe you missed "return" here.

Thanks,
Guoqing

> +	}
> +
> +	fi->bio = bio;
> +	fi->mddev = mddev;
> +	f_bio = &fi->flush_bio;
> +	bio_init(f_bio, NULL, 0);
> +	f_bio->bi_private = fi;
> +	f_bio->bi_end_io = md_end_flush;
>   
> -	INIT_WORK(&mddev->flush_work, md_submit_flush_data);
> -	atomic_set(&mddev->flush_pending, 1);
>   	rcu_read_lock();
>   	rdev_for_each_rcu(rdev, mddev)
>   		if (rdev->raid_disk >= 0 &&
> @@ -449,54 +477,16 @@ static void submit_flushes(struct work_struct *ws)
>   			atomic_inc(&rdev->nr_pending);
>   			rcu_read_unlock();
>   			bi = bio_alloc_mddev(GFP_NOIO, 0, mddev);
> -			bi->bi_end_io = md_end_flush;
> -			bi->bi_private = rdev;
>   			bio_set_dev(bi, rdev->bdev);
>   			bi->bi_opf = REQ_OP_WRITE | REQ_PREFLUSH;
> -			atomic_inc(&mddev->flush_pending);
> +			bio_chain(bi, f_bio);
>   			submit_bio(bi);
>   			rcu_read_lock();
>   			rdev_dec_pending(rdev, mddev);
>   		}
>   	rcu_read_unlock();
> -	if (atomic_dec_and_test(&mddev->flush_pending))
> -		queue_work(md_wq, &mddev->flush_work);
> -}
> -
> -static void md_submit_flush_data(struct work_struct *ws)
> -{
> -	struct mddev *mddev = container_of(ws, struct mddev, flush_work);
> -	struct bio *bio = mddev->flush_bio;
> -
> -	/*
> -	 * must reset flush_bio before calling into md_handle_request to avoid a
> -	 * deadlock, because other bios passed md_handle_request suspend check
> -	 * could wait for this and below md_handle_request could wait for those
> -	 * bios because of suspend check
> -	 */
> -	mddev->flush_bio = NULL;
> -	wake_up(&mddev->sb_wait);
> -
> -	if (bio->bi_iter.bi_size == 0)
> -		/* an empty barrier - all done */
> -		bio_endio(bio);
> -	else {
> -		bio->bi_opf &= ~REQ_PREFLUSH;
> -		md_handle_request(mddev, bio);
> -	}
> -}
> -
> -void md_flush_request(struct mddev *mddev, struct bio *bio)
> -{
> -	spin_lock_irq(&mddev->lock);
> -	wait_event_lock_irq(mddev->sb_wait,
> -			    !mddev->flush_bio,
> -			    mddev->lock);
> -	mddev->flush_bio = bio;
> -	spin_unlock_irq(&mddev->lock);
>   
> -	INIT_WORK(&mddev->flush_work, submit_flushes);
> -	queue_work(md_wq, &mddev->flush_work);
> +	bio_endio(f_bio);
>   }
>   EXPORT_SYMBOL(md_flush_request);
>   
> @@ -555,7 +545,6 @@ void mddev_init(struct mddev *mddev)
>   	atomic_set(&mddev->openers, 0);
>   	atomic_set(&mddev->active_io, 0);
>   	spin_lock_init(&mddev->lock);
> -	atomic_set(&mddev->flush_pending, 0);
>   	init_waitqueue_head(&mddev->sb_wait);
>   	init_waitqueue_head(&mddev->recovery_wait);
>   	mddev->reshape_position = MaxSector;
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 7d6bcf0..16e7f03 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -252,6 +252,13 @@ enum mddev_sb_flags {
>   	MD_SB_NEED_REWRITE,	/* metadata write needs to be repeated */
>   };
>   
> +struct flush_info {
> +	struct bio *bio;
> +	struct bio flush_bio;
> +	struct mddev *mddev;
> +	struct work_struct flush_work;
> +};
> +
>   struct mddev {
>   	void				*private;
>   	struct md_personality		*pers;
> @@ -457,13 +464,6 @@ struct mddev {
>   						   * metadata and bitmap writes
>   						   */
>   
> -	/* Generic flush handling.
> -	 * The last to finish preflush schedules a worker to submit
> -	 * the rest of the request (without the REQ_PREFLUSH flag).
> -	 */
> -	struct bio *flush_bio;
> -	atomic_t flush_pending;
> -	struct work_struct flush_work;
>   	struct work_struct event_work;	/* used by dm to report failure event */
>   	void (*sync_super)(struct mddev *mddev, struct md_rdev *rdev);
>   	struct md_cluster_info		*cluster_info;

Re: [RFC PATCH] MD: fix lock contention for flush bios

[Xiao Ni]

----- Original Message -----
> From: "Guoqing Jiang" <gqjiang@suse.com>
> To: "Xiao Ni" <xni@redhat.com>, linux-raid@vger.kernel.org
> Cc: shli@kernel.org, neilb@suse.com, "ming lei" <ming.lei@redhat.com>, ncroxon@redhat.com
> Sent: Wednesday, January 24, 2018 5:02:57 PM
> Subject: Re: [RFC PATCH] MD: fix lock contention for flush bios
> 
> 
> 
> On 01/24/2018 10:43 AM, Xiao Ni wrote:
> > There is a lock contention when there are many processes which send flush
> > bios
> > to md device. eg. Create many lvs on one raid device and mkfs.xfs on each
> > lv.
> >
> > Now it just can handle flush request sequentially. It needs to wait
> > mddev->flush_bio
> > to be NULL, otherwise get mddev->lock.
> 
> With the new approach, can we still keep the synchronization across all
> devices?
> I found the previous commit a2826aa92e2e ("md: support barrier requests
> on all
> personalities") did want to keep synchronization.


When one flush bio is sumbitted to md, it creates one bio for each rdev to send flush request.
If it must waits for all the flush requests to return, my patch breaks the rule.

Process A submits a flush bio to md, we call this flush bio bio-a.
Process B submits a flush bio to md, we call this flush bio bio-b.

Before my patch there is only one process can handle flush bio. Process B waits for the returning
of bio-b from Process B. After my patch it can handle the flush bios from all processes. Can't 
we handle flush bios from different processes at the same time?

Xiao

> 
> [snip]
> 
> > Suggested-by: Ming Lei <ming.lei@redhat.com>
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >   drivers/md/md.c | 105
> >   +++++++++++++++++++++++++-------------------------------
> >   drivers/md/md.h |  14 ++++----
> >   2 files changed, 54 insertions(+), 65 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 4e4dee0..1e562f5 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -409,33 +409,61 @@ static int md_congested(void *data, int bits)
> >   	return mddev_congested(mddev, bits);
> >   }
> >   
> > -/*
> > - * Generic flush handling for md
> > - */
> > +static void submit_flushes(struct work_struct *ws)
> > +{
> > +	struct flush_info *fi = container_of(ws, struct flush_info, flush_work);
> > +	struct mddev *mddev = fi->mddev;
> > +	struct bio *bio = fi->bio;
> > +
> > +	bio->bi_opf &= ~REQ_PREFLUSH;
> > +	md_handle_request(mddev, bio);
> >   
> > -static void md_end_flush(struct bio *bio)
> > +	kfree(fi);
> > +}
> > +
> > +
> 
> An extra blank line above.
> 
> > +static void md_end_flush(struct bio *flush_bio)
> >   {
> > -	struct md_rdev *rdev = bio->bi_private;
> > -	struct mddev *mddev = rdev->mddev;
> > +	struct flush_info *fi = flush_bio->bi_private;
> > +	struct bio *bio = fi->bio;
> > +	struct mddev *mddev = fi->mddev;
> > +	struct md_rdev *rdev;
> >   
> > -	rdev_dec_pending(rdev, mddev);
> > +	rcu_read_lock();
> > +		rdev_for_each_rcu(rdev, mddev)
> > +			rdev_dec_pending(rdev, mddev);
> 
> Unnecessary indentation.
> 
> > +	rcu_read_unlock();
> >   
> > -	if (atomic_dec_and_test(&mddev->flush_pending)) {
> > -		/* The pre-request flush has finished */
> > -		queue_work(md_wq, &mddev->flush_work);
> > +	if (bio->bi_iter.bi_size == 0)
> > +		/* an empty barrier - all done */
> > +		bio_endio(bio);
> > +	else {
> > +		INIT_WORK(&fi->flush_work, submit_flushes);
> > +		queue_work(md_wq, &fi->flush_work);
> >   	}
> > -	bio_put(bio);
> >   }
> >   
> > -static void md_submit_flush_data(struct work_struct *ws);
> > -
> > -static void submit_flushes(struct work_struct *ws)
> > +void md_flush_request(struct mddev *mddev, struct bio *bio)
> >   {
> > -	struct mddev *mddev = container_of(ws, struct mddev, flush_work);
> >   	struct md_rdev *rdev;
> > +	struct flush_info *fi;
> > +	struct bio *f_bio;
> > +
> > +	fi = kmalloc(sizeof(*fi), GFP_NOIO);
> > +	if (fi == NULL) {
> > +		pr_err("md: %s failed to alloc memory for flush bio\n",
> > +		       mdname(mddev));
> > +		bio->bi_status = BLK_STS_IOERR;
> > +		bio_endio(bio);
> 
> Maybe you missed "return" here.
> 
> Thanks,
> Guoqing
> 
> > +	}
> > +
> > +	fi->bio = bio;
> > +	fi->mddev = mddev;
> > +	f_bio = &fi->flush_bio;
> > +	bio_init(f_bio, NULL, 0);
> > +	f_bio->bi_private = fi;
> > +	f_bio->bi_end_io = md_end_flush;
> >   
> > -	INIT_WORK(&mddev->flush_work, md_submit_flush_data);
> > -	atomic_set(&mddev->flush_pending, 1);
> >   	rcu_read_lock();
> >   	rdev_for_each_rcu(rdev, mddev)
> >   		if (rdev->raid_disk >= 0 &&
> > @@ -449,54 +477,16 @@ static void submit_flushes(struct work_struct *ws)
> >   			atomic_inc(&rdev->nr_pending);
> >   			rcu_read_unlock();
> >   			bi = bio_alloc_mddev(GFP_NOIO, 0, mddev);
> > -			bi->bi_end_io = md_end_flush;
> > -			bi->bi_private = rdev;
> >   			bio_set_dev(bi, rdev->bdev);
> >   			bi->bi_opf = REQ_OP_WRITE | REQ_PREFLUSH;
> > -			atomic_inc(&mddev->flush_pending);
> > +			bio_chain(bi, f_bio);
> >   			submit_bio(bi);
> >   			rcu_read_lock();
> >   			rdev_dec_pending(rdev, mddev);
> >   		}
> >   	rcu_read_unlock();
> > -	if (atomic_dec_and_test(&mddev->flush_pending))
> > -		queue_work(md_wq, &mddev->flush_work);
> > -}
> > -
> > -static void md_submit_flush_data(struct work_struct *ws)
> > -{
> > -	struct mddev *mddev = container_of(ws, struct mddev, flush_work);
> > -	struct bio *bio = mddev->flush_bio;
> > -
> > -	/*
> > -	 * must reset flush_bio before calling into md_handle_request to avoid a
> > -	 * deadlock, because other bios passed md_handle_request suspend check
> > -	 * could wait for this and below md_handle_request could wait for those
> > -	 * bios because of suspend check
> > -	 */
> > -	mddev->flush_bio = NULL;
> > -	wake_up(&mddev->sb_wait);
> > -
> > -	if (bio->bi_iter.bi_size == 0)
> > -		/* an empty barrier - all done */
> > -		bio_endio(bio);
> > -	else {
> > -		bio->bi_opf &= ~REQ_PREFLUSH;
> > -		md_handle_request(mddev, bio);
> > -	}
> > -}
> > -
> > -void md_flush_request(struct mddev *mddev, struct bio *bio)
> > -{
> > -	spin_lock_irq(&mddev->lock);
> > -	wait_event_lock_irq(mddev->sb_wait,
> > -			    !mddev->flush_bio,
> > -			    mddev->lock);
> > -	mddev->flush_bio = bio;
> > -	spin_unlock_irq(&mddev->lock);
> >   
> > -	INIT_WORK(&mddev->flush_work, submit_flushes);
> > -	queue_work(md_wq, &mddev->flush_work);
> > +	bio_endio(f_bio);
> >   }
> >   EXPORT_SYMBOL(md_flush_request);
> >   
> > @@ -555,7 +545,6 @@ void mddev_init(struct mddev *mddev)
> >   	atomic_set(&mddev->openers, 0);
> >   	atomic_set(&mddev->active_io, 0);
> >   	spin_lock_init(&mddev->lock);
> > -	atomic_set(&mddev->flush_pending, 0);
> >   	init_waitqueue_head(&mddev->sb_wait);
> >   	init_waitqueue_head(&mddev->recovery_wait);
> >   	mddev->reshape_position = MaxSector;
> > diff --git a/drivers/md/md.h b/drivers/md/md.h
> > index 7d6bcf0..16e7f03 100644
> > --- a/drivers/md/md.h
> > +++ b/drivers/md/md.h
> > @@ -252,6 +252,13 @@ enum mddev_sb_flags {
> >   	MD_SB_NEED_REWRITE,	/* metadata write needs to be repeated */
> >   };
> >   
> > +struct flush_info {
> > +	struct bio *bio;
> > +	struct bio flush_bio;
> > +	struct mddev *mddev;
> > +	struct work_struct flush_work;
> > +};
> > +
> >   struct mddev {
> >   	void				*private;
> >   	struct md_personality		*pers;
> > @@ -457,13 +464,6 @@ struct mddev {
> >   						   * metadata and bitmap writes
> >   						   */
> >   
> > -	/* Generic flush handling.
> > -	 * The last to finish preflush schedules a worker to submit
> > -	 * the rest of the request (without the REQ_PREFLUSH flag).
> > -	 */
> > -	struct bio *flush_bio;
> > -	atomic_t flush_pending;
> > -	struct work_struct flush_work;
> >   	struct work_struct event_work;	/* used by dm to report failure event */
> >   	void (*sync_super)(struct mddev *mddev, struct md_rdev *rdev);
> >   	struct md_cluster_info		*cluster_info;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC PATCH] MD: fix lock contention for flush bios

[Ming Lei]

Hi Guoqing,

On Fri, Jan 26, 2018 at 09:10:11AM +0800, Guoqing Jiang wrote:
> 
> 
> On 01/24/2018 09:41 PM, Xiao Ni wrote:
> > ----- Original Message -----
> > > From: "Guoqing Jiang"<gqjiang@suse.com>
> > > To: "Xiao Ni"<xni@redhat.com>,linux-raid@vger.kernel.org
> > > Cc:shli@kernel.org,neilb@suse.com, "ming lei"<ming.lei@redhat.com>,ncroxon@redhat.com
> > > Sent: Wednesday, January 24, 2018 5:02:57 PM
> > > Subject: Re: [RFC PATCH] MD: fix lock contention for flush bios
> > > 
> > > 
> > > 
> > > On 01/24/2018 10:43 AM, Xiao Ni wrote:
> > > > There is a lock contention when there are many processes which send flush
> > > > bios
> > > > to md device. eg. Create many lvs on one raid device and mkfs.xfs on each
> > > > lv.
> > > > 
> > > > Now it just can handle flush request sequentially. It needs to wait
> > > > mddev->flush_bio
> > > > to be NULL, otherwise get mddev->lock.
> > > With the new approach, can we still keep the synchronization across all
> > > devices?
> > > I found the previous commit a2826aa92e2e ("md: support barrier requests
> > > on all
> > > personalities") did want to keep synchronization.
> > When one flush bio is sumbitted to md, it creates one bio for each rdev to send flush request.
> > If it must waits for all the flush requests to return, my patch breaks the rule.
> > 
> > Process A submits a flush bio to md, we call this flush bio bio-a.
> > Process B submits a flush bio to md, we call this flush bio bio-b.
> > 
> > Before my patch there is only one process can handle flush bio. Process B waits for the returning
> > of bio-b from Process B. After my patch it can handle the flush bios from all processes. Can't
> > we handle flush bios from different processes at the same time?
> 
> Hmm, barrier does need the synchronization though it caused the contention.

Seems this patch only touches code of handling MD flush request, not related
with barrier, could you explain a bit if it is?

> For flush/fua, I guess
> we may not need it since fs need to ensure that certain requests are
> executed in order, but I am not
> so sure about it.

From block layer's view, there isn't order in handling flush request
among different IOs, and it is just a command sent to hardware for flushing
the internal cache in drive.

The current strictly serialized style of handling MD flush request does hurt
performance, and IMO, the idea of the patch should be correct, at least from
the view of handling flush request.

Thanks,
Ming

Re: [RFC PATCH] MD: fix lock contention for flush bios

[Guoqing Jiang]

On 01/26/2018 10:07 AM, Ming Lei wrote:
> Hi Guoqing,
>
> On Fri, Jan 26, 2018 at 09:10:11AM +0800, Guoqing Jiang wrote:
>>
>> On 01/24/2018 09:41 PM, Xiao Ni wrote:
>>> ----- Original Message -----
>>>> From: "Guoqing Jiang"<gqjiang@suse.com>
>>>> To: "Xiao Ni"<xni@redhat.com>,linux-raid@vger.kernel.org
>>>> Cc:shli@kernel.org,neilb@suse.com, "ming lei"<ming.lei@redhat.com>,ncroxon@redhat.com
>>>> Sent: Wednesday, January 24, 2018 5:02:57 PM
>>>> Subject: Re: [RFC PATCH] MD: fix lock contention for flush bios
>>>>
>>>>
>>>>
>>>> On 01/24/2018 10:43 AM, Xiao Ni wrote:
>>>>> There is a lock contention when there are many processes which send flush
>>>>> bios
>>>>> to md device. eg. Create many lvs on one raid device and mkfs.xfs on each
>>>>> lv.
>>>>>
>>>>> Now it just can handle flush request sequentially. It needs to wait
>>>>> mddev->flush_bio
>>>>> to be NULL, otherwise get mddev->lock.
>>>> With the new approach, can we still keep the synchronization across all
>>>> devices?
>>>> I found the previous commit a2826aa92e2e ("md: support barrier requests
>>>> on all
>>>> personalities") did want to keep synchronization.
>>> When one flush bio is sumbitted to md, it creates one bio for each rdev to send flush request.
>>> If it must waits for all the flush requests to return, my patch breaks the rule.
>>>
>>> Process A submits a flush bio to md, we call this flush bio bio-a.
>>> Process B submits a flush bio to md, we call this flush bio bio-b.
>>>
>>> Before my patch there is only one process can handle flush bio. Process B waits for the returning
>>> of bio-b from Process B. After my patch it can handle the flush bios from all processes. Can't
>>> we handle flush bios from different processes at the same time?
>> Hmm, barrier does need the synchronization though it caused the contention.
> Seems this patch only touches code of handling MD flush request, not related
> with barrier, could you explain a bit if it is?

Hmm, I didn't notice the barrier had been converted to flush/fua long 
time ago.
And some comments inside the code need to be updated I think.

>> For flush/fua, I guess
>> we may not need it since fs need to ensure that certain requests are
>> executed in order, but I am not
>> so sure about it.
>  From block layer's view, there isn't order in handling flush request
> among different IOs, and it is just a command sent to hardware for flushing
> the internal cache in drive.
>
> The current strictly serialized style of handling MD flush request does hurt
> performance, and IMO, the idea of the patch should be correct, at least from
> the view of handling flush request.

Thanks for the explanation, no more concern from me.

Regards,
Guoqing

Re: [RFC PATCH] MD: fix lock contention for flush bios

[Shaohua Li]

On Wed, Jan 24, 2018 at 10:43:54AM +0800, Xiao Ni wrote:
> There is a lock contention when there are many processes which send flush bios
> to md device. eg. Create many lvs on one raid device and mkfs.xfs on each lv.
> 
> Now it just can handle flush request sequentially. It needs to wait mddev->flush_bio
> to be NULL, otherwise get mddev->lock.
> 
> This patch remove mddev->flush_bio and allocat one bio for each flush bio.
> I did a test with command dbench -s 128 -t 4800. This is the test result:
> 
> =================Before the patch============================
> 
>  Operation      Count    AvgLat    MaxLat
>  ----------------------------------------
>  NTCreateX     124271     5.128 12752.344
>  Close          96922     0.645  6250.826
>  Rename          5141     1.556  6747.033
>  Unlink         20283     5.045 12752.294
>  Qpathinfo     111872     0.012   250.069
>  Qfileinfo      25684     0.004     0.293
>  Qfsinfo        18535     0.007     0.646
>  Sfileinfo      11763     0.014     0.571
>  Find           41018     0.026     0.869
>  WriteX         88616  6750.094 80740.875
>  ReadX         179250     0.018  1749.302
>  LockX            328     0.008     0.042
>  UnlockX          328     0.004     0.023
>  Flush           9650  1688.288 38998.814
> 
> Throughput 1.01668 MB/sec (sync open)  128 clients  128 procs  max_latency=80740.883 ms
> 
> ========================after patch============================
>  128  cleanup 600 sec
>    0  cleanup 600 sec
> 
>  Operation      Count    AvgLat    MaxLat
>  ----------------------------------------
>  NTCreateX      52444     1.403  1499.920
>  Close          45740     0.035  1000.301
>  Rename          2090     0.073     1.591
>  Unlink          5032     5.855  1251.375
>  Qpathinfo      44493     0.167   501.498
>  Qfileinfo      16261     0.004     0.577
>  Qfsinfo         5887     0.011     1.133
>  Sfileinfo       6303     0.022     1.277
>  Find           15321     0.034     1.041
>  WriteX         62730  1197.684  3247.813
>  ReadX          60379     0.013     2.408
>  LockX             48     0.010     0.039
>  UnlockX           48     0.004     0.006
>  Flush           4370   382.070   999.735
> 
> Throughput 5.06007 MB/sec (sync open)  128 clients  128 procs  max_latency=3247.823 ms
> 
> The test was did on one raid10 with 4 SSDs.
> 
> Suggested-by: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  drivers/md/md.c | 105 +++++++++++++++++++++++++-------------------------------
>  drivers/md/md.h |  14 ++++----
>  2 files changed, 54 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 4e4dee0..1e562f5 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -409,33 +409,61 @@ static int md_congested(void *data, int bits)
>  	return mddev_congested(mddev, bits);
>  }
>  
> -/*
> - * Generic flush handling for md
> - */
> +static void submit_flushes(struct work_struct *ws)
> +{
> +	struct flush_info *fi = container_of(ws, struct flush_info, flush_work);
> +	struct mddev *mddev = fi->mddev;
> +	struct bio *bio = fi->bio;
> +
> +	bio->bi_opf &= ~REQ_PREFLUSH;
> +	md_handle_request(mddev, bio);
>  
> -static void md_end_flush(struct bio *bio)
> +	kfree(fi);
> +}
> +
> +
> +static void md_end_flush(struct bio *flush_bio)
>  {
> -	struct md_rdev *rdev = bio->bi_private;
> -	struct mddev *mddev = rdev->mddev;
> +	struct flush_info *fi = flush_bio->bi_private;
> +	struct bio *bio = fi->bio;
> +	struct mddev *mddev = fi->mddev;
> +	struct md_rdev *rdev;
>  
> -	rdev_dec_pending(rdev, mddev);
> +	rcu_read_lock();
> +		rdev_for_each_rcu(rdev, mddev)
> +			rdev_dec_pending(rdev, mddev);
> +	rcu_read_unlock();
>  
> -	if (atomic_dec_and_test(&mddev->flush_pending)) {
> -		/* The pre-request flush has finished */
> -		queue_work(md_wq, &mddev->flush_work);
> +	if (bio->bi_iter.bi_size == 0)
> +		/* an empty barrier - all done */
> +		bio_endio(bio);
> +	else {
> +		INIT_WORK(&fi->flush_work, submit_flushes);
> +		queue_work(md_wq, &fi->flush_work);
>  	}
> -	bio_put(bio);
>  }
>  
> -static void md_submit_flush_data(struct work_struct *ws);
> -
> -static void submit_flushes(struct work_struct *ws)
> +void md_flush_request(struct mddev *mddev, struct bio *bio)
>  {
> -	struct mddev *mddev = container_of(ws, struct mddev, flush_work);
>  	struct md_rdev *rdev;
> +	struct flush_info *fi;
> +	struct bio *f_bio;
> +
> +	fi = kmalloc(sizeof(*fi), GFP_NOIO);
> +	if (fi == NULL) {
> +		pr_err("md: %s failed to alloc memory for flush bio\n",
> +		       mdname(mddev));
> +		bio->bi_status = BLK_STS_IOERR;
> +		bio_endio(bio);
> +	}

So we allocate flush_info for each thread which calling into flush. This is a
little concern to me. I'm wondering if this optimization is really necessary.
In practice, flush is called very rare.

Thanks,
Shaohua
> +	fi->bio = bio;
> +	fi->mddev = mddev;
> +	f_bio = &fi->flush_bio;
> +	bio_init(f_bio, NULL, 0);
> +	f_bio->bi_private = fi;
> +	f_bio->bi_end_io = md_end_flush;
>  
> -	INIT_WORK(&mddev->flush_work, md_submit_flush_data);
> -	atomic_set(&mddev->flush_pending, 1);
>  	rcu_read_lock();
>  	rdev_for_each_rcu(rdev, mddev)
>  		if (rdev->raid_disk >= 0 &&
> @@ -449,54 +477,16 @@ static void submit_flushes(struct work_struct *ws)
>  			atomic_inc(&rdev->nr_pending);
>  			rcu_read_unlock();
>  			bi = bio_alloc_mddev(GFP_NOIO, 0, mddev);
> -			bi->bi_end_io = md_end_flush;
> -			bi->bi_private = rdev;
>  			bio_set_dev(bi, rdev->bdev);
>  			bi->bi_opf = REQ_OP_WRITE | REQ_PREFLUSH;
> -			atomic_inc(&mddev->flush_pending);
> +			bio_chain(bi, f_bio);
>  			submit_bio(bi);
>  			rcu_read_lock();
>  			rdev_dec_pending(rdev, mddev);
>  		}
>  	rcu_read_unlock();
> -	if (atomic_dec_and_test(&mddev->flush_pending))
> -		queue_work(md_wq, &mddev->flush_work);
> -}
> -
> -static void md_submit_flush_data(struct work_struct *ws)
> -{
> -	struct mddev *mddev = container_of(ws, struct mddev, flush_work);
> -	struct bio *bio = mddev->flush_bio;
> -
> -	/*
> -	 * must reset flush_bio before calling into md_handle_request to avoid a
> -	 * deadlock, because other bios passed md_handle_request suspend check
> -	 * could wait for this and below md_handle_request could wait for those
> -	 * bios because of suspend check
> -	 */
> -	mddev->flush_bio = NULL;
> -	wake_up(&mddev->sb_wait);
> -
> -	if (bio->bi_iter.bi_size == 0)
> -		/* an empty barrier - all done */
> -		bio_endio(bio);
> -	else {
> -		bio->bi_opf &= ~REQ_PREFLUSH;
> -		md_handle_request(mddev, bio);
> -	}
> -}
> -
> -void md_flush_request(struct mddev *mddev, struct bio *bio)
> -{
> -	spin_lock_irq(&mddev->lock);
> -	wait_event_lock_irq(mddev->sb_wait,
> -			    !mddev->flush_bio,
> -			    mddev->lock);
> -	mddev->flush_bio = bio;
> -	spin_unlock_irq(&mddev->lock);
>  
> -	INIT_WORK(&mddev->flush_work, submit_flushes);
> -	queue_work(md_wq, &mddev->flush_work);
> +	bio_endio(f_bio);
>  }
>  EXPORT_SYMBOL(md_flush_request);
>  
> @@ -555,7 +545,6 @@ void mddev_init(struct mddev *mddev)
>  	atomic_set(&mddev->openers, 0);
>  	atomic_set(&mddev->active_io, 0);
>  	spin_lock_init(&mddev->lock);
> -	atomic_set(&mddev->flush_pending, 0);
>  	init_waitqueue_head(&mddev->sb_wait);
>  	init_waitqueue_head(&mddev->recovery_wait);
>  	mddev->reshape_position = MaxSector;
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 7d6bcf0..16e7f03 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -252,6 +252,13 @@ enum mddev_sb_flags {
>  	MD_SB_NEED_REWRITE,	/* metadata write needs to be repeated */
>  };
>  
> +struct flush_info {
> +	struct bio *bio;
> +	struct bio flush_bio;
> +	struct mddev *mddev;
> +	struct work_struct flush_work;
> +};
> +
>  struct mddev {
>  	void				*private;
>  	struct md_personality		*pers;
> @@ -457,13 +464,6 @@ struct mddev {
>  						   * metadata and bitmap writes
>  						   */
>  
> -	/* Generic flush handling.
> -	 * The last to finish preflush schedules a worker to submit
> -	 * the rest of the request (without the REQ_PREFLUSH flag).
> -	 */
> -	struct bio *flush_bio;
> -	atomic_t flush_pending;
> -	struct work_struct flush_work;
>  	struct work_struct event_work;	/* used by dm to report failure event */
>  	void (*sync_super)(struct mddev *mddev, struct md_rdev *rdev);
>  	struct md_cluster_info		*cluster_info;
> -- 
> 2.7.4
>