Re: [linux-lvm] [PATCH] fix oops when snapshots get full

[Andreas Dilger <adilger@turbolabs.com>]

On Oct 11, 2001  13:49 -0400, Chris Mason wrote:
> --- 0.21/drivers/md/lvm.c Sun, 07 Oct 2001 22:15:54 -0400 
> +++ 0.21(w)/drivers/md/lvm.c Mon, 08 Oct 2001 15:54:42 -0400 
> @@ -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);

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
=========================================================================
diff -u -u -r1.7 lvm-snap.c
--- kernel/lvm-snap.c	2001/09/27 08:34:43	1.7
+++ kernel/lvm-snap.c	2001/10/11 19:55:02
@@ -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;
diff -u -u -r1.46 lvm.c
--- kernel/lvm.c	2001/10/02 21:14:41	1.46
+++ kernel/lvm.c	2001/10/11 19:55:03
@@ -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