[RFC PATCH 2/3] discover: Add support for device-mapper snapshots

[Cyril Bur]

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.

> 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 = 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
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;

> +}
> +
> +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(...)


> +	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,