[PATCH 0/3] add resync speed control for dm-raid1

[PATCH 0/3] add resync speed control for dm-raid1

[Guangliang Zhao]

Hi,

These patches are used to add resync speed control for dm-raid1. The
second and third patch provide support for user-space tool dmsetup.

Guangliang Zhao (3):
  dm raid1: add resync speed control for dm-raid1
  dm raid1: add interface to set resync speed
  dm raid1: add interface to get resync speed

 drivers/md/dm-raid1.c |   93 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 90 insertions(+), 3 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] dm raid1: add resync speed control for dm-raid1

[Guangliang Zhao]

The IO Performance on the already available lv is very bad
during the initial sync when we create a mirror lv. This
patch add the rate limit for every mirror target to control
resync speed.

Signed-off-by: Guangliang Zhao <gzhao@suse.com>
---
 drivers/md/dm-raid1.c |   30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
index 9bfd057..fb87779 100644
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -13,6 +13,7 @@
 #include <linux/pagemap.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
+#include <linux/ratelimit.h>
 #include <linux/device-mapper.h>
 #include <linux/dm-io.h>
 #include <linux/dm-dirty-log.h>
@@ -26,6 +27,11 @@
 #define DM_RAID1_HANDLE_ERRORS 0x01
 #define errors_handled(p)	((p)->features & DM_RAID1_HANDLE_ERRORS)
 
+/* Default resync interval*/
+#define RESYNC_JIFFIES	HZ
+/* Default max resync speed in KB/s */
+#define DEFAULT_RESYNC_SPEED		(100 << 10)
+
 static DECLARE_WAIT_QUEUE_HEAD(_kmirrord_recovery_stopped);
 
 /*-----------------------------------------------------------------
@@ -69,6 +75,9 @@ struct mirror_set {
 	int log_failure;
 	int leg_failure;
 	atomic_t suspend;
+	/* recovery speed control */
+	struct ratelimit_state ms_rlimit;
+	struct delayed_work resync_waker;
 
 	atomic_t default_mirror;	/* Default mirror */
 
@@ -383,12 +392,18 @@ static void do_recovery(struct mirror_set *ms)
 	/*
 	 * Copy any already quiesced regions.
 	 */
-	while ((reg = dm_rh_recovery_start(ms->rh))) {
+	while (__ratelimit(&ms->ms_rlimit) &&
+			(reg = dm_rh_recovery_start(ms->rh))) {
 		r = recover(ms, reg);
 		if (r)
 			dm_rh_recovery_end(reg, 0);
 	}
 
+	/* Broke off the resync process, so wake up kmirrord later on */
+	if (!ms->in_sync)
+		schedule_delayed_work(&ms->resync_waker,
+				      ms->ms_rlimit.interval);
+
 	/*
 	 * Update the in sync flag.
 	 */
@@ -842,6 +857,14 @@ static void do_mirror(struct work_struct *work)
 	do_failures(ms, &failures);
 }
 
+static void do_resync_wake(struct work_struct *work)
+{
+	struct mirror_set *ms =
+		container_of(work, struct mirror_set, resync_waker.work);
+
+	wakeup_mirrord(ms);
+}
+
 /*-----------------------------------------------------------------
  * Target functions
  *---------------------------------------------------------------*/
@@ -852,6 +875,7 @@ static struct mirror_set *alloc_context(unsigned int nr_mirrors,
 {
 	size_t len;
 	struct mirror_set *ms = NULL;
+	int burst;
 
 	len = sizeof(*ms) + (sizeof(ms->mirror[0]) * nr_mirrors);
 
@@ -875,6 +899,10 @@ static struct mirror_set *alloc_context(unsigned int nr_mirrors,
 	ms->leg_failure = 0;
 	atomic_set(&ms->suspend, 0);
 	atomic_set(&ms->default_mirror, DEFAULT_MIRROR);
+	burst = (DEFAULT_RESYNC_SPEED * (RESYNC_JIFFIES / HZ)) << 1;
+	burst = DIV_ROUND_UP(burst, region_size);
+	ratelimit_state_init(&ms->ms_rlimit, RESYNC_JIFFIES, burst);
+	INIT_DELAYED_WORK(&ms->resync_waker, do_resync_wake);
 
 	ms->read_record_pool = mempool_create_slab_pool(MIN_READ_RECORDS,
 						_dm_raid1_read_record_cache);
-- 
1.7.10.4

[PATCH 2/3] dm raid1: add interface to set resync speed

[Guangliang Zhao]

Add ioctl to control resync speed, userspace tool
is dmsetup message, message format is:
	dmsetup message $device 0 "set $speed"
e.g.
	dmsetup message /dev/dm-2 "set 12345"

Signed-off-by: Guangliang Zhao <gzhao@suse.com>
---
 drivers/md/dm-raid1.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
index fb87779..1da25eb 100644
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -1361,6 +1361,49 @@ static void mirror_resume(struct dm_target *ti)
 }
 
 /*
+ * convert speed into members of ratelimit_state
+ */
+static void speed_to_rlimit(struct mirror_set *ms, struct ratelimit_state *rl,
+			    unsigned int speed)
+{
+	sector_t region_size = dm_rh_get_region_size(ms->rh);
+
+	rl->interval = RESYNC_JIFFIES;
+	rl->burst = (speed * (RESYNC_JIFFIES / HZ)) << 1;
+	rl->burst = DIV_ROUND_UP(rl->burst, region_size);
+}
+
+/*
+ * Message interface
+ *	set speed(KB/s)
+ */
+static int mirror_message(struct dm_target *ti, unsigned argc, char **argv)
+{
+	struct mirror_set *ms = ti->private;
+	unsigned int speed;
+	int ret = 0;
+
+	if (!strcasecmp(argv[0], "set")) {
+		if (sscanf(argv[1], "%u", &speed) != 1) {
+			DMWARN("invalid speed parameter %s", argv[1]);
+			ret = -EINVAL;
+			goto error;
+		}
+
+		speed_to_rlimit(ms, &ms->ms_rlimit, speed);
+		goto out;
+	} else {
+		ret = -EINVAL;
+		goto error;
+	}
+
+error:
+	DMWARN("unrecognised message received.");
+out:
+	return ret;
+}
+
+/*
  * device_status_char
  * @m: mirror device/leg we want the status of
  *
@@ -1450,6 +1493,7 @@ static struct target_type mirror_target = {
 	.presuspend = mirror_presuspend,
 	.postsuspend = mirror_postsuspend,
 	.resume	 = mirror_resume,
+	.message = mirror_message,
 	.status	 = mirror_status,
 	.iterate_devices = mirror_iterate_devices,
 };
-- 
1.7.10.4

[PATCH 3/3] dm raid1: add interface to get resync speed

[Guangliang Zhao]

Add ioctl to get resync speed, userspace tool
is dmsetup status:
	dmsetup status $device
e.g.
	dmsetup status /dev/dm-2

Signed-off-by: Guangliang Zhao <gzhao@suse.com>
---
 drivers/md/dm-raid1.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
index 1da25eb..b7bd93a 100644
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -1427,6 +1427,20 @@ static char device_status_char(struct mirror *m)
 		(test_bit(DM_RAID1_READ_ERROR, &(m->error_type))) ? 'R' : 'U';
 }
 
+/*
+ * get speed from ratelimit_state
+ */
+static unsigned int rlimit_to_speed(struct mirror_set *ms,
+				    struct ratelimit_state *rl)
+{
+	sector_t region_size = dm_rh_get_region_size(ms->rh);
+	unsigned int time, burst;
+
+	time = rl->interval / HZ;
+	burst = (rl->burst * region_size) >> 1;
+
+	return burst / time;
+}
 
 static int mirror_status(struct dm_target *ti, status_type_t type,
 			 char *result, unsigned int maxlen)
@@ -1445,9 +1459,10 @@ static int mirror_status(struct dm_target *ti, status_type_t type,
 		}
 		buffer[m] = '\0';
 
-		DMEMIT("%llu/%llu 1 %s ",
+		DMEMIT("%llu/%llu 1 %u 1 %s ",
 		      (unsigned long long)log->type->get_sync_count(log),
-		      (unsigned long long)ms->nr_regions, buffer);
+		      (unsigned long long)ms->nr_regions,
+		      rlimit_to_speed(ms, &ms->ms_rlimit), buffer);
 
 		sz += log->type->status(log, type, result+sz, maxlen-sz);
 
-- 
1.7.10.4

[PATCH 0/3] add resync speed control for dm-raid1

[Guangliang Zhao]

Hi,

These patches are used to add resync speed control for dm-raid1. The
second and third patch provide support for user-space tool dmsetup.

Guangliang Zhao (3):
  dm raid1: add resync speed control for dm-raid1
  dm raid1: add interface to set resync speed
  dm raid1: add interface to get resync speed

 drivers/md/dm-raid1.c |   93 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 90 insertions(+), 3 deletions(-)

-- 
1.7.10.4

Re: [dm-devel] [PATCH 2/3] dm raid1: add interface to set resync speed

[Lars Marowsky-Bree]

On 2012-11-22T14:27:52, Guangliang Zhao <gzhao@suse.com> wrote:

Hi Guangliang,

thanks for adding this. I think this approach is a good direction to
take, just one feedback:

> Add ioctl to control resync speed, userspace tool
> is dmsetup message, message format is:
> 	dmsetup message $device 0 "set $speed"
> e.g.
> 	dmsetup message /dev/dm-2 "set 12345"

I think this should be "set-max-resync-rate" or something; "set" is very
generic and not very extensible going forward, should the need arise.

Regards,
    Lars

-- 
Architect Storage/HA
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 21284 (AG Nürnberg)
"Experience is the name everyone gives to their mistakes." -- Oscar Wilde

Re: [dm-devel] [PATCH 2/3] dm raid1: add interface to set resync speed

[Guangliang Zhao]

On Fri, Nov 30, 2012 at 12:39:28PM +0100, Lars Marowsky-Bree wrote:
> On 2012-11-22T14:27:52, Guangliang Zhao <gzhao@suse.com> wrote:
> 
> Hi Guangliang,
> 
> thanks for adding this. I think this approach is a good direction to
> take, just one feedback:
> 
> > Add ioctl to control resync speed, userspace tool
> > is dmsetup message, message format is:
> > 	dmsetup message $device 0 "set $speed"
> > e.g.
> > 	dmsetup message /dev/dm-2 "set 12345"
> 
> I think this should be "set-max-resync-rate" or something; "set" is very
> generic and not very extensible going forward, should the need arise.
That's would be more clear, thanks.

> 
> Regards,
>     Lars
> 
> -- 
> Architect Storage/HA
> SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 21284 (AG Nürnberg)
> "Experience is the name everyone gives to their mistakes." -- Oscar Wilde
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

-- 
Best regards,
Guangliang

Re: [dm-devel] [PATCH 0/3] add resync speed control for dm-raid1

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

On Thu, 22 Nov 2012 14:27:50 +0800 Guangliang Zhao <gzhao@suse.com> wrote:

> Hi,
> 
> These patches are used to add resync speed control for dm-raid1. The
> second and third patch provide support for user-space tool dmsetup.
> 
> Guangliang Zhao (3):
>   dm raid1: add resync speed control for dm-raid1
>   dm raid1: add interface to set resync speed
>   dm raid1: add interface to get resync speed
> 
>  drivers/md/dm-raid1.c |   93 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 90 insertions(+), 3 deletions(-)
> 

The problem with this approach is that it slows down resync even when there
is no other IO happening.
If that is deemed to be acceptable, then the patch set seems fine, though I
would probably make the default a lot higher so as not to change current
default behaviour for anyone.

If it isn't acceptable, then you either need to monitor the number of
requests going to the underlying devices - like md does - or monitor the
number of requests coming in to the dm-raid1 target - which is probably
easier with dm.

i.e. only impose the rate limit if there have been any requests for the
dm-raid1 target in the last 'RESYNC_JIFFIES'.

What do you think of that?

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [dm-devel] [PATCH 0/3] add resync speed control for dm-raid1

[Guangliang Zhao]

On Mon, Dec 10, 2012 at 01:21:23PM +1100, NeilBrown wrote:
> On Thu, 22 Nov 2012 14:27:50 +0800 Guangliang Zhao <gzhao@suse.com> wrote:
> 
> > Hi,
> > 
> > These patches are used to add resync speed control for dm-raid1. The
> > second and third patch provide support for user-space tool dmsetup.
> > 
> > Guangliang Zhao (3):
> >   dm raid1: add resync speed control for dm-raid1
> >   dm raid1: add interface to set resync speed
> >   dm raid1: add interface to get resync speed
> > 
> >  drivers/md/dm-raid1.c |   93 +++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 90 insertions(+), 3 deletions(-)
> > 
> 
> The problem with this approach is that it slows down resync even when there
> is no other IO happening.
> If that is deemed to be acceptable, then the patch set seems fine, though I
> would probably make the default a lot higher so as not to change current
> default behaviour for anyone.
Yes, the users who don't need this feature will not be affected, fully
accepted.

> 
> If it isn't acceptable, then you either need to monitor the number of
> requests going to the underlying devices - like md does - or monitor the
> number of requests coming in to the dm-raid1 target - which is probably
> easier with dm.
> 
> i.e. only impose the rate limit if there have been any requests for the
> dm-raid1 target in the last 'RESYNC_JIFFIES'.
Good idea, that's would be on my TODO list. But anyway these patches are
still required.

> 
> What do you think of that?
> 
> NeilBrown



> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel


-- 
Best regards,
Guangliang

Re: [PATCH 0/3] add resync speed control for dm-raid1

[Lars Marowsky-Bree]

On 2012-12-10T13:21:23, NeilBrown <neilb@suse.de> wrote:

> The problem with this approach is that it slows down resync even when there
> is no other IO happening.
> If that is deemed to be acceptable, then the patch set seems fine, though I
> would probably make the default a lot higher so as not to change current
> default behaviour for anyone.

I agree to the latter part.

The difficulty is that our primary use case here is preventing IO
starvation while cluster raid is resyncing; and we don't know the IO
load on other nodes, or what other LVs might inflict on the same backend
store / PV. Hence, a static limit probably is the easiest way to
start.

I agree that a more dynamic approach would be desirable, but that
appears to be very complex to get right.


Thanks,
    Lars

-- 
Architect Storage/HA
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 21284 (AG Nürnberg)
"Experience is the name everyone gives to their mistakes." -- Oscar Wilde

Re: [dm-devel] [PATCH 0/3] add resync speed control for dm-raid1

[Lars Marowsky-Bree]

On 2012-12-10T13:21:23, NeilBrown <neilb@suse.de> wrote:

> The problem with this approach is that it slows down resync even when there
> is no other IO happening.
> If that is deemed to be acceptable, then the patch set seems fine, though I
> would probably make the default a lot higher so as not to change current
> default behaviour for anyone.

I agree to the latter part.

The difficulty is that our primary use case here is preventing IO
starvation while cluster raid is resyncing; and we don't know the IO
load on other nodes, or what other LVs might inflict on the same backend
store / PV. Hence, a static limit probably is the easiest way to
start.

I agree that a more dynamic approach would be desirable, but that
appears to be very complex to get right.


Thanks,
    Lars

-- 
Architect Storage/HA
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 21284 (AG Nürnberg)
"Experience is the name everyone gives to their mistakes." -- Oscar Wilde