From agk@redhat.com Tue May 6 15:30:41 2008 Date: Tue, 6 May 2008 20:29:41 +0100 From: Alasdair G Kergon To: Mikulas Patocka Cc: Milan Broz Subject: Re: snapshot merging First patch: dm-snapshot-track-snapshot-reads.patch There's nothing in the tracking that's specific to reads, so I dropped that from the name and factored out insert/remove functions. [I believe this tracking needs moving into the core of dm eventually to share with other targets and to be used with suspend.] Upstream frowns on unnecessary braces - I removed a couple of pairs. if (x) one_statement; not if (x) { one_statement; } The KMEM_CACHE() arg becomes visible to userspace sysadmins so needs to be meaningful and indicate where it came from - forces a dm_snap prefix on that struct! Alasdair. From: Mikulas Patocka Whenever a snapshot read gets mapped through to the origin, track it in a per-snapshot hash table indexed by chunk number, using memory allocated from a new per-snapshot mempool. We need to track these reads to avoid race conditions which will be fixed by patches that follow. Signed-off-by: Mikulas Patocka Signed-off-by: Alasdair G Kergon --- drivers/md/dm-snap.c | 104 +++++++++++++++++++++++++++++++++++++++++++++------ drivers/md/dm-snap.h | 8 +++ 2 files changed, 100 insertions(+), 12 deletions(-) Index: linux-2.6.26-rc4/drivers/md/dm-snap.c =================================================================== --- linux-2.6.26-rc4.orig/drivers/md/dm-snap.c 2008-06-02 23:11:23.000000000 +0200 +++ linux-2.6.26-rc4/drivers/md/dm-snap.c 2008-06-03 16:26:03.000000000 +0200 @@ -40,6 +40,11 @@ */ #define SNAPSHOT_PAGES (((1UL << 20) >> PAGE_SHIFT) ? : 1) +/* + * The size of mempool + */ +#define MIN_IOS 256 + static struct workqueue_struct *ksnapd; static void flush_queued_bios(struct work_struct *work); @@ -93,6 +98,41 @@ static struct kmem_cache *exception_cach static struct kmem_cache *pending_cache; static mempool_t *pending_pool; +struct dm_snap_tracked_chunk { + struct hlist_node node; + chunk_t chunk; +}; + +static struct kmem_cache *tracked_chunk_cache; + +static struct dm_snap_tracked_chunk *track_chunk(struct dm_snapshot *s, + chunk_t chunk) +{ + struct dm_snap_tracked_chunk *c = mempool_alloc(s->tracked_chunk_pool, + GFP_NOIO); + unsigned long flags; + + c->chunk = chunk; + + spin_lock_irqsave(&s->tracked_chunk_lock, flags); + hlist_add_head(&c->node, + &s->tracked_chunk_hash[DM_TRACKED_CHUNK_HASH(chunk)]); + spin_unlock_irqrestore(&s->tracked_chunk_lock, flags); + + return c; +} + +static void stop_tracking_chunk(struct dm_snapshot *s, struct dm_snap_tracked_chunk *c) +{ + unsigned long flags; + + spin_lock_irqsave(&s->tracked_chunk_lock, flags); + hlist_del(&c->node); + spin_unlock_irqrestore(&s->tracked_chunk_lock, flags); + + mempool_free(c, s->tracked_chunk_pool); +} + /* * One of these per registered origin, held in the snapshot_origins hash */ @@ -482,6 +522,7 @@ static int set_chunk_size(struct dm_snap static int snapshot_ctr(struct dm_target *ti, unsigned int argc, char **argv) { struct dm_snapshot *s; + int i; int r = -EINVAL; char persistent; char *origin_path; @@ -564,11 +605,22 @@ static int snapshot_ctr(struct dm_target goto bad5; } + s->tracked_chunk_pool = mempool_create_slab_pool(MIN_IOS, tracked_chunk_cache); + if (!s->tracked_chunk_pool) { + ti->error = "Could not allocate tracked_chunk mempool for tracking reads"; + goto bad6; + } + + for (i = 0; i < DM_TRACKED_CHUNK_HASH_SIZE; i++) + INIT_HLIST_HEAD(&s->tracked_chunk_hash[i]); + + spin_lock_init(&s->tracked_chunk_lock); + /* Metadata must only be loaded into one table at once */ r = s->store.read_metadata(&s->store); if (r < 0) { ti->error = "Failed to read snapshot metadata"; - goto bad6; + goto bad7; } else if (r > 0) { s->valid = 0; DMWARN("Snapshot is marked invalid."); @@ -582,7 +634,7 @@ static int snapshot_ctr(struct dm_target if (register_snapshot(s)) { r = -EINVAL; ti->error = "Cannot register snapshot origin"; - goto bad6; + goto bad7; } ti->private = s; @@ -590,6 +642,9 @@ static int snapshot_ctr(struct dm_target return 0; + bad7: + mempool_destroy(s->tracked_chunk_pool); + bad6: dm_kcopyd_client_destroy(s->kcopyd_client); @@ -624,6 +679,7 @@ static void __free_exceptions(struct dm_ static void snapshot_dtr(struct dm_target *ti) { + int i; struct dm_snapshot *s = ti->private; flush_workqueue(ksnapd); @@ -632,6 +688,11 @@ static void snapshot_dtr(struct dm_targe /* After this returns there can be no new kcopyd jobs. */ unregister_snapshot(s); + for (i = 0; i < DM_TRACKED_CHUNK_HASH_SIZE; i++) + BUG_ON(!hlist_empty(&s->tracked_chunk_hash[i])); + + mempool_destroy(s->tracked_chunk_pool); + __free_exceptions(s); dm_put_device(ti, s->origin); @@ -974,14 +1035,10 @@ static int snapshot_map(struct dm_target start_copy(pe); goto out; } - } else - /* - * FIXME: this read path scares me because we - * always use the origin when we have a pending - * exception. However I can't think of a - * situation where this is wrong - ejt. - */ + } else { bio->bi_bdev = s->origin->bdev; + map_context->ptr = track_chunk(s, chunk); + } out_unlock: up_write(&s->lock); @@ -989,6 +1046,18 @@ static int snapshot_map(struct dm_target return r; } +static int snapshot_end_io(struct dm_target *ti, struct bio *bio, + int error, union map_info *map_context) +{ + struct dm_snapshot *s = ti->private; + struct dm_snap_tracked_chunk *c = map_context->ptr; + + if (c) + stop_tracking_chunk(s, c); + + return 0; +} + static void snapshot_resume(struct dm_target *ti) { struct dm_snapshot *s = ti->private; @@ -1266,6 +1335,7 @@ static struct target_type snapshot_targe .ctr = snapshot_ctr, .dtr = snapshot_dtr, .map = snapshot_map, + .end_io = snapshot_end_io, .resume = snapshot_resume, .status = snapshot_status, }; @@ -1306,24 +1376,33 @@ static int __init dm_snapshot_init(void) goto bad4; } + tracked_chunk_cache = KMEM_CACHE(dm_snap_tracked_chunk, 0); + if (!tracked_chunk_cache) { + DMERR("Couldn't create chunk in use cache."); + r = -ENOMEM; + goto bad5; + } + pending_pool = mempool_create_slab_pool(128, pending_cache); if (!pending_pool) { DMERR("Couldn't create pending pool."); r = -ENOMEM; - goto bad5; + goto bad6; } ksnapd = create_singlethread_workqueue("ksnapd"); if (!ksnapd) { DMERR("Failed to create ksnapd workqueue."); r = -ENOMEM; - goto bad6; + goto bad7; } return 0; - bad6: + bad7: mempool_destroy(pending_pool); + bad6: + kmem_cache_destroy(tracked_chunk_cache); bad5: kmem_cache_destroy(pending_cache); bad4: @@ -1355,6 +1434,7 @@ static void __exit dm_snapshot_exit(void mempool_destroy(pending_pool); kmem_cache_destroy(pending_cache); kmem_cache_destroy(exception_cache); + kmem_cache_destroy(tracked_chunk_cache); } /* Module hooks */ Index: linux-2.6.26-rc4/drivers/md/dm-snap.h =================================================================== --- linux-2.6.26-rc4.orig/drivers/md/dm-snap.h 2008-06-02 23:11:23.000000000 +0200 +++ linux-2.6.26-rc4/drivers/md/dm-snap.h 2008-06-03 16:26:03.000000000 +0200 @@ -130,6 +130,9 @@ struct exception_store { void *context; }; +#define DM_TRACKED_CHUNK_HASH_SIZE 16 +#define DM_TRACKED_CHUNK_HASH(x) ((unsigned long)(x) & (DM_TRACKED_CHUNK_HASH_SIZE - 1)) + struct dm_snapshot { struct rw_semaphore lock; struct dm_target *ti; @@ -174,6 +177,11 @@ struct dm_snapshot { /* Queue of snapshot writes for ksnapd to flush */ struct bio_list queued_bios; struct work_struct queued_bios_work; + + /* Chunks with outstanding reads */ + mempool_t *tracked_chunk_pool; + spinlock_t tracked_chunk_lock; + struct hlist_head tracked_chunk_hash[DM_TRACKED_CHUNK_HASH_SIZE]; }; /*