linux-raid.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] More fixups for raid5
@ 2022-08-11 17:14 Logan Gunthorpe
  2022-08-11 17:14 ` [PATCH 1/5] md: Flush workqueue md_rdev_misc_wq in md_alloc() Logan Gunthorpe
                   ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: Logan Gunthorpe @ 2022-08-11 17:14 UTC (permalink / raw)
  To: linux-kernel, linux-raid, Song Liu
  Cc: Christoph Hellwig, Guoqing Jiang, Stephen Bates, Martin Oliveira,
	David Sloan, Logan Gunthorpe

Hey,

The first patch in this series is a fix for another race issue with
the test infrastructure.

The remaining 4 patches address Christoph's feedback in my previous
patchset that I sent rather late in the cycle. (Sorry about that).

This series is based on current md-next (ae0a80935d6a6).

Logan

--

David Sloan (1):
  md: Flush workqueue md_rdev_misc_wq in md_alloc()

Logan Gunthorpe (4):
  md/raid5: Refactor raid5_get_active_stripe()
  md/raid5: Drop extern on function declarations in raid5.h
  md/raid5: Cleanup prototype of raid5_get_active_stripe()
  md/raid5: Don't read ->active_stripes if it's not needed

 drivers/md/md.c          |   1 +
 drivers/md/raid5-cache.c |   3 +-
 drivers/md/raid5.c       | 132 ++++++++++++++++++++-------------------
 drivers/md/raid5.h       |  32 ++++++----
 4 files changed, 90 insertions(+), 78 deletions(-)


base-commit: ae0a80935d6a65764b0db00c8b03d3807b4110a6
--
2.30.2

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/5] md: Flush workqueue md_rdev_misc_wq in md_alloc()
  2022-08-11 17:14 [PATCH 0/5] More fixups for raid5 Logan Gunthorpe
@ 2022-08-11 17:14 ` Logan Gunthorpe
  2022-08-11 17:46   ` Song Liu
  2022-08-11 17:14 ` [PATCH 2/5] md/raid5: Refactor raid5_get_active_stripe() Logan Gunthorpe
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 9+ messages in thread
From: Logan Gunthorpe @ 2022-08-11 17:14 UTC (permalink / raw)
  To: linux-kernel, linux-raid, Song Liu
  Cc: Christoph Hellwig, Guoqing Jiang, Stephen Bates, Martin Oliveira,
	David Sloan, David Sloan, Logan Gunthorpe

From: David Sloan <david.sloan@eideticom.com>

A race condition still exists when removing and re-creating md devices
in test cases. However, it is only seen on some setups.

The race condition was tracked down to a reference still being held
to the kobject by the rdev in the md_rdev_misc_wq which will be released
in rdev_delayed_delete().

md_alloc() waits for previous deletions by waiting on the md_misc_wq,
but the md_rdev_misc_wq may still be holding a reference to a recently
removed device.

To fix this, also flush the md_rdev_misc_wq in md_alloc().

Signed-off-by: David Sloan <david.sloan@eideticom.com>
[logang@deltatee.com: rewrote commit message]
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/md/md.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index afaf36b2f6ab..71d221601bf8 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5620,6 +5620,7 @@ struct mddev *md_alloc(dev_t dev, char *name)
 	 * removed (mddev_delayed_delete).
 	 */
 	flush_workqueue(md_misc_wq);
+	flush_workqueue(md_rdev_misc_wq);
 
 	mutex_lock(&disks_mutex);
 	mddev = mddev_alloc(dev);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/5] md/raid5: Refactor raid5_get_active_stripe()
  2022-08-11 17:14 [PATCH 0/5] More fixups for raid5 Logan Gunthorpe
  2022-08-11 17:14 ` [PATCH 1/5] md: Flush workqueue md_rdev_misc_wq in md_alloc() Logan Gunthorpe
@ 2022-08-11 17:14 ` Logan Gunthorpe
  2022-08-29 22:23   ` Song Liu
  2022-08-11 17:14 ` [PATCH 3/5] md/raid5: Drop extern on function declarations in raid5.h Logan Gunthorpe
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 9+ messages in thread
From: Logan Gunthorpe @ 2022-08-11 17:14 UTC (permalink / raw)
  To: linux-kernel, linux-raid, Song Liu
  Cc: Christoph Hellwig, Guoqing Jiang, Stephen Bates, Martin Oliveira,
	David Sloan, Logan Gunthorpe

Refactor raid5_get_active_stripe() without the gotos with an
explicit infinite loop and some additional nesting.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/md/raid5.c | 82 +++++++++++++++++++++++-----------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 4456ac51f7c5..1288ef9e1571 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -811,54 +811,54 @@ static struct stripe_head *__raid5_get_active_stripe(struct r5conf *conf,
 
 	spin_lock_irq(conf->hash_locks + hash);
 
-retry:
-	if (!noquiesce && conf->quiesce) {
-		/*
-		 * Must release the reference to batch_last before waiting,
-		 * on quiesce, otherwise the batch_last will hold a reference
-		 * to a stripe and raid5_quiesce() will deadlock waiting for
-		 * active_stripes to go to zero.
-		 */
-		if (ctx && ctx->batch_last) {
-			raid5_release_stripe(ctx->batch_last);
-			ctx->batch_last = NULL;
-		}
-
-		wait_event_lock_irq(conf->wait_for_quiescent, !conf->quiesce,
-				    *(conf->hash_locks + hash));
-	}
+	for (;;) {
+		if (!noquiesce && conf->quiesce) {
+			/*
+			 * Must release the reference to batch_last before
+			 * waiting, on quiesce, otherwise the batch_last will
+			 * hold a reference to a stripe and raid5_quiesce()
+			 * will deadlock waiting for active_stripes to go to
+			 * zero.
+			 */
+			if (ctx && ctx->batch_last) {
+				raid5_release_stripe(ctx->batch_last);
+				ctx->batch_last = NULL;
+			}
 
-	sh = find_get_stripe(conf, sector, conf->generation - previous, hash);
-	if (sh)
-		goto out;
+			wait_event_lock_irq(conf->wait_for_quiescent,
+					    !conf->quiesce,
+					    *(conf->hash_locks + hash));
+		}
 
-	if (test_bit(R5_INACTIVE_BLOCKED, &conf->cache_state))
-		goto wait_for_stripe;
+		sh = find_get_stripe(conf, sector, conf->generation - previous,
+				     hash);
+		if (sh)
+			break;
 
-	sh = get_free_stripe(conf, hash);
-	if (sh) {
-		r5c_check_stripe_cache_usage(conf);
-		init_stripe(sh, sector, previous);
-		atomic_inc(&sh->count);
-		goto out;
-	}
+		if (!test_bit(R5_INACTIVE_BLOCKED, &conf->cache_state)) {
+			sh = get_free_stripe(conf, hash);
+			if (sh) {
+				r5c_check_stripe_cache_usage(conf);
+				init_stripe(sh, sector, previous);
+				atomic_inc(&sh->count);
+				break;
+			}
 
-	if (!test_bit(R5_DID_ALLOC, &conf->cache_state))
-		set_bit(R5_ALLOC_MORE, &conf->cache_state);
+			if (!test_bit(R5_DID_ALLOC, &conf->cache_state))
+				set_bit(R5_ALLOC_MORE, &conf->cache_state);
+		}
 
-wait_for_stripe:
-	if (noblock)
-		goto out;
+		if (noblock)
+			break;
 
-	set_bit(R5_INACTIVE_BLOCKED, &conf->cache_state);
-	r5l_wake_reclaim(conf->log, 0);
-	wait_event_lock_irq(conf->wait_for_stripe,
-			    is_inactive_blocked(conf, hash),
-			    *(conf->hash_locks + hash));
-	clear_bit(R5_INACTIVE_BLOCKED, &conf->cache_state);
-	goto retry;
+		set_bit(R5_INACTIVE_BLOCKED, &conf->cache_state);
+		r5l_wake_reclaim(conf->log, 0);
+		wait_event_lock_irq(conf->wait_for_stripe,
+				    is_inactive_blocked(conf, hash),
+				    *(conf->hash_locks + hash));
+		clear_bit(R5_INACTIVE_BLOCKED, &conf->cache_state);
+	}
 
-out:
 	spin_unlock_irq(conf->hash_locks + hash);
 	return sh;
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/5] md/raid5: Drop extern on function declarations in raid5.h
  2022-08-11 17:14 [PATCH 0/5] More fixups for raid5 Logan Gunthorpe
  2022-08-11 17:14 ` [PATCH 1/5] md: Flush workqueue md_rdev_misc_wq in md_alloc() Logan Gunthorpe
  2022-08-11 17:14 ` [PATCH 2/5] md/raid5: Refactor raid5_get_active_stripe() Logan Gunthorpe
