Skip to contentSkip to conversation

md: Introduction

PATCH New NeilBrown 3 participants 8 messages Aug 29, 2006 – Sep 4, 2006 MD RAID

lore
Share & tools
lore

NeilBrown

author
#0 lore raw

PATCH 1/1 md: Introduction

Following are 4 patches for md, suitable for 2.6.19.

The first three define "congested_fn" functions for various raid level - I all just found out about the need for these. These could address responsiveness problems that some people have reported, particularly while resync is running.

The last improves some messages when resync happens so e.g. it doesn't like your array is being resynced when you only asked for it to be checked.

Thanks,
NeilBrown

[PATCH 001 of 4] md: Define backing_dev_info.congested_fn for raid0 and linear [PATCH 002 of 4] md: Define ->congested_fn for raid1, raid10, and multipath [PATCH 003 of 4] md: Add a ->congested_fn function for raid5/6 [PATCH 004 of 4] md: Make messages about resync/recovery etc more specific.

author
#1 lore raw

PATCH 1/1 md: Define backing_dev_info.congested_fn for raid0 and linear

Each backing_dev needs to be able to report whether it is congested, either by modulating BDI_*_congested in ->state, or by defining a ->congested_fn.
md/raid did neither of these. This patch add a congested_fn which simply checks all component devices to see if they are congested.

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./drivers/md/linear.c |   15 +++++++++++++++
 ./drivers/md/raid0.c  |   17 +++++++++++++++++
 2 files changed, 32 insertions(+)
drivers/md/linear.c +15 -0
diff .prev/drivers/md/linear.c ./drivers/md/linear.c
@@ -111,6 +111,19 @@ static int linear_issue_flush(request_qu
 	return ret;
 }
 
+static int linear_congested(void *data, int bits)
+{
+	mddev_t *mddev = data;
+	linear_conf_t *conf = mddev_to_conf(mddev);
+	int i, ret = 0;
+
+	for (i = 0; i < mddev->raid_disks && !ret ; i++) {
+		request_queue_t *q = bdev_get_queue(conf->disks[i].rdev->bdev);
+		ret |= bdi_congested(&q->backing_dev_info, bits);
+	}
+	return ret;
+}
+
 static linear_conf_t *linear_conf(mddev_t *mddev, int raid_disks)
 {
 	linear_conf_t *conf;

@@ -269,6 +282,8 @@ static int linear_run (mddev_t *mddev)
 	blk_queue_merge_bvec(mddev->queue, linear_mergeable_bvec);
 	mddev->queue->unplug_fn = linear_unplug;
 	mddev->queue->issue_flush_fn = linear_issue_flush;
+	mddev->queue->backing_dev_info.congested_fn = linear_congested;
+	mddev->queue->backing_dev_info.congested_data = mddev;
 	return 0;
 }
drivers/md/raid0.c +17 -0
diff .prev/drivers/md/raid0.c ./drivers/md/raid0.c
@@ -60,6 +60,21 @@ static int raid0_issue_flush(request_que
 	return ret;
 }
 
+static int raid0_congested(void *data, int bits)
+{
+	mddev_t *mddev = data;
+	raid0_conf_t *conf = mddev_to_conf(mddev);
+	mdk_rdev_t **devlist = conf->strip_zone[0].dev;
+	int i, ret = 0;
+
+	for (i = 0; i < mddev->raid_disks && !ret ; i++) {
+		request_queue_t *q = bdev_get_queue(devlist[i]->bdev);
+
+		ret |= bdi_congested(&q->backing_dev_info, bits);
+	}
+	return ret;
+}
+
 
 static int create_strip_zones (mddev_t *mddev)
 {

@@ -236,6 +251,8 @@ static int create_strip_zones (mddev_t *
 	mddev->queue->unplug_fn = raid0_unplug;
 
 	mddev->queue->issue_flush_fn = raid0_issue_flush;
+	mddev->queue->backing_dev_info.congested_fn = raid0_congested;
+	mddev->queue->backing_dev_info.congested_data = mddev;
 
 	printk("raid0: done.\n");
 	return 0;
#2 lore raw

Re: [PATCH 001 of 4] md: Define backing_dev_info.congested_fn for raid0 and linear

Quoted 10 lines · On Tue, 29 Aug 2006 15:39:24 +1000 NeilBrown <[email protected]> wrote:

Each backing_dev needs to be able to report whether it is congested, either by modulating BDI_*_congested in ->state, or by defining a ->congested_fn.
md/raid did neither of these. This patch add a congested_fn which simply checks all component devices to see if they are congested.

Signed-off-by Neil Brown <[email protected]>

drivers/md/linear.c static int linear_issue_flush(request_qu · line 122

Quoted 9 lines
drivers/md/linear.c +9 -0
+static int linear_congested(void *data, int bits)
+{
+	mddev_t *mddev = data;
+	linear_conf_t *conf = mddev_to_conf(mddev);
+	int i, ret = 0;
+
+	for (i = 0; i < mddev->raid_disks && !ret ; i++) {
+		request_queue_t *q = bdev_get_queue(conf->disks[i].rdev->bdev);
+		ret |= bdi_congested(&q->backing_dev_info, bits);

nit: `ret = ' would suffice here.

drivers/md/raid0.c static int raid0_issue_flush(request_que · line 73

Quoted 11 lines
drivers/md/raid0.c +11 -0
+static int raid0_congested(void *data, int bits)
+{
+	mddev_t *mddev = data;
+	raid0_conf_t *conf = mddev_to_conf(mddev);
+	mdk_rdev_t **devlist = conf->strip_zone[0].dev;
+	int i, ret = 0;
+
+	for (i = 0; i < mddev->raid_disks && !ret ; i++) {
+		request_queue_t *q = bdev_get_queue(devlist[i]->bdev);
+
+		ret |= bdi_congested(&q->backing_dev_info, bits);

And here.

Quoted 16 lines
drivers/md/raid0.c +6 -0
+	}
+	return ret;
+}
+
 
 static int create_strip_zones (mddev_t *mddev)
 {

@@ -236,6 +251,8 @@ static int create_strip_zones (mddev_t *
 	mddev->queue->unplug_fn = raid0_unplug;
 
 	mddev->queue->issue_flush_fn = raid0_issue_flush;
+	mddev->queue->backing_dev_info.congested_fn = raid0_congested;
+	mddev->queue->backing_dev_info.congested_data = mddev;
 
 	printk("raid0: done.\n");
 	return 0;
author
#3 lore raw

Re: [PATCH 001 of 4] md: Define backing_dev_info.congested_fn for raid0 and linear

Quoted 13 lines · On Monday August 28, [email protected] wrote:
Quoted 10 lines · On Tue, 29 Aug 2006 15:39:24 +1000 NeilBrown <[email protected]> wrote:

Each backing_dev needs to be able to report whether it is congested, either by modulating BDI_*_congested in ->state, or by defining a ->congested_fn.
md/raid did neither of these. This patch add a congested_fn which simply checks all component devices to see if they are congested.

Signed-off-by Neil Brown <[email protected]>
Quoted 12 lines

drivers/md/linear.c static int linear_issue_flush(request_qu · line 122

Quoted 9 lines
drivers/md/linear.c +9 -0
+static int linear_congested(void *data, int bits)
+{
+	mddev_t *mddev = data;
+	linear_conf_t *conf = mddev_to_conf(mddev);
+	int i, ret = 0;
+
+	for (i = 0; i < mddev->raid_disks && !ret ; i++) {
+		request_queue_t *q = bdev_get_queue(conf->disks[i].rdev->bdev);
+		ret |= bdi_congested(&q->backing_dev_info, bits);

nit: `ret = ' would suffice here.

Yeh.... I was looking at linear_issue_flush, which aborts when ret == 0, and dm_table_any_congested, which 'or's together the results and combining to two. And I kept changing my mind as to which way I wanted to go for linear_congested et.al, and ended up going both ways :-(
I hate indecision. I think. Well, probably I do. Maybe.

NeilBrown

Signature · 1 line
VGER BF report: U 0.5
author
#4 lore raw

PATCH 1/1 md: Define ->congested_fn for raid1, raid10, and multipath

In reply to NeilBrown

raid1, raid10 and multipath don't report their 'congested' status through bdi_*_congested, but should.

This patch adds the appropriate functions which just check the 'congested' status of all active members (with appropriate locking).

raid1 read_balance should be modified to prefer devices where bdi_read_congested returns false. Then we could use the '&' branch rather than the '|' branch. However that should would need some benchmarking first to make sure it is actually a good idea.

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./drivers/md/multipath.c |   24 ++++++++++++++++++++++++
 ./drivers/md/raid1.c     |   28 ++++++++++++++++++++++++++++
 ./drivers/md/raid10.c    |   22 ++++++++++++++++++++++
 3 files changed, 74 insertions(+)
drivers/md/multipath.c +24 -0
diff .prev/drivers/md/multipath.c ./drivers/md/multipath.c
@@ -228,6 +228,28 @@ static int multipath_issue_flush(request
 	rcu_read_unlock();
 	return ret;
 }
+static int multipath_congested(void *data, int bits)
+{
+	mddev_t *mddev = data;
+	multipath_conf_t *conf = mddev_to_conf(mddev);
+	int i, ret = 0;
+
+	rcu_read_lock();
+	for (i = 0; i < mddev->raid_disks ; i++) {
+		mdk_rdev_t *rdev = rcu_dereference(conf->multipaths[i].rdev);
+		if (rdev && !test_bit(Faulty, &rdev->flags)) {
+			request_queue_t *q = bdev_get_queue(rdev->bdev);
+
+			ret |= bdi_congested(&q->backing_dev_info, bits);
+			/* Just like multipath_map, we just check the
+			 * first available device
+			 */
+			break;
+		}
+	}
+	rcu_read_unlock();
+	return ret;
+}
 
 /*
  * Careful, this can execute in IRQ contexts as well!

@@ -509,6 +531,8 @@ static int multipath_run (mddev_t *mddev
 
 	mddev->queue->unplug_fn = multipath_unplug;
 	mddev->queue->issue_flush_fn = multipath_issue_flush;
+	mddev->queue->backing_dev_info.congested_fn = multipath_congested;
+	mddev->queue->backing_dev_info.congested_data = mddev;
 
 	return 0;
drivers/md/raid1.c +28 -0
diff .prev/drivers/md/raid1.c ./drivers/md/raid1.c
@@ -601,6 +601,32 @@ static int raid1_issue_flush(request_que
 	return ret;
 }
 
+static int raid1_congested(void *data, int bits)
+{
+	mddev_t *mddev = data;
+	conf_t *conf = mddev_to_conf(mddev);
+	int i, ret = 0;
+
+	rcu_read_lock();
+	for (i = 0; i < mddev->raid_disks; i++) {
+		mdk_rdev_t *rdev = rcu_dereference(conf->mirrors[i].rdev);
+		if (rdev && !test_bit(Faulty, &rdev->flags)) {
+			request_queue_t *q = bdev_get_queue(rdev->bdev);
+
+			/* Note the '|| 1' - when read_balance prefers
+			 * non-congested targets, it can be removed
+			 */
+			if ((bits & (1<<BDI_write_congested)) || 1)
+				ret |= bdi_congested(&q->backing_dev_info, bits);
+			else
+				ret &= bdi_congested(&q->backing_dev_info, bits);
+		}
+	}
+	rcu_read_unlock();
+	return ret;
+}
+
+
 /* Barriers....
  * Sometimes we need to suspend IO while we do something else,
  * either some resync/recovery, or reconfigure the array.

@@ -1965,6 +1991,8 @@ static int run(mddev_t *mddev)
 
 	mddev->queue->unplug_fn = raid1_unplug;
 	mddev->queue->issue_flush_fn = raid1_issue_flush;
+	mddev->queue->backing_dev_info.congested_fn = raid1_congested;
+	mddev->queue->backing_dev_info.congested_data = mddev;
 
 	return 0;
drivers/md/raid10.c +22 -0
diff .prev/drivers/md/raid10.c ./drivers/md/raid10.c
@@ -648,6 +648,26 @@ static int raid10_issue_flush(request_qu
 	return ret;
 }
 
+static int raid10_congested(void *data, int bits)
+{
+	mddev_t *mddev = data;
+	conf_t *conf = mddev_to_conf(mddev);
+	int i, ret = 0;
+
+	rcu_read_lock();
+	for (i = 0; i < mddev->raid_disks && ret == 0; i++) {
+		mdk_rdev_t *rdev = rcu_dereference(conf->mirrors[i].rdev);
+		if (rdev && !test_bit(Faulty, &rdev->flags)) {
+			request_queue_t *q = bdev_get_queue(rdev->bdev);
+
+			ret |= bdi_congested(&q->backing_dev_info, bits);
+		}
+	}
+	rcu_read_unlock();
+	return ret;
+}
+
+
 /* Barriers....
  * Sometimes we need to suspend IO while we do something else,
  * either some resync/recovery, or reconfigure the array.

@@ -2094,6 +2114,8 @@ static int run(mddev_t *mddev)
 
 	mddev->queue->unplug_fn = raid10_unplug;
 	mddev->queue->issue_flush_fn = raid10_issue_flush;
+	mddev->queue->backing_dev_info.congested_fn = raid10_congested;
+	mddev->queue->backing_dev_info.congested_data = mddev;
 
 	/* Calculate max read-ahead size.
 	 * We need to readahead at least twice a whole stripe....
#5 lore raw

Re: [PATCH 002 of 4] md: Define ->congested_fn for raid1, raid10, and multipath

Since we're all about nits, I'll do my part:

drivers/md/multipath.c static int multipath_issue_flush(request · line 231

Quoted 8 lines

diff .prev/drivers/md/multipath.c ./drivers/md/multipath.c

drivers/md/multipath.c +1 -0
@@ -228,6 +228,28 @@ static int multipath_issue_flush(request
 	rcu_read_unlock();
 	return ret;
 }
+static int multipath_congested(void *data, int bits)

Missing white line.

Quoted 24 lines

diff .prev/drivers/md/raid1.c ./drivers/md/raid1.c

drivers/md/raid1.c +17 -0
@@ -601,6 +601,32 @@ static int raid1_issue_flush(request_que
 	return ret;
 }
 
+static int raid1_congested(void *data, int bits)
+{
+	mddev_t *mddev = data;
+	conf_t *conf = mddev_to_conf(mddev);
+	int i, ret = 0;
+
+	rcu_read_lock();
+	for (i = 0; i < mddev->raid_disks; i++) {
+		mdk_rdev_t *rdev = rcu_dereference(conf->mirrors[i].rdev);
+		if (rdev && !test_bit(Faulty, &rdev->flags)) {
+			request_queue_t *q = bdev_get_queue(rdev->bdev);
+
+			/* Note the '|| 1' - when read_balance prefers
+			 * non-congested targets, it can be removed
+			 */
+			if ((bits & (1<<BDI_write_congested)) || 1)
+				ret |= bdi_congested(&q->backing_dev_info, bits);

drivers/md/raid1.c static int raid1_issue_flush(request_que · line 630

Quoted 10 lines
drivers/md/raid1.c +9 -0
+			else
+				ret &= bdi_congested(&q->backing_dev_info, bits);
+		}
+	}
+	rcu_read_unlock();
+	return ret;
+}
+
+
 /* Barriers....

And one white line too much, but YMMV ;-)

Jan Engelhardt

author
#6 lore raw

PATCH 1/1 md: Add a ->congested_fn function for raid5/6

In reply to NeilBrown

This is very different from other raid levels and all requests go through a 'stripe cache', and it has congestion management already.

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./drivers/md/raid5.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
drivers/md/raid5.c +21 -0
diff .prev/drivers/md/raid5.c ./drivers/md/raid5.c
@@ -2593,6 +2593,24 @@ static int raid5_issue_flush(request_que
 	return ret;
 }
 
+static int raid5_congested(void *data, int bits)
+{
+	mddev_t *mddev = data;
+	raid5_conf_t *conf = mddev_to_conf(mddev);
+
+	/* No difference between reads and writes.  Just check
+	 * how busy the stripe_cache is
+	 */
+	if (conf->inactive_blocked)
+		return 1;
+	if (conf->quiesce)
+		return 1;
+	if (list_empty_careful(&conf->inactive_list))
+		return 1;
+
+	return 0;
+}
+
 static int make_request(request_queue_t *q, struct bio * bi)
 {
 	mddev_t *mddev = q->queuedata;

@@ -3296,6 +3314,9 @@ static int run(mddev_t *mddev)
 
 	mddev->queue->unplug_fn = raid5_unplug_device;
 	mddev->queue->issue_flush_fn = raid5_issue_flush;
+	mddev->queue->backing_dev_info.congested_fn = raid5_congested;
+	mddev->queue->backing_dev_info.congested_data = mddev;
+
 	mddev->array_size =  mddev->size * (conf->previous_raid_disks -
 					    conf->max_degraded);
author
#7 lore raw

PATCH 1/1 md: Make messages about resync/recovery etc more specific.

In reply to NeilBrown

It is possible to request a 'check' of an md/raid array where the whole array is read and consistancies are reported.

This uses the same mechanisms as 'resync' and so reports in the kernel logs that a resync is being started.
This understandably confuses/worries people.

Also the text in /proc/mdstat suggests a 'resync' is happen when it is just a check.

This patch changes those messages to be more specific about what is happening.

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./drivers/md/md.c |   47 +++++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 16 deletions(-)
drivers/md/md.c +31 -16
diff .prev/drivers/md/md.c ./drivers/md/md.c
@@ -4641,9 +4641,11 @@ static void status_resync(struct seq_fil
 	seq_printf(seq, " %s =%3u.%u%% (%llu/%llu)",
 		   (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery)?
 		    "reshape" :
-		      (test_bit(MD_RECOVERY_SYNC, &mddev->recovery) ?
-		       "resync" : "recovery")),
-		      per_milli/10, per_milli % 10,
+		    (test_bit(MD_RECOVERY_CHECK, &mddev->recovery)?
+		     "check" :
+		     (test_bit(MD_RECOVERY_SYNC, &mddev->recovery) ?
+		      "resync" : "recovery"))),
+		   per_milli/10, per_milli % 10,
 		   (unsigned long long) resync,
 		   (unsigned long long) max_blocks);
 

