[1,of,2] DM Snapshot: remove redundant valid test

Commit Message

Jonthan Brassow Sept. 23, 2009, 3:19 p.m. UTC

Patch name: dm-snapshot-remove-redundant-valid-test.patch

There is another valid test when we take the lock, the previous test can
be dropped.

Signed-off-by: Mikulas Patocka <[e-mail]>
Reviewed-by: Jonathan Brassow <[e-mail]>
Reviewed-by: Mike Snitzer <[e-mail]>


--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Alasdair G Kergon Sept. 24, 2009, 12:41 p.m. UTC | #1

On Wed, Sep 23, 2009 at 10:19:08AM -0500, Jon Brassow wrote:
> Patch name: dm-snapshot-remove-redundant-valid-test.patch
> There is another valid test when we take the lock, the previous test can
> be dropped.
 
I'm going to leave this one for now, until I've seen the performance analysis.
- Is the argument here that the gain from not checking it twice while the snapshot
is valid outweighs the cost of the extra locking when the snapshot is full and
pending I/O is being failed as rapidly as possible and by that time there'll be
no other threads fighting for that lock?

Alasdair

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Mikulas Patocka Sept. 24, 2009, 2:05 p.m. UTC | #2

On Thu, 24 Sep 2009, Alasdair G Kergon wrote:

> On Wed, Sep 23, 2009 at 10:19:08AM -0500, Jon Brassow wrote:
> > Patch name: dm-snapshot-remove-redundant-valid-test.patch
> > There is another valid test when we take the lock, the previous test can
> > be dropped.
>  I'm going to leave this one for now, until I've seen the performance 
> analysis. - Is the argument here that the gain from not checking it 
> twice while the snapshot is valid outweighs the cost of the extra 
> locking when the snapshot is full and pending I/O is being failed as 
> rapidly as possible and by that time there'll be no other threads 
> fighting for that lock?

The argument is that it is pointless to check the same variable twice. 
This is not about execution performance, this is about programmer 
performance --- everybody who looks at the code starts thinking "oh, why 
is it checked twice, what's hapenning there?". So the need for double 
checking should be either explained or it should be removed.

Performance of I/O on full snapshot is irrelevant, it doesn't matter if 
you return error few tens or hundreds of cycles earlies or later, the data 
will be lost anyway :)

Mikulas

> Alasdair
> 

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Patch

Index: linux-2.6/drivers/md/dm-snap.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-snap.c
+++ linux-2.6/drivers/md/dm-snap.c
@@ -1043,15 +1043,11 @@  static int snapshot_map(struct dm_target
 
 	chunk = sector_to_chunk(s->store, bio->bi_sector);
 
-	/* Full snapshots are not usable */
-	/* To get here the table must be live so s->active is always set. */
-	if (!s->valid)
-		return -EIO;
-
 	/* FIXME: should only take write lock if we need
 	 * to copy an exception */
 	down_write(&s->lock);
 
+	/* Full snapshots are not usable */
 	if (!s->valid) {
 		r = -EIO;
 		goto out_unlock;