From 6d45d93ead319423099b82a4efd775bc0f159121 Mon Sep 17 00:00:00 2001 From: Mikulas Patocka Date: Fri, 16 Oct 2009 23:18:14 +0100 Subject: dm snapshot: sort by chunk size to fix race Avoid a race causing corruption when snapshots of the same origin have different chunk sizes by sorting the internal list of snapshots by chunk size, largest first. https://bugzilla.redhat.com/show_bug.cgi?id=182659 For example, let's have two snapshots with different chunk sizes. The first snapshot (1) has small chunk size and the second snapshot (2) has large chunk size. Let's have chunks A, B, C in these snapshots: snapshot1: ====A==== ====B==== snapshot2: ==========C========== (Chunk size is a power of 2. Chunks are aligned.) A write to the origin at a position within A and C comes along. It triggers reallocation of A, then reallocation of C and links them together using A as the 'primary' exception. Then another write to the origin comes along at a position within B and C. It creates pending exception for B. C already has a reallocation in progress and it already has a primary exception (A), so nothing is done to it: B and C are not linked. If the reallocation of B finishes before the reallocation of C, because there is no link with the pending exception for C it does not know to wait for it and, the second write is dispatched to the origin and causes data corruption in the chunk C in snapshot2. To avoid this situation, we maintain snapshots sorted in descending order of chunk size. This leads to a guaranteed ordering on the links between the pending exceptions and avoids the problem explained above - both A and B now get linked to C. Cc: stable@kernel.org Signed-off-by: Mikulas Patocka Signed-off-by: Alasdair G Kergon --- drivers/md/dm-snap.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'drivers/md/dm-snap.c') diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c index 57f1bf7f3b7..3a53a5a9bec 100644 --- a/drivers/md/dm-snap.c +++ b/drivers/md/dm-snap.c @@ -296,6 +296,7 @@ static void __insert_origin(struct origin *o) */ static int register_snapshot(struct dm_snapshot *snap) { + struct dm_snapshot *l; struct origin *o, *new_o; struct block_device *bdev = snap->origin->bdev; @@ -319,7 +320,11 @@ static int register_snapshot(struct dm_snapshot *snap) __insert_origin(o); } - list_add_tail(&snap->list, &o->snapshots); + /* Sort the list according to chunk size, largest-first smallest-last */ + list_for_each_entry(l, &o->snapshots, list) + if (l->store->chunk_size < snap->store->chunk_size) + break; + list_add_tail(&snap->list, &l->list); up_write(&_origins_lock); return 0; -- cgit v1.2.3 From 034a186d29dbcef099e57ab23ec39440596be911 Mon Sep 17 00:00:00 2001 From: Jonathan Brassow Date: Fri, 16 Oct 2009 23:18:14 +0100 Subject: dm snapshot: free exception store on init failure While initializing the snapshot module, if we fail to register the snapshot target then we must back-out the exception store module initialization. Cc: stable@kernel.org Signed-off-by: Jonathan Brassow Reviewed-by: Mikulas Patocka Reviewed-by: Mike Snitzer Signed-off-by: Alasdair G Kergon --- drivers/md/dm-snap.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'drivers/md/dm-snap.c') diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c index 3a53a5a9bec..53f4063f7ea 100644 --- a/drivers/md/dm-snap.c +++ b/drivers/md/dm-snap.c @@ -1470,7 +1470,7 @@ static int __init dm_snapshot_init(void) r = dm_register_target(&snapshot_target); if (r) { DMERR("snapshot target register failed %d", r); - return r; + goto bad_register_snapshot_target; } r = dm_register_target(&origin_target); @@ -1527,6 +1527,9 @@ bad2: dm_unregister_target(&origin_target); bad1: dm_unregister_target(&snapshot_target); + +bad_register_snapshot_target: + dm_exception_store_exit(); return r; } -- cgit v1.2.3 From 3f2412dc85260e5aae7ebb03bf50d5b1407e3083 Mon Sep 17 00:00:00 2001 From: Mikulas Patocka Date: Fri, 16 Oct 2009 23:18:16 +0100 Subject: dm snapshot: require non zero chunk size by end of ctr If we are creating snapshot with memory-stored exception store, fail if the user didn't specify chunk size. Zero chunk size would probably crash a lot of places in the rest of snapshot code. Cc: stable@kernel.org Signed-off-by: Mikulas Patocka Reviewed-by: Jonathan Brassow Reviewed-by: Mike Snitzer Signed-off-by: Alasdair G Kergon --- drivers/md/dm-snap.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'drivers/md/dm-snap.c') diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c index 53f4063f7ea..9cb392b3e92 100644 --- a/drivers/md/dm-snap.c +++ b/drivers/md/dm-snap.c @@ -673,6 +673,11 @@ static int snapshot_ctr(struct dm_target *ti, unsigned int argc, char **argv) bio_list_init(&s->queued_bios); INIT_WORK(&s->queued_bios_work, flush_queued_bios); + if (!s->store->chunk_size) { + ti->error = "Chunk size not set"; + goto bad_load_and_register; + } + /* Add snapshot to the list of snapshots for this origin */ /* Exceptions aren't triggered till snapshot_resume() is called */ if (register_snapshot(s)) { -- cgit v1.2.3 From 4c6fff445d7aa753957856278d4d93bcad6e2c14 Mon Sep 17 00:00:00 2001 From: Mikulas Patocka Date: Fri, 16 Oct 2009 23:18:16 +0100 Subject: dm snapshot: lock snapshot while supplying status This patch locks the snapshot when returning status. It fixes a race when it could return an invalid number of free chunks if someone was simultaneously modifying it. Cc: stable@kernel.org Signed-off-by: Mikulas Patocka Signed-off-by: Alasdair G Kergon --- drivers/md/dm-snap.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'drivers/md/dm-snap.c') diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c index 9cb392b3e92..9bc814aa2bb 100644 --- a/drivers/md/dm-snap.c +++ b/drivers/md/dm-snap.c @@ -1152,6 +1152,8 @@ static int snapshot_status(struct dm_target *ti, status_type_t type, unsigned sz = 0; struct dm_snapshot *snap = ti->private; + down_write(&snap->lock); + switch (type) { case STATUSTYPE_INFO: if (!snap->valid) @@ -1183,6 +1185,8 @@ static int snapshot_status(struct dm_target *ti, status_type_t type, break; } + up_write(&snap->lock); + return 0; } -- cgit v1.2.3 From df96eee679ba28c98cf722fa7c9f4286ee1ed0bd Mon Sep 17 00:00:00 2001 From: Mikulas Patocka Date: Fri, 16 Oct 2009 23:18:17 +0100 Subject: dm snapshot: use unsigned integer chunk size Use unsigned integer chunk size. Maximum chunk size is 512kB, there won't ever be need to use 4GB chunk size, so the number can be 32-bit. This fixes compiler failure on 32-bit systems with large block devices. Cc: stable@kernel.org Signed-off-by: Mikulas Patocka Signed-off-by: Mike Snitzer Reviewed-by: Jonathan Brassow Signed-off-by: Alasdair G Kergon --- drivers/md/dm-snap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/md/dm-snap.c') diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c index 9bc814aa2bb..3a3ba46e6d4 100644 --- a/drivers/md/dm-snap.c +++ b/drivers/md/dm-snap.c @@ -961,7 +961,7 @@ static void start_copy(struct dm_snap_pending_exception *pe) src.bdev = bdev; src.sector = chunk_to_sector(s->store, pe->e.old_chunk); - src.count = min(s->store->chunk_size, dev_size - src.sector); + src.count = min((sector_t)s->store->chunk_size, dev_size - src.sector); dest.bdev = s->store->cow->bdev; dest.sector = chunk_to_sector(s->store, pe->e.new_chunk); @@ -1402,7 +1402,7 @@ static void origin_resume(struct dm_target *ti) struct dm_dev *dev = ti->private; struct dm_snapshot *snap; struct origin *o; - chunk_t chunk_size = 0; + unsigned chunk_size = 0; down_read(&_origins_lock); o = __lookup_origin(dev->bdev); -- cgit v1.2.3