lkml.org 
[lkml]   [2006]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    Subject[RFC PATCH 33/33] Add Xen virtual block device driver.
    From
    On Tue, 18 Jul 2006 00:00:33 -0700, Chris Wright <chrisw@sous-sol.org> said:
    >
    > The block device frontend driver allows the kernel to access block
    > devices exported exported by a virtual machine containing a physical
    > block device driver.

    First, I think this belongs in drivers/block (and the network driver
    belongs in drivers/net). If we're going to bring xen to the party,
    lets not leave it hiding out in a corner.

    > +static void connect(struct blkfront_info *);
    > +static void blkfront_closing(struct xenbus_device *);
    > +static int blkfront_remove(struct xenbus_device *);
    > +static int talk_to_backend(struct xenbus_device *, struct blkfront_info *);
    > +static int setup_blkring(struct xenbus_device *, struct blkfront_info *);
    > +
    > +static void kick_pending_request_queues(struct blkfront_info *);
    > +
    > +static irqreturn_t blkif_int(int irq, void *dev_id, struct pt_regs *ptregs);
    > +static void blkif_restart_queue(void *arg);
    > +static void blkif_recover(struct blkfront_info *);
    > +static void blkif_completion(struct blk_shadow *);
    > +static void blkif_free(struct blkfront_info *, int);

    I'm pretty sure you can rearrange the code to get rid of the forward
    references.

    > +/**
    > + * We are reconnecting to the backend, due to a suspend/resume, or a backend
    > + * driver restart. We tear down our blkif structure and recreate it, but
    > + * leave the device-layer structures intact so that this is transparent to the
    > + * rest of the kernel.
    > + */
    > +static int blkfront_resume(struct xenbus_device *dev)
    > +{
    > + struct blkfront_info *info = dev->dev.driver_data;
    > + int err;
    > +
    > + DPRINTK("blkfront_resume: %s\n", dev->nodename);
    > +
    > + blkif_free(info, 1);
    > +
    > + err = talk_to_backend(dev, info);
    > + if (!err)
    > + blkif_recover(info);
    > +
    > + return err;
    > +}
    Should blkfront_resume grab blkif_io_lock?

    > + switch (backend_state) {
    > + case XenbusStateUnknown:
    > + case XenbusStateInitialising:
    > + case XenbusStateInitWait:
    > + case XenbusStateInitialised:
    > + case XenbusStateClosed:

    This actually should get fixed elsewhere, but SillyCaps???

    > +static inline int GET_ID_FROM_FREELIST(
    > + struct blkfront_info *info)
    > +{
    > + unsigned long free = info->shadow_free;
    > + BUG_ON(free > BLK_RING_SIZE);
    > + info->shadow_free = info->shadow[free].req.id;
    > + info->shadow[free].req.id = 0x0fffffee; /* debug */
    > + return free;
    > +}
    > +
    > +static inline void ADD_ID_TO_FREELIST(
    > + struct blkfront_info *info, unsigned long id)
    > +{
    > + info->shadow[id].req.id = info->shadow_free;
    > + info->shadow[id].request = 0;
    > + info->shadow_free = id;
    > +}

    A real nit..but why are these routines SHOUTING?

    > +int blkif_release(struct inode *inode, struct file *filep)
    > +{
    > + struct blkfront_info *info = inode->i_bdev->bd_disk->private_data;
    > + info->users--;
    > + if (info->users == 0) {

    Hrm...this strikes me as racey. Don't you need at least a memory
    barrier here to handle SMP?


    > +static struct xlbd_major_info xvd_major_info = {
    > + .major = 201,
    > + .type = &xvd_type_info
    > +};

    I've forgotten what the current policy is around new major numbers.


    Dave B
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-07-18 15:17    [W:4.257 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site