Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   Device-mapper Development (http://www.linux-archive.org/device-mapper-development/)
-   -   dm-kcopyd: introduce per-module throttle structure (http://www.linux-archive.org/device-mapper-development/533422-dm-kcopyd-introduce-per-module-throttle-structure.html)

Ankit Jain 06-01-2011 06:18 AM

dm-kcopyd: introduce per-module throttle structure
 
Just some comments, inline -

On 06/01/2011 03:33 AM, Mikulas Patocka wrote:
> Hi
>
> Here I'm sending new kcopyd throttling patches that allow the throttle to
> be set per module (i.e. one throttle for mirror and the other for
> snapshots).
>
> Mikulas
>
> ---
>
> dm-kcopyd: introduce per-module throttle structure
>
> The structure contains the throttle parameter (it could be set in
> /sys/module/*/parameters and auxulary variables for activity counting.
>
> The throttle does nothing, it will be activated in the next patch.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
>
> ---
> drivers/md/dm-kcopyd.c | 2 +-
> drivers/md/dm-raid1.c | 5 ++++-
> drivers/md/dm-snap.c | 5 ++++-
> include/linux/dm-kcopyd.h | 15 ++++++++++++++-
> 4 files changed, 23 insertions(+), 4 deletions(-)
>
> Index: linux-2.6.39-fast/drivers/md/dm-kcopyd.c
> ================================================== =================
> --- linux-2.6.39-fast.orig/drivers/md/dm-kcopyd.c 2011-05-31 23:46:18.000000000 +0200
> +++ linux-2.6.39-fast/drivers/md/dm-kcopyd.c 2011-05-31 23:49:47.000000000 +0200
> @@ -617,7 +617,7 @@ int kcopyd_cancel(struct kcopyd_job *job
> /*-----------------------------------------------------------------
> * Client setup
> *---------------------------------------------------------------*/
> -struct dm_kcopyd_client *dm_kcopyd_client_create(void)
> +struct dm_kcopyd_client *dm_kcopyd_client_create(struct dm_kcopyd_throttle *throttle)

IMHO, the other patch should be the first one, and this change should be
part of it. Basically, the first patch should add throttle support and
the related infrastructure for using it, and the second one can add
*usage/declaration* of those throttles in raid1/snapshot etc.

[...]
> {
> int r = -ENOMEM;
> struct dm_kcopyd_client *kc;
> Index: linux-2.6.39-fast/drivers/md/dm-raid1.c
> ================================================== =================
> --- linux-2.6.39-fast.orig/drivers/md/dm-raid1.c 2011-05-31 23:46:18.000000000 +0200
> +++ linux-2.6.39-fast/drivers/md/dm-raid1.c 2011-05-31 23:46:22.000000000 +0200
> @@ -83,6 +83,9 @@ struct mirror_set {
> struct mirror mirror[0];
> };
>
> +dm_kcopyd_throttle_declare(raid1_resync_throttle,
> + "A percentage of time allocated for raid resynchronization");
> +
> static void wakeup_mirrord(void *context)
> {
> struct mirror_set *ms = context;
> @@ -1115,7 +1118,7 @@ static int mirror_ctr(struct dm_target *
> goto err_destroy_wq;
> }
>
> - ms->kcopyd_client = dm_kcopyd_client_create();
> + ms->kcopyd_client = dm_kcopyd_client_create(&dm_kcopyd_throttle);

IMHO, its not very clear where the dm_kcopyd_throttle variable here,
comes from, until you find the definition for dm_kcopyd_throttle_declare.

> if (IS_ERR(ms->kcopyd_client)) {
> r = PTR_ERR(ms->kcopyd_client);
> goto err_destroy_wq;
> Index: linux-2.6.39-fast/drivers/md/dm-snap.c
> ================================================== =================
> --- linux-2.6.39-fast.orig/drivers/md/dm-snap.c 2011-05-31 23:46:18.000000000 +0200
> +++ linux-2.6.39-fast/drivers/md/dm-snap.c 2011-05-31 23:46:22.000000000 +0200
> @@ -135,6 +135,9 @@ struct dm_snapshot {
> #define RUNNING_MERGE 0
> #define SHUTDOWN_MERGE 1
>
> +dm_kcopyd_throttle_declare(snapshot_copy_throttle ,
> + "A percentage of time allocated for copy on write");
> +
> struct dm_dev *dm_snap_origin(struct dm_snapshot *s)
> {
> return s->origin;
> @@ -1111,7 +1114,7 @@ static int snapshot_ctr(struct dm_target
> goto bad_hash_tables;
> }
>
> - s->kcopyd_client = dm_kcopyd_client_create();
> + s->kcopyd_client = dm_kcopyd_client_create(&dm_kcopyd_throttle);
> if (IS_ERR(s->kcopyd_client)) {
> r = PTR_ERR(s->kcopyd_client);
> ti->error = "Could not create kcopyd client";
> Index: linux-2.6.39-fast/include/linux/dm-kcopyd.h
> ================================================== =================
> --- linux-2.6.39-fast.orig/include/linux/dm-kcopyd.h 2011-05-31 23:46:18.000000000 +0200
> +++ linux-2.6.39-fast/include/linux/dm-kcopyd.h 2011-05-31 23:46:22.000000000 +0200
> @@ -21,11 +21,24 @@
>
> #define DM_KCOPYD_IGNORE_ERROR 1
>
> +struct dm_kcopyd_throttle {
> + unsigned throttle;
> + unsigned long num_io_jobs;
> + unsigned io_period;
> + unsigned total_period;
> + unsigned last_jiffies;
> +};
> +
> +#define dm_kcopyd_throttle_declare(name, description)
> +static struct dm_kcopyd_throttle dm_kcopyd_throttle = { 100, 0, 0, 0, 0 };
> +module_param_named(name, dm_kcopyd_throttle.throttle, uint, 0644);
> +MODULE_PARM_DESC(name, description)
> +

I'm just trying to understand, how is it determined, when to use macros
with UPPER_CASE_LETTERS and when otherwise. To me, UPPER_CASE_LETTERS
makes it very clear that I'm using a macro, and since this seems to be
doing declaration etc also, that would seem helpful. But I don't know
the general style or reasoning followed in the kernel, hence the question.

> /*
> * To use kcopyd you must first create a dm_kcopyd_client object.
> */
> struct dm_kcopyd_client;
> -struct dm_kcopyd_client *dm_kcopyd_client_create(void);
> +struct dm_kcopyd_client *dm_kcopyd_client_create(struct dm_kcopyd_throttle *throttle);
> void dm_kcopyd_client_destroy(struct dm_kcopyd_client *kc);
>
> /*
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>

Thanks,
--
Ankit Jain
SUSE Labs

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

Ankit Jain 06-01-2011 07:13 AM

dm-kcopyd: introduce per-module throttle structure
 
On 06/01/2011 11:48 AM, Ankit Jain wrote:
>> +
>> +#define dm_kcopyd_throttle_declare(name, description)
>> +static struct dm_kcopyd_throttle dm_kcopyd_throttle = { 100, 0, 0, 0, 0 };
>> +module_param_named(name, dm_kcopyd_throttle.throttle, uint, 0644);
>> +MODULE_PARM_DESC(name, description)
>> +
>
> I'm just trying to understand, how is it determined, when to use macros
> with UPPER_CASE_LETTERS and when otherwise. To me, UPPER_CASE_LETTERS
> makes it very clear that I'm using a macro, and since this seems to be
> doing declaration etc also, that would seem helpful. But I don't know
> the general style or reasoning followed in the kernel, hence the question.
>

According to Documentation/CodingStyle, a macro can be named in lower
case, if it resembles a function, which it doesn't in this case. Um and
maybe name it like DECLARE_.. ?

Thanks,
--
Ankit Jain
SUSE Labs

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

Joe Thornber 06-01-2011 09:51 AM

dm-kcopyd: introduce per-module throttle structure
 
On Tue, May 31, 2011 at 06:03:39PM -0400, Mikulas Patocka wrote:
> Hi
>
> Here I'm sending new kcopyd throttling patches that allow the throttle to
> be set per module (i.e. one throttle for mirror and the other for
> snapshots).

I'm sorry Mikulas, but these patches still really worry me.

i) Your throttling is time based, rather than throughput.

ii) No account is taken of the source and destination devices. If I
have an idle mirror, I would like the resyncing of a new leg to go at
100% speed. Other mirrors in the system may be under load so should
resync in a more sympathetic manner. Should we be using the
congestion functions to see if the source or destination are too busy?

iii) calling msleep in kernel code really makes me shudder, especially
when you call it with a fixed, small duration and then loop. Can't
you work out how long you should sleep for? Why not defer the worker
thread until this time? (i.e. use queue_delayed_work()).

iv) you haven't explained how the sys admin works out the correct
throttle value. If we write this down then maybe we can think about
automating it.

- Joe

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

Mikulas Patocka 06-02-2011 07:16 PM

dm-kcopyd: introduce per-module throttle structure
 
On Wed, 1 Jun 2011, Ankit Jain wrote:

> Just some comments, inline -
>
> On 06/01/2011 03:33 AM, Mikulas Patocka wrote:
> > Hi
> >
> > Here I'm sending new kcopyd throttling patches that allow the throttle to
> > be set per module (i.e. one throttle for mirror and the other for
> > snapshots).
> >
> > Mikulas
> >
> > ---
> >
> > dm-kcopyd: introduce per-module throttle structure
> >
> > The structure contains the throttle parameter (it could be set in
> > /sys/module/*/parameters and auxulary variables for activity counting.
> >
> > The throttle does nothing, it will be activated in the next patch.
> >
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> >
> > ---
> > drivers/md/dm-kcopyd.c | 2 +-
> > drivers/md/dm-raid1.c | 5 ++++-
> > drivers/md/dm-snap.c | 5 ++++-
> > include/linux/dm-kcopyd.h | 15 ++++++++++++++-
> > 4 files changed, 23 insertions(+), 4 deletions(-)
> >
> > Index: linux-2.6.39-fast/drivers/md/dm-kcopyd.c
> > ================================================== =================
> > --- linux-2.6.39-fast.orig/drivers/md/dm-kcopyd.c 2011-05-31 23:46:18.000000000 +0200
> > +++ linux-2.6.39-fast/drivers/md/dm-kcopyd.c 2011-05-31 23:49:47.000000000 +0200
> > @@ -617,7 +617,7 @@ int kcopyd_cancel(struct kcopyd_job *job
> > /*-----------------------------------------------------------------
> > * Client setup
> > *---------------------------------------------------------------*/
> > -struct dm_kcopyd_client *dm_kcopyd_client_create(void)
> > +struct dm_kcopyd_client *dm_kcopyd_client_create(struct dm_kcopyd_throttle *throttle)
>
> IMHO, the other patch should be the first one, and this change should be
> part of it. Basically, the first patch should add throttle support and
> the related infrastructure for using it, and the second one can add
> *usage/declaration* of those throttles in raid1/snapshot etc.
>
> [...]
> > {
> > int r = -ENOMEM;
> > struct dm_kcopyd_client *kc;
> > Index: linux-2.6.39-fast/drivers/md/dm-raid1.c
> > ================================================== =================
> > --- linux-2.6.39-fast.orig/drivers/md/dm-raid1.c 2011-05-31 23:46:18.000000000 +0200
> > +++ linux-2.6.39-fast/drivers/md/dm-raid1.c 2011-05-31 23:46:22.000000000 +0200
> > @@ -83,6 +83,9 @@ struct mirror_set {
> > struct mirror mirror[0];
> > };
> >
> > +dm_kcopyd_throttle_declare(raid1_resync_throttle,
> > + "A percentage of time allocated for raid resynchronization");
> > +
> > static void wakeup_mirrord(void *context)
> > {
> > struct mirror_set *ms = context;
> > @@ -1115,7 +1118,7 @@ static int mirror_ctr(struct dm_target *
> > goto err_destroy_wq;
> > }
> >
> > - ms->kcopyd_client = dm_kcopyd_client_create();
> > + ms->kcopyd_client = dm_kcopyd_client_create(&dm_kcopyd_throttle);
>
> IMHO, its not very clear where the dm_kcopyd_throttle variable here,
> comes from, until you find the definition for dm_kcopyd_throttle_declare.
>
> > if (IS_ERR(ms->kcopyd_client)) {
> > r = PTR_ERR(ms->kcopyd_client);
> > goto err_destroy_wq;
> > Index: linux-2.6.39-fast/drivers/md/dm-snap.c
> > ================================================== =================
> > --- linux-2.6.39-fast.orig/drivers/md/dm-snap.c 2011-05-31 23:46:18.000000000 +0200
> > +++ linux-2.6.39-fast/drivers/md/dm-snap.c 2011-05-31 23:46:22.000000000 +0200
> > @@ -135,6 +135,9 @@ struct dm_snapshot {
> > #define RUNNING_MERGE 0
> > #define SHUTDOWN_MERGE 1
> >
> > +dm_kcopyd_throttle_declare(snapshot_copy_throttle ,
> > + "A percentage of time allocated for copy on write");
> > +
> > struct dm_dev *dm_snap_origin(struct dm_snapshot *s)
> > {
> > return s->origin;
> > @@ -1111,7 +1114,7 @@ static int snapshot_ctr(struct dm_target
> > goto bad_hash_tables;
> > }
> >
> > - s->kcopyd_client = dm_kcopyd_client_create();
> > + s->kcopyd_client = dm_kcopyd_client_create(&dm_kcopyd_throttle);
> > if (IS_ERR(s->kcopyd_client)) {
> > r = PTR_ERR(s->kcopyd_client);
> > ti->error = "Could not create kcopyd client";
> > Index: linux-2.6.39-fast/include/linux/dm-kcopyd.h
> > ================================================== =================
> > --- linux-2.6.39-fast.orig/include/linux/dm-kcopyd.h 2011-05-31 23:46:18.000000000 +0200
> > +++ linux-2.6.39-fast/include/linux/dm-kcopyd.h 2011-05-31 23:46:22.000000000 +0200
> > @@ -21,11 +21,24 @@
> >
> > #define DM_KCOPYD_IGNORE_ERROR 1
> >
> > +struct dm_kcopyd_throttle {
> > + unsigned throttle;
> > + unsigned long num_io_jobs;
> > + unsigned io_period;
> > + unsigned total_period;
> > + unsigned last_jiffies;
> > +};
> > +
> > +#define dm_kcopyd_throttle_declare(name, description)
> > +static struct dm_kcopyd_throttle dm_kcopyd_throttle = { 100, 0, 0, 0, 0 };
> > +module_param_named(name, dm_kcopyd_throttle.throttle, uint, 0644);
> > +MODULE_PARM_DESC(name, description)
> > +
>
> I'm just trying to understand, how is it determined, when to use macros
> with UPPER_CASE_LETTERS and when otherwise. To me, UPPER_CASE_LETTERS
> makes it very clear that I'm using a macro, and since this seems to be
> doing declaration etc also, that would seem helpful. But I don't know
> the general style or reasoning followed in the kernel, hence the question.
>
> > /*
> > * To use kcopyd you must first create a dm_kcopyd_client object.
> > */
> > struct dm_kcopyd_client;
> > -struct dm_kcopyd_client *dm_kcopyd_client_create(void);
> > +struct dm_kcopyd_client *dm_kcopyd_client_create(struct dm_kcopyd_throttle *throttle);
> > void dm_kcopyd_client_destroy(struct dm_kcopyd_client *kc);
> >
> > /*
> >
> > --
> > dm-devel mailing list
> > dm-devel@redhat.com
> > https://www.redhat.com/mailman/listinfo/dm-devel
> >
>
> Thanks,
> --
> Ankit Jain
> SUSE Labs

Hi

This is the updated patch with macro name in capital letters.

I think the order of patches doesn't matter much, the result will be the
same anyway.

Mikulas

---

dm-kcopyd: introduce per-module throttle structure

The structure contains the throttle parameter (it could be set in
/sys/module/*/parameters and auxulary variables for activity counting.

The throttle does nothing, it will be activated in the next patch.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
drivers/md/dm-kcopyd.c | 2 +-
drivers/md/dm-raid1.c | 5 ++++-
drivers/md/dm-snap.c | 5 ++++-
include/linux/dm-kcopyd.h | 15 ++++++++++++++-
4 files changed, 23 insertions(+), 4 deletions(-)

Index: linux-2.6.39-fast/drivers/md/dm-kcopyd.c
================================================== =================
--- linux-2.6.39-fast.orig/drivers/md/dm-kcopyd.c 2011-05-31 23:46:18.000000000 +0200
+++ linux-2.6.39-fast/drivers/md/dm-kcopyd.c 2011-05-31 23:49:47.000000000 +0200
@@ -617,7 +617,7 @@ int kcopyd_cancel(struct kcopyd_job *job
/*-----------------------------------------------------------------
* Client setup
*---------------------------------------------------------------*/
-struct dm_kcopyd_client *dm_kcopyd_client_create(void)
+struct dm_kcopyd_client *dm_kcopyd_client_create(struct dm_kcopyd_throttle *throttle)
{
int r = -ENOMEM;
struct dm_kcopyd_client *kc;
Index: linux-2.6.39-fast/drivers/md/dm-raid1.c
================================================== =================
--- linux-2.6.39-fast.orig/drivers/md/dm-raid1.c 2011-05-31 23:46:18.000000000 +0200
+++ linux-2.6.39-fast/drivers/md/dm-raid1.c 2011-05-31 23:46:22.000000000 +0200
@@ -83,6 +83,9 @@ struct mirror_set {
struct mirror mirror[0];
};

+DECLARE_DM_KCOPYD_THROTTLE(raid1_resync_throttle,
+ "A percentage of time allocated for raid resynchronization");
+
static void wakeup_mirrord(void *context)
{
struct mirror_set *ms = context;
@@ -1115,7 +1118,7 @@ static int mirror_ctr(struct dm_target *
goto err_destroy_wq;
}

- ms->kcopyd_client = dm_kcopyd_client_create();
+ ms->kcopyd_client = dm_kcopyd_client_create(&dm_kcopyd_throttle);
if (IS_ERR(ms->kcopyd_client)) {
r = PTR_ERR(ms->kcopyd_client);
goto err_destroy_wq;
Index: linux-2.6.39-fast/drivers/md/dm-snap.c
================================================== =================
--- linux-2.6.39-fast.orig/drivers/md/dm-snap.c 2011-05-31 23:46:18.000000000 +0200
+++ linux-2.6.39-fast/drivers/md/dm-snap.c 2011-05-31 23:46:22.000000000 +0200
@@ -135,6 +135,9 @@ struct dm_snapshot {
#define RUNNING_MERGE 0
#define SHUTDOWN_MERGE 1

+DECLARE_DM_KCOPYD_THROTTLE(snapshot_copy_throttle ,
+ "A percentage of time allocated for copy on write");
+
struct dm_dev *dm_snap_origin(struct dm_snapshot *s)
{
return s->origin;
@@ -1111,7 +1114,7 @@ static int snapshot_ctr(struct dm_target
goto bad_hash_tables;
}

- s->kcopyd_client = dm_kcopyd_client_create();
+ s->kcopyd_client = dm_kcopyd_client_create(&dm_kcopyd_throttle);
if (IS_ERR(s->kcopyd_client)) {
r = PTR_ERR(s->kcopyd_client);
ti->error = "Could not create kcopyd client";
Index: linux-2.6.39-fast/include/linux/dm-kcopyd.h
================================================== =================
--- linux-2.6.39-fast.orig/include/linux/dm-kcopyd.h 2011-05-31 23:46:18.000000000 +0200
+++ linux-2.6.39-fast/include/linux/dm-kcopyd.h 2011-05-31 23:46:22.000000000 +0200
@@ -21,11 +21,24 @@

#define DM_KCOPYD_IGNORE_ERROR 1

+struct dm_kcopyd_throttle {
+ unsigned throttle;
+ unsigned long num_io_jobs;
+ unsigned io_period;
+ unsigned total_period;
+ unsigned last_jiffies;
+};
+
+#define DECLARE_DM_KCOPYD_THROTTLE(name, description)
+static struct dm_kcopyd_throttle dm_kcopyd_throttle = { 100, 0, 0, 0, 0 };
+module_param_named(name, dm_kcopyd_throttle.throttle, uint, 0644);
+MODULE_PARM_DESC(name, description)
+
/*
* To use kcopyd you must first create a dm_kcopyd_client object.
*/
struct dm_kcopyd_client;
-struct dm_kcopyd_client *dm_kcopyd_client_create(void);
+struct dm_kcopyd_client *dm_kcopyd_client_create(struct dm_kcopyd_throttle *throttle);
void dm_kcopyd_client_destroy(struct dm_kcopyd_client *kc);

/*

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

Mikulas Patocka 06-02-2011 07:55 PM

dm-kcopyd: introduce per-module throttle structure
 
On Wed, 1 Jun 2011, Joe Thornber wrote:

> On Tue, May 31, 2011 at 06:03:39PM -0400, Mikulas Patocka wrote:
> > Hi
> >
> > Here I'm sending new kcopyd throttling patches that allow the throttle to
> > be set per module (i.e. one throttle for mirror and the other for
> > snapshots).
>
> I'm sorry Mikulas, but these patches still really worry me.
>
> i) Your throttling is time based, rather than throughput.

It is impossible to determine what throughput the underlying device has.

> ii) No account is taken of the source and destination devices. If I
> have an idle mirror, I would like the resyncing of a new leg to go at
> 100% speed. Other mirrors in the system may be under load so should
> resync in a more sympathetic manner. Should we be using the
> congestion functions to see if the source or destination are too busy?

You don't know how much I/O is there on the underlying device.

If you want this kind of control, ask the people who develop the I/O
scheduler.

> iii) calling msleep in kernel code really makes me shudder, especially
> when you call it with a fixed, small duration and then loop. Can't
> you work out how long you should sleep for?

I tried to sleep shorter time more often, but it didn't work. If you sleep
10 times for 10ms, it doesn't limit disk throughput much as if you sleep
once for 100ms. I don't know why, but smaller sleeps don't work (a
possible reason could be readahead/writeback cache in the disk, but if I
disabled it setting WCE=0, RCD=1, smaller sleeps still didn't limit).

> Why not defer the worker
> thread until this time? (i.e. use queue_delayed_work()).

Because msleep is simpler.

> iv) you haven't explained how the sys admin works out the correct
> throttle value.

There is no "correct" value. The "correct" value depends on how important
is copying itself v.s. other i/o.

> If we write this down then maybe we can think about automating it.

In theory (if disk scheduler were perfect), we wouldn't need any
throttling. The disk scheduler should recognize that the kcopyd process is
sending way more requests than any other process and should lower the
i/o priority of kcopyd process.

In practice, the disk scheduler doesn't do it well, so kcopyd hurts the
users. If you want an automated fix, fix the disk scheduler. But don't put
disk scheduler logic into device mapper --- it dosn't belong there.

Mikulas

> - Joe

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

Joe Thornber 06-03-2011 11:01 AM

dm-kcopyd: introduce per-module throttle structure
 
On Thu, Jun 02, 2011 at 03:55:16PM -0400, Mikulas Patocka wrote:
> > iv) you haven't explained how the sys admin works out the correct
> > throttle value.
>
> There is no "correct" value. The "correct" value depends on how important
> is copying itself v.s. other i/o.

So who is going to set this? Do you really have no advice for them
beyond 'there is no correct value'?

> In theory (if disk scheduler were perfect), we wouldn't need any
> throttling. The disk scheduler should recognize that the kcopyd process is
> sending way more requests than any other process and should lower the
> i/o priority of kcopyd process.
>
> In practice, the disk scheduler doesn't do it well, so kcopyd hurts the
> users. If you want an automated fix, fix the disk scheduler. But don't put
> disk scheduler logic into device mapper --- it dosn't belong there.

I totally agree with these two paragraphs. Any throttling you add to
kcopyd is always going to be a hack.

- Joe

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

Mikulas Patocka 06-07-2011 05:50 PM

dm-kcopyd: introduce per-module throttle structure
 
On Fri, 3 Jun 2011, Joe Thornber wrote:

> On Thu, Jun 02, 2011 at 03:55:16PM -0400, Mikulas Patocka wrote:
> > > iv) you haven't explained how the sys admin works out the correct
> > > throttle value.
> >
> > There is no "correct" value. The "correct" value depends on how important
> > is copying itself v.s. other i/o.
>
> So who is going to set this? Do you really have no advice for them
> beyond 'there is no correct value'?

I did some tests. I did some random and sequential I/O on the same disk
while mirror was resynchronizing. The result is this:

random read:
mirror throttle / IOs per second
100% 94
75% 113
50% 127
25% 145
idle 160

sequential read:
mirror throttle / throughput MB/s
100% 104
75% 118
50% 122
25% 127
idle 134

Mikulas

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

Joe Thornber 06-09-2011 09:47 AM

dm-kcopyd: introduce per-module throttle structure
 
Mikulas,

On Tue, Jun 07, 2011 at 01:50:16PM -0400, Mikulas Patocka wrote:
> I did some tests. I did some random and sequential I/O on the same disk
> while mirror was resynchronizing. The result is this:
>
> random read:
> mirror throttle / IOs per second
> 100% 94
> 75% 113
> 50% 127
> 25% 145
> idle 160
>
> sequential read:
> mirror throttle / throughput MB/s
> 100% 104
> 75% 118
> 50% 122
> 25% 127
> idle 134

This is nice to see, thanks for providing it. I'm still uneasy about
this throttling being time based though.

What we're trying to do is avoid kcopyd issuing so much io that it
interferes with userland io.

Now you've already posted some nice patches that remove the fixed
sized buffers in kcopyd clients, instead grabbing extra memory if it's
available, otherwise making do with a tiny preallocated buffer to
ensure progress. So as I see it, the amount of io that kcopyd submits
is dependent on the amount of memory available.

i) If there is lots of memory available can your throttling patch
still manage to issue too much io in the time that kcopyd is active?

ii) If there is little memory available few ios will be issued. But
your throttling will still occur, slowing things down even more.

I think it makes much more sense to throttle based on amount of io
issued by kcopyd. Either tracking throughput, or even just putting a
limit on the amount of io that can be in flight at any one time.

- Joe

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

Mikulas Patocka 06-09-2011 04:08 PM

dm-kcopyd: introduce per-module throttle structure
 
On Thu, 9 Jun 2011, Joe Thornber wrote:

> Mikulas,
>
> On Tue, Jun 07, 2011 at 01:50:16PM -0400, Mikulas Patocka wrote:
> > I did some tests. I did some random and sequential I/O on the same disk
> > while mirror was resynchronizing. The result is this:
> >
> > random read:
> > mirror throttle / IOs per second
> > 100% 94
> > 75% 113
> > 50% 127
> > 25% 145
> > idle 160
> >
> > sequential read:
> > mirror throttle / throughput MB/s
> > 100% 104
> > 75% 118
> > 50% 122
> > 25% 127
> > idle 134
>
> This is nice to see, thanks for providing it. I'm still uneasy about
> this throttling being time based though.
>
> What we're trying to do is avoid kcopyd issuing so much io that it
> interferes with userland io.

But you don't know if there is some userland IO or not to the same disk.

> Now you've already posted some nice patches that remove the fixed
> sized buffers in kcopyd clients, instead grabbing extra memory if it's
> available, otherwise making do with a tiny preallocated buffer to
> ensure progress. So as I see it, the amount of io that kcopyd submits
> is dependent on the amount of memory available.
>
> i) If there is lots of memory available can your throttling patch
> still manage to issue too much io in the time that kcopyd is active?

It issues as much IO as it can in the active period.

> ii) If there is little memory available few ios will be issued. But
> your throttling will still occur, slowing things down even more.

Yes. Memory pressure and throttiling are independent things.

> I think it makes much more sense to throttle based on amount of io
> issued by kcopyd. Either tracking throughput,

You don't know what is the throughput of the device. So throttling to
something like "50% throughput" can't be done.

Measuring device throughput is unreliable. For example, if there is
concurrent IO to the same device, your observed throughput is lower than
the actual device throughput. But you never know that there is a
concurrent IO, so you can't find that the measuring is skewed.

Also, throughput depends on the disk region you are accessing --- the same
disk has higher throughput in the beginning than in the end.

Also, one dm-raid leg can span multiple disks, and they can have different
throughput.

You can't reliably determine the device throughput, so you can't reliably
throttle based throughput.

> or even just putting a
> limit on the amount of io that can be in flight at any one time.

Which is much less reliable throttling than time slots.
the resync spee is:
8 sub jobs --- 76MB/s
2 sub jobs --- 74MB/s
1 sub job --- 65MB/s

> - Joe

Mikulas

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

Mikulas Patocka 06-09-2011 04:08 PM

dm-kcopyd: introduce per-module throttle structure
 
On Thu, 9 Jun 2011, Joe Thornber wrote:

> Mikulas,
>
> On Tue, Jun 07, 2011 at 01:50:16PM -0400, Mikulas Patocka wrote:
> > I did some tests. I did some random and sequential I/O on the same disk
> > while mirror was resynchronizing. The result is this:
> >
> > random read:
> > mirror throttle / IOs per second
> > 100% 94
> > 75% 113
> > 50% 127
> > 25% 145
> > idle 160
> >
> > sequential read:
> > mirror throttle / throughput MB/s
> > 100% 104
> > 75% 118
> > 50% 122
> > 25% 127
> > idle 134
>
> This is nice to see, thanks for providing it. I'm still uneasy about
> this throttling being time based though.
>
> What we're trying to do is avoid kcopyd issuing so much io that it
> interferes with userland io.

But you don't know if there is some userland IO or not to the same disk.

> Now you've already posted some nice patches that remove the fixed
> sized buffers in kcopyd clients, instead grabbing extra memory if it's
> available, otherwise making do with a tiny preallocated buffer to
> ensure progress. So as I see it, the amount of io that kcopyd submits
> is dependent on the amount of memory available.
>
> i) If there is lots of memory available can your throttling patch
> still manage to issue too much io in the time that kcopyd is active?

It issues as much IO as it can in the active period.

> ii) If there is little memory available few ios will be issued. But
> your throttling will still occur, slowing things down even more.

Yes. Memory pressure and throttiling are independent things.

> I think it makes much more sense to throttle based on amount of io
> issued by kcopyd. Either tracking throughput,

You don't know what is the throughput of the device. So throttling to
something like "50% throughput" can't be done.

Measuring device throughput is unreliable. For example, if there is
concurrent IO to the same device, your observed throughput is lower than
the actual device throughput. But you never know that there is a
concurrent IO, so you can't find that the measuring is skewed.

Also, throughput depends on the disk region you are accessing --- the same
disk has higher throughput in the beginning than in the end.

Also, one dm-raid leg can span multiple disks, and they can have different
throughput.

You can't reliably determine the device throughput, so you can't reliably
throttle based throughput.

> or even just putting a
> limit on the amount of io that can be in flight at any one time.

Which is much less reliable throttling than time slots.
the resync spee is:
8 sub jobs --- 76MB/s
2 sub jobs --- 74MB/s
1 sub job --- 65MB/s

> - Joe

Mikulas

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


All times are GMT. The time now is 05:28 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.