Re: [PATCH V4 2/3] xen-blkfront: teach blkfront driver to handle discard requests

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Sep 02, 2011 at 02:30:03PM +0800, Li Dongyang wrote:
> On Thu, Sep 1, 2011 at 11:25 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > On Thu, Sep 01, 2011 at 06:39:09PM +0800, Li Dongyang wrote:
> >> The blkfront driver now will read discard related nodes from xenstore,
> >> and set up the request queue, then we can forward the
> >> discard requests to backend driver.
> >
> >
> > A better description is as follow:
> >
> > xen-blkfront: Handle discard requests.
> >
> >    If the backend advertises 'feature-discard', then interrogate
> >    the backend for alignment, granularity, and max discard block size.
> >    Setup the request queue with the appropiate values and send the
> >    discard operation as required.
> >
> thanks for the description, way better than mine :-)

OK. Lets use that.

.. snip..
> >> +     if (info->feature_discard) {
> >> +             queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, rq);
> >> +             blk_queue_max_discard_sectors(rq, get_capacity(gd));
> >
> > This is not correct. I took a look at the SCSI support for this
> > ('sd_config_discard') and if you look there carefully you will see that
> > the value can be 64KB for example.
> well I don't think so, max_discard_sectors are used to split the bio,
> make them not to
> exceed the max_discard_sectors, see blkdev_issue_discard, and
> that's fine if we use the capacity as max_discard_sectors
> in guest: for file backend, there's no need for max_discard_sectors,
> and for phy backend, we'll make the blkfront sent out a single big
> discard request,

Right, one sector for the whole disk size.

> and when we deal with that in blkback by calling blkdev_issue_discard,
> the real max_discard_sectors
> from phy dev will be used, and take care of splitting bios, Thanks

Ah, you are right:

   if (nr_sects > max_discard_sectors) {
                        bio->bi_size = max_discard_sectors << 9;
                        nr_sects -= max_discard_sectors;
                        sector += max_discard_sectors;
                } else {

handles when that value (nr_sects == get_capacity(x))
is extremely large.


OK, that was the only issue I had - and that has been resolved.

So stuck your patchset series in my tree - but they are not yet
visisble at kernel.org

> 
> >
> > You need to provide a mechanism similar to 'discard-*' to fetch that data
> > from the backend.
> >
> >> +             rq->limits.discard_granularity = info->discard_granularity;
> >> +             rq->limits.discard_alignment = info->discard_alignment;
> >> +     }
> >> +
> >>       /* Hard sector size and max sectors impersonate the equiv. hardware. */
> >>       blk_queue_logical_block_size(rq, sector_size);
> >>       blk_queue_max_hw_sectors(rq, 512);
> >> @@ -722,6 +740,19 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
> >>
> >>               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);
> >> +                             error = -EOPNOTSUPP;
> >> +                             info->feature_discard = 0;
> >> +                             spin_lock(rq->queue_lock);
> >> +                             queue_flag_clear(QUEUE_FLAG_DISCARD, rq);
> >> +                             spin_unlock(rq->queue_lock);
> >> +                     }
> >> +                     __blk_end_request_all(req, error);
> >> +                     break;
> >>               case BLKIF_OP_FLUSH_DISKCACHE:
> >>               case BLKIF_OP_WRITE_BARRIER:
> >>                       if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
> >> @@ -1098,6 +1129,33 @@ blkfront_closing(struct blkfront_info *info)
> >>       bdput(bdev);
> >>  }
> >>
> >> +static void blkfront_setup_discard(struct blkfront_info *info)
> >> +{
> >> +     int err;
> >> +     char *type;
> >> +     unsigned int discard_granularity;
> >> +     unsigned int discard_alignment;
> >> +
> >> +     type = xenbus_read(XBT_NIL, info->xbdev->otherend, "type", NULL);
> >> +     if (IS_ERR(type))
> >> +             return;
> >> +
> >> +     if (strncmp(type, "phy", 3) == 0) {
> >> +             err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> >> +                     "discard-granularity", "%u", &discard_granularity,
> >> +                     "discard-alignment", "%u", &discard_alignment,
> >> +                     NULL);
> >> +             if (!err) {
> >> +                     info->feature_discard = 1;
> >> +                     info->discard_granularity = discard_granularity;
> >> +                     info->discard_alignment = discard_alignment;
> >> +             }
> >> +     } else if (strncmp(type, "file", 4) == 0)
> >> +             info->feature_discard = 1;
> >> +
> >> +     kfree(type);
> >> +}
> >> +
> >>  /*
> >>   * Invoked when the backend is finally 'ready' (and has told produced
> >>   * the details about the physical device - #sectors, size, etc).
> >> @@ -1108,7 +1166,7 @@ static void blkfront_connect(struct blkfront_info *info)
> >>       unsigned long sector_size;
> >>       unsigned int binfo;
> >>       int err;
> >> -     int barrier, flush;
> >> +     int barrier, flush, discard;
> >>
> >>       switch (info->connected) {
> >>       case BLKIF_STATE_CONNECTED:
> >> @@ -1178,7 +1236,14 @@ static void blkfront_connect(struct blkfront_info *info)
> >>               info->feature_flush = REQ_FLUSH;
> >>               info->flush_op = BLKIF_OP_FLUSH_DISKCACHE;
> >>       }
> >> -
> >> +
> >> +     err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> >> +                         "feature-discard", "%d", &discard,
> >> +                         NULL);
> >> +
> >> +     if (!err && discard)
> >> +             blkfront_setup_discard(info);
> >> +
> >>       err = xlvbd_alloc_gendisk(sectors, info, binfo, sector_size);
> >>       if (err) {
> >>               xenbus_dev_fatal(info->xbdev, err, "xlvbd_add at %s",
> >> --
> >> 1.7.6
> >