Patchworkβ [v2,02/17] dm snapshot: add suspended flag to dm_snapshot

login
register
about
Submitter Mike Snitzer
Date 2009-10-20 22:46:50
Message ID <1256078825-11331-3-git-send-email-snitzer@redhat.com>
Download mbox | patch
Permalink /patch/55021/
State Awaiting Upstream
Delegated to: Alasdair Kergon
Headers show

Comments

Mike Snitzer - 2009-10-20 22:46:50
Allow the snapshot target to be able to verify its state relative to a
requested operation.  Allows for invalid operations to be prevented,
e.g. an attempt handover an "old" snapshot's exceptions without it
having been suspended first.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-snap.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)
Alasdair Kergon - 2009-11-05 00:56:35
On Tue, Oct 20, 2009 at 06:46:50PM -0400, Mike Snitzer wrote:
> Allow the snapshot target to be able to verify its state relative to a
> requested operation.  Allows for invalid operations to be prevented,
> e.g. an attempt handover an "old" snapshot's exceptions without it
> having been suspended first.
 
OK - same as we do in dm-raid1.

But just be aware that:
        /* This does not get reverted if there's an error later. */
        dm_table_presuspend_targets(map);

and so it's not identical to querying the dm_suspended() property directly.

Alasdair

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Mike Snitzer - 2009-11-16 18:17:17
On Wed, Nov 04 2009 at  7:56pm -0500,
Alasdair G Kergon <agk@redhat.com> wrote:

> On Tue, Oct 20, 2009 at 06:46:50PM -0400, Mike Snitzer wrote:
> > Allow the snapshot target to be able to verify its state relative to a
> > requested operation.  Allows for invalid operations to be prevented,
> > e.g. an attempt handover an "old" snapshot's exceptions without it
> > having been suspended first.
>  
> OK - same as we do in dm-raid1.
> 
> But just be aware that:
>         /* This does not get reverted if there's an error later. */
>         dm_table_presuspend_targets(map);
> 
> and so it's not identical to querying the dm_suspended() property directly.

Right, it is now clear that I should be using postsuspend.  The handover
patch has a need to know that another snapshot target has been
suspended; setting dm-snapshot's 'suspended' in .presuspend doesn't give
us that information.

So the dm-snapshot-track-suspended-state-in-target.patch that you have
queued should be changed with: s/presuspend/postsuspend/

(the same search and replace is needed in the v7 handover patch)

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Alasdair Kergon - 2009-11-17 02:24:30
On Mon, Nov 16, 2009 at 01:17:17PM -0500, Mike Snitzer wrote:
> So the dm-snapshot-track-suspended-state-in-target.patch that you have
> queued should be changed with: s/presuspend/postsuspend/
 
Changed.

As discussed earlier, as we have something that works, we'll leave the
suspended state tracking as-is for now.  Then once all the batches of
patches for the next kernel are signed off, we can return to this and
clean it up in all the targets.

Alasdair

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

Patch

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index 6181878..be37439 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -72,6 +72,9 @@  struct dm_snapshot {
 	/* Origin writes don't trigger exceptions until this is set */
 	int active;
 
+	/* Set if the parent device is suspended */
+	int suspended;
+
 	mempool_t *pending_pool;
 
 	atomic_t pending_exceptions_count;
@@ -632,6 +635,7 @@  static int snapshot_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	s->ti = ti;
 	s->valid = 1;
 	s->active = 0;
+	s->suspended = 0;
 	atomic_set(&s->pending_exceptions_count, 0);
 	init_rwsem(&s->lock);
 	spin_lock_init(&s->pe_lock);
@@ -1138,12 +1142,22 @@  static int snapshot_end_io(struct dm_target *ti, struct bio *bio,
 	return 0;
 }
 
+static void snapshot_presuspend(struct dm_target *ti)
+{
+	struct dm_snapshot *s = ti->private;
+
+	down_write(&s->lock);
+	s->suspended = 1;
+	up_write(&s->lock);
+}
+
 static void snapshot_resume(struct dm_target *ti)
 {
 	struct dm_snapshot *s = ti->private;
 
 	down_write(&s->lock);
 	s->active = 1;
+	s->suspended = 0;
 	up_write(&s->lock);
 }
 
@@ -1434,12 +1448,13 @@  static struct target_type origin_target = {
 
 static struct target_type snapshot_target = {
 	.name    = "snapshot",
-	.version = {1, 8, 0},
+	.version = {1, 9, 0},
 	.module  = THIS_MODULE,
 	.ctr     = snapshot_ctr,
 	.dtr     = snapshot_dtr,
 	.map     = snapshot_map,
 	.end_io  = snapshot_end_io,
+	.presuspend = snapshot_presuspend,
 	.resume  = snapshot_resume,
 	.status  = snapshot_status,
 	.iterate_devices = snapshot_iterate_devices,