public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [2.6.22 PATCH 23/26] dm delay:
@ 2007-05-08 19:48 Alasdair G Kergon
  2007-05-09  0:54 ` Andrew Morton
  0 siblings, 1 reply; 5+ messages in thread
From: Alasdair G Kergon @ 2007-05-08 19:48 UTC (permalink / raw)
  To: Andrew Morton; +Cc: dm-devel, linux-kernel, Heinz Mauelshagen

From: Heinz Mauelshagen <mauelshagen@redhat.com>

New device-mapper target that can delay I/O (for testing).
Reads can be separated from writes, redirected to different underlying
devices and delayed by differing amounts of time.

Signed-off-by: Heinz Mauelshagen <mauelshagen@redhat.com>
Signed-off-by: Milan Broz <mbroz@redhat.com>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>
---

 Documentation/device-mapper/delay.txt |   26 ++
 drivers/md/Kconfig                    |    9 
 drivers/md/Makefile                   |    1 
 drivers/md/dm-delay.c                 |  383 ++++++++++++++++++++++++++++++++++
 4 files changed, 419 insertions(+)

Index: linux-2.6.21/Documentation/device-mapper/delay.txt
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.21/Documentation/device-mapper/delay.txt	2007-05-01 17:40:55.000000000 +0100
@@ -0,0 +1,26 @@
+dm-delay
+========
+
+Device-Mapper's "delay" target delays reads and/or writes
+and maps them to different devices.
+
+Parameters:
+    <device> <offset> <delay> [<write_device> <write_offset> <write_delay>]
+
+With separate write parameters, the first set is only used for reads.
+Delays are specified in milliseconds.
+
+Example scripts
+===============
+[[
+#!/bin/sh
+# Create device delaying rw operation for 500ms
+echo "0 `blockdev --getsize $1` delay $1 0 500" | dmsetup create delayed
+]]
+
+[[
+#!/bin/sh
+# Create device delaying only write operation for 500ms and
+# splitting reads and writes to different devices $1 $2
+echo "0 `blockdev --getsize $1` delay $1 0 0 $2 0 500" | dmsetup create delayed
+]]
Index: linux-2.6.21/drivers/md/Kconfig
===================================================================
--- linux-2.6.21.orig/drivers/md/Kconfig	2007-05-01 17:38:31.000000000 +0100
+++ linux-2.6.21/drivers/md/Kconfig	2007-05-01 17:40:55.000000000 +0100
@@ -262,6 +262,15 @@ config DM_MULTIPATH_EMC
 	---help---
 	  Multipath support for EMC CX/AX series hardware.
 
+config DM_DELAY
+	tristate "I/O delaying target (EXPERIMENTAL)"
+	depends on BLK_DEV_DM && EXPERIMENTAL
+	---help---
+	A target that delays reads and/or writes and can send
+	them to different devices.  Useful for testing.
+
+	If unsure, say N.
+
 endmenu
 
 endif
Index: linux-2.6.21/drivers/md/Makefile
===================================================================
--- linux-2.6.21.orig/drivers/md/Makefile	2007-05-01 17:38:31.000000000 +0100
+++ linux-2.6.21/drivers/md/Makefile	2007-05-01 17:40:55.000000000 +0100
@@ -31,6 +31,7 @@ obj-$(CONFIG_MD_FAULTY)		+= faulty.o
 obj-$(CONFIG_BLK_DEV_MD)	+= md-mod.o
 obj-$(CONFIG_BLK_DEV_DM)	+= dm-mod.o
 obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o
+obj-$(CONFIG_DM_DELAY)		+= dm-delay.o
 obj-$(CONFIG_DM_MULTIPATH)	+= dm-multipath.o dm-round-robin.o
 obj-$(CONFIG_DM_MULTIPATH_EMC)	+= dm-emc.o
 obj-$(CONFIG_DM_SNAPSHOT)	+= dm-snapshot.o
Index: linux-2.6.21/drivers/md/dm-delay.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.21/drivers/md/dm-delay.c	2007-05-01 17:40:55.000000000 +0100
@@ -0,0 +1,383 @@
+/*
+ * Copyright (C) 2005-2007 Red Hat GmbH
+ *
+ * A target that delays reads and/or writes and can send
+ * them to different devices.
+ *
+ * This file is released under the GPL.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/blkdev.h>
+#include <linux/bio.h>
+#include <linux/slab.h>
+
+#include "dm.h"
+#include "dm-bio-list.h"
+
+#define DM_MSG_PREFIX "delay"
+
+struct delay_c {
+	struct timer_list delay_timer;
+	struct semaphore timer_lock;
+	struct work_struct flush_expired_bios;
+	struct list_head delayed_bios;
+	atomic_t may_delay;
+	mempool_t *delayed_pool;
+
+	struct dm_dev *dev_read;
+	sector_t start_read;
+	unsigned read_delay;
+	unsigned reads;
+
+	struct dm_dev *dev_write;
+	sector_t start_write;
+	unsigned write_delay;
+	unsigned writes;
+};
+
+struct delay_info {
+	struct delay_c *context;
+	struct list_head list;
+	struct bio *bio;
+	unsigned long expires;
+};
+
+static DEFINE_MUTEX(delayed_bios_lock);
+
+static struct workqueue_struct *kdelayd_wq;
+static struct kmem_cache *delayed_cache;
+
+static void handle_delayed_timer(unsigned long data)
+{
+	struct delay_c *dc = (struct delay_c *)data;
+
+	queue_work(kdelayd_wq, &dc->flush_expired_bios);
+}
+
+static void queue_timeout(struct delay_c *dc, unsigned long expires)
+{
+	down(&dc->timer_lock);
+
+	if (!timer_pending(&dc->delay_timer) || expires < dc->delay_timer.expires)
+		mod_timer(&dc->delay_timer, expires);
+
+	up(&dc->timer_lock);
+}
+
+static void flush_bios(struct bio *bio)
+{
+	struct bio *n;
+
+	while (bio) {
+		n = bio->bi_next;
+		bio->bi_next = NULL;
+		generic_make_request(bio);
+		bio = n;
+	}
+}
+
+static struct bio *flush_delayed_bios(struct delay_c *dc, int flush_all)
+{
+	struct delay_info *delayed, *next;
+	unsigned long next_expires = 0;
+	int start_timer = 0;
+	BIO_LIST(flush_bios);
+
+	mutex_lock(&delayed_bios_lock);
+	list_for_each_entry_safe(delayed, next, &dc->delayed_bios, list) {
+		if (flush_all || time_after_eq(jiffies, delayed->expires)) {
+			list_del(&delayed->list);
+			bio_list_add(&flush_bios, delayed->bio);
+			if ((bio_data_dir(delayed->bio) == WRITE))
+				delayed->context->writes--;
+			else
+				delayed->context->reads--;
+			mempool_free(delayed, dc->delayed_pool);
+			continue;
+		}
+
+		if (!start_timer) {
+			start_timer = 1;
+			next_expires = delayed->expires;
+		} else
+			next_expires = min(next_expires, delayed->expires);
+	}
+
+	mutex_unlock(&delayed_bios_lock);
+
+	if (start_timer)
+		queue_timeout(dc, next_expires);
+
+	return bio_list_get(&flush_bios);
+}
+
+static void flush_expired_bios(struct work_struct *work)
+{
+	struct delay_c *dc;
+
+	dc = container_of(work, struct delay_c, flush_expired_bios);
+	flush_bios(flush_delayed_bios(dc, 0));
+}
+
+/*
+ * Mapping parameters:
+ *    <device> <offset> <delay> [<write_device> <write_offset> <write_delay>]
+ *
+ * With separate write parameters, the first set is only used for reads.
+ * Delays are specified in milliseconds.
+ */
+static int delay_ctr(struct dm_target *ti, unsigned int argc, char **argv)
+{
+	struct delay_c *dc;
+	unsigned long long tmpll;
+
+	if (argc != 3 && argc != 6) {
+		ti->error = "requires exactly 3 or 6 arguments";
+		return -EINVAL;
+	}
+
+	dc = kmalloc(sizeof(*dc), GFP_KERNEL);
+	if (!dc) {
+		ti->error = "Cannot allocate context";
+		return -ENOMEM;
+	}
+
+	dc->reads = dc->writes = 0;
+
+	if (sscanf(argv[1], "%llu", &tmpll) != 1) {
+		ti->error = "Invalid device sector";
+		goto bad;
+	}
+	dc->start_read = tmpll;
+
+	if (sscanf(argv[2], "%u", &dc->read_delay) != 1) {
+		ti->error = "Invalid delay";
+		goto bad;
+	}
+
+	if (dm_get_device(ti, argv[0], dc->start_read, ti->len,
+			  dm_table_get_mode(ti->table), &dc->dev_read)) {
+		ti->error = "Device lookup failed";
+		goto bad;
+	}
+
+	if (argc == 3) {
+		dc->dev_write = NULL;
+		goto out;
+	}
+
+	if (sscanf(argv[4], "%llu", &tmpll) != 1) {
+		ti->error = "Invalid write device sector";
+		goto bad;
+	}
+	dc->start_write = tmpll;
+
+	if (sscanf(argv[5], "%u", &dc->write_delay) != 1) {
+		ti->error = "Invalid write delay";
+		goto bad;
+	}
+
+	if (dm_get_device(ti, argv[3], dc->start_write, ti->len,
+			  dm_table_get_mode(ti->table), &dc->dev_write)) {
+		ti->error = "Write device lookup failed";
+		dm_put_device(ti, dc->dev_read);
+		goto bad;
+	}
+
+out:
+	dc->delayed_pool = mempool_create_slab_pool(128, delayed_cache);
+	if (!dc->delayed_pool) {
+		DMERR("Couldn't create delayed bio pool.");
+		goto bad;
+	}
+
+	init_timer(&dc->delay_timer);
+	dc->delay_timer.function = handle_delayed_timer;
+	dc->delay_timer.data = (unsigned long)dc;
+
+	INIT_WORK(&dc->flush_expired_bios, flush_expired_bios);
+	INIT_LIST_HEAD(&dc->delayed_bios);
+	init_MUTEX(&dc->timer_lock);
+	atomic_set(&dc->may_delay, 1);
+
+	ti->private = dc;
+	return 0;
+
+bad:
+	kfree(dc);
+	return -EINVAL;
+}
+
+static void delay_dtr(struct dm_target *ti)
+{
+	struct delay_c *dc = ti->private;
+
+	flush_workqueue(kdelayd_wq);
+
+	dm_put_device(ti, dc->dev_read);
+
+	if (dc->dev_write)
+		dm_put_device(ti, dc->dev_write);
+
+	mempool_destroy(dc->delayed_pool);
+	kfree(dc);
+}
+
+static int delay_bio(struct delay_c *dc, int delay, struct bio *bio)
+{
+	struct delay_info *delayed;
+	unsigned long expires = 0;
+
+	if (!delay || !atomic_read(&dc->may_delay))
+		return 1;
+
+	delayed = mempool_alloc(dc->delayed_pool, GFP_NOIO);
+
+	delayed->context = dc;
+	delayed->bio = bio;
+	delayed->expires = expires = jiffies + (delay * HZ / 1000);
+
+	mutex_lock(&delayed_bios_lock);
+
+	if (bio_data_dir(bio) == WRITE)
+		dc->writes++;
+	else
+		dc->reads++;
+
+	list_add_tail(&delayed->list, &dc->delayed_bios);
+
+	mutex_unlock(&delayed_bios_lock);
+
+	queue_timeout(dc, expires);
+
+	return 0;
+}
+
+static void delay_presuspend(struct dm_target *ti)
+{
+	struct delay_c *dc = ti->private;
+
+	atomic_set(&dc->may_delay, 0);
+	del_timer_sync(&dc->delay_timer);
+	flush_bios(flush_delayed_bios(dc, 1));
+}
+
+static void delay_resume(struct dm_target *ti)
+{
+	struct delay_c *dc = ti->private;
+
+	atomic_set(&dc->may_delay, 1);
+}
+
+static int delay_map(struct dm_target *ti, struct bio *bio,
+		     union map_info *map_context)
+{
+	struct delay_c *dc = ti->private;
+
+	if ((bio_data_dir(bio) == WRITE) && (dc->dev_write)) {
+		bio->bi_bdev = dc->dev_write->bdev;
+		bio->bi_sector = dc->start_write +
+				 (bio->bi_sector - ti->begin);
+
+		return delay_bio(dc, dc->write_delay, bio);
+	}
+
+	bio->bi_bdev = dc->dev_read->bdev;
+	bio->bi_sector = dc->start_read +
+			 (bio->bi_sector - ti->begin);
+
+	return delay_bio(dc, dc->read_delay, bio);
+}
+
+static int delay_status(struct dm_target *ti, status_type_t type,
+			char *result, unsigned maxlen)
+{
+	struct delay_c *dc = ti->private;
+	int sz = 0;
+
+	switch (type) {
+	case STATUSTYPE_INFO:
+		DMEMIT("%u %u", dc->reads, dc->writes);
+		break;
+
+	case STATUSTYPE_TABLE:
+		DMEMIT("%s %llu %u", dc->dev_read->name,
+		       (unsigned long long) dc->start_read,
+		       dc->read_delay);
+		if (dc->dev_write)
+			DMEMIT("%s %llu %u", dc->dev_write->name,
+			       (unsigned long long) dc->start_write,
+			       dc->write_delay);
+		break;
+	}
+
+	return 0;
+}
+
+static struct target_type delay_target = {
+	.name	     = "delay",
+	.version     = {1, 0, 2},
+	.module      = THIS_MODULE,
+	.ctr	     = delay_ctr,
+	.dtr	     = delay_dtr,
+	.map	     = delay_map,
+	.presuspend  = delay_presuspend,
+	.resume	     = delay_resume,
+	.status	     = delay_status,
+};
+
+static int __init dm_delay_init(void)
+{
+	int r = -ENOMEM;
+
+	kdelayd_wq = create_workqueue("kdelayd");
+	if (!kdelayd_wq) {
+		DMERR("Couldn't start kdelayd");
+		goto bad_queue;
+	}
+
+	delayed_cache = kmem_cache_create("dm-delay",
+					  sizeof(struct delay_info),
+					  __alignof__(struct delay_info),
+					  0, NULL, NULL);
+	if (!delayed_cache) {
+		DMERR("Couldn't create delayed bio cache.");
+		goto bad_memcache;
+	}
+
+	r = dm_register_target(&delay_target);
+	if (r < 0) {
+		DMERR("register failed %d", r);
+		goto bad_register;
+	}
+
+	return 0;
+
+bad_register:
+	kmem_cache_destroy(delayed_cache);
+bad_memcache:
+	destroy_workqueue(kdelayd_wq);
+bad_queue:
+	return r;
+}
+
+static void __exit dm_delay_exit(void)
+{
+	int r = dm_unregister_target(&delay_target);
+
+	if (r < 0)
+		DMERR("unregister failed %d", r);
+
+	kmem_cache_destroy(delayed_cache);
+	destroy_workqueue(kdelayd_wq);
+}
+
+/* Module hooks */
+module_init(dm_delay_init);
+module_exit(dm_delay_exit);
+
+MODULE_DESCRIPTION(DM_NAME " delay target");
+MODULE_AUTHOR("Heinz Mauelshagen <mauelshagen@redhat.com>");
+MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [2.6.22 PATCH 23/26] dm delay:
  2007-05-08 19:48 [2.6.22 PATCH 23/26] dm delay: Alasdair G Kergon
@ 2007-05-09  0:54 ` Andrew Morton
  2007-05-10 19:02   ` Andrew Morton
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2007-05-09  0:54 UTC (permalink / raw)
  To: Alasdair G Kergon
  Cc: dm-devel, linux-kernel, Heinz Mauelshagen, Christoph Lameter

On Tue, 8 May 2007 20:48:59 +0100
Alasdair G Kergon <agk@redhat.com> wrote:

> From: Heinz Mauelshagen <mauelshagen@redhat.com>
> 
> New device-mapper target that can delay I/O (for testing).
> Reads can be separated from writes, redirected to different underlying
> devices and delayed by differing amounts of time.
> 
> ..
>
> +/*
> + * Copyright (C) 2005-2007 Red Hat GmbH
> + *
> + * A target that delays reads and/or writes and can send
> + * them to different devices.
> + *
> + * This file is released under the GPL.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/blkdev.h>
> +#include <linux/bio.h>
> +#include <linux/slab.h>
> +
> +#include "dm.h"
> +#include "dm-bio-list.h"
> +
> +#define DM_MSG_PREFIX "delay"
> +
> +struct delay_c {
> +	struct timer_list delay_timer;
> +	struct semaphore timer_lock;

Can we get this converted to a mutex asap, please?

It's only used in a single place and perhaps we don't need this lock at
all?

> +	struct work_struct flush_expired_bios;
> +	struct list_head delayed_bios;
> +	atomic_t may_delay;
> +	mempool_t *delayed_pool;
> +
> +	struct dm_dev *dev_read;
> +	sector_t start_read;
> +	unsigned read_delay;
> +	unsigned reads;
> +
> +	struct dm_dev *dev_write;
> +	sector_t start_write;
> +	unsigned write_delay;
> +	unsigned writes;
> +};
> +
> +struct delay_info {
> +	struct delay_c *context;
> +	struct list_head list;
> +	struct bio *bio;
> +	unsigned long expires;
> +};
> +
> +static DEFINE_MUTEX(delayed_bios_lock);
> +
> +static struct workqueue_struct *kdelayd_wq;
> +static struct kmem_cache *delayed_cache;
> +
> +static void handle_delayed_timer(unsigned long data)
> +{
> +	struct delay_c *dc = (struct delay_c *)data;
> +
> +	queue_work(kdelayd_wq, &dc->flush_expired_bios);
> +}
> +
> +static void queue_timeout(struct delay_c *dc, unsigned long expires)
> +{
> +	down(&dc->timer_lock);
> +
> +	if (!timer_pending(&dc->delay_timer) || expires < dc->delay_timer.expires)
> +		mod_timer(&dc->delay_timer, expires);
> +
> +	up(&dc->timer_lock);
> +}
>
> ...
>
> +
> +	init_timer(&dc->delay_timer);
> +	dc->delay_timer.function = handle_delayed_timer;
> +	dc->delay_timer.data = (unsigned long)dc;

setup_timer() could be used here.

> +	INIT_WORK(&dc->flush_expired_bios, flush_expired_bios);
> +	INIT_LIST_HEAD(&dc->delayed_bios);
> +	init_MUTEX(&dc->timer_lock);
> +	atomic_set(&dc->may_delay, 1);
> +
> +	ti->private = dc;
> +	return 0;
> +
> +bad:
> +	kfree(dc);
> +	return -EINVAL;
> +}
> +
>
> ...
>
> +
> +static int __init dm_delay_init(void)
> +{
> +	int r = -ENOMEM;
> +
> +	kdelayd_wq = create_workqueue("kdelayd");
> +	if (!kdelayd_wq) {
> +		DMERR("Couldn't start kdelayd");
> +		goto bad_queue;
> +	}

Do we really really need one instance of kdelayd on each CPU?

I suspect a single-threaded workqueue would suffice here.  We have a big
global lock in this driver anyway....

Probably create_workqueue() should have defaulted to single-threaded on day
one.  Oh well.


> +	delayed_cache = kmem_cache_create("dm-delay",
> +					  sizeof(struct delay_info),
> +					  __alignof__(struct delay_info),
> +					  0, NULL, NULL);

We have an ugly^Wnice new KMEM_CACHE macro for this now.

What's that __alignof__ doing in there?

> +	if (!delayed_cache) {
> +		DMERR("Couldn't create delayed bio cache.");
> +		goto bad_memcache;
> +	}
> +
> +	r = dm_register_target(&delay_target);
> +	if (r < 0) {
> +		DMERR("register failed %d", r);
> +		goto bad_register;
> +	}
> +
> +	return 0;
> +
> +bad_register:
> +	kmem_cache_destroy(delayed_cache);
> +bad_memcache:
> +	destroy_workqueue(kdelayd_wq);
> +bad_queue:
> +	return r;
> +}
> +


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [2.6.22 PATCH 23/26] dm delay:
  2007-05-09  0:54 ` Andrew Morton
