Patchwork : set up rbd snapshot handling

login
register
mail settings
Submitter Gregory Farnum
Date Jan. 10, 2012, 8:01 p.m.
Message ID <CAF3hT9Cu0BZQfrgCpBRRaPHyfSy3V-A=jJLe3qGr4UoFMcniOQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/135305/
State New
Headers show

Comments

Gregory Farnum - Jan. 10, 2012, 8:01 p.m.
rbd: wire up snapshot removal and rollback functionality

Signed-off-by: Greg Farnum <gregory.farnum@dreamhost.com>
Reviewed-by: Josh Durgin <josh.durgin@dreamhost.com>
---
 block/rbd.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)
Stefan Hajnoczi - Jan. 11, 2012, 9:58 a.m.
On Tue, Jan 10, 2012 at 8:01 PM, Gregory Farnum
<gregory.farnum@dreamhost.com> wrote:
> +static int qemu_rbd_snap_remove(BlockDriverState *bs,
> +                                const char *snapshot_name)
> +{
> +    BDRVRBDState *s = bs->opaque;
> +    int r;
> +
> +    r = rbd_snap_remove(s->image, snapshot_name);
> +    if (r < 0) {
> +        error_report("failed to remove snap: %s", strerror(-r));
> +        return r;

There's no need to report an error message here.  This function should
return -errno and let the caller decide how to show the error to the
user.  If you look at callers in the codebase they already print an
equivalent error message.

Stefan
Christoph Hellwig - Jan. 11, 2012, 10 a.m.
> +static int qemu_rbd_snap_remove(BlockDriverState *bs,
> +                                const char *snapshot_name)
> +{
> +    BDRVRBDState *s = bs->opaque;
> +    int r;
> +
> +    r = rbd_snap_remove(s->image, snapshot_name);

> +    r = rbd_snap_rollback(s->image, snapshot_name);

Have these functions been available since day 1 in librbd or should they
get a version checks like the cache flush call?
Gregory Farnum - Jan. 11, 2012, 7:48 p.m.
On Wed, Jan 11, 2012 at 1:58 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jan 10, 2012 at 8:01 PM, Gregory Farnum
> <gregory.farnum@dreamhost.com> wrote:
>> +static int qemu_rbd_snap_remove(BlockDriverState *bs,
>> +                                const char *snapshot_name)
>> +{
>> +    BDRVRBDState *s = bs->opaque;
>> +    int r;
>> +
>> +    r = rbd_snap_remove(s->image, snapshot_name);
>> +    if (r < 0) {
>> +        error_report("failed to remove snap: %s", strerror(-r));
>> +        return r;
>
> There's no need to report an error message here.  This function should
> return -errno and let the caller decide how to show the error to the
> user.  If you look at callers in the codebase they already print an
> equivalent error message.
>
> Stefan

Oh yep, guess I was a little too formulaic. Resend in a moment...

On Wed, Jan 11, 2012 at 2:00 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> +static int qemu_rbd_snap_remove(BlockDriverState *bs,
>> +                                const char *snapshot_name)
>> +{
>> +    BDRVRBDState *s = bs->opaque;
>> +    int r;
>> +
>> +    r = rbd_snap_remove(s->image, snapshot_name);
>
>> +    r = rbd_snap_rollback(s->image, snapshot_name);
>
> Have these functions been available since day 1 in librbd or should they
> get a version checks like the cache flush call?

Day 1. :)

Patch

diff --git a/block/rbd.c b/block/rbd.c
index 7a2384c..f52c1ca 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -787,6 +787,36 @@  static int qemu_rbd_snap_create(BlockDriverState *bs,
     return 0;
 }

+static int qemu_rbd_snap_remove(BlockDriverState *bs,
+                                const char *snapshot_name)
+{
+    BDRVRBDState *s = bs->opaque;
+    int r;
+
+    r = rbd_snap_remove(s->image, snapshot_name);
+    if (r < 0) {
+        error_report("failed to remove snap: %s", strerror(-r));
+        return r;
+    }
+
+    return 0;
+}
+
+static int qemu_rbd_snap_rollback(BlockDriverState *bs,
+                                  const char *snapshot_name)
+{
+    BDRVRBDState *s = bs->opaque;
+    int r;
+
+    r = rbd_snap_rollback(s->image, snapshot_name);
+    if (r < 0) {
+        error_report("failed to rollback to snap: %s", strerror(-r));
+        return r;
+    }
+
+    return 0;
+}
+
 static int qemu_rbd_snap_list(BlockDriverState *bs,
                               QEMUSnapshotInfo **psn_tab)
 {
@@ -860,7 +890,9 @@  static BlockDriver bdrv_rbd = {
     .bdrv_co_flush_to_disk  = qemu_rbd_co_flush,

     .bdrv_snapshot_create   = qemu_rbd_snap_create,
+    .bdrv_snapshot_delete   = qemu_rbd_snap_remove,
     .bdrv_snapshot_list     = qemu_rbd_snap_list,
+    .bdrv_snapshot_goto     = qemu_rbd_snap_rollback,
 };

 static void bdrv_rbd_init(void)