[PATCH] dm-kcopyd: monitor io activity

[PATCH] dm-kcopyd: monitor io activity

[Mikulas Patocka]

Hi

Here I'm sending three patches that limit kcopyd speed. There is global 
limit for all kcopyds running in a system. The user can set percentage of 
kcopyd speed in /sys/module/dm_mod/parameters/dm_kcopyd_throttle

Mikulas

---

dm-kcopyd: monitor io activity

There are two activity counters, "total_period" and "io_period".
total_period counts all time ticks, io_period counts timer ticks when
some I/O is active.

Thus, (100 * io_period / total_period) represents the percentage of
time when kcopyd is active.

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

---
 drivers/md/dm-kcopyd.c |   67 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

Index: linux-2.6.39-fast/drivers/md/dm-kcopyd.c
===================================================================
--- linux-2.6.39-fast.orig/drivers/md/dm-kcopyd.c	2011-05-30 18:04:08.000000000 +0200
+++ linux-2.6.39-fast/drivers/md/dm-kcopyd.c	2011-05-30 18:07:36.000000000 +0200
@@ -66,6 +66,69 @@ struct dm_kcopyd_client {
 	struct list_head pages_jobs;
 };
 
+static DEFINE_SPINLOCK(activity_spinlock);
+static unsigned long num_io_jobs = 0;
+
+/*
+ * kcopyd is active (100 * io_period / total_period) percent of time.
+ */
+static unsigned io_period = 0;
+static unsigned total_period = 0;
+static unsigned last_jiffies = 0;
+
+/*
+ * IO/IDLE accounting slowly decays after (1 << ACOUNT_INTERVAL_SHIFT) period.
+ * When total_period >= (1 << ACOUNT_INTERVAL_SHIFT) the counters are divided
+ * by 2.
+ */
+#define ACOUNT_INTERVAL_SHIFT		SHIFT_HZ
+
+static void io_job_start(void)
+{
+	unsigned now, difference;
+
+	spin_lock_irq(&activity_spinlock);
+
+	now = jiffies;
+	difference = now - last_jiffies;
+	last_jiffies = now;
+	if (num_io_jobs)
+		io_period += difference;
+	total_period += difference;
+
+	if (unlikely(total_period >= (1 << ACOUNT_INTERVAL_SHIFT))) {
+		int shift = fls(total_period >> ACOUNT_INTERVAL_SHIFT);
+		total_period >>= shift;
+		io_period >>= shift;
+	}
+
+	num_io_jobs++;
+
+	spin_unlock_irq(&activity_spinlock);
+}
+
+static void io_job_finish(void)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&activity_spinlock, flags);
+
+	num_io_jobs--;
+
+	if (!num_io_jobs) {
+		unsigned now, difference;
+
+		now = jiffies;
+		difference = now - last_jiffies;
+		last_jiffies = now;
+
+		io_period += difference;
+		total_period += difference;
+	}
+
+	spin_unlock_irqrestore(&activity_spinlock, flags);
+}
+
 static void wake(struct dm_kcopyd_client *kc)
 {
 	queue_work(kc->kcopyd_wq, &kc->kcopyd_work);
@@ -324,6 +387,8 @@ static void complete_io(unsigned long er
 	struct kcopyd_job *job = (struct kcopyd_job *) context;
 	struct dm_kcopyd_client *kc = job->kc;
 
+	io_job_finish();
+
 	if (error) {
 		if (job->rw == WRITE)
 			job->write_err |= error;
@@ -365,6 +430,8 @@ static int run_io_job(struct kcopyd_job 
 		.client = job->kc->io_client,
 	};
 
+	io_job_start();
+
 	if (job->rw == READ)
 		r = dm_io(&io_req, 1, &job->source, NULL);
 	else

Re: [PATCH] dm-kcopyd: monitor io activity

[Joe Thornber]

On Mon, May 30, 2011 at 12:40:39PM -0400, Mikulas Patocka wrote:
> Here I'm sending three patches that limit kcopyd speed. There is global 
> limit for all kcopyds running in a system. The user can set percentage of 
> kcopyd speed in /sys/module/dm_mod/parameters/dm_kcopyd_throttle

Could you give some explanation of how to choose an appropriate value
for this please?  Should it be 100% unless there is an issue?  How
does the sys admin spot such issues?

Different kcopyd clients have different performance requirements, for
instance the initial sync of a new mirror leg may have less priority
than a copy-on-write exception for a snapshot.  This isn't something
we've addressed so far, but if you're starting to slow kcopyd down
(i.e. so the mirror sync doesn't saturate the system), then I think
you'll get some complaints from the snapshot users.

- joe

Re: [PATCH] dm-kcopyd: monitor io activity

[Mikulas Patocka]

On Tue, 31 May 2011, Joe Thornber wrote:

> On Tue, May 31, 2011 at 10:12:24AM +0100, Alasdair G Kergon wrote:
> > On Tue, May 31, 2011 at 10:09:01AM +0100, Joe Thornber wrote:
> > > Different kcopyd clients have different performance requirements, for
> > > instance the initial sync of a new mirror leg may have less priority
> > > than a copy-on-write exception for a snapshot.  This isn't something
> > > we've addressed so far, but if you're starting to slow kcopyd down
> > > (i.e. so the mirror sync doesn't saturate the system), then I think
> > > you'll get some complaints from the snapshot users.

I think it's a good comment. We could make different throttles for 
different clases of clients --- i.e. have one throttle for mirrors, one 
for snapshots, one for multisnap, etc.

The variables "dm_kcopyd_throttle", "activity_spinlock", "num_io_jobs", 
"io_period", "total_period", "last_jiffies" could be placed into a 
structure and the structure could be moved to every module that uses 
kcopyd. Thus, the modules could be throttled independently.

> > Is it plausible to differentiate between the classes of clients,
> > and prioritise snapshot i/o above mirror or have different throttling
> > parameters?
> 
> Well I think it's plausible to have a priority field as part of the
> request structure.  This could be either a direct priority, or a
> 'class' mapping onto a throttling parameter as you suggest.  As long
> as we're careful to avoid starvation it shouldn't be too hard to
> implement.
> 
> In general I'd rather avoid introducing paramaters for the sys admin.

If you don't make a parameter, the whole idea of throttling is useless. 
The throttling should happen only at admin's request, never do it 
automatically. If we slow down I/O deliberately, people will surely come 
up with scenarios where it hurts performance.

> It's more work for them, and in my experience they rarely get set
> properly.  So is there a way to infer priorities for different
> clients?  Perhaps a function of number and size of recently submitted
> kcopyd jobs?

This is duplicating work of i/o scheduler. Such decisions should be made 
in the i/o scheduler and not in kcopyd.

> If one client has submitted a request to copy 64k of
> data and the other 1G does it always make sense to do the small job in
> a more timely manner?
> 
> - Joe

Mikulas