WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

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@xxxxxxxxx>
> # 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@xxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel