[PATCH 000 of 4] md: Introduction

[PATCH 000 of 4] md: Introduction

[NeilBrown]

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.

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

[NeilBrown]

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 <neilb@suse.de>

### Diffstat output
 ./drivers/md/linear.c |   15 +++++++++++++++
 ./drivers/md/raid0.c  |   17 +++++++++++++++++
 2 files changed, 32 insertions(+)

diff .prev/drivers/md/linear.c ./drivers/md/linear.c
--- .prev/drivers/md/linear.c	2006-08-29 12:17:10.000000000 +1000
+++ ./drivers/md/linear.c	2006-08-29 14:12:26.000000000 +1000
@@ -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;
 }
 

diff .prev/drivers/md/raid0.c ./drivers/md/raid0.c
--- .prev/drivers/md/raid0.c	2006-08-29 12:50:07.000000000 +1000
+++ ./drivers/md/raid0.c	2006-08-29 14:11:49.000000000 +1000
@@ -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;

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

[Andrew Morton]

On Tue, 29 Aug 2006 15:39:24 +1000
NeilBrown <neilb@suse.de> 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 <neilb@suse.de>
> 
> +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.

> +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.

> +	}
> +	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;

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

[Neil Brown]

On Monday August 28, akpm@osdl.org wrote:
> On Tue, 29 Aug 2006 15:39:24 +1000
> NeilBrown <neilb@suse.de> 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 <neilb@suse.de>
> > 
> > +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

-- 
VGER BF report: U 0.5

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

[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 <neilb@suse.de>

### Diffstat output
 ./drivers/md/multipath.c |   24 ++++++++++++++++++++++++
 ./drivers/md/raid1.c     |   28 ++++++++++++++++++++++++++++
 ./drivers/md/raid10.c    |   22 ++++++++++++++++++++++
 3 files changed, 74 insertions(+)

diff .prev/drivers/md/multipath.c ./drivers/md/multipath.c
--- .prev/drivers/md/multipath.c	2006-08-29 14:52:50.000000000 +1000
+++ ./drivers/md/multipath.c	2006-08-29 14:33:34.000000000 +1000
@@ -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;
 

diff .prev/drivers/md/raid1.c ./drivers/md/raid1.c
--- .prev/drivers/md/raid1.c	2006-08-29 14:52:50.000000000 +1000
+++ ./drivers/md/raid1.c	2006-08-29 14:26:59.000000000 +1000
@@ -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;
 

diff .prev/drivers/md/raid10.c ./drivers/md/raid10.c
--- .prev/drivers/md/raid10.c	2006-08-29 14:50:34.000000000 +1000
+++ ./drivers/md/raid10.c	2006-08-29 14:56:51.000000000 +1000
@@ -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....

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

[Jan Engelhardt]

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

>diff .prev/drivers/md/multipath.c ./drivers/md/multipath.c
>--- .prev/drivers/md/multipath.c	2006-08-29 14:52:50.000000000 +1000
>+++ ./drivers/md/multipath.c	2006-08-29 14:33:34.000000000 +1000
>@@ -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.

>diff .prev/drivers/md/raid1.c ./drivers/md/raid1.c
>--- .prev/drivers/md/raid1.c	2006-08-29 14:52:50.000000000 +1000
>+++ ./drivers/md/raid1.c	2006-08-29 14:26:59.000000000 +1000
>@@ -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....

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


Jan Engelhardt
-- 

[PATCH 003 of 4] md: Add a ->congested_fn function for raid5/6

[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 <neilb@suse.de>

### Diffstat output
 ./drivers/md/raid5.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff .prev/drivers/md/raid5.c ./drivers/md/raid5.c
--- .prev/drivers/md/raid5.c	2006-08-29 14:57:09.000000000 +1000
+++ ./drivers/md/raid5.c	2006-08-29 14:57:05.000000000 +1000
@@ -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);
 

[PATCH 004 of 4] md: Make messages about resync/recovery etc more specific.

[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 <neilb@suse.de>

### Diffstat output
 ./drivers/md/md.c |   47 +++++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff .prev/drivers/md/md.c ./drivers/md/md.c
--- .prev/drivers/md/md.c	2006-08-29 15:22:40.000000000 +1000
+++ ./drivers/md/md.c	2006-08-29 15:23:57.000000000 +1000
@@ -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