@ 2022-08-11 17:14 ` Logan Gunthorpe
  2022-08-11 17:14 ` [PATCH 4/5] md/raid5: Cleanup prototype of raid5_get_active_stripe() Logan Gunthorpe
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Logan Gunthorpe @ 2022-08-11 17:14 UTC (permalink / raw)
  To: linux-kernel, linux-raid, Song Liu
  Cc: Christoph Hellwig, Guoqing Jiang, Stephen Bates, Martin Oliveira,
	David Sloan, Logan Gunthorpe

externs should not be used in function declarations, so clean those
up.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/md/raid5.h | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index a5082bed83c8..4be2feb9e74a 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -803,16 +803,14 @@ raid5_get_dev_page(struct stripe_head *sh, int disk_idx)
 }
 #endif
 
-extern void md_raid5_kick_device(struct r5conf *conf);
-extern int raid5_set_cache_size(struct mddev *mddev, int size);
-extern sector_t raid5_compute_blocknr(struct stripe_head *sh, int i, int previous);
-extern void raid5_release_stripe(struct stripe_head *sh);
-extern sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
-				     int previous, int *dd_idx,
-				     struct stripe_head *sh);
-extern struct stripe_head *
-raid5_get_active_stripe(struct r5conf *conf, sector_t sector,
-			bool previous, bool noblock, bool noquiesce);
-extern int raid5_calc_degraded(struct r5conf *conf);
-extern int r5c_journal_mode_set(struct mddev *mddev, int journal_mode);
+void md_raid5_kick_device(struct r5conf *conf);
+int raid5_set_cache_size(struct mddev *mddev, int size);
+sector_t raid5_compute_blocknr(struct stripe_head *sh, int i, int previous);
+void raid5_release_stripe(struct stripe_head *sh);
+sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
+		int previous, int *dd_idx, struct stripe_head *sh);
+struct stripe_head *raid5_get_active_stripe(struct r5conf *conf,
+		sector_t sector, bool previous, bool noblock, bool noquiesce);
+int raid5_calc_degraded(struct r5conf *conf);
+int r5c_journal_mode_set(struct mddev *mddev, int journal_mode);
 #endif
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/5] md/raid5: Cleanup prototype of raid5_get_active_stripe()
  2022-08-11 17:14 [PATCH 0/5] More fixups for raid5 Logan Gunthorpe
                   ` (2 preceding siblings ...)
  2022-08-11 17:14 ` [PATCH 3/5] md/raid5: Drop extern on function declarations in raid5.h Logan Gunthorpe
@ 2022-08-11 17:14 ` Logan Gunthorpe
  2022-08-11 17:14 ` [PATCH 5/5] md/raid5: Don't read ->active_stripes if it's not needed Logan Gunthorpe
  2022-08-13  6:31 ` [PATCH 0/5] More fixups for raid5 Christoph Hellwig
  5 siblings, 0 replies; 9+ messages in thread
From: Logan Gunthorpe @ 2022-08-11 17:14 UTC (permalink / raw)
  To: linux-kernel, linux-raid, Song Liu
  Cc: Christoph Hellwig, Guoqing Jiang, Stephen Bates, Martin Oliveira,
	David Sloan, Logan Gunthorpe

Drop the three bools in the prototype of raid5_get_active_stripe()
and replace them with a flags parameter.

At the same time, drop the distinction with __raid5_get_active_stripe().

Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/md/raid5-cache.c |  3 ++-
 drivers/md/raid5.c       | 49 +++++++++++++++++++++-------------------
 drivers/md/raid5.h       | 12 +++++++++-
 3 files changed, 39 insertions(+), 25 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index f4e1cc1ece43..5e35a7321ded 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -1923,7 +1923,8 @@ r5c_recovery_alloc_stripe(
 {
 	struct stripe_head *sh;
 
-	sh = raid5_get_active_stripe(conf, stripe_sect, 0, noblock, 0);
+	sh = raid5_get_active_stripe(conf, NULL, stripe_sect,
+				     noblock ? R5_GAS_NOBLOCK : 0);
 	if (!sh)
 		return NULL;  /* no more stripe available */
 
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 1288ef9e1571..7e7205bb40f1 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -800,19 +800,20 @@ static bool is_inactive_blocked(struct r5conf *conf, int hash)
 	return active < (conf->max_nr_stripes * 3 / 4);
 }
 
-static struct stripe_head *__raid5_get_active_stripe(struct r5conf *conf,
+struct stripe_head *raid5_get_active_stripe(struct r5conf *conf,
 		struct stripe_request_ctx *ctx, sector_t sector,
-		bool previous, bool noblock, bool noquiesce)
+		unsigned int flags)
 {
 	struct stripe_head *sh;
 	int hash = stripe_hash_locks_hash(conf, sector);
+	int previous = !!(flags & R5_GAS_PREVIOUS);
 
 	pr_debug("get_stripe, sector %llu\n", (unsigned long long)sector);
 
 	spin_lock_irq(conf->hash_locks + hash);
 
 	for (;;) {
-		if (!noquiesce && conf->quiesce) {
+		if (!(flags & R5_GAS_NOQUIESCE) && conf->quiesce) {
 			/*
 			 * Must release the reference to batch_last before
 			 * waiting, on quiesce, otherwise the batch_last will
@@ -848,7 +849,7 @@ static struct stripe_head *__raid5_get_active_stripe(struct r5conf *conf,
 				set_bit(R5_ALLOC_MORE, &conf->cache_state);
 		}
 
-		if (noblock)
+		if (flags & R5_GAS_NOBLOCK)
 			break;
 
 		set_bit(R5_INACTIVE_BLOCKED, &conf->cache_state);
@@ -863,13 +864,6 @@ static struct stripe_head *__raid5_get_active_stripe(struct r5conf *conf,
 	return sh;
 }
 
-struct stripe_head *raid5_get_active_stripe(struct r5conf *conf,
-		sector_t sector, bool previous, bool noblock, bool noquiesce)
-{
-	return __raid5_get_active_stripe(conf, NULL, sector, previous, noblock,
-					 noquiesce);
-}
-
 static bool is_full_stripe_write(struct stripe_head *sh)
 {
 	BUG_ON(sh->overwrite_disks > (sh->disks - sh->raid_conf->max_degraded));
@@ -4636,7 +4630,8 @@ static void handle_stripe_expansion(struct r5conf *conf, struct stripe_head *sh)
 			sector_t bn = raid5_compute_blocknr(sh, i, 1);
 			sector_t s = raid5_compute_sector(conf, bn, 0,
 							  &dd_idx, NULL);
-			sh2 = raid5_get_active_stripe(conf, s, 0, 1, 1);
+			sh2 = raid5_get_active_stripe(conf, NULL, s,
+				R5_GAS_NOBLOCK | R5_GAS_NOQUIESCE);
 			if (sh2 == NULL)
 				/* so far only the early blocks of this stripe
 				 * have been requested.  When later blocks
@@ -5273,7 +5268,9 @@ static void handle_stripe(struct stripe_head *sh)
 	/* Finish reconstruct operations initiated by the expansion process */
 	if (sh->reconstruct_state == reconstruct_state_result) {
 		struct stripe_head *sh_src
-			= raid5_get_active_stripe(conf, sh->sector, 1, 1, 1);
+			= raid5_get_active_stripe(conf, NULL, sh->sector,
+					R5_GAS_PREVIOUS | R5_GAS_NOBLOCK |
+					R5_GAS_NOQUIESCE);
 		if (sh_src && test_bit(STRIPE_EXPAND_SOURCE, &sh_src->state)) {
 			/* sh cannot be written until sh_src has been read.
 			 * so arrange for sh to be delayed a little
@@ -5823,7 +5820,7 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
 		DEFINE_WAIT(w);
 		int d;
 	again:
-		sh = raid5_get_active_stripe(conf, logical_sector, 0, 0, 0);
+		sh = raid5_get_active_stripe(conf, NULL, logical_sector, 0);
 		prepare_to_wait(&conf->wait_for_overlap, &w,
 				TASK_UNINTERRUPTIBLE);
 		set_bit(R5_Overlap, &sh->dev[sh->pd_idx].flags);
@@ -5978,7 +5975,7 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
 	enum stripe_result ret;
 	struct stripe_head *sh;
 	sector_t new_sector;
-	int previous = 0;
+	int previous = 0, flags = 0;
 	int seq, dd_idx;
 
 	seq = read_seqcount_begin(&conf->gen_lock);
@@ -6012,8 +6009,11 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
 	pr_debug("raid456: %s, sector %llu logical %llu\n", __func__,
 		 new_sector, logical_sector);
 
-	sh = __raid5_get_active_stripe(conf, ctx, new_sector, previous,
-				       (bi->bi_opf & REQ_RAHEAD), 0);
+	if (previous)
+		flags |= R5_GAS_PREVIOUS;
+	if (bi->bi_opf & REQ_RAHEAD)
+		flags |= R5_GAS_NOBLOCK;
+	sh = raid5_get_active_stripe(conf, ctx, new_sector, flags);
 	if (unlikely(!sh)) {
 		/* cannot get stripe, just give-up */
 		bi->bi_status = BLK_STS_IOERR;
@@ -6362,7 +6362,8 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr, int *sk
 	for (i = 0; i < reshape_sectors; i += RAID5_STRIPE_SECTORS(conf)) {
 		int j;
 		int skipped_disk = 0;
-		sh = raid5_get_active_stripe(conf, stripe_addr+i, 0, 0, 1);
+		sh = raid5_get_active_stripe(conf, NULL, stripe_addr+i,
+					     R5_GAS_NOQUIESCE);
 		set_bit(STRIPE_EXPANDING, &sh->state);
 		atomic_inc(&conf->reshape_stripes);
 		/* If any of this stripe is beyond the end of the old
@@ -6411,7 +6412,8 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr, int *sk
 	if (last_sector >= mddev->dev_sectors)
 		last_sector = mddev->dev_sectors - 1;
 	while (first_sector <= last_sector) {
-		sh = raid5_get_active_stripe(conf, first_sector, 1, 0, 1);
+		sh = raid5_get_active_stripe(conf, NULL, first_sector,
+				R5_GAS_PREVIOUS | R5_GAS_NOQUIESCE);
 		set_bit(STRIPE_EXPAND_SOURCE, &sh->state);
 		set_bit(STRIPE_HANDLE, &sh->state);
 		raid5_release_stripe(sh);
@@ -6531,9 +6533,10 @@ static inline sector_t raid5_sync_request(struct mddev *mddev, sector_t sector_n
 
 	md_bitmap_cond_end_sync(mddev->bitmap, sector_nr, false);
 
-	sh = raid5_get_active_stripe(conf, sector_nr, 0, 1, 0);
+	sh = raid5_get_active_stripe(conf, NULL, sector_nr,
+				     R5_GAS_NOBLOCK);
 	if (sh == NULL) {
-		sh = raid5_get_active_stripe(conf, sector_nr, 0, 0, 0);
+		sh = raid5_get_active_stripe(conf, NULL, sector_nr, 0);
 		/* make sure we don't swamp the stripe cache if someone else
 		 * is trying to get access
 		 */
@@ -6596,8 +6599,8 @@ static int  retry_aligned_read(struct r5conf *conf, struct bio *raid_bio,
 			/* already done this stripe */
 			continue;
 
-		sh = raid5_get_active_stripe(conf, sector, 0, 1, 1);
-
+		sh = raid5_get_active_stripe(conf, NULL, sector,
+				R5_GAS_NOBLOCK | R5_GAS_NOQUIESCE);
 		if (!sh) {
 			/* failed to get a stripe - must wait */
 			conf->retry_read_aligned = raid_bio;
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 4be2feb9e74a..e873938a6125 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -809,8 +809,18 @@ sector_t raid5_compute_blocknr(struct stripe_head *sh, int i, int previous);
 void raid5_release_stripe(struct stripe_head *sh);
 sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
 		int previous, int *dd_idx, struct stripe_head *sh);
+
+struct stripe_request_ctx;
+/* get stripe from previous generation (when reshaping) */
+#define R5_GAS_PREVIOUS		(1 << 0)
+/* do not block waiting for a free stripe */
+#define R5_GAS_NOBLOCK		(1 << 1)
+/* do not block waiting for quiesce to be released */
+#define R5_GAS_NOQUIESCE	(1 << 2)
 struct stripe_head *raid5_get_active_stripe(struct r5conf *conf,
-		sector_t sector, bool previous, bool noblock, bool noquiesce);
+		struct stripe_request_ctx *ctx, sector_t sector,
+		unsigned int flags);
+
 int raid5_calc_degraded(struct r5conf *conf);
 int r5c_journal_mode_set(struct mddev *mddev, int journal_mode);
 #endif
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 5/5] md/raid5: Don't read ->active_stripes if it's not needed
  2022-08-11 17:14 [PATCH 0/5] More fixups for raid5 Logan Gunthorpe
                   ` (3 preceding siblings ...)
  2022-08-11 17:14 ` [PATCH 4/5] md/raid5: Cleanup prototype of raid5_get_active_stripe() Logan Gunthorpe
@ 2022-08-11 17:14 ` Logan Gunthorpe
  2022-08-13  6:31 ` [PATCH 0/5] More fixups for raid5 Christoph Hellwig
  5 siblings, 0 replies; 9+ messages in thread
From: Logan Gunthorpe @ 2022-08-11 17:14 UTC (permalink / raw)
  To: linux-kernel, linux-raid, Song Liu
  Cc: Christoph Hellwig, Guoqing Jiang, Stephen Bates, Martin Oliveira,
	David Sloan, Logan Gunthorpe

The atomic_read() is not needed in many cases so only do
the read after the first checks are done.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/md/raid5.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 7e7205bb40f1..0b73eeea1b19 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -789,15 +789,14 @@ struct stripe_request_ctx {
  */
 static bool is_inactive_blocked(struct r5conf *conf, int hash)
 {
-	int active = atomic_read(&conf->active_stripes);
-
 	if (list_empty(conf->inactive_list + hash))
 		return false;
 
 	if (!test_bit(R5_INACTIVE_BLOCKED, &conf->cache_state))
 		return true;
 
-	return active < (conf->max_nr_stripes * 3 / 4);
+	return (atomic_read(&conf->active_stripes) <
+		(conf->max_nr_stripes * 3 / 4));
 }
 
 struct stripe_head *raid5_get_active_stripe(struct r5conf *conf,
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/5] md: Flush workqueue md_rdev_misc_wq in md_alloc()
  2022-08-11 17:14 ` [PATCH 1/5] md: Flush workqueue md_rdev_misc_wq in md_alloc() Logan Gunthorpe
