[RFC PATCH 3/3] discover: Create dm-snapshots for all eligible devices
Cyril Bur
cyril.bur at au1.ibm.com
Wed Apr 29 01:42:45 AEST 2015
On Tue, 2015-04-28 at 10:53 +1000, Samuel Mendoza-Jonas wrote:
> On 28/04/15 06:49, Cyril Bur wrote:
> > On Mon, 2015-04-27 at 15:52 +1000, Samuel Mendoza-Jonas wrote:
> >> Create a dm-snapshot for any device with a valid filesystem. This
> >> protects the backing disk from any writes Petitboot may attempt to make
> >> regardless of mount type, such as journal replays.
> >>
> >> Signed-off-by: Samuel Mendoza-Jonas <sam.mj at au1.ibm.com>
> >> ---
> >> discover/device-handler.c | 40 +++++++++++++++++++++++++++++-----------
> >> discover/udev.c | 20 ++++++++++++++++++--
> >> 2 files changed, 47 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/discover/device-handler.c b/discover/device-handler.c
> >> index 69cd699..edf3a0c 100644
> >> --- a/discover/device-handler.c
> >> +++ b/discover/device-handler.c
> >> @@ -1279,11 +1279,14 @@ static const char *fs_parameters(unsigned int rw_flags, const char *fstype)
> >> static bool check_existing_mount(struct discover_device *dev)
> >> {
> >> struct stat devstat, mntstat;
> >> + const char *device_path;
> >> struct mntent *mnt;
> >> FILE *fp;
> >> int rc;
> >>
> >> - rc = stat(dev->device_path, &devstat);
> >> + device_path = dev->ramdisk ? dev->ramdisk->snapshot : dev->device_path;
> >> +
> >> + rc = stat(device_path, &devstat);
> >> if (rc) {
> >> pb_debug("%s: stat failed: %s\n", __func__, strerror(errno));
> >> return false;
> >> @@ -1333,7 +1336,7 @@ static bool check_existing_mount(struct discover_device *dev)
> >>
> >> static int mount_device(struct discover_device *dev)
> >> {
> >> - const char *fstype;
> >> + const char *fstype, *device_path;
> >> int rc;
> >>
> >> if (!dev->device_path)
> >> @@ -1365,9 +1368,11 @@ static int mount_device(struct discover_device *dev)
> >> goto err_free;
> >> }
> >>
> >> + device_path = dev->ramdisk ? dev->ramdisk->snapshot : dev->device_path;
> >> +
> >
> > You've done this a bunch of times, could you turn it into a simple
> > accessor?
> > static inline const char *get_device_path(struct discover_device *dev)
> > {
> > return dev->ramdisk ? dev->ramdisk->snapshot : dev->device_path;
> > }
>
> Yep
>
> >
> >> pb_log("mounting device %s read-only\n", dev->device_path);
> >> errno = 0;
> >> - rc = mount(dev->device_path, dev->mount_path, fstype,
> >> + rc = mount(device_path, dev->mount_path, fstype,
> >> MS_RDONLY | MS_SILENT,
> >> fs_parameters(MS_RDONLY, fstype));
> >> if (!rc) {
> >> @@ -1378,7 +1383,10 @@ static int mount_device(struct discover_device *dev)
> >> }
> >>
> >> pb_log("couldn't mount device %s: mount failed: %s\n",
> >> - dev->device_path, strerror(errno));
> >> + device_path, strerror(errno));
> >> +
> >> + /* If mount fails clean up any snapshot */
> >> + discover_device_destroy_snapshot(dev);
> >>
> >> pb_rmdir_recursive(mount_base(), dev->mount_path);
> >> err_free:
> >> @@ -1389,17 +1397,21 @@ err_free:
> >>
> >> static int umount_device(struct discover_device *dev)
> >> {
> >> + const char *device_path;
> >> int rc;
> >>
> >> if (!dev->mounted || !dev->unmount)
> >> return 0;
> >>
> >> - pb_log("unmounting device %s\n", dev->device_path);
> >> + device_path = dev->ramdisk ? dev->ramdisk->snapshot : dev->device_path;
> >> +
> >> + pb_log("unmounting device %s\n", device_path);
> >> rc = umount(dev->mount_path);
> >> if (rc)
> >> return -1;
> >>
> >> dev->mounted = false;
> >> + discover_device_destroy_snapshot(dev);
> >>
> >> pb_rmdir_recursive(mount_base(), dev->mount_path);
> >>
> >> @@ -1411,7 +1423,7 @@ static int umount_device(struct discover_device *dev)
> >>
> >> int device_request_write(struct discover_device *dev, bool *release)
> >> {
> >> - const char *fstype;
> >> + const char *fstype, *device_path;
> >> int rc;
> >>
> >> *release = false;
> >> @@ -1419,19 +1431,25 @@ int device_request_write(struct discover_device *dev, bool *release)
> >> if (!dev->mounted)
> >> return -1;
> >>
> >> + //FIXME: how do we decide to commit writes, eg from grub save actions?
> >> + if (!dev->ramdisk)
> >> + return -1;
> > Might want to add an log message if you hit this, at least for the life
> > of the comment.
>
> Yeah this is definitely RFC material - ideally I'll solve this before I commit anything :)
>
Wasn't clear if you intended to have a follow up patchset or if you
weren't going to commit this until you'd solved that problem, obviously
my comment is irrelevant if you're going to address it in what goes in.
Although, depending on the time required to address that fixme, perhaps
you would want to merge this patch first and follow it up separately?
> >
> >> +
> >> if (dev->mounted_rw)
> >> return 0;
> >>
> >> fstype = discover_device_get_param(dev, "ID_FS_TYPE");
> >>
> >> - pb_log("remounting device %s read-write\n", dev->device_path);
> >> + device_path = dev->ramdisk ? dev->ramdisk->snapshot : dev->device_path;
> >> +
> >> + pb_log("remounting device %s read-write\n", device_path);
> >>
> >> rc = umount(dev->mount_path);
> >> if (rc) {
> >> pb_log("Failed to unmount %s\n", dev->mount_path);
> >> return -1;
> >> }
> >> - rc = mount(dev->device_path, dev->mount_path, fstype,
> >> + rc = mount(device_path, dev->mount_path, fstype,
> >> MS_SILENT,
> >> fs_parameters(MS_REMOUNT, fstype));
> >> if (rc)
> >> @@ -1442,12 +1460,12 @@ int device_request_write(struct discover_device *dev, bool *release)
> >> return 0;
> >>
> >> mount_ro:
> >> - pb_log("Unable to remount device %s read-write\n", dev->device_path);
> >> - rc = mount(dev->device_path, dev->mount_path, fstype,
> >> + pb_log("Unable to remount device %s read-write\n", device_path);
> >> + rc = mount(device_path, dev->mount_path, fstype,
> >> MS_RDONLY | MS_SILENT,
> >> fs_parameters(MS_RDONLY, fstype));
> >> if (rc)
> >> - pb_log("Unable to recover mount for %s\n", dev->device_path);
> >> + pb_log("Unable to recover mount for %s\n", device_path);
> >> return -1;
> >> }
> >>
> >> diff --git a/discover/udev.c b/discover/udev.c
> >> index 6ccb8d4..8a3cabb 100644
> >> --- a/discover/udev.c
> >> +++ b/discover/udev.c
> >> @@ -94,12 +94,16 @@ static int udev_handle_block_add(struct pb_udev *udev, struct udev_device *dev,
> >>
> >> node = udev_device_get_devnode(dev);
> >> path = udev_device_get_devpath(dev);
> >> - if (path && (strstr(path, "virtual/block/loop")
> >> - || strstr(path, "virtual/block/ram"))) {
> >> + if (path && strstr(path, "virtual/block/loop")) {
> >> pb_log("SKIP: %s: ignored (path=%s)\n", name, path);
> >> return 0;
> >> }
> >>
> >> + if (path && strstr(path, "virtual/block/ram")) {
> >> + device_handler_add_ramdisk(udev->handler, node);
> > tbh I have no idea, just came to mind, do you want to log something
> > here?
>
> I did originally but removed it due to noise - I may re-add it in a pb_debug() call.
>
Yeah I figured noise was a possible reason it wasn't there, had to be
sure. Feel free to ignore me in that case.
> >> + return 0;
> >> + }
> >> +
> >> cdrom = node && !!udev_device_get_property_value(dev, "ID_CDROM");
> >> if (cdrom) {
> >> /* CDROMs require a little initialisation, to get
> >> @@ -111,6 +115,14 @@ static int udev_handle_block_add(struct pb_udev *udev, struct udev_device *dev,
> >> }
> >> }
> >>
> >> + /* dm-snapshots don't appear as udev events currently, but catch this
> >> + * in case udev/libdm are updated in Petitboot's environment */
> >> + const char *devname = udev_device_get_property_value(dev, "DM_NAME");
> >> + if (devname) {
> >> + pb_log("This is a snapshot! %s\n", devname);
> >> + return 0;
> >> + }
> >> +
> >> type = udev_device_get_property_value(dev, "ID_FS_TYPE");
> >> if (!type) {
> >> pb_log("SKIP: %s: no ID_FS_TYPE property\n", name);
> >> @@ -142,6 +154,10 @@ static int udev_handle_block_add(struct pb_udev *udev, struct udev_device *dev,
> >>
> >> udev_setup_device_params(dev, ddev);
> >>
> >> + /* Create snapshot first if needed */
> >> + if (ddev->device->type == DEVICE_TYPE_DISK)
> >> + device_handler_create_snapshot(udev->handler, ddev);
> >> +
> >> device_handler_discover(udev->handler, ddev);
> >>
> >> return 0;
> >
>
>
More information about the Petitboot
mailing list