Skip to contentSkip to conversation

fix oops when snapshots get full

PATCH In review Chris Mason 3 participants 5 messages Oct 11, 2001 – Oct 12, 2001

lore
Share & tools
lore

Chris Mason

author
#0 lore raw

PATCH 1/1 fix oops when snapshots get full

Hi guys,

Here's a repost of the patch that fixes oopsen when snapshots get full. Please comment and/or include ;-)

Against 1.0.1rc4, the bug should affect all kernel versions.

-chris

drivers/md/lvm.c Mon, 08 Oct 2001 15:54:42 -0400 renamed from drivers/md/lvm.c Sun, 07 Oct 2001 22:15:54 -0400 +5 -3
@@ -1142,7 +1142,8 @@
 
 	/* we must redo lvm_snapshot_remap_block in order to avoid a
 	   race condition in the gap where no lock was held */
-	if (!lvm_snapshot_remap_block(&rdev, &rsector, pe_start, lv) &&
+	if (lv->lv_block_exception && 
+	    !lvm_snapshot_remap_block(&rdev, &rsector, pe_start, lv) &&
 	    !lvm_snapshot_COW(rdev, rsector, pe_start, rsector, vg, lv))
 		lvm_write_COW_table_block(vg, lv);
 

@@ -1151,11 +1152,12 @@
 
 static inline void _remap_snapshot(kdev_t rdev, ulong rsector,
 				   ulong pe_start, lv_t *lv, vg_t *vg) {
-	int r;
+	int r = 0;
 
 	/* check to see if this chunk is already in the snapshot */
 	down_read(&lv->lv_lock);
-	r = lvm_snapshot_remap_block(&rdev, &rsector, pe_start, lv);
+	if (lv->lv_block_exception)
+		r = lvm_snapshot_remap_block(&rdev, &rsector, pe_start, lv);
 	up_read(&lv->lv_lock);
 
 	if (!r)

Index: 0.21/drivers/md/lvm-snap.c

drivers/md/lvm-snap.c Mon, 08 Oct 2001 15:13:10 -0400 root (linux/i/c/38_lvm-snap.c 1.1.2.1.2.1 644) renamed from drivers/md/lvm-snap.c Sat, 06 Oct 2001 00:07:22 -0400 root (linux/i/c/38_lvm-snap.c 1.1.2.1.2.1 644) +2 -0
@@ -140,6 +140,8 @@
 	unsigned long mask = lv->lv_snapshot_hash_mask;
 	int chunk_size = lv->lv_chunk_size;
 
+	if (!hash_table)
+		BUG() ;
 	hash_table = &hash_table[hashfn(org_dev, org_start, mask, chunk_size)];
 	list_add(&exception->hash, hash_table);
 }
#1 lore raw
Quoted 15 lines · Chris Mason
drivers/md/lvm.c Mon, 08 Oct 2001 15:54:42 -0400 renamed from drivers/md/lvm.c Sun, 07 Oct 2001 22:15:54 -0400 +2 -1
@@ -1142,7 +1142,8 @@
 
 	/* we must redo lvm_snapshot_remap_block in order to avoid a
 	   race condition in the gap where no lock was held */
-	if (!lvm_snapshot_remap_block(&rdev, &rsector, pe_start, lv) &&
+	if (lv->lv_block_exception && 
+	    !lvm_snapshot_remap_block(&rdev, &rsector, pe_start, lv) &&
 	    !lvm_snapshot_COW(rdev, rsector, pe_start, rsector, vg, lv))
 		lvm_write_COW_table_block(vg, lv);
 

@@ -1151,11 +1152,12 @@
 
 static inline void _remap_snapshot(kdev_t rdev, ulong rsector,

drivers/md/lvm.c Mon, 08 Oct 2001 15:54:42 -0400 line 1161

Quoted 10 lines
drivers/md/lvm.c Mon, 08 Oct 2001 15:54:42 -0400 renamed from drivers/md/lvm.c Sun, 07 Oct 2001 22:15:54 -0400 +3 -2
 				   ulong pe_start, lv_t *lv, vg_t *vg) {
-	int r;
+	int r = 0;
 
 	/* check to see if this chunk is already in the snapshot */
 	down_read(&lv->lv_lock);
-	r = lvm_snapshot_remap_block(&rdev, &rsector, pe_start, lv);
+	if (lv->lv_block_exception)
+		r = lvm_snapshot_remap_block(&rdev, &rsector, pe_start, lv);
 	up_read(&lv->lv_lock);

I was looking at this, and thought the following:

1) Shouldn't we just initialize r=1 in _remap_snapshot(), and then if lv->lv_block_exception is NULL we will not enter __remap_snapshot() at all? That would remove the need to get the write semaphore for no reason.
2) Initially, I thought we could just "optimize" the checking of lv->lv_block_exception, but I supposed there are race conditions in dropping the read lock and getting the write lock, so I guess we still need to check lv->lv_block_exception in __remap_snapshot() also?
3) The alternative would be to check lv->lv_block_exception inside lvm_snapshot_remap_block() instead of in the callers (returning "1" if it is NULL, and we don't want to do remap). This would avoid any problems in the future if someone else calls lvm_snapshot_remap_block() without checking lv_block_exception first. Untested patch below which should be equivalent to your previous patch.

Cheers, Andreas
=========================================================================

lvm-snap.c +9 -0
diff -u -u -r1.7 lvm-snap.c
@@ -157,6 +157,12 @@
 	list_add(&exception->hash, hash_table);
 }
 