@ 2022-08-11 17:46   ` Song Liu
  0 siblings, 0 replies; 9+ messages in thread
From: Song Liu @ 2022-08-11 17:46 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: open list, linux-raid, Christoph Hellwig, Guoqing Jiang,
	Stephen Bates, Martin Oliveira, David Sloan

On Thu, Aug 11, 2022 at 10:14 AM Logan Gunthorpe <logang@deltatee.com> wrote:
>
> From: David Sloan <david.sloan@eideticom.com>
>
> A race condition still exists when removing and re-creating md devices
> in test cases. However, it is only seen on some setups.
>
> The race condition was tracked down to a reference still being held
> to the kobject by the rdev in the md_rdev_misc_wq which will be released
> in rdev_delayed_delete().
>
> md_alloc() waits for previous deletions by waiting on the md_misc_wq,
> but the md_rdev_misc_wq may still be holding a reference to a recently
> removed device.
>
> To fix this, also flush the md_rdev_misc_wq in md_alloc().
>
> Signed-off-by: David Sloan <david.sloan@eideticom.com>
> [logang@deltatee.com: rewrote commit message]
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Applied 1/5 to md-fixes.

Thanks!
Song

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/5] More fixups for raid5
  2022-08-11 17:14 [PATCH 0/5] More fixups for raid5 Logan Gunthorpe
                   ` (4 preceding siblings ...)
  2022-08-11 17:14 ` [PATCH 5/5] md/raid5: Don't read ->active_stripes if it's not needed Logan Gunthorpe
@ 2022-08-13  6:31 ` Christoph Hellwig
  5 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2022-08-13  6:31 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: linux-kernel, linux-raid, Song Liu, Christoph Hellwig,
	Guoqing Jiang, Stephen Bates, Martin Oliveira, David Sloan

The series looks fine to me, but all except for patch 1 really
is 6.1 material.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/5] md/raid5: Refactor raid5_get_active_stripe()
  2022-08-11 17:14 ` [PATCH 2/5] md/raid5: Refactor raid5_get_active_stripe() Logan Gunthorpe
@ 2022-08-29 22:23   ` Song Liu
  0 siblings, 0 replies; 9+ messages in thread
From: Song Liu @ 2022-08-29 22:23 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: open list, linux-raid, Christoph Hellwig, Guoqing Jiang,
	Stephen Bates, Martin Oliveira, David Sloan

On Thu, Aug 11, 2022 at 10:14 AM Logan Gunthorpe <logang@deltatee.com> wrote:
>
> Refactor raid5_get_active_stripe() without the gotos with an
> explicit infinite loop and some additional nesting.
>
> Suggested-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Applied to md-next.

Thanks!
Song

> ---
>  drivers/md/raid5.c | 82 +++++++++++++++++++++++-----------------------
>  1 file changed, 41 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 4456ac51f7c5..1288ef9e1571 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -811,54 +811,54 @@ static struct stripe_head *__raid5_get_active_stripe(struct r5conf *conf,
>
>         spin_lock_irq(conf->hash_locks + hash);
>
> -retry:
> -       if (!noquiesce && conf->quiesce) {
> -               /*
> -                * Must release the reference to batch_last before waiting,
> -                * on quiesce, otherwise the batch_last will hold a reference
> -                * to a stripe and raid5_quiesce() will deadlock waiting for
> -                * active_stripes to go to zero.
> -                */
> -               if (ctx && ctx->batch_last) {
> -                       raid5_release_stripe(ctx->batch_last);
> -                       ctx->batch_last = NULL;
> -               }
> -
> -               wait_event_lock_irq(conf->wait_for_quiescent, !conf->quiesce,
> -                                   *(conf->hash_locks + hash));
> -       }
> +       for (;;) {
> +               if (!noquiesce && conf->quiesce) {
> +                       /*
> +                        * Must release the reference to batch_last before
> +                        * waiting, on quiesce, otherwise the batch_last will
> +                        * hold a reference to a stripe and raid5_quiesce()
> +                        * will deadlock waiting for active_stripes to go to
> +                        * zero.
> +                        */
> +                       if (ctx && ctx->batch_last) {
> +                               raid5_release_stripe(ctx->batch_last);
> +                               ctx->batch_last = NULL;
> +                       }
>
> -       sh = find_get_stripe(conf, sector, conf->generation - previous, hash);
> -       if (sh)
> -               goto out;
> +                       wait_event_lock_irq(conf->wait_for_quiescent,
> +                                           !conf->quiesce,
> +                                           *(conf->hash_locks + hash));
> +               }
>
> -       if (test_bit(R5_INACTIVE_BLOCKED, &conf->cache_state))
> -               goto wait_for_stripe;
> +               sh = find_get_stripe(conf, sector, conf->generation - previous,
> +                                    hash);
> +               if (sh)
> +                       break;
>
> -       sh = get_free_stripe(conf, hash);
> -       if (sh) {
> -               r5c_check_stripe_cache_usage(conf);
> -               init_stripe(sh, sector, previous);
> -               atomic_inc(&sh->count);
> -               goto out;
> -       }
> +               if (!test_bit(R5_INACTIVE_BLOCKED, &conf->cache_state)) {
> +                       sh = get_free_stripe(conf, hash);
> +                       if (sh) {
> +                               r5c_check_stripe_cache_usage(conf);
> +                               init_stripe(sh, sector, previous);
> +                               atomic_inc(&sh->count);
> +                               break;
> +                       }
>
> -       if (!test_bit(R5_DID_ALLOC, &conf->cache_state))
> -               set_bit(R5_ALLOC_MORE, &conf->cache_state);
> +                       if (!test_bit(R5_DID_ALLOC, &conf->cache_state))
> +                               set_bit(R5_ALLOC_MORE, &conf->cache_state);
> +               }
>
> -wait_for_stripe:
> -       if (noblock)
> -               goto out;
> +               if (noblock)
> +                       break;
>
> -       set_bit(R5_INACTIVE_BLOCKED, &conf->cache_state);
> -       r5l_wake_reclaim(conf->log, 0);
> -       wait_event_lock_irq(conf->wait_for_stripe,
> -                           is_inactive_blocked(conf, hash),
> -                           *(conf->hash_locks + hash));
> -       clear_bit(R5_INACTIVE_BLOCKED, &conf->cache_state);
> -       goto retry;
> +               set_bit(R5_INACTIVE_BLOCKED, &conf->cache_state);
> +               r5l_wake_reclaim(conf->log, 0);
> +               wait_event_lock_irq(conf->wait_for_stripe,
> +                                   is_inactive_blocked(conf, hash),
> +                                   *(conf->hash_locks + hash));
> +               clear_bit(R5_INACTIVE_BLOCKED, &conf->cache_state);
> +       }
>
> -out:
>         spin_unlock_irq(conf->hash_locks + hash);
>         return sh;
>  }
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-08-29 22:23 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-11 17:14 [PATCH 0/5] More fixups for raid5 Logan Gunthorpe
2022-08-11 17:14 ` [PATCH 1/5] md: Flush workqueue md_rdev_misc_wq in md_alloc() Logan Gunthorpe
2022-08-11 17:46   ` Song Liu
2022-08-11 17:14 ` [PATCH 2/5] md/raid5: Refactor raid5_get_active_stripe() Logan Gunthorpe
2022-08-29 22:23   ` Song Liu
2022-08-11 17:14 ` [PATCH 3/5] md/raid5: Drop extern on function declarations in raid5.h Logan Gunthorpe
2022-08-11 17:14 ` [PATCH 4/5] md/raid5: Cleanup prototype of raid5_get_active_stripe() Logan Gunthorpe
2022-08-11 17:14 ` [PATCH 5/5] md/raid5: Don't read ->active_stripes if it's not needed Logan Gunthorpe
2022-08-13  6:31 ` [PATCH 0/5] More fixups for raid5 Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).