Hi

While working on snapshot merging, I found that there is "active" entry in
struct dm_snapshot and it looks that it has no meaning:


This variable is set to 0 on snapshot creation in snapshot_ctr
It is set to 1 on snapshot resume (call is made after every creation)
This variable is never set to 0 again.

This variable is read only in __origin_write and snapshots with active ==
0 are skipped.


It looks to me, like it could be removed, the origin is resumed after all
snapshots, so there is no window where active could be "0" in
__origin_write. Furthermore, if such condition existed, writing to origin
without writing to snapshot would damage the snapshot --- so even if
"active" ever has some effect, it would only result in data corruption.


I need it to be removed for merging, I start merging when snapshot is
resumed and at this point all other snapshots must be able to accept new
exceptions, if I excluded a snapshot, it would be corrupted.


Would you agree with this patch to remove the "active" field, or do you
see some reason for it?


Mikulas

---
drivers/md/dm-snap.c | 17 ++---------------
drivers/md/dm-snap.h | 3 ---
2 files changed, 2 insertions(+), 18 deletions(-)

Index: linux-2.6.25/drivers/md/dm-snap.c
================================================== =================
--- linux-2.6.25.orig/drivers/md/dm-snap.c 2008-05-01 17:16:02.000000000 +0200
+++ linux-2.6.25/drivers/md/dm-snap.c 2008-05-01 17:17:41.000000000 +0200
@@ -557,7 +557,6 @@ static int snapshot_ctr(struct dm_target
s->type = persistent;

s->valid = 1;
- s->active = 0;
s->last_percent = 0;
init_rwsem(&s->lock);
s->ti = ti;
@@ -616,7 +615,6 @@ static int snapshot_ctr(struct dm_target
}

/* Add snapshot to the list of snapshots for this origin */
- /* Exceptions aren't triggered till snapshot_resume() is called */
if (register_snapshot(s)) {
r = -EINVAL;
ti->error = "Cannot register snapshot origin";
@@ -954,7 +952,6 @@ static int snapshot_map(struct dm_target
chunk = sector_to_chunk(s, 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;

@@ -1027,15 +1024,6 @@ static int snapshot_end_io(struct dm_tar
return 0;
}

-static void snapshot_resume(struct dm_target *ti)
-{
- struct dm_snapshot *s = ti->private;
-
- down_write(&s->lock);
- s->active = 1;
- up_write(&s->lock);
-}
-
static int snapshot_status(struct dm_target *ti, status_type_t type,
char *result, unsigned int maxlen)
{
@@ -1099,8 +1087,8 @@ static int __origin_write(struct list_he

down_write(&snap->lock);

- /* Only deal with valid and active snapshots */
- if (!snap->valid || !snap->active)
+ /* Only deal with valid snapshots */
+ if (!snap->valid)
goto next_snapshot;

/* Nothing to do if writing beyond end of snapshot */
@@ -1288,7 +1276,6 @@ static struct target_type snapshot_targe
.dtr = snapshot_dtr,
.map = snapshot_map,
.end_io = snapshot_end_io,
- .resume = snapshot_resume,
.status = snapshot_status,
};

Index: linux-2.6.25/drivers/md/dm-snap.h
================================================== =================
--- linux-2.6.25.orig/drivers/md/dm-snap.h 2008-05-01 17:15:37.000000000 +0200
+++ linux-2.6.25/drivers/md/dm-snap.h 2008-05-01 17:16:30.000000000 +0200
@@ -151,9 +151,6 @@ struct dm_snapshot {
/* You can't use a snapshot if this is 0 (e.g. if full) */
int valid;

- /* Origin writes don't trigger exceptions until this is set */
- int active;
-
/* Used for display of table */
char type;

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