@ 2007-05-10 19:02   ` Andrew Morton
  2007-05-10 19:04     ` Christoph Lameter
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2007-05-10 19:02 UTC (permalink / raw)
  To: Alasdair G Kergon, dm-devel, linux-kernel, Heinz Mauelshagen,
	Christoph Lameter

On Tue, 8 May 2007 17:54:10 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 8 May 2007 20:48:59 +0100
> Alasdair G Kergon <agk@redhat.com> wrote:
> 
> > From: Heinz Mauelshagen <mauelshagen@redhat.com>
> > 
> > New device-mapper target that can delay I/O (for testing).
> > Reads can be separated from writes, redirected to different underlying
> > devices and delayed by differing amounts of time.
> > 
> > ..
> >
> > +/*
> > + * Copyright (C) 2005-2007 Red Hat GmbH
> > + *
> > + * A target that delays reads and/or writes and can send
> > + * them to different devices.
> > + *
> > + * This file is released under the GPL.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/blkdev.h>
> > +#include <linux/bio.h>
> > +#include <linux/slab.h>
> > +
> > +#include "dm.h"
> > +#include "dm-bio-list.h"
> > +
> > +#define DM_MSG_PREFIX "delay"
> > +
> > +struct delay_c {
> > +	struct timer_list delay_timer;
> > +	struct semaphore timer_lock;
> 
> Can we get this converted to a mutex asap, please?
> 
> It's only used in a single place and perhaps we don't need this lock at
> all?
> 
> > +	struct work_struct flush_expired_bios;
> > +	struct list_head delayed_bios;
> > +	atomic_t may_delay;
> > +	mempool_t *delayed_pool;
> > +
> > +	struct dm_dev *dev_read;
> > +	sector_t start_read;
> > +	unsigned read_delay;
> > +	unsigned reads;
> > +
> > +	struct dm_dev *dev_write;
> > +	sector_t start_write;
> > +	unsigned write_delay;
> > +	unsigned writes;
> > +};
> > +
> > +struct delay_info {
> > +	struct delay_c *context;
> > +	struct list_head list;
> > +	struct bio *bio;
> > +	unsigned long expires;
> > +};
> > +
> > +static DEFINE_MUTEX(delayed_bios_lock);
> > +
> > +static struct workqueue_struct *kdelayd_wq;
> > +static struct kmem_cache *delayed_cache;
> > +
> > +static void handle_delayed_timer(unsigned long data)
> > +{
> > +	struct delay_c *dc = (struct delay_c *)data;
> > +
> > +	queue_work(kdelayd_wq, &dc->flush_expired_bios);
> > +}
> > +
> > +static void queue_timeout(struct delay_c *dc, unsigned long expires)
> > +{
> > +	down(&dc->timer_lock);
> > +
> > +	if (!timer_pending(&dc->delay_timer) || expires < dc->delay_timer.expires)
> > +		mod_timer(&dc->delay_timer, expires);
> > +
> > +	up(&dc->timer_lock);
> > +}
> >
> > ...
> >
> > +
> > +	init_timer(&dc->delay_timer);
> > +	dc->delay_timer.function = handle_delayed_timer;
> > +	dc->delay_timer.data = (unsigned long)dc;
> 
> setup_timer() could be used here.
> 
> > +	INIT_WORK(&dc->flush_expired_bios, flush_expired_bios);
> > +	INIT_LIST_HEAD(&dc->delayed_bios);
> > +	init_MUTEX(&dc->timer_lock);
> > +	atomic_set(&dc->may_delay, 1);
> > +
> > +	ti->private = dc;
> > +	return 0;
> > +
> > +bad:
> > +	kfree(dc);
> > +	return -EINVAL;
> > +}
> > +
> >
> > ...
> >
> > +
> > +static int __init dm_delay_init(void)
> > +{
> > +	int r = -ENOMEM;
> > +
> > +	kdelayd_wq = create_workqueue("kdelayd");
> > +	if (!kdelayd_wq) {
> > +		DMERR("Couldn't start kdelayd");
> > +		goto bad_queue;
> > +	}
> 
> Do we really really need one instance of kdelayd on each CPU?
> 
> I suspect a single-threaded workqueue would suffice here.  We have a big
> global lock in this driver anyway....
> 
> Probably create_workqueue() should have defaulted to single-threaded on day
> one.  Oh well.
> 
> 
> > +	delayed_cache = kmem_cache_create("dm-delay",
> > +					  sizeof(struct delay_info),
> > +					  __alignof__(struct delay_info),
> > +					  0, NULL, NULL);
> 
> We have an ugly^Wnice new KMEM_CACHE macro for this now.
> 
> What's that __alignof__ doing in there?
> 
> > +	if (!delayed_cache) {
> > +		DMERR("Couldn't create delayed bio cache.");
> > +		goto bad_memcache;
> > +	}
> > +
> > +	r = dm_register_target(&delay_target);
> > +	if (r < 0) {
> > +		DMERR("register failed %d", r);
> > +		goto bad_register;
> > +	}
> > +
> > +	return 0;
> > +
> > +bad_register:
> > +	kmem_cache_destroy(delayed_cache);
> > +bad_memcache:
> > +	destroy_workqueue(kdelayd_wq);
> > +bad_queue:
> > +	return r;
> > +}
> > +
> 

Guys, could I please get some response to this before I forget about it? 
The new per-cpu kernel thread problem is particularly important.  We already
fail to boot on 4096-CPU due to running out of pids.

Thanks.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [2.6.22 PATCH 23/26] dm delay:
  2007-05-10 19:02   ` Andrew Morton
@ 2007-05-10 19:04     ` Christoph Lameter
  2007-05-10 19:37       ` Andrew Morton
  0 siblings, 1 reply; 5+ messages in thread
From: Christoph Lameter @ 2007-05-10 19:04 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Alasdair G Kergon, dm-devel, linux-kernel, Heinz Mauelshagen

On Thu, 10 May 2007, Andrew Morton wrote:

> > > +	delayed_cache = kmem_cache_create("dm-delay",
> > > +					  sizeof(struct delay_info),
> > > +					  __alignof__(struct delay_info),
> > > +					  0, NULL, NULL);
> > 
> > We have an ugly^Wnice new KMEM_CACHE macro for this now.

How could I nicify the macro? Lower case?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [2.6.22 PATCH 23/26] dm delay:
  2007-05-10 19:04     ` Christoph Lameter
@ 2007-05-10 19:37       ` Andrew Morton
  0 siblings, 0 replies; 5+ messages in thread
From: Andrew Morton @ 2007-05-10 19:37 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Alasdair G Kergon, dm-devel, linux-kernel, Heinz Mauelshagen

On Thu, 10 May 2007 12:04:50 -0700 (PDT) Christoph Lameter <clameter@sgi.com> wrote:

> On Thu, 10 May 2007, Andrew Morton wrote:
> 
> > > > +	delayed_cache = kmem_cache_create("dm-delay",
> > > > +					  sizeof(struct delay_info),
> > > > +					  __alignof__(struct delay_info),
> > > > +					  0, NULL, NULL);
> > > 
> > > We have an ugly^Wnice new KMEM_CACHE macro for this now.
> 
> How could I nicify the macro? Lower case?

Lower case would be worse, IMO.  Upper-case says "this is a macro".  Lower-case
macros are OK when they are macros pretending to be functions, but KMEM_CACHE
is not such a thing.

I don't think it can be nicified, except by removing it.  We generally
prefer to open-code things rather than wrapping them up in macros.

But given the amount of variation amongst callers and the number of times
this got screwed up, I agree that burying those details at a single site
makes sense in this instance.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2007-05-10 19:38 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-05-08 19:48 [2.6.22 PATCH 23/26] dm delay: Alasdair G Kergon
2007-05-09  0:54 ` Andrew Morton
2007-05-10 19:02   ` Andrew Morton
2007-05-10 19:04     ` Christoph Lameter
2007-05-10 19:37       ` Andrew Morton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox