[RFC PATCH 2/3] discover: Add support for device-mapper snapshots
Samuel Mendoza-Jonas
sam.mj at au1.ibm.com
Tue Apr 28 10:51:02 AEST 2015
On 28/04/15 02:09, Cyril Bur wrote:
> On Mon, 2015-04-27 at 15:52 +1000, Samuel Mendoza-Jonas wrote:
>> Device Mapper allows the creation of CoW snapshots backed by ramdisks.
>> This will allow Petitboot to perform potentially destructive actions
>> such as filesystem recovery without affecting the backing disk.
>> This commit adds several functions to create and destroy these snapshots
>> and to keep track of available ramdisks.
>>
> Hey Sam,
>
> Been waiting to see how you did this, looks nice!
>
> Got a few comments.
>
> Firstly, is there a real advantage to the in_use flag? It looks like you
> create and destroy the DM device regardless, all you're saving is a few
> allocations? Could you do it in such a way that you could save the
> DM_DEVICE_CREATE/DM_DEVICE_REMOVE actions, I feel like they're the most
> costly.
Not strictly required, more an artifact of some older checking. I'm not sure I
follow you on avoiding the DM_DEVICE_* actions - there are two things we
essentially keep track of:
- Available /dev/ramX ramdisks - these are not the snapshots themselves just
the backing memory device. Petitboot boots up with 16 of these (IIRC) so we
use these first before making new nodes.
- A snapshot for every eligible disk: We DM_DEVICE_CREATE a snapshot when a
new valid disk is recognised, and DM_DEVICE_REMOVE it when the disk is unmounted.
There's not a way to 'avoid' the actions since the snapshot exists for the life
of the disk as far as Petitboot is concerned.
However I don't think I've read your question right so let me know :)
>
>> No functionality change in this patch.
>>
>> Signed-off-by: Samuel Mendoza-Jonas <sam.mj at au1.ibm.com>
>> ---
>> discover/Makefile.am | 4 ++
>> discover/device-handler.c | 176 ++++++++++++++++++++++++++++++++++++++++++++++
>> discover/device-handler.h | 6 ++
>> 3 files changed, 186 insertions(+)
>>
>> diff --git a/discover/Makefile.am b/discover/Makefile.am
>> index 7808110..c6bc8af 100644
>> --- a/discover/Makefile.am
>> +++ b/discover/Makefile.am
>> @@ -58,6 +58,10 @@ discover_pb_discover_LDADD = \
>> $(core_lib) \
>> $(UDEV_LIBS)
>>
>> +discover_pb_discover_LDFLAGS = \
>> + $(AM_LDFLAGS) \
>> + -ldevmapper
>> +
>> discover_pb_discover_CPPFLAGS = \
>> $(AM_CPPFLAGS) \
>> -DLOCAL_STATE_DIR='"$(localstatedir)"' \
>> diff --git a/discover/device-handler.c b/discover/device-handler.c
>> index f411d9f..69cd699 100644
>> --- a/discover/device-handler.c
>> +++ b/discover/device-handler.c
>> @@ -10,6 +10,8 @@
>> #include <sys/wait.h>
>> #include <sys/mount.h>
>>
>> +#include <libdevmapper.h>
>> +
>> #include <talloc/talloc.h>
>> #include <list/list.h>
>> #include <log/log.h>
>> @@ -46,6 +48,12 @@ enum default_priority {
>> DEFAULT_PRIORITY_DISABLED = 0xff,
>> };
>>
>> +struct ramdisk_device {
>> + const char *path;
>> + bool in_use;
>> + char *snapshot;
>> +};
>> +
>> struct device_handler {
>> struct discover_server *server;
>> int dry_run;
>> @@ -57,6 +65,9 @@ struct device_handler {
>> struct discover_device **devices;
>> unsigned int n_devices;
>>
>> + struct ramdisk_device **ramdisks;
>> + unsigned int n_ramdisks;
>> +
>> struct waitset *waitset;
>> struct waiter *timeout_waiter;
>> bool autoboot_enabled;
>> @@ -747,6 +758,171 @@ void device_handler_add_device(struct device_handler *handler,
>> network_register_device(handler->network, device);
>> }
>>
>> +void device_handler_add_ramdisk(struct device_handler *handler,
>> + const char *path)
>> +{
>> + struct ramdisk_device *dev;
>> + unsigned int i;
>> +
>> + if (!path)
>> + return;
>> +
>> + for (i = 0; i < handler->n_ramdisks; i++)
>> + if (!strcmp(handler->ramdisks[i]->path, path))
>> + return;
>> +
>> + dev = talloc_zero(handler, struct ramdisk_device);
>> + dev->path = talloc_strdup(handler, path);
>
> You've not checked for NULL when using many of the talloc_* functions?
> Is there a good reason for this? I feel like you might be opening
> yourself up to mystery segfault.
> Of course on the other hand, I'll probably end up agreeing that there
> really isn't much that can be done if they ever fail.
I choose to plead RFC :P But as you say this one in particular is tricky -
if we fail we probably can't make a snapshot, so we *could* mount the
disk itself - but then we throw away any read-only guarantee.
Maybe something like this (later on in the mounting code)
if (snapshot available)
mount(snapshot)
else
mount(disk) with -norecovery
>
>> +
>> + i = handler->n_ramdisks++;
>> +
>> + handler->ramdisks = talloc_realloc(handler, handler->ramdisks,
>> + struct ramdisk_device *, handler->n_ramdisks);
>> + handler->ramdisks[i] = dev;
>
> So could be just me, these three lines caused me a minifreakout, it took
> me a second to see that it's correct, any reason you're incrementing
> n_ramdisks before you resize the allocation? What if the allocation
Faith in my memory allocator? :)
> fails? Why even use i? Might I suggest:
>
> handler->ramdisks = talloc_realloc(handler, handler->ramdisks,
> struct ramdisk_device *, handler->n_ramdisks + 1);
> if (!hander->ramdisks) {
> /* I still think you should freak out */
> }
>
> handler->ramdisks[handler->n_ramdisks++] = dev;
Yep, recovers better.
>
>> +}
>> +
>> +void device_handler_create_snapshot(struct device_handler *handler,
>> + struct discover_device *device)
>> +{
>> + unsigned int sectors, i;
>> + struct dm_task *task;
>> + char *ttype, *params;
>> + uint32_t cookie = 0;
>> + uint16_t flags = 0;
>> + const char *tmp;
>> +
>> + tmp = discover_device_get_param(device, "ID_PART_ENTRY_SIZE");
>> + if (!tmp) {
>> + pb_log("Could not retrieve sector size for %s\n",
>> + device->device_path);
>> + return;
>> + }
>> +
>> + sectors = strtoul(tmp, NULL, 0);
>> + if (!sectors) {
>> + pb_log("Error reading sector count for %s\n",
>> + device->device_path);
>> + return;
>> + }
>> +
>> + pb_log("Creating a snapshot for %s, %u sectors, named %s\n",
>> + device->device_path, sectors, device->device->id);
>> +
>> + /* Check if free ramdisk exists */
>> + for (i = 0; i < handler->n_ramdisks; i++)
>> + if (!handler->ramdisks[i]->in_use)
>> + break;
>> +
>> + /* Otherwise create a new one */
>> + if (i == handler->n_ramdisks) {
>> + char *name = talloc_asprintf(handler, "/dev/ram%d",
>> + handler->n_ramdisks);
>> + dev_t id = makedev(1, handler->n_ramdisks);
>> + if (mknod(name, S_IFBLK, id)) {
>> + if (errno == EEXIST) {
>> + /* We haven't yet received updates for existing
>> + * ramdisks - add and use this one */
>> + pb_debug("Using untracked ramdisk %s\n", name);
>> + } else {
>> + pb_log("Failed to create new ramdisk %s: %s\n",
>> + name, strerror(errno));
>> + return;
>> + }
>> + }
>> + device_handler_add_ramdisk(handler, name);
>> + talloc_free(name);
>> + }
>> +
> Perhaps have the above code be something along the lines of
> get_backing_ramdisk(...)
Could do although this doesn't get called anywhere else - snapshot-merges
may change that however.
>
>
>> + ttype = talloc_asprintf(handler, "snapshot");
>> + params = talloc_asprintf(handler, "%s %s N 1",
>> + device->device_path, handler->ramdisks[i]->path);
>> +
>> + task = dm_task_create(DM_DEVICE_CREATE);
>> + if (!task) {
>> + pb_log("Error creating new dm-task\n");
>> + goto err1;
>> + }
>> +
>> + if (!dm_task_set_name(task, device->device->id))
>> + goto err2;
>> +
>> + /* Set the table for this dm-device */
>> + if (!dm_task_add_target(task, 0, sectors, ttype, params))
>> + goto err2;
>> +
>> + if (!dm_task_set_add_node(task, DM_ADD_NODE_ON_CREATE))
>> + goto err2;
>> +
>> + /* Petitboot's libdm isn't compiled with --enable-udev_sync, so we set
>> + * empty cookie and flags */
>> + if (!dm_task_set_cookie(task, &cookie, flags))
>> + goto err2;
>> +
>> + if (!dm_task_run(task)) {
>> + pb_log("Error executing dm-task\n");
>> + goto err2;
>> + }
>> +
>> + /* This is the magic incantation to get /dev/mapper/sdaX appearing */
>> + dm_udev_wait(cookie);
>> +
>> + device->ramdisk = handler->ramdisks[i];
>> + handler->ramdisks[i]->in_use = true;
>> + handler->ramdisks[i]->snapshot = talloc_asprintf(handler, "/dev/mapper/%s",
>> + device->device->id);
>> +
>> + pb_log("Snapshot created for device %s\n", device->device->id);
>> +
>> +err2:
>> + dm_task_destroy(task);
>> +err1:
>> + talloc_free(params);
>> + talloc_free(ttype);
>> +}
>> +
>> +int discover_device_destroy_snapshot(struct discover_device *device)
>> +{
>> + /* Assume we've already unmounted the snapshot */
>> + struct dm_task *task;
>> + uint32_t cookie = 0;
>> + uint16_t flags = 0;
>> + int rc = -1;
>> +
>> + if (!device->ramdisk)
>> + return 0;
>> +
>> + task = dm_task_create(DM_DEVICE_REMOVE);
>> + if (!task) {
>> + pb_log("Could not create dm_task DM_DEVICE_REMOVE\n");
>> + return -1;
>> + }
>> +
>> + if (!dm_task_set_name(task, device->device->id)) {
>> + pb_log("No dm-snapshot named '%s'\n", device->device->id);
>> + goto out;
>> + }
>> +
>> + /* Petitboot's libdm isn't compiled with --enable-udev_sync, so we set
>> + * empty cookie and flags */
>> + if (!dm_task_set_cookie(task, &cookie, flags))
>> + goto out;
>> +
>> + if (!dm_task_run(task)) {
>> + pb_log("Unable to remove snapshot '%s'\n", device->device->id);
>> + goto out;
>> + }
>> +
>> + /* Wait for /dev/mapper/ entries to be removed */
>> + dm_udev_wait(cookie);
>> +
>> + device->ramdisk->in_use = false;
>> + device->ramdisk = NULL;
>> + rc = 0;
>> +out:
>> + dm_task_destroy(task);
>> + return rc;
>> +}
>> +
>> /* Start discovery on a hotplugged device. The device will be in our devices
>> * array, but has only just been initialised by the hotplug source.
>> */
>> diff --git a/discover/device-handler.h b/discover/device-handler.h
>> index b592c46..2425b7b 100644
>> --- a/discover/device-handler.h
>> +++ b/discover/device-handler.h
>> @@ -26,6 +26,7 @@ struct discover_device {
>>
>> char *mount_path;
>> const char *device_path;
>> + struct ramdisk_device *ramdisk;
>> bool mounted;
>> bool mounted_rw;
>> bool unmount;
>> @@ -72,6 +73,11 @@ struct discover_device *discover_device_create(struct device_handler *handler,
>> const char *id);
>> void device_handler_add_device(struct device_handler *handler,
>> struct discover_device *device);
>> +void device_handler_add_ramdisk(struct device_handler *handler,
>> + const char *path);
>> +void device_handler_create_snapshot(struct device_handler *handler,
>> + struct discover_device *device);
>> +int discover_device_destroy_snapshot(struct discover_device *device);
>> int device_handler_discover(struct device_handler *handler,
>> struct discover_device *dev);
>> int device_handler_dhcp(struct device_handler *handler,
>
--
-----------
LTC Ozlabs
IBM
More information about the Petitboot
mailing list