lkml.org 
[lkml]   [2012]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.
    >>> On 07.06.12 at 23:59, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
    > --- a/drivers/block/xen-blkfront.c
    > +++ b/drivers/block/xen-blkfront.c
    > @@ -141,14 +141,33 @@ static int get_id_from_freelist(struct blkfront_info
    > *info)
    > return free;
    > }
    >
    > -static void add_id_to_freelist(struct blkfront_info *info,
    > +static int add_id_to_freelist(struct blkfront_info *info,
    > unsigned long id)
    > {
    > + if (info->shadow[id].req.u.rw.id != id)
    > + return -EINVAL;
    > + if (info->shadow[id].request == NULL)
    > + return -EINVAL;
    > info->shadow[id].req.u.rw.id = info->shadow_free;
    > info->shadow[id].request = NULL;
    > info->shadow_free = id;
    > + return 0;
    > }
    >
    > +static const char *op_name(int op)
    > +{
    > + static const char *names[] = {

    Could/should really be "static const char *const names[]".

    > + [BLKIF_OP_READ] = "read",
    > + [BLKIF_OP_WRITE] = "write",
    > + [BLKIF_OP_WRITE_BARRIER] = "barrier",
    > + [BLKIF_OP_FLUSH_DISKCACHE] = "flush",
    > + [BLKIF_OP_RESERVED_1] = "reserved",
    > + [BLKIF_OP_DISCARD] = "discard" };
    > +
    > + if (op < 0 || op >= ARRAY_SIZE(names))
    > + return "unknown";
    > + return names[op];
    > +}
    > static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
    > {
    > unsigned int end = minor + nr;
    > @@ -744,20 +763,31 @@ static irqreturn_t blkif_interrupt(int irq, void
    > *dev_id)
    >
    > bret = RING_GET_RESPONSE(&info->ring, i);
    > id = bret->id;
    > + /*
    > + * The backend has messed up and given us an id that we would
    > + * never have given to it (we stamp it up to BLK_RING_SIZE -
    > + * look in get_id_from_freelist.
    > + */
    > + if (id >= BLK_RING_SIZE)
    > + /* We can't safely get the 'struct request' as
    > + * the id is busted. So limp along. */
    > + goto err_out;

    Getting out of the loop here isn't really what I'd call "limp along" -
    it'd likely get the communication to a halt (if there are more
    responses ready). I'd rather see this print something, and then
    continue the loop. Or alternatively, if we really want to stop
    communication in such a case, initiate tear down of the device.

    > +
    > req = info->shadow[id].request;
    >
    > if (bret->operation != BLKIF_OP_DISCARD)
    > blkif_completion(&info->shadow[id]);
    >
    > - add_id_to_freelist(info, id);
    > + if (add_id_to_freelist(info, id))
    > + goto err_out;

    Same here, obviously.

    >
    > error = (bret->status == BLKIF_RSP_OKAY) ? 0 : -EIO;
    > switch (bret->operation) {
    > case BLKIF_OP_DISCARD:
    > if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
    > struct request_queue *rq = info->rq;
    > - printk(KERN_WARNING "blkfront: %s: discard op failed\n",
    > - info->gd->disk_name);
    > + printk(KERN_WARNING "blkfront: %s: %s op failed\n",
    > + info->gd->disk_name, op_name(bret->operation));
    > error = -EOPNOTSUPP;
    > info->feature_discard = 0;
    > info->feature_secdiscard = 0;
    > @@ -769,18 +799,14 @@ static irqreturn_t blkif_interrupt(int irq, void
    > *dev_id)
    > case BLKIF_OP_FLUSH_DISKCACHE:
    > case BLKIF_OP_WRITE_BARRIER:
    > if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
    > - printk(KERN_WARNING "blkfront: %s: write %s op failed\n",
    > - info->flush_op == BLKIF_OP_WRITE_BARRIER ?
    > - "barrier" : "flush disk cache",
    > - info->gd->disk_name);
    > + printk(KERN_WARNING "blkfront: %s: %s op failed\n",
    > + info->gd->disk_name, op_name(bret->operation));
    > error = -EOPNOTSUPP;
    > }
    > if (unlikely(bret->status == BLKIF_RSP_ERROR &&
    > info->shadow[id].req.u.rw.nr_segments == 0)) {
    > - printk(KERN_WARNING "blkfront: %s: empty write %s op failed\n",
    > - info->flush_op == BLKIF_OP_WRITE_BARRIER ?
    > - "barrier" : "flush disk cache",
    > - info->gd->disk_name);
    > + printk(KERN_WARNING "blkfront: %s: empty %s op failed\n",
    > + info->gd->disk_name, op_name(bret->operation));
    > error = -EOPNOTSUPP;
    > }
    > if (unlikely(error)) {
    > @@ -813,12 +839,18 @@ static irqreturn_t blkif_interrupt(int irq, void
    > *dev_id)
    > goto again;
    > } else
    > info->ring.sring->rsp_event = i + 1;
    > -
    > kick_pending_request_queues(info);
    >
    > spin_unlock_irqrestore(&blkif_io_lock, flags);
    >
    > return IRQ_HANDLED;
    > +err_out:
    > + WARN(1, "%s: response to %s has incorrect id\n",
    > + info->gd->disk_name, op_name(bret->operation));
    > +
    > + spin_unlock_irqrestore(&info->io_lock, flags);
    > +
    > + return IRQ_HANDLED;
    > }
    >
    >
    > diff --git a/include/xen/interface/io/blkif.h
    > b/include/xen/interface/io/blkif.h
    > index ee338bf..bc75c75 100644
    > --- a/include/xen/interface/io/blkif.h
    > +++ b/include/xen/interface/io/blkif.h
    > @@ -59,6 +59,12 @@ typedef uint64_t blkif_sector_t;
    > #define BLKIF_OP_FLUSH_DISKCACHE 3
    >
    > /*
    > + * Used in SLES sources for device specific command packet
    > + * contained within the request. Reserved for that purpose.
    > + */
    > +#define BLKIF_OP_RESERVED_1 4

    If you really want to give this a numeric tag, wouldn't it be better
    to make this _4? But maybe you could leave out the definition here
    altogether, and leave the corresponding names[] slot set to NULL?

    Jan

    > +
    > +/*
    > * Recognised only if "feature-discard" is present in backend xenbus info.
    > * The "feature-discard" node contains a boolean indicating whether trim
    > * (ATA) or unmap (SCSI) - conviently called discard requests are likely
    > --
    > 1.7.7.6




    \
     
     \ /
      Last update: 2012-06-08 10:01    [W:0.034 / U:154.500 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site