diff mbox

Re: 2.6.28.2 & dm-snapshot or kcopyd Oops

Message ID Pine.LNX.4.64.0902110552550.20020@hs20-bc2-1.build.redhat.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Mikulas Patocka Feb. 11, 2009, 10:54 a.m. UTC
> Hi
> 
> I have found another bug in dm-snapshots, I think it won't fix your crash 
> but it is a bug anyway, so it should be fixed. Apply the below patch.
> 
> I'll send you some more debug patches to pinpoint the crash you are 
> seeing.
> 
> Mikulas

Hi

Here is another patch to pinpoint your crashes, so apply it and run the 
same workload. Thanks for testing it.

Mikulas

---
 drivers/md/dm-exception-store.c |   63 +++++++++++++++++++++++++++++++++++-----
 drivers/md/dm-snap.c            |   51 +++++++++++++++++++++++++++++---
 drivers/md/dm-snap.h            |    3 +
 3 files changed, 106 insertions(+), 11 deletions(-)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Mikulas Patocka Feb. 13, 2009, 6:41 a.m. UTC | #1
Hi

Few more questions:
- do you write to the snapshots?
- do you create/delete snapshots while the crash happens?
- do all the snapshots have the same chunk size?
- how many snapshots do you have?

Mikulas

> Hi
> 
> With the two patches of yesterday, the latest debug infomation is as follow:
> 
> [  197.073630] ------------[ cut here ]------------
> [  197.073633] kernel BUG at drivers/md/dm-snap.c:405!
> [  197.073635] invalid opcode: 0000 [#1] SMP 
> [  197.073637] last sysfs file: /sys/devices/virtual/block/dm-6/dev
> [  197.073639] Modules linked in: iscsi_trgt arcmsr bonding e1000
> [  197.073642] 
> [  197.073645] Pid: 4268, comm: kcopyd Not tainted (2.6.28.2-dm #8) S5000PSL
> [  197.073647] EIP: 0060:[<c03c5da8>] EFLAGS: 00010246 CPU: 2
> [  197.073653] EIP is at free_pending_exception+0x88/0x90
> [  197.073654] EAX: c05a0ce8 EBX: f55449f0 ECX: 00000282 EDX: ef3056d0
> [  197.073656] ESI: f6c65bc0 EDI: 00000000 EBP: ef2a34e0 ESP: ef3d5ecc
> [  197.073657]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> [  197.073659] Process kcopyd (pid: 4268, ti=ef3d4000 task=f6d77bc0 task.ti=ef3d4000)
> [  197.073660] Stack:
> [  197.073661]  f55449f0 00000000 c03c6918 f75fbf70 ef2843c8 f75fbec0 00000000 ef2409c0
> [  197.073665]  00000001 00000001 ef2a34e0 c03c82ca c03c6a20 002689a4 00000000 000030d8
> [  197.073668]  00000000 f75fbf2c ef2a34e0 f75fbec0 eeded3c0 c03c69e2 ef2a34e0 eeded418
> [  197.073672] Call Trace:
> [  197.073674]  [<c03c6918>] pending_complete+0x2d8/0x360
> [  197.073677]  [<c03c82ca>] persistent_commit+0x34a/0x380
> [  197.073679]  [<c03c6a20>] commit_callback+0x0/0x40
> [  197.073681]  [<c03c69e2>] copy_callback+0x42/0x80
> [  197.073683]  [<c03c69a0>] copy_callback+0x0/0x80
> [  197.073686]  [<c03c1858>] run_complete_job+0x108/0x140
> [  197.073688]  [<c03c119c>] process_jobs+0x4c/0xe0
> [  197.073690]  [<c03c1750>] run_complete_job+0x0/0x140
> [  197.073692]  [<c03c1230>] do_work+0x0/0x50
> [  197.073694]  [<c03c124e>] do_work+0x1e/0x50
> [  197.073696]  [<c012ef32>] run_workqueue+0x72/0x100
> [  197.073700]  [<c0132570>] prepare_to_wait+0x20/0x60
> [  197.073703]  [<c012f840>] worker_thread+0x0/0xb0
> [  197.073705]  [<c012f8b9>] worker_thread+0x79/0xb0
> [  197.073707]  [<c01323d0>] autoremove_wake_function+0x0/0x50
> [  197.073710]  [<c012f840>] worker_thread+0x0/0xb0
> [  197.073712]  [<c01320d2>] kthread+0x42/0x70
> [  197.073713]  [<c0132090>] kthread+0x0/0x70
> [  197.073715]  [<c0103eff>] kernel_thread_helper+0x7/0x18
> [  197.073718] Code: 04 89 42 04 89 10 c7 41 04 00 02 20 00 c7 43 40 00 01 10 00 fe 05 94 41 65 c0 8b 56 48 89 d8 e8 3f 30 d8 ff f0 ff 4e 4c 5b 5e c3 <0f> 0b eb fe 8d 74 26 00 55 89 c5 57 56 53 83 ec 0c 8b 75 30 8b 
> [  197.073737] EIP: [<c03c5da8>] free_pending_exception+0x88/0x90 SS:ESP 0068:ef3d5ecc
> [  197.073754] ---[ end trace beb1c34d6186bec5 ]---
> 
> Jacky Kim
> .

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox

Patch

Index: linux-2.6.28-clean/drivers/md/dm-exception-store.c
===================================================================
--- linux-2.6.28-clean.orig/drivers/md/dm-exception-store.c	2009-02-11 00:39:23.000000000 +0100
+++ linux-2.6.28-clean/drivers/md/dm-exception-store.c	2009-02-11 10:33:11.000000000 +0100
@@ -619,14 +619,18 @@  struct dm_snap_pending_exception {
 	 * kcopyd.
 	 */
 	int started;
+
+	struct list_head list_all;
 };
 
+static DEFINE_SPINLOCK(callback_spinlock);
+
 static void persistent_commit(struct exception_store *store,
 			      struct dm_snap_exception *e,
 			      void (*callback) (void *, int success),
 			      void *callback_context)
 {
-	unsigned int i;
+	unsigned int i, n;
 	struct pstore *ps = get_info(store);
 	struct disk_exception de;
 	struct commit_callback *cb;
@@ -641,32 +645,41 @@  static void persistent_commit(struct exc
 	BUG_ON(pe->e.hash_list.next == LIST_POISON1);
 	BUG_ON(pe->e.hash_list.prev == LIST_POISON2);
 
-	de.old_chunk = e->old_chunk;
-	de.new_chunk = e->new_chunk;
-	write_exception(ps, ps->current_committed++, &de);
-
 	for (i = 0; i < ps->callback_count; i++) {
 		cb = ps->callbacks + i;
 		pe = cb->context;
 		BUG_ON(pe->e.hash_list.next == LIST_POISON1);
 		BUG_ON(pe->e.hash_list.prev == LIST_POISON2);
 		BUG_ON(pe == callback_context);
+		BUG_ON(pe->started != 2);
+	}
+	for (i = 0; i < ps->current_committed; i++) {
+		read_exception(ps, i, &de);
+		BUG_ON(de.old_chunk == e->old_chunk);
+		BUG_ON(de.new_chunk == e->new_chunk);
 	}
+	de.old_chunk = e->old_chunk;
+	de.new_chunk = e->new_chunk;
+	write_exception(ps, ps->current_committed++, &de);
+
 	/*
 	 * Add the callback to the back of the array.  This code
 	 * is the only place where the callback array is
 	 * manipulated, and we know that it will never be called
 	 * multiple times concurrently.
 	 */
+	spin_lock_irq(&callback_spinlock);
 	cb = ps->callbacks + ps->callback_count++;
 	cb->callback = callback;
 	cb->context = callback_context;
+	spin_unlock_irq(&callback_spinlock);
 
 	for (i = 0; i < ps->callback_count; i++) {
 		cb = ps->callbacks + i;
 		pe = cb->context;
 		BUG_ON(pe->e.hash_list.next == LIST_POISON1);
 		BUG_ON(pe->e.hash_list.prev == LIST_POISON2);
+		BUG_ON(pe->started != 2);
 	}
 	/*
 	 * If there are exceptions in flight and we have not yet
@@ -677,6 +690,7 @@  static void persistent_commit(struct exc
 		pe = callback_context;
 		BUG_ON(pe->e.hash_list.next == LIST_POISON1);
 		BUG_ON(pe->e.hash_list.prev == LIST_POISON2);
+		BUG_ON(pe->started != 2);
 		return;
 	}
 	for (i = 0; i < ps->callback_count; i++) {
@@ -684,6 +698,7 @@  static void persistent_commit(struct exc
 		pe = cb->context;
 		BUG_ON(pe->e.hash_list.next == LIST_POISON1);
 		BUG_ON(pe->e.hash_list.prev == LIST_POISON2);
+		BUG_ON(pe->started != 2);
 	}
 
 
@@ -714,17 +729,45 @@  static void persistent_commit(struct exc
 		pe = cb->context;
 		BUG_ON(pe->e.hash_list.next == LIST_POISON1);
 		BUG_ON(pe->e.hash_list.prev == LIST_POISON2);
+		BUG_ON(pe->started != 2);
 	}
 
-	for (i = 0; i < ps->callback_count; i++) {
+	spin_lock_irq(&callback_spinlock);
+	n = ps->callback_count;
+	ps->callback_count = 0;
+	spin_unlock_irq(&callback_spinlock);
+	for (i = 0; i < n; i++) {
 		cb = ps->callbacks + i;
 		pe = cb->context;
+		BUG_ON(pe->started != 2);
 		BUG_ON(pe->e.hash_list.next == LIST_POISON1);
 		BUG_ON(pe->e.hash_list.prev == LIST_POISON2);
 		cb->callback(cb->context, ps->valid);
 	}
+}
 
-	ps->callback_count = 0;
+static void persistent_check(struct exception_store *store, void *context, int line)
+{
+	struct pstore *ps = get_info(store);
+	unsigned long flags;
+	unsigned i;
+	struct commit_callback *cb;
+	struct dm_snap_pending_exception *pe;
+
+	spin_lock_irqsave(&callback_spinlock, flags);
+	for (i = 0; i < ps->callback_count; i++) {
+		cb = ps->callbacks + i;
+		pe = cb->context;
+		if (pe == context) {
+			printk("CALLED FROM LINE %d\n", line);
+			BUG();
+		}
+		if (pe->started != 2) {
+			printk("CALLED FROM LINE %d\n", line);
+			BUG();
+		}
+	}
+	spin_unlock_irqrestore(&callback_spinlock, flags);
 }
 
 static void persistent_drop(struct exception_store *store)
@@ -769,6 +812,7 @@  int dm_create_persistent(struct exceptio
 	store->read_metadata = persistent_read_metadata;
 	store->prepare_exception = persistent_prepare;
 	store->commit_exception = persistent_commit;
+	store->check_pending_exception = persistent_check;
 	store->drop_snapshot = persistent_drop;
 	store->fraction_full = persistent_fraction_full;
 	store->context = ps;
@@ -824,6 +868,10 @@  static void transient_fraction_full(stru
 	*denominator = get_dev_size(store->snap->cow->bdev);
 }
 
+static void transient_check(struct exception_store *store, void *context, int line)
+{
+}
+
 int dm_create_transient(struct exception_store *store)
 {
 	struct transient_c *tc;
@@ -832,6 +880,7 @@  int dm_create_transient(struct exception
 	store->read_metadata = transient_read_metadata;
 	store->prepare_exception = transient_prepare;
 	store->commit_exception = transient_commit;
+	store->check_pending_exception = transient_check;
 	store->drop_snapshot = NULL;
 	store->fraction_full = transient_fraction_full;
 
Index: linux-2.6.28-clean/drivers/md/dm-snap.h
===================================================================
--- linux-2.6.28-clean.orig/drivers/md/dm-snap.h	2009-02-11 00:39:36.000000000 +0100
+++ linux-2.6.28-clean/drivers/md/dm-snap.h	2009-02-11 09:54:45.000000000 +0100
@@ -114,6 +114,9 @@  struct exception_store {
 				  void (*callback) (void *, int success),
 				  void *callback_context);
 
+	void (*check_pending_exception)(struct exception_store *store,
+				void *callback_context, int line);
+
 	/*
 	 * The snapshot is invalid, note this in the metadata.
 	 */
Index: linux-2.6.28-clean/drivers/md/dm-snap.c
===================================================================
--- linux-2.6.28-clean.orig/drivers/md/dm-snap.c	2009-02-11 03:15:26.000000000 +0100
+++ linux-2.6.28-clean/drivers/md/dm-snap.c	2009-02-11 10:20:55.000000000 +0100
@@ -88,6 +88,8 @@  struct dm_snap_pending_exception {
 	 * kcopyd.
 	 */
 	int started;
+
+	struct list_head list_all;
 };
 
 /*
@@ -365,20 +367,45 @@  static void free_exception(struct dm_sna
 	kmem_cache_free(exception_cache, e);
 }
 
+static DEFINE_SPINLOCK(pending_spinlock);
+static LIST_HEAD(pending_all);
+
 static struct dm_snap_pending_exception *alloc_pending_exception(struct dm_snapshot *s)
 {
 	struct dm_snap_pending_exception *pe = mempool_alloc(s->pending_pool,
 							     GFP_NOIO);
+	struct dm_snap_pending_exception *pe2;
 
 	atomic_inc(&s->pending_exceptions_count);
 	pe->snap = s;
 
+	spin_lock(&pending_spinlock);
+	list_for_each_entry(pe2, &pending_all, list_all) {
+		BUG_ON(pe2 == pe);
+	}
+	list_add(&pe->list_all, &pending_all);
+	spin_unlock(&pending_spinlock);
+
+	s->store.check_pending_exception(&s->store, pe, __LINE__);
+
 	return pe;
 }
 
 static void free_pending_exception(struct dm_snap_pending_exception *pe)
 {
 	struct dm_snapshot *s = pe->snap;
+	struct dm_snap_pending_exception *pe2;
+
+	s->store.check_pending_exception(&s->store, pe, __LINE__);
+
+	spin_lock(&pending_spinlock);
+	list_for_each_entry(pe2, &pending_all, list_all) {
+		if (pe2 == pe) goto found;
+	}
+	BUG();
+	found:
+	list_del(&pe->list_all);
+	spin_unlock(&pending_spinlock);
 
 	mempool_free(pe, s->pending_pool);
 	smp_mb__before_atomic_dec();
@@ -831,6 +858,8 @@  static struct bio *put_pending_exception
 	struct dm_snap_pending_exception *primary_pe;
 	struct bio *origin_bios = NULL;
 
+	pe->snap->store.check_pending_exception(&pe->snap->store, pe, __LINE__);
+
 	primary_pe = pe->primary_pe;
 
 	/*
@@ -862,12 +891,15 @@  static void pending_complete(struct dm_s
 	struct bio *snapshot_bios = NULL;
 	int error = 0;
 
+	s->store.check_pending_exception(&s->store, pe, __LINE__);
+
 	BUG_ON(pe->e.hash_list.next == LIST_POISON1);
 	BUG_ON(pe->e.hash_list.prev == LIST_POISON2);
 
 	if (!success) {
 		/* Read/write error - snapshot is unusable */
 		down_write(&s->lock);
+		s->store.check_pending_exception(&s->store, pe, __LINE__);
 		__invalidate_snapshot(s, -EIO);
 	BUG_ON(pe->e.hash_list.next == LIST_POISON1);
 	BUG_ON(pe->e.hash_list.prev == LIST_POISON2);
@@ -885,6 +917,7 @@  static void pending_complete(struct dm_s
 
 	if (!e) {
 		down_write(&s->lock);
+		s->store.check_pending_exception(&s->store, pe, __LINE__);
 		__invalidate_snapshot(s, -ENOMEM);
 		error = 1;
 		goto out;
@@ -892,6 +925,7 @@  static void pending_complete(struct dm_s
 	*e = pe->e;
 
 	down_write(&s->lock);
+	s->store.check_pending_exception(&s->store, pe, __LINE__);
 	BUG_ON(pe->e.hash_list.next == LIST_POISON1);
 	BUG_ON(pe->e.hash_list.prev == LIST_POISON2);
 
@@ -943,6 +977,7 @@  static void commit_callback(void *contex
 {
 	struct dm_snap_pending_exception *pe = context;
 
+	pe->snap->store.check_pending_exception(&pe->snap->store, pe, __LINE__);
 	BUG_ON(pe->e.hash_list.next == LIST_POISON1);
 	BUG_ON(pe->e.hash_list.prev == LIST_POISON2);
 
@@ -961,13 +996,15 @@  static void copy_callback(int read_err, 
 	BUG_ON(pe->e.hash_list.next == LIST_POISON1);
 	BUG_ON(pe->e.hash_list.prev == LIST_POISON2);
 
-	if (read_err || write_err)
+	if (read_err || write_err) {
+		s->store.check_pending_exception(&s->store, pe, __LINE__);
 		pending_complete(pe, 0);
-
-	else
+	} else {
+		s->store.check_pending_exception(&s->store, pe, __LINE__);
 		/* Update the metadata if we are persistent */
 		s->store.commit_exception(&s->store, &pe->e, commit_callback,
 					  pe);
+	}
 }
 
 /*
@@ -979,6 +1016,7 @@  static void start_copy(struct dm_snap_pe
 	struct dm_io_region src, dest;
 	struct block_device *bdev = s->origin->bdev;
 	sector_t dev_size;
+	s->store.check_pending_exception(&s->store, pe, __LINE__);
 	BUG_ON(!pe->started);
 	BUG_ON(pe->started == 2);
 	BUG_ON(pe->started != 1);
@@ -1038,6 +1076,7 @@  __find_pending_exception(struct dm_snaps
 	up_write(&s->lock);
 	pe = alloc_pending_exception(s);
 	down_write(&s->lock);
+	s->store.check_pending_exception(&s->store, pe, __LINE__);
 
 	if (!s->valid) {
 		free_pending_exception(pe);
@@ -1071,6 +1110,7 @@  __find_pending_exception(struct dm_snaps
 
 	get_pending_exception(pe);
 	insert_exception(&s->pending, &pe->e);
+	s->store.check_pending_exception(&s->store, pe, __LINE__);
 
  out:
 	return pe;
@@ -1291,6 +1331,7 @@  static int __origin_write(struct list_he
 		}
 
 		if (!pe->started) {
+			snap->store.check_pending_exception(&snap->store, pe, __LINE__);
 			pe->started = 1;
 			list_add_tail(&pe->list, &pe_queue);
 		}
@@ -1319,8 +1360,10 @@  static int __origin_write(struct list_he
 	/*
 	 * Now that we have a complete pe list we can start the copying.
 	 */
-	list_for_each_entry_safe(pe, next_pe, &pe_queue, list)
+	list_for_each_entry_safe(pe, next_pe, &pe_queue, list) {
+		pe->snap->store.check_pending_exception(&pe->snap->store, pe, __LINE__);
 		start_copy(pe);
+	}
 
 	return r;
 }