On Tuesday 29 July 2003 15:22, Kevin Corry wrote: > Joe, > > We talked on Saturday about adding code in the origin_ctr() path to > allocate an origin structure if one was not already present. This would > allow an origin device to be active without any active snapshots, and thus > prevent hitting the BUG in do_origin. You said you had a patch that did > this, but in case you've misplaced it, I put one together this afternoon. > If you do have your patch available, could you please post it here, and I > will test with your version instead of this one. > > Thanks! Damn. Well, I found a bug in the patch I sent in yesterday. :( If you create an origin device, then create a snapshot device, then delete the snapshot device, the struct origin gets deleted by the snapshot_dtr, which leads right back to the BUG we're trying to avoid in do_origin(). This new version adds a referrence count to the struct origin and deallocates it only when the count goes to zero. (diff did something weird with this one, so the patch looks a bit strange in places...but it still works.) -- Kevin Corry kevcorry@xxxxxxxxxx http://evms.sourceforge.net/ --- a/drivers/md/dm-snapshot.c 4 Jul 2003 19:38:50 -0000 +++ b/drivers/md/dm-snapshot.c 30 Jul 2003 19:29:25 -0000 @@ -92,6 +92,9 @@ /* List of snapshots for this origin */ struct list_head snapshots; + + /* Count of snapshots and origins referrencing this structure. */ + unsigned int count; }; /* @@ -155,6 +158,36 @@ } /* + * Allocate and initialize an origin structure. + */ +static struct origin * __alloc_origin(kdev_t dev) +{ + struct origin *o = kmalloc(sizeof(*o), GFP_KERNEL); + if (o) { + o->dev = dev; + o->count = 0; + INIT_LIST_HEAD(&o->hash_list); + INIT_LIST_HEAD(&o->snapshots); + __insert_origin(o); + } + return o; +} + +static void __get_origin(struct origin *o) +{ + o->count++; +} + +static void __put_origin(struct origin *o) +{ + o->count--; + if (o->count == 0) { + list_del(&o->hash_list); + kfree(o); + } +} + +/* * Make a note of the snapshot and its origin so we can look it * up when the origin has a write on it. */ @@ -168,20 +201,37 @@ if (!o) { /* New origin */ - o = kmalloc(sizeof(*o), GFP_KERNEL); + o = __alloc_origin(dev); if (!o) { up_write(&_origins_lock); return -ENOMEM; } + } - /* Initialise the struct */ - INIT_LIST_HEAD(&o->snapshots); - o->dev = dev; + __get_origin(o); + list_add_tail(&snap->list, &o->snapshots); - __insert_origin(o); + up_write(&_origins_lock); + return 0; +} + +static int register_origin(kdev_t dev) +{ + struct origin *o; + + down_write(&_origins_lock); + o = __lookup_origin(dev); + + if (!o) { + /* New origin */ + o = __alloc_origin(dev); + if (!o) { + up_write(&_origins_lock); + return -ENOMEM; + } } - list_add_tail(&snap->list, &o->snapshots); + __get_origin(o); up_write(&_origins_lock); return 0; @@ -195,11 +245,18 @@ o = __lookup_origin(s->origin->dev); list_del(&s->list); - if (list_empty(&o->snapshots)) { - list_del(&o->hash_list); - kfree(o); - } + __put_origin(o); + + up_write(&_origins_lock); +} + +static void unregister_origin(kdev_t dev) +{ + struct origin *o; + down_write(&_origins_lock); + o = __lookup_origin(dev); + __put_origin(o); up_write(&_origins_lock); } @@ -1050,6 +1104,13 @@ return r; } + r = register_origin(dev->dev); + if (r) { + ti->error = "Cannot register origin"; + dm_put_device(ti, dev); + return r; + } + ti->private = dev; return 0; } @@ -1057,6 +1118,7 @@ static void origin_dtr(struct dm_target *ti) { struct dm_dev *dev = (struct dm_dev *) ti->private; + unregister_origin(dev->dev); dm_put_device(ti, dev); }