[dm-devel] [PATCH] dm ssdcache: fix and/or tweak various low hanging fruit
Mike Snitzer
snitzer at redhat.com
Mon Mar 19 13:57:01 UTC 2012
Initial review (which hasn't yet touched on design, algorithms,
naming, etc) uncovered some small things:
- remove ': ' from DM_MSG_PREFIX, tweaked {D,W}PRINTK
- eliminate 2 4-byte holes in ssdcache_md structure
- clean up pool_init() error handling, switched to using KMEM_CACHE()
- fix ssd_cache_ctr() error path, dm_put_device for target_dev was
missing if failed to get cache_dev
- fix ssdcache_iterate_devices() to consult cache_dev too because it
is in the data path (resulting ssdcache dev now stacks limits
properly, e.g.: if you mix a 4K ssd with a 512b target dev)
- wrap sio_in_flight with SSD_DEBUG
- document ssdcache_ctr (still have yet to make sense of the options)
- small s/eject/evict/ comment tweak
Signed-off-by: Mike Snitzer <snitzer at redhat.com>
---
drivers/md/dm-ssdcache.c | 130 ++++++++++++++++++++++-----------------------
1 files changed, 64 insertions(+), 66 deletions(-)
diff --git a/drivers/md/dm-ssdcache.c b/drivers/md/dm-ssdcache.c
index f05e40f..1ab6cb3 100644
--- a/drivers/md/dm-ssdcache.c
+++ b/drivers/md/dm-ssdcache.c
@@ -1,6 +1,4 @@
/*
- * dm-ssdcache.c
- *
* Copyright (c) 2011 Hannes Reinecke, SUSE Linux Products GmbH
*
* This file is released under the GPL.
@@ -17,21 +15,21 @@
#include <linux/dm-io.h>
#include <linux/dm-kcopyd.h>
-#define DM_MSG_PREFIX "ssdcache: "
+#define DM_MSG_PREFIX "ssdcache"
// #define SSD_DEBUG
#define SSD_LOG
#define SSDCACHE_USE_RADIX_TREE
#ifdef SSD_LOG
-#define DPRINTK( s, arg... ) printk(DM_MSG_PREFIX s "\n", ##arg)
-#define WPRINTK( w, s, arg... ) printk(DM_MSG_PREFIX "%lu: %s (cte %lx:%02lx): "\
- s "\n", (w)->nr, __FUNCTION__, \
- (w)->cmd->hash, \
- (w)->cte_idx, ##arg)
+#define DPRINTK(s, arg...) printk(DM_MSG_PREFIX ": " s "\n", ## arg)
+#define WPRINTK(w, s, arg...) printk(DM_MSG_PREFIX ": %lu: %s (cte %lx:%02lx): " \
+ s "\n", (w)->nr, __FUNCTION__, \
+ (w)->cmd->hash, \
+ (w)->cte_idx, ## arg)
#else
-#define DPRINTK( s, arg... )
-#define WPRINTK( w, s, arg... )
+#define DPRINTK(s, arg...)
+#define WPRINTK(w, s, arg...)
#endif
#define SSDCACHE_COPY_PAGES 1024
@@ -57,6 +55,8 @@ enum ssdcache_strategy_t {
CACHE_LFU,
};
+/* FIXME: add 'dm_' prefix to ssdcache_{md,io,te} structures */
+
struct ssdcache_md;
struct ssdcache_io;
@@ -74,8 +74,8 @@ struct ssdcache_te {
struct ssdcache_md {
spinlock_t lock; /* Lock to protect operations on the bio list */
- unsigned long hash; /* Hash number */
unsigned int num_cte; /* Number of table entries */
+ unsigned long hash; /* Hash number */
unsigned long atime;
struct ssdcache_ctx *sc;
struct ssdcache_te *te[DEFAULT_ALIASING]; /* RCU Table entries */
@@ -198,63 +198,47 @@ enum cte_match_t {
static int pool_init(void)
{
- _sio_cache = kmem_cache_create("ssdcache-sio",
- sizeof(struct ssdcache_io),
- __alignof__(struct ssdcache_io),
- 0, NULL);
+ _sio_cache = KMEM_CACHE(ssdcache_io, 0);
if (!_sio_cache)
return -ENOMEM;
- _cmd_cache = kmem_cache_create("ssdcache-cmd",
- sizeof(struct ssdcache_md),
- __alignof__(struct ssdcache_md),
- 0, NULL);
-
- if (!_cmd_cache) {
- kmem_cache_destroy(_sio_cache);
- return -ENOMEM;
- }
-
- _cte_cache = kmem_cache_create("ssdcache-cte",
- sizeof(struct ssdcache_te),
- __alignof__(struct ssdcache_te),
- 0, NULL);
+ _cmd_cache = KMEM_CACHE(ssdcache_md, 0);
+ if (!_cmd_cache)
+ goto bad_cmd_cache;
- if (!_cte_cache) {
- kmem_cache_destroy(_cmd_cache);
- kmem_cache_destroy(_sio_cache);
- return -ENOMEM;
- }
+ _cte_cache = KMEM_CACHE(ssdcache_te, 0);
+ if (!_cte_cache)
+ goto bad_cte_cache;
_sio_pool = mempool_create(MIN_SIO_ITEMS, mempool_alloc_slab,
- mempool_free_slab, _sio_cache);
- if (!_sio_pool) {
- kmem_cache_destroy(_cte_cache);
- kmem_cache_destroy(_cmd_cache);
- kmem_cache_destroy(_sio_cache);
- return -ENOMEM;
- }
+ mempool_free_slab, _sio_cache);
+ if (!_sio_pool)
+ goto bad_sio_pool;
_cmd_pool = mempool_create(MIN_CMD_NUM, mempool_alloc_slab,
mempool_free_slab, _cmd_cache);
- if (!_cmd_pool) {
- mempool_destroy(_sio_pool);
- kmem_cache_destroy(_cte_cache);
- kmem_cache_destroy(_cmd_cache);
- kmem_cache_destroy(_sio_cache);
- }
+ if (!_cmd_pool)
+ goto bad_cmd_pool;
_cte_pool = mempool_create(MIN_CTE_NUM, mempool_alloc_slab,
mempool_free_slab, _cte_cache);
- if (!_cte_pool) {
- mempool_destroy(_cmd_pool);
- mempool_destroy(_sio_pool);
- kmem_cache_destroy(_cte_cache);
- kmem_cache_destroy(_cmd_cache);
- kmem_cache_destroy(_sio_cache);
- }
+ if (!_cte_pool)
+ goto bad_cte_pool;
return 0;
+
+bad_cte_pool:
+ mempool_destroy(_cmd_pool);
+bad_cmd_pool:
+ mempool_destroy(_sio_pool);
+bad_sio_pool:
+ kmem_cache_destroy(_cte_cache);
+bad_cte_cache:
+ kmem_cache_destroy(_cmd_cache);
+bad_cmd_cache:
+ kmem_cache_destroy(_sio_cache);
+
+ return -ENOMEM;
}
static void pool_exit(void)
@@ -1280,7 +1264,7 @@ retry:
busy++;
continue;
}
- /* Can only eject CLEAN entries */
+ /* Can only evict CLEAN entries */
if (!cte_is_clean(cte, sio->bio_mask)) {
#ifdef SSD_DEBUG
DPRINTK("%lu: %s (cte %lx:%x): skip not-clean cte",
@@ -1387,6 +1371,7 @@ static void sio_lookup_async(struct ssdcache_io *sio)
}
}
+#ifdef SSD_DEBUG
static void sio_in_flight(void)
{
struct ssdcache_io *sio;
@@ -1400,6 +1385,7 @@ static void sio_in_flight(void)
spin_unlock_irqrestore(&_work_lock, flags);
DPRINTK("%d sios in flight", in_flight);
}
+#endif
/*
* process_sio
@@ -1726,7 +1712,7 @@ static int ssdcache_parse_options(struct dm_target *ti,
unsigned int argc;
const char *opt_name;
static struct dm_arg _args[] = {
- {0, 5, "invalid number of options"},
+ {0, 7, "invalid number of options"},
};
r = dm_read_arg_group(_args, as, &argc, &ti->error);
@@ -1831,7 +1817,12 @@ void ssdcache_format_options(struct ssdcache_ctx *sc, char *optstr)
}
/*
- * Construct a ssdcache mapping: <target_dev_path> <cache_dev_path>
+ * Construct an ssdcache mapping:
+ *
+ * ssdcache <target_dev_path> <cache_dev_path>
+ * [blocksize <value>] [assoc <value>] [writeback|writethrough|readcache]
+ * [options <#option args> [lfu|lru] [async_lookup] [queue_busy]
+ * [disable_writeback] [skip_write_insert] [evict_on_write] [cmd_preload] ]
*/
static int ssdcache_ctr(struct dm_target *ti, unsigned int argc, char **argv)
{
@@ -1849,35 +1840,35 @@ static int ssdcache_ctr(struct dm_target *ti, unsigned int argc, char **argv)
sc = kzalloc(sizeof(*sc), GFP_KERNEL);
if (sc == NULL) {
- ti->error = "dm-ssdcache: Cannot allocate ssdcache context";
+ ti->error = "Cannot allocate ssdcache context";
return -ENOMEM;
}
devname = dm_shift_arg(&as);
if (!devname) {
- ti->error = "dm-ssdcache: Target device is not specified";
+ ti->error = "Target device is not specified";
r = -EINVAL;
goto bad;
}
if (dm_get_device(ti, devname, dm_table_get_mode(ti->table),
&sc->target_dev)) {
- ti->error = "dm-ssdcache: Target device lookup failed";
+ ti->error = "Target device lookup failed";
r = -EINVAL;
goto bad;
}
devname = dm_shift_arg(&as);
if (!devname) {
- ti->error = "dm-ssdcache: Cache device is not specified";
+ ti->error = "Cache device is not specified";
r = -EINVAL;
- goto bad;
+ goto bad_cache_dev;
}
if (dm_get_device(ti, devname, dm_table_get_mode(ti->table),
&sc->cache_dev)) {
- ti->error = "dm-ssdcache: Cache device lookup failed";
+ ti->error = "Cache device lookup failed";
dm_put_device(ti, sc->target_dev);
r = -EINVAL;
- goto bad;
+ goto bad_cache_dev;
}
sc->block_size = DEFAULT_BLOCKSIZE;
@@ -1968,8 +1959,9 @@ static int ssdcache_ctr(struct dm_target *ti, unsigned int argc, char **argv)
return 0;
bad_io_client:
- dm_put_device(ti, sc->target_dev);
dm_put_device(ti, sc->cache_dev);
+bad_cache_dev:
+ dm_put_device(ti, sc->target_dev);
bad:
kfree(sc);
return r;
@@ -2121,11 +2113,17 @@ static int ssdcache_status(struct dm_target *ti, status_type_t type,
static int ssdcache_iterate_devices(struct dm_target *ti,
iterate_devices_callout_fn fn, void *data)
{
+ int r = 0;
struct ssdcache_ctx *sc = ti->private;
if (!sc)
return 0;
- return fn(ti, sc->target_dev, 0, ti->len, data);
+
+ r = fn(ti, sc->cache_dev, 0, ti->len, data);
+ if (!r)
+ r = fn(ti, sc->target_dev, 0, ti->len, data);
+
+ return r;
}
static struct target_type ssdcache_target = {
More information about the dm-devel
mailing list