+/*
+ * Determine if we already have a snapshot chunk for this block.
+ * Return: 1 if it the chunk already exists
+ *         0 if we need to COW this block and allocate a new chunk
+ *        -1 if the snapshot was disabled because it ran out of space
+ */
 int lvm_snapshot_remap_block(kdev_t * org_dev, unsigned long * org_sector,
 			     unsigned long pe_start, lv_t * lv)
 {

@@ -166,6 +172,9 @@
 	int chunk_size = lv->lv_chunk_size;
 	lv_block_exception_t * exception;
 
+	if (!lv->lv_block_exception)
+		return -1;
+
 	pe_off = pe_start % chunk_size;
 	pe_adjustment = (*org_sector-pe_off) % chunk_size;
 	__org_start = *org_sector - pe_adjustment;
lvm.c +2 -4
diff -u -u -r1.46 lvm.c
@@ -1365,10 +1359,8 @@
 		goto out;
 
 	if (lv->lv_access & LV_SNAPSHOT) { /* remap snapshot */
-		if (lv->lv_block_exception)
-			lvm_snapshot_remap_block(&rdev_map, &rsector_map,
-						 pe_start, lv);
-		else
+		if (lvm_snapshot_remap_block(&rdev_map, &rsector_map,
+					     pe_start, lv) < 0)
 			goto bad;
 
 	} else if (rw == WRITE || rw == WRITEA) { /* snapshot origin */

--
Andreas Dilger \ "If a man ate a pound of pasta and a pound of antipasto,
\ would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/ -- Dogbert

author
#2 lore raw
Quoted 6 lines · On Thursday, October 11, 2001 01:57:15 PM -0600 Andreas Dilger…

I was looking at this, and thought the following:

1) Shouldn't we just initialize r=1 in _remap_snapshot(), and then if lv->lv_block_exception is NULL we will not enter __remap_snapshot() at all? That would remove the need to get the write semaphore for no reason.

Good point.

Quoted 5 lines

2) Initially, I thought we could just "optimize" the checking of lv->lv_block_exception, but I supposed there are race conditions in dropping the read lock and getting the write lock, so I guess we still need to check lv->lv_block_exception in __remap_snapshot() also?

Yes, I think we do.

Quoted 6 lines

3) The alternative would be to check lv->lv_block_exception inside lvm_snapshot_remap_block() instead of in the callers (returning "1" if it is NULL, and we don't want to do remap). This would avoid any problems in the future if someone else calls lvm_snapshot_remap_block() without checking lv_block_exception first. Untested patch below which should be equivalent to your previous patch.

The only thing I don't like about this is that it makes it hides the fact that lvm_snapshot_COW and a few other calls depend on block_exception being valid. The code certainly looks right though, just a matter of style.

thanks,
Chris

#3 lore raw
Quoted 11 lines · Chris Mason
Quoted 6 lines

3) The alternative would be to check lv->lv_block_exception inside lvm_snapshot_remap_block() instead of in the callers (returning "1" if it is NULL, and we don't want to do remap). This would avoid any problems in the future if someone else calls lvm_snapshot_remap_block() without checking lv_block_exception first. Untested patch below which should be equivalent to your previous patch.

The only thing I don't like about this is that it makes it hides the fact that lvm_snapshot_COW and a few other calls depend on block_exception being valid. The code certainly looks right though, just a matter of style.

Well, lvm_snapshot_COW() is only called from one place, AFAICS, which is in __remap_snapshot() after we call lvm_snapshot_remap_block(), so we should be OK (could add a comment about this, and also the fact that you need to hold the lv_lock to call this function in the first place.

Cheers, Andreas
--
Andreas Dilger \ "If a man ate a pound of pasta and a pound of antipasto,
\ would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/ -- Dogbert