From mboxrd@z Thu Jan  1 00:00:00 1970
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: Re: [PATCH V2 3/3] xen-blkback: handle trim request in
	backend driver
Date: Mon, 22 Aug 2011 09:27:31 -0400
Message-ID: <20110822132731.GB8944@dumpdata.com>
References: <1313660071-25230-1-git-send-email-lidongyang@novell.com>
	<1313660071-25230-4-git-send-email-lidongyang@novell.com>
	<20110818145611.GC23922@dumpdata.com>
	<CAKH3R49tQ1y6uyPFDQ0QFRPa3vwt=iuYLUMQxGB1GcXLS_zO6A@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xensource.com>
Content-Disposition: inline
In-Reply-To: <CAKH3R49tQ1y6uyPFDQ0QFRPa3vwt=iuYLUMQxGB1GcXLS_zO6A@mail.gmail.com>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Li Dongyang <jerry87905@gmail.com>
Cc: xen-devel@lists.xensource.com, owen.smith@citrix.com, JBeulich@novell.com
List-Id: xen-devel@lists.xenproject.org

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>
> >> ---
> >> =A0drivers/block/xen-blkback/blkback.c | =A0 85 ++++++++++++++++++++=
+++++++++------
> >> =A0drivers/block/xen-blkback/common.h =A0| =A0 =A04 +-
> >> =A0drivers/block/xen-blkback/xenbus.c =A0| =A0 61 ++++++++++++++++++=
+++++++
> >> =A03 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 @@
> >> =A0#include <linux/list.h>
> >> =A0#include <linux/delay.h>
> >> =A0#include <linux/freezer.h>
> >> +#include <linux/loop.h>
> >> +#include <linux/falloc.h>
> >> +#include <linux/fs.h>
> >>
> >> =A0#include <xen/events.h>
> >> =A0#include <xen/page.h>
> >> @@ -258,13 +261,16 @@ irqreturn_t xen_blkif_be_int(int irq, void *de=
v_id)
> >>
> >> =A0static void print_stats(struct xen_blkif *blkif)
> >> =A0{
> >> - =A0 =A0 pr_info("xen-blkback (%s): oo %3d =A0| =A0rd %4d =A0| =A0w=
r %4d =A0| =A0f %4d\n",
> >> + =A0 =A0 pr_info("xen-blkback (%s): oo %3d =A0| =A0rd %4d =A0| =A0w=
r %4d =A0| =A0f %4d"
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0" =A0| =A0tr %4d\n",
> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0current->comm, blkif->st_oo_req,
> >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0blkif->st_rd_req, blkif->st_wr_req, blk=
if->st_f_req);
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0blkif->st_rd_req, blkif->st_wr_req,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0blkif->st_f_req, blkif->st_tr_req);
> >> =A0 =A0 =A0 blkif->st_print =3D jiffies + msecs_to_jiffies(10 * 1000=
);
> >> =A0 =A0 =A0 blkif->st_rd_req =3D 0;
> >> =A0 =A0 =A0 blkif->st_wr_req =3D 0;
> >> =A0 =A0 =A0 blkif->st_oo_req =3D 0;
> >> + =A0 =A0 blkif->st_tr_req =3D 0;
> >> =A0}
> >>
> >> =A0int xen_blkif_schedule(void *arg)
> >> @@ -563,6 +569,10 @@ static int dispatch_rw_block_io(struct xen_blki=
f *blkif,
> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 blkif->st_f_req++;
> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 operation =3D WRITE_FLUSH;
> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> >> + =A0 =A0 case BLKIF_OP_TRIM:
> >> + =A0 =A0 =A0 =A0 =A0 =A0 blkif->st_tr_req++;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 operation =3D REQ_DISCARD;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 break;
> >> =A0 =A0 =A0 case BLKIF_OP_WRITE_BARRIER:
> >> =A0 =A0 =A0 default:
> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 operation =3D 0; /* make gcc happy */
> >> @@ -572,7 +582,7 @@ static int dispatch_rw_block_io(struct xen_blkif=
 *blkif,
> >>
> >> =A0 =A0 =A0 /* Check that the number of segments is sane. */
> >> =A0 =A0 =A0 nseg =3D req->nr_segments;
> >> - =A0 =A0 if (unlikely(nseg =3D=3D 0 && operation !=3D WRITE_FLUSH) =
||
> >> + =A0 =A0 if (unlikely(nseg =3D=3D 0 && operation !=3D (WRITE_FLUSH =
| REQ_DISCARD)) ||
> >> =A0 =A0 =A0 =A0 =A0 unlikely(nseg > BLKIF_MAX_SEGMENTS_PER_REQUEST))=
 {
> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 pr_debug(DRV_PFX "Bad number of segments=
 in request (%d)\n",
> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0nseg);
> >> @@ -627,10 +637,13 @@ static int dispatch_rw_block_io(struct xen_blk=
if *blkif,
> >> =A0 =A0 =A0 =A0* the hypercall to unmap the grants - that is all don=
e in
> >> =A0 =A0 =A0 =A0* xen_blkbk_unmap.
> >> =A0 =A0 =A0 =A0*/
> >> - =A0 =A0 if (xen_blkbk_map(req, pending_req, seg))
> >> + =A0 =A0 if (operation !=3D BLKIF_OP_TRIM && xen_blkbk_map(req, pen=
ding_req, seg))
> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto fail_flush;
> >>
> >> - =A0 =A0 /* This corresponding xen_blkif_put is done in __end_block=
_io_op */
> >> + =A0 =A0 /*
> >> + =A0 =A0 =A0* This corresponding xen_blkif_put is done in __end_blo=
ck_io_op, or
> >> + =A0 =A0 =A0* below if we are handling a BLKIF_OP_TRIM.
> >> + =A0 =A0 =A0*/
> >> =A0 =A0 =A0 xen_blkif_get(blkif);
> >>
> >> =A0 =A0 =A0 for (i =3D 0; i < nseg; i++) {
> >> @@ -654,18 +667,62 @@ static int dispatch_rw_block_io(struct xen_blk=
if *blkif,
> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 preq.sector_number +=3D seg[i].nsec;
> >> =A0 =A0 =A0 }
> >>
> >> - =A0 =A0 /* This will be hit if the operation was a flush. */
> >> + =A0 =A0 /* This will be hit if the operation was a flush or trim. =
*/
> >> =A0 =A0 =A0 if (!bio) {
> >> - =A0 =A0 =A0 =A0 =A0 =A0 BUG_ON(operation !=3D WRITE_FLUSH);
> >> + =A0 =A0 =A0 =A0 =A0 =A0 BUG_ON(operation !=3D (WRITE_FLUSH | REQ_D=
ISCARD));
> >>
> >> - =A0 =A0 =A0 =A0 =A0 =A0 bio =3D bio_alloc(GFP_KERNEL, 0);
> >> - =A0 =A0 =A0 =A0 =A0 =A0 if (unlikely(bio =3D=3D NULL))
> >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto fail_put_bio;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 if (operation =3D=3D WRITE_FLUSH) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bio =3D bio_alloc(GFP_KERN=
EL, 0);
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (unlikely(bio =3D=3D NU=
LL))
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto fail_=
put_bio;
> >>
> >> - =A0 =A0 =A0 =A0 =A0 =A0 biolist[nbio++] =3D bio;
> >> - =A0 =A0 =A0 =A0 =A0 =A0 bio->bi_bdev =A0 =A0=3D preq.bdev;
> >> - =A0 =A0 =A0 =A0 =A0 =A0 bio->bi_private =3D pending_req;
> >> - =A0 =A0 =A0 =A0 =A0 =A0 bio->bi_end_io =A0=3D end_block_io_op;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 biolist[nbio++] =3D bio;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bio->bi_bdev =A0 =A0=3D pr=
eq.bdev;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bio->bi_private =3D pendin=
g_req;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bio->bi_end_io =A0=3D end_=
block_io_op;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 } else if (operation =3D=3D REQ_DISCARD) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int err =3D 0;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int status =3D BLKIF_RSP_O=
KAY;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct block_device *bdev =
=3D blkif->vbd.bdev;
> >> +
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 preq.nr_sects =3D req->u.t=
rim.nr_sectors;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (blkif->vbd.type & VDIS=
K_PHY_BACKEND)
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* just fo=
rward the trim request */
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D bl=
kdev_issue_discard(bdev,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 preq.sector_number,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 preq.nr_sects,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 GFP_KERNEL, 0);
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 else if (blkif->vbd.type &=
 VDISK_FILE_BACKEND) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* punch a=
 hole in the backing file */
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct loo=
p_device *lo =3D
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 bdev->bd_disk->private_data;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct fil=
e *file =3D lo->lo_backing_file;
> >> +
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (file->=
f_op->fallocate)
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 err =3D file->f_op->fallocate(file,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 FALLOC_FL_KEEP_SIZE |
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 FALLOC_FL_PUNCH_HOLE,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 preq.sector_number << 9,
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 preq.nr_sects << 9);
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 else
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 err =3D -EOPNOTSUPP;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 status =3D=
 BLKIF_RSP_EOPNOTSUPP;
> >> +
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (err =3D=3D -EOPNOTSUPP=
) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 DPRINTK("b=
lkback: discard op failed, "
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 "not supported\n");
> >
> > Use pr_debug like the rest of the file does.
> gonna fix
> >
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 status =3D=
 BLKIF_RSP_EOPNOTSUPP;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else if (err)
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 status =3D=
 BLKIF_RSP_ERROR;
> >> +
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (status =3D=3D BLKIF_RS=
P_OKAY)
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 blkif->st_=
tr_sect +=3D preq.nr_sects;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 make_response(blkif, req->=
id, req->operation, status);
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 xen_blkif_put(blkif);
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 free_req(pending_req);
> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 }
> >
> > 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 =3D=3D WRITE_FLUSH) {
            bio =3D bio_alloc(..)
            .. snip (the same as your patch has it.
        } else if (operation =3D=3D REQ_DISCARD) {
            xen_blk_trim(blkif, preq);
            return 0;
        }

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