Re: [PATCH V2 3/3] xen-blkback: handle trim request in backend driver

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

On Mon, Aug 22, 2011 at 05:43:28PM +0800, Li Dongyang wrote:
> On Thu, Aug 18, 2011 at 10:56 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > On Thu, Aug 18, 2011 at 05:34:31PM +0800, Li Dongyang wrote:
> >> Now blkback driver can handle the trim request from guest, we will
> >> forward the request to phy device if it really has trim support, or we'll
> >> punch a hole on the image file.
> >>
> >> Signed-off-by: Li Dongyang <lidongyang@novell.com>
> >> ---
> >>  drivers/block/xen-blkback/blkback.c |   85 +++++++++++++++++++++++++++++------
> >>  drivers/block/xen-blkback/common.h  |    4 +-
> >>  drivers/block/xen-blkback/xenbus.c  |   61 +++++++++++++++++++++++++
> >>  3 files changed, 135 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> >> index 2330a9a..5acc37a 100644
> >> --- a/drivers/block/xen-blkback/blkback.c
> >> +++ b/drivers/block/xen-blkback/blkback.c
> >> @@ -39,6 +39,9 @@
> >>  #include <linux/list.h>
> >>  #include <linux/delay.h>
> >>  #include <linux/freezer.h>
> >> +#include <linux/loop.h>
> >> +#include <linux/falloc.h>
> >> +#include <linux/fs.h>
> >>
> >>  #include <xen/events.h>
> >>  #include <xen/page.h>
> >> @@ -258,13 +261,16 @@ irqreturn_t xen_blkif_be_int(int irq, void *dev_id)
> >>
> >>  static void print_stats(struct xen_blkif *blkif)
> >>  {
> >> -     pr_info("xen-blkback (%s): oo %3d  |  rd %4d  |  wr %4d  |  f %4d\n",
> >> +     pr_info("xen-blkback (%s): oo %3d  |  rd %4d  |  wr %4d  |  f %4d"
> >> +              "  |  tr %4d\n",
> >>                current->comm, blkif->st_oo_req,
> >> -              blkif->st_rd_req, blkif->st_wr_req, blkif->st_f_req);
> >> +              blkif->st_rd_req, blkif->st_wr_req,
> >> +              blkif->st_f_req, blkif->st_tr_req);
> >>       blkif->st_print = jiffies + msecs_to_jiffies(10 * 1000);
> >>       blkif->st_rd_req = 0;
> >>       blkif->st_wr_req = 0;
> >>       blkif->st_oo_req = 0;
> >> +     blkif->st_tr_req = 0;
> >>  }
> >>
> >>  int xen_blkif_schedule(void *arg)
> >> @@ -563,6 +569,10 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
> >>               blkif->st_f_req++;
> >>               operation = WRITE_FLUSH;
> >>               break;
> >> +     case BLKIF_OP_TRIM:
> >> +             blkif->st_tr_req++;
> >> +             operation = REQ_DISCARD;
> >> +             break;
> >>       case BLKIF_OP_WRITE_BARRIER:
> >>       default:
> >>               operation = 0; /* make gcc happy */
> >> @@ -572,7 +582,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
> >>
> >>       /* Check that the number of segments is sane. */
> >>       nseg = req->nr_segments;
> >> -     if (unlikely(nseg == 0 && operation != WRITE_FLUSH) ||
> >> +     if (unlikely(nseg == 0 && operation != (WRITE_FLUSH | REQ_DISCARD)) ||
> >>           unlikely(nseg > BLKIF_MAX_SEGMENTS_PER_REQUEST)) {
> >>               pr_debug(DRV_PFX "Bad number of segments in request (%d)\n",
> >>                        nseg);
> >> @@ -627,10 +637,13 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
> >>        * the hypercall to unmap the grants - that is all done in
> >>        * xen_blkbk_unmap.
> >>        */
> >> -     if (xen_blkbk_map(req, pending_req, seg))
> >> +     if (operation != BLKIF_OP_TRIM && xen_blkbk_map(req, pending_req, seg))
> >>               goto fail_flush;
> >>
> >> -     /* This corresponding xen_blkif_put is done in __end_block_io_op */
> >> +     /*
> >> +      * This corresponding xen_blkif_put is done in __end_block_io_op, or
> >> +      * below if we are handling a BLKIF_OP_TRIM.
> >> +      */
> >>       xen_blkif_get(blkif);
> >>
> >>       for (i = 0; i < nseg; i++) {
> >> @@ -654,18 +667,62 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
> >>               preq.sector_number += seg[i].nsec;
> >>       }
> >>
> >> -     /* This will be hit if the operation was a flush. */
> >> +     /* This will be hit if the operation was a flush or trim. */
> >>       if (!bio) {
> >> -             BUG_ON(operation != WRITE_FLUSH);
> >> +             BUG_ON(operation != (WRITE_FLUSH | REQ_DISCARD));
> >>
> >> -             bio = bio_alloc(GFP_KERNEL, 0);
> >> -             if (unlikely(bio == NULL))
> >> -                     goto fail_put_bio;
> >> +             if (operation == WRITE_FLUSH) {
> >> +                     bio = bio_alloc(GFP_KERNEL, 0);
> >> +                     if (unlikely(bio == NULL))
> >> +                             goto fail_put_bio;
> >>
> >> -             biolist[nbio++] = bio;
> >> -             bio->bi_bdev    = preq.bdev;
> >> -             bio->bi_private = pending_req;
> >> -             bio->bi_end_io  = end_block_io_op;
> >> +                     biolist[nbio++] = bio;
> >> +                     bio->bi_bdev    = preq.bdev;
> >> +                     bio->bi_private = pending_req;
> >> +                     bio->bi_end_io  = end_block_io_op;
> >> +             } else if (operation == REQ_DISCARD) {
> >> +                     int err = 0;
> >> +                     int status = BLKIF_RSP_OKAY;
> >> +                     struct block_device *bdev = blkif->vbd.bdev;
> >> +
> >> +                     preq.nr_sects = req->u.trim.nr_sectors;
> >> +                     if (blkif->vbd.type & VDISK_PHY_BACKEND)
> >> +                             /* just forward the trim request */
> >> +                             err = blkdev_issue_discard(bdev,
> >> +                                             preq.sector_number,
> >> +                                             preq.nr_sects,
> >> +                                             GFP_KERNEL, 0);
> >> +                     else if (blkif->vbd.type & VDISK_FILE_BACKEND) {
> >> +                             /* punch a hole in the backing file */
> >> +                             struct loop_device *lo =
> >> +                                     bdev->bd_disk->private_data;
> >> +                             struct file *file = lo->lo_backing_file;
> >> +
> >> +                             if (file->f_op->fallocate)
> >> +                                     err = file->f_op->fallocate(file,
> >> +                                             FALLOC_FL_KEEP_SIZE |
> >> +                                             FALLOC_FL_PUNCH_HOLE,
> >> +                                             preq.sector_number << 9,
> >> +                                             preq.nr_sects << 9);
> >> +                             else
> >> +                                     err = -EOPNOTSUPP;
> >> +                     } else
> >> +                             status = BLKIF_RSP_EOPNOTSUPP;
> >> +
> >> +                     if (err == -EOPNOTSUPP) {
> >> +                             DPRINTK("blkback: discard op failed, "
> >> +                                             "not supported\n");
> >
> > Use pr_debug like the rest of the file does.
> gonna fix
> >
> >> +                             status = BLKIF_RSP_EOPNOTSUPP;
> >> +                     } else if (err)
> >> +                             status = BLKIF_RSP_ERROR;
> >> +
> >> +                     if (status == BLKIF_RSP_OKAY)
> >> +                             blkif->st_tr_sect += preq.nr_sects;
> >> +                     make_response(blkif, req->id, req->operation, status);
> >> +                     xen_blkif_put(blkif);
> >> +                     free_req(pending_req);
> >> +                     return 0;
> >> +             }
> >
> > All of this should really be moved to its own function.
> not quite clear about this, do you mean we should make sth like
> dispatch_trim_block_io only for OP_TRIM?
> I added the trim handling stuff to dispatch_rw_block_io because it
> also handles flush stuff.

That function is getting quite busy - it has a lot of code. My thought
was that you could seperate it out. Like

    if (!bio) {
        if (operation == WRITE_FLUSH) {
            bio = bio_alloc(..)
            .. snip (the same as your patch has it.
        } else if (operation == REQ_DISCARD) {
            xen_blk_trim(blkif, preq);
            return 0;
        }

And do all of the operation in xen_blk_trim. Including the make_response ..