[dm-devel] Re: dm snapshot: allow live exception store handover between tables
Mike Snitzer
snitzer at redhat.com
Wed Nov 11 12:41:39 UTC 2009
On Tue, Nov 10 2009 at 10:26pm -0500,
Mikulas Patocka <mpatocka at redhat.com> wrote:
> > + /*
> > + * 'is_handover_destination' denotes another snapshot with the same
> > + * cow block device (as identified with find_snapshot_using_cow)
> > + * will hand over its exception store to this snapshot.
> > + *
> > + * 'is_handover_destination' is set in snapshot_ctr if an existing
> > + * snapshot has the same cow device. The handover is performed,
> > + * and 'is_handover_destination' is cleared, when either of the
> > + * following occurs:
> > + * - src (old) snapshot, that is handing over, is destructed
> > + * - dest (new) snapshot, that is accepting the handover, is resumed
> > + */
> > + int is_handover_destination;
> > +
> > + /*
> > + * reference to the other snapshot that will participate in the
> > + * exception store handover; src references dest, dest references src
> > + */
> > + struct dm_snapshot *handover_snap;
>
> Hi
>
> Please drop snapshot-linking through this "handover_snap" field, it will
> create real or possible problems with locking or with dangling pointers
> (i.e. are both properly locked? Which to lock first? If you destroy a
> snapshot, you must check if something links to it, etc. --- these problems
> could be solved, but if they can be avoided by dropping linking, it's
> better to avoid them).
I agree, lock_snapshots_for_handover() is too complicated. But by dropping
these 'handover_snap' associations we'll lose the ability to have tight
constraints on what handover will take place on resume of new snapshot
(ideally a source of handover will only have one destination).
The new snapshot (destination of exception handover) is not registered
with the origin until after resume's handover_exceptions (via
register_snapshot). And therefore it can't be found via
find_snapshot_using_cow(). But that is a good thing as we'll know that
if find_snapshot_using_cow does return a snapshot then it is the source
of the handover.
If we keep regitration of the new snapshot late, as I believe we need
to, then we do not have a way to check for a conflicting handover (in
snapshot_ctr). We'd then have a situation where N new snapshots _could_
be racing to get the exceptions handed over from a single old snapshot.
So I think we'll need the snapshot to have a 'is_handover_source' flag;
it would allow us to check of the 'handover_snap' in snapshot_ctr()
already 'is_handover_source' and if so fail the new snapshot_ctr().
I'll look to add this after I have coded the handover to not use any
state. Looking will be dead simple (just need to take the old
snapshot's lock).
> When you are about to do a handover (Alasdair is arranging generic dm core
> do handover only un resume), just search for a possible handover candidate
> with find_snapshot_using_cow.
As I think you're aware: Alasdair's core DM changes have nothing to do
with actually constraining handover to be on resume. His changes make
handover on resume safe; as we can guarantee that handover on resume
actually worked (and if not rollback to old map). A very welcomed
advance.
Thanks for your review,
Mike
More information about the dm-devel
mailing list