@@ -5032,6 +5034,7 @@ void md_do_sync(mddev_t *mddev)
 	int skipped = 0;
 	struct list_head *rtmp;
 	mdk_rdev_t *rdev;
+	char *desc;
 
 	/* just incase thread restarts... */
 	if (test_bit(MD_RECOVERY_DONE, &mddev->recovery))

@@ -5039,6 +5042,18 @@ void md_do_sync(mddev_t *mddev)
 	if (mddev->ro) /* never try to sync a read-only array */
 		return;
 
+	if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery)) {
+		if (test_bit(MD_RECOVERY_CHECK, &mddev->recovery))
+			desc = "data-check";
+		else if (test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery))
+			desc = "requested-resync";
+		else
+			desc = "resync";
+	} else if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery))
+		desc = "reshape";
+	else
+		desc = "recovery";
+
 	/* we overload curr_resync somewhat here.
 	 * 0 == not engaged in resync at all
 	 * 2 == checking that there is no conflict with another sync

@@ -5082,10 +5097,10 @@ void md_do_sync(mddev_t *mddev)
 				prepare_to_wait(&resync_wait, &wq, TASK_UNINTERRUPTIBLE);
 				if (!kthread_should_stop() &&
 				    mddev2->curr_resync >= mddev->curr_resync) {
-					printk(KERN_INFO "md: delaying resync of %s"
-					       " until %s has finished resync (they"
+					printk(KERN_INFO "md: delaying %s of %s"
+					       " until %s has finished (they"
 					       " share one or more physical units)\n",
-					       mdname(mddev), mdname(mddev2));
+					       desc, mdname(mddev), mdname(mddev2));
 					mddev_put(mddev2);
 					schedule();
 					finish_wait(&resync_wait, &wq);

@@ -5121,12 +5136,12 @@ void md_do_sync(mddev_t *mddev)
 				j = rdev->recovery_offset;
 	}
 
-	printk(KERN_INFO "md: syncing RAID array %s\n", mdname(mddev));
-	printk(KERN_INFO "md: minimum _guaranteed_ reconstruction speed:"
-		" %d KB/sec/disc.\n", speed_min(mddev));
+	printk(KERN_INFO "md: %s of RAID array %s\n", desc, mdname(mddev));
+	printk(KERN_INFO "md: minimum _guaranteed_  speed:"
+		" %d KB/sec/disk.\n", speed_min(mddev));
 	printk(KERN_INFO "md: using maximum available idle IO bandwidth "
-	       "(but not more than %d KB/sec) for reconstruction.\n",
-	       speed_max(mddev));
+	       "(but not more than %d KB/sec) for %s.\n",
+	       speed_max(mddev), desc);
 
 	is_mddev_idle(mddev); /* this also initializes IO event counters */
 

@@ -5152,8 +5167,8 @@ void md_do_sync(mddev_t *mddev)
 
 	if (j>2) {
 		printk(KERN_INFO 
-			"md: resuming recovery of %s from checkpoint.\n",
-			mdname(mddev));
+		       "md: resuming %s of %s from checkpoint.\n",
+		       desc, mdname(mddev));
 		mddev->curr_resync = j;
 	}
 

@@ -5236,7 +5251,7 @@ void md_do_sync(mddev_t *mddev)
 			}
 		}
 	}
-	printk(KERN_INFO "md: %s: sync done.\n",mdname(mddev));
+	printk(KERN_INFO "md: %s: %s done.\n",mdname(mddev), desc);
 	/*
 	 * this also signals 'finished resyncing' to md_stop
 	 */

@@ -5256,8 +5271,8 @@ void md_do_sync(mddev_t *mddev)
 			if (test_bit(MD_RECOVERY_INTR, &mddev->recovery)) {
 				if (mddev->curr_resync >= mddev->recovery_cp) {
 					printk(KERN_INFO
-					       "md: checkpointing recovery of %s.\n",
-					       mdname(mddev));
+					       "md: checkpointing %s of %s.\n",
+					       desc, mdname(mddev));
 					mddev->recovery_cp = mddev->curr_resync;
 				}
 			} else