Re: [PATCH 08 of 11] blktap2: configurable driver chains

[Daniel Stodden <daniel.stodden@citrix.com>]

Hey Ryan.

The command line idea is ok, but how about "!"?

Also, please check the line width.

Also, please don't alter the kernel list macros [..or submit yours to
lkml].

Daniel

On Thu, 2009-11-05 at 17:58 -0500, Brendan Cully wrote:
> # HG changeset patch
> # User Ryan O'Connor <rjo@cs.ubc.ca>
> # Date 1252530408 25200
> # Node ID 4ca00ee41ce9731b8725c736b27c841b484dce5d
> # Parent  6ca67fe3514ada809a62282c30fe46d5df5ce265
> blktap2: configurable driver chains
> 
> Blktap2 allows block device drivers to be layered to create more
> advanced virtual block devices. However, composing a layered driver is
> not exposed to the user. This patch fixes this, and allows the user to
> explicitly specify a driver chain when starting a tapdisk process, using
> the pipe character ('|') to explicitly seperate layers in a blktap2
> configuration string.
> 
> for example, the command:
>   ~$ tapdisk2 -n "log:|aio:/path/to/file.img"
> will create a blktap2 device where read and write requests are passed to
> the 'log' driver, then forwarded to the 'aio' driver.
> 
> Signed-off-by: Ryan O'Connor <rjo@cs.ubc.ca>
> 
> diff --git a/tools/blktap2/drivers/tapdisk-vbd.c b/tools/blktap2/drivers/tapdisk-vbd.c
> --- a/tools/blktap2/drivers/tapdisk-vbd.c
> +++ b/tools/blktap2/drivers/tapdisk-vbd.c
> @@ -106,6 +106,7 @@
>         vbd->callback = tapdisk_vbd_callback;
>         vbd->argument = vbd;
> 
> +       INIT_LIST_HEAD(&vbd->driver_stack);
>         INIT_LIST_HEAD(&vbd->images);
>         INIT_LIST_HEAD(&vbd->new_requests);
>         INIT_LIST_HEAD(&vbd->pending_requests);
> @@ -541,6 +542,105 @@
>         goto out;
>  }
> 
> +/* TODO: ugh, lets not call it parent info... */
> +static struct list_head *
> +tapdisk_vbd_open_level(td_vbd_t *vbd, char* params, int driver_type, td_disk_info_t *parent_info, td_flag_t flags)
> +{
> +        char *name;
> +       int type, err;
> +       td_image_t *image;
> +       td_disk_id_t id;
> +       struct  list_head *images;
> +       td_driver_t *driver;
> +
> +       images = calloc(1, sizeof(struct list_head));
> +       INIT_LIST_HEAD(images);
> +
> +       name   = params;
> +       type   = driver_type;
> +
> +       for (;;) {
> +               err   = -ENOMEM;
> +               image = tapdisk_image_allocate(name, type,
> +                                              vbd->storage, flags, vbd);
> +
> +               /* free 'name' if it was created by td_get_parent_id() */
> +               if (name != params) {
> +                       free(name);
> +                       name = NULL;
> +               }
> +
> +               if (!image)
> +                       return NULL;
> +
> +
> +               /* We have to do this to set the driver info for child drivers.  this conflicts with td_open */
> +               driver = image->driver;
> +               if (!driver) {
> +                       driver = tapdisk_driver_allocate(image->type,
> +                                                        image->name,
> +                                                        image->flags,
> +                                                        image->storage);
> +                       if (!driver)
> +                               return NULL;
> +               }
> +               /* the image has a driver, set the info and driver */
> +               image->driver = driver;
> +               image->info = driver->info;
> +
> +               /* XXX: we don't touch driver->refcount, broken? */
> +               /* XXX: we've replicated about 90% of td_open() gross! */
> +               /* XXX: this breaks if a driver modifies its info within a layer */
> +
> +               /* if the parent info is set, pass it to the child */
> +               if(parent_info)
> +               {
> +                       image->driver->info = *parent_info;
> +               }
> +
> +               err = td_load(image);
> +               if (err) {
> +                       if (err != -ENODEV)
> +                               return NULL;
> +
> +                       err = td_open(image);
> +                       if (err)
> +                               return NULL;
> +               }
> +
> +               /* TODO: non-sink drivers that don't care about their child
> +                 * currently return EINVAL. Could return TD_PARENT_OK or
> +                * TD_ANY_PARENT */
> +
> +               err = td_get_parent_id(image, &id);
> +               if (err && (err != TD_NO_PARENT && err != -EINVAL)) {
> +                       td_close(image);
> +                       return NULL;
> +               }
> +
> +               if (!image->storage)
> +                       image->storage = vbd->storage;
> +
> +               /* add this image to the end of the list */
> +               list_add_tail(&image->next, images);
> +
> +               image = NULL;
> +
> +               /* if the image does not have a parent we return the
> +                * list of images generated by this level of the stack */
> +               if (err == TD_NO_PARENT || err == -EINVAL)
> +                       break;
> +
> +               name   = id.name;
> +               type   = id.drivertype;
> +#if 0
> +                /* catch this by validate, not here */
> +               flags |= (TD_OPEN_RDONLY | TD_OPEN_SHAREABLE);
> +#endif
> +       }
> +       return images;
> +}
> +
>  static int
>  __tapdisk_vbd_open_vdi(td_vbd_t *vbd, td_flag_t extra_flags)
>  {
> @@ -548,58 +648,36 @@
>         int err, type;
>         td_flag_t flags;
>         td_disk_id_t id;
> -       td_image_t *image, *tmp;
> +       td_image_t *tmp;
>         struct tfilter *filter = NULL;
> +       td_vbd_driver_info_t *driver_info;
> +       struct list_head *images;
> +       td_disk_info_t *parent_info = NULL;
> 
>         err = tapdisk_vbd_reactivate_volumes(vbd, 0);
>         if (err)
>                 return err;
> 
>         flags = (vbd->flags & ~TD_OPEN_SHAREABLE) | extra_flags;
> -       file  = vbd->name;
> -       type  = vbd->type;
> 
> -       for (;;) {
> -               err   = -ENOMEM;
> -               image = tapdisk_image_allocate(file, type,
> -                                              vbd->storage, flags, vbd);
> 
> -               if (file != vbd->name) {
> -                       free(file);
> -                       file = NULL;
> -               }
> +       /* loop on each user specified driver.
> +         * NOTE: driver_info is in reverse order. That is, the first
> +         * item is the 'parent' or 'sink' driver */
> +       list_for_each_entry(driver_info, &vbd->driver_stack, next) {
> +               file = driver_info->params;
> +               type = driver_info->type;
> +               images = tapdisk_vbd_open_level(vbd, file, type, parent_info, flags);
> +               if (!images)
> +                       return -EINVAL;
> 
> -               if (!image)
> -                       goto fail;
> +               /* after each loop, append the created stack to the result stack */
> +               list_splice(images, &vbd->images);
> +               free(images);
> 
> -               err = td_load(image);
> -               if (err) {
> -                       if (err != -ENODEV)
> -                               goto fail;
> -
> -                       err = td_open(image);
> -                       if (err)
> -                               goto fail;
> -               }
> -
> -               err = td_get_parent_id(image, &id);
> -               if (err && err != TD_NO_PARENT) {
> -                       td_close(image);
> -                       goto fail;
> -               }
> -
> -               if (!image->storage)
> -                       image->storage = vbd->storage;
> -
> -               tapdisk_vbd_add_image(vbd, image);
> -               image = NULL;
> -
> -               if (err == TD_NO_PARENT)
> -                       break;
> -
> -               file   = id.name;
> -               type   = id.drivertype;
> -               flags |= (TD_OPEN_RDONLY | TD_OPEN_SHAREABLE);
> +               /* set the parent_info to the first diskinfo on the stack */
> +               tmp = tapdisk_vbd_first_image(vbd);
> +               parent_info = &tmp->info;
>         }
> 
>         if (td_flag_test(vbd->flags, TD_OPEN_LOG_DIRTY)) {
> @@ -623,14 +701,91 @@
>         return 0;
> 
>  fail:
> +
> +/* TODO: loop over vbd to free images? maybe do that in vbd_close_vdi */
> +#if 0
>         if (image)
>                 tapdisk_image_free(image);
> +#endif
> 
> +       /* TODO: handle partial stack createion? */
>         tapdisk_vbd_close_vdi(vbd);
> 
>         return err;
>  }
> 
> +/* this populates a vbd type based on path */
> +int
> +tapdisk_vbd_parse_stack(td_vbd_t *vbd, const char *path)
> +{
> +       int err;
> +       char *params, *driver_str;
> +       td_vbd_driver_info_t *driver;
> +
> +       /* make a copy of path */
> +        /* TODO: check against MAX_NAME_LEM ? */
> +       err = tapdisk_namedup(&params, path);
> +       if(err)
> +               goto error;
> +
> +
> +       /* tokenize params based on pipe '|' */
> +       driver_str = strtok(params, "|");
> +       while(driver_str != NULL)
> +       {
> +               /* parse driver info and add to vbd */
> +               driver = calloc(1, sizeof(td_vbd_driver_info_t));
> +               INIT_LIST_HEAD(&driver->next);
> +               err = tapdisk_parse_disk_type(driver_str, &driver->params, &driver->type);
> +               if(err)
> +                       goto error;
> +
> +               /* build the list backwards as the last driver will be the first
> +                * driver to open in the stack */
> +               list_add(&driver->next, &vbd->driver_stack);
> +
> +               /* get next driver string */
> +               driver_str = strtok(NULL, "|");
> +       }
> +
> +       return 0;
> +
> +       /* error: free any driver_info's and params */
> +error:
> +       while(!list_empty(&vbd->driver_stack)) {
> +               driver = list_entry(vbd->driver_stack.next, td_vbd_driver_info_t, next);
> +               list_del(&driver->next);
> +               free(driver);
> +        }
> +
> +       return err;
> +}
> +
> +/* NOTE: driver type, etc. must be set */
> +static int
> +tapdisk_vbd_open_stack(td_vbd_t *vbd, uint16_t storage, td_flag_t flags)
> +{
> +       int i, err;
> +
> +       vbd->flags   = flags;
> +       vbd->storage = storage;
> +
> +       for (i = 0; i < TD_VBD_EIO_RETRIES; i++) {
> +               err = __tapdisk_vbd_open_vdi(vbd, 0);
> +               if (err != -EIO)
> +                       break;
> +
> +               sleep(TD_VBD_EIO_SLEEP);
> +       }
> +       if (err)
> +               goto fail;
> +
> +       return 0;
> +
> +fail:
> +       return err;
> +}
> +
>  int
>  tapdisk_vbd_open_vdi(td_vbd_t *vbd, const char *path,
>                      uint16_t drivertype, uint16_t storage, td_flag_t flags)
> @@ -759,7 +914,7 @@
>  {
>         int err;
> 
> -       err = tapdisk_vbd_open_vdi(vbd, name, type, storage, flags);
> +       err = tapdisk_vbd_open_stack(vbd, storage, flags);
>         if (err)
>                 goto out;
> 
> diff --git a/tools/blktap2/drivers/tapdisk-vbd.h b/tools/blktap2/drivers/tapdisk-vbd.h
> --- a/tools/blktap2/drivers/tapdisk-vbd.h
> +++ b/tools/blktap2/drivers/tapdisk-vbd.h
> @@ -53,6 +53,7 @@
> 
>  typedef struct td_ring              td_ring_t;
>  typedef struct td_vbd_request       td_vbd_request_t;
> +typedef struct td_vbd_driver_info   td_vbd_driver_info_t;
>  typedef struct td_vbd_handle        td_vbd_t;
>  typedef void (*td_vbd_cb_t)        (void *, blkif_response_t *);
> 
> @@ -79,12 +80,20 @@
>         struct list_head            next;
>  };
> 
> +struct td_vbd_driver_info {
> +       char                       *params;
> +       int                         type;
> +       struct list_head            next;
> +};
> +
>  struct td_vbd_handle {
>         char                       *name;
> 
>         td_uuid_t                   uuid;
>         int                         type;
> 
> +       struct list_head            driver_stack;
> +
>         int                         storage;
> 
>         uint8_t                     reopened;
> @@ -164,6 +173,7 @@
> 
>  int tapdisk_vbd_initialize(int, int, td_uuid_t);
>  void tapdisk_vbd_set_callback(td_vbd_t *, td_vbd_cb_t, void *);
> +int tapdisk_vbd_parse_stack(td_vbd_t *vbd, const char *path);
>  int tapdisk_vbd_open(td_vbd_t *, const char *, uint16_t,
>                      uint16_t, const char *, td_flag_t);
>  int tapdisk_vbd_close(td_vbd_t *);
> diff --git a/tools/blktap2/drivers/tapdisk2.c b/tools/blktap2/drivers/tapdisk2.c
> --- a/tools/blktap2/drivers/tapdisk2.c
> +++ b/tools/blktap2/drivers/tapdisk2.c
> @@ -264,6 +264,13 @@
>                 return err;
>         }
> 
> +       err = tapdisk_vbd_parse_stack(vbd, name);
> +       if (err) {
> +               CHILD_ERR(err, "vbd_parse_stack failed: %d\n", err);
> +               return err;
> +       }
> +
> +       /* TODO: clean this up */
>         err = tapdisk_vbd_open(vbd, path, type,
>                                TAPDISK_STORAGE_TYPE_DEFAULT,
>                                devname, 0);
> diff --git a/tools/blktap2/include/list.h b/tools/blktap2/include/list.h
> --- a/tools/blktap2/include/list.h
> +++ b/tools/blktap2/include/list.h
> @@ -87,6 +87,26 @@
>         return list->next == head;
>  }
> 
> +static inline void __list_splice(struct list_head *list,
> +                                 struct list_head *head)
> +{
> +        struct list_head *first = list->next;
> +        struct list_head *last = list->prev;
> +        struct list_head *at = head->next;
> +
> +        first->prev = head;
> +        head->next = first;
> +
> +        last->next = at;
> +        at->prev = last;
> +}
> +
> +static inline void list_splice(struct list_head *list, struct list_head *head)
> +{
> +        if (!list_empty(list))
> +                __list_splice(list, head);
> +}
> +
>  #define list_entry(ptr, type, member)                                   \
>          ((type *)((char *)(ptr)-(unsigned long)(&((type *)0)->member)))
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel