From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@novell.com>
Subject: Re: <subject missing #2>
Date: Thu, 18 Aug 2011 11:18:00 +0100
Message-ID: <4E4D02F80200007800051CB6@nat28.tlf.novell.com>
References: <6688c4e4.427@victor.provo.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <6688c4e4.427@victor.provo.novell.com>
Content-Disposition: inline
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: xen-devel@lists.xensource.com, Dong Yang Li <lidongyang@novell.com>
Cc: owen.smith@citrix.com
List-Id: xen-devel@lists.xenproject.org

	>>> On 18.08.11 at 11:35, Li Dongyang <lidongyang@novell.com> =
wrote:
> JBeulich@novell.com=20
> Subject: [PATCH V2 2/3] xen-blkfront: teach blkfront driver handle =
trim=20
> request
> Date: Thu, 18 Aug 2011 17:34:30 +0800
> Message-Id: <1313660071-25230-3-git-send-email-lidongyang@novell.com>
> X-Mailer: git-send-email 1.7.6
> In-Reply-To: <1313660071-25230-1-git-send-email-lidongyang@novell.com>
> References: <1313660071-25230-1-git-send-email-lidongyang@novell.com>
>=20
> The blkfront driver now will read feature-trim from xenstore,
> and set up the request queue with trim params, then we can forward the
> discard requests to backend driver.
>=20
> Signed-off-by: Li Dongyang <lidongyang@novell.com>
> ---
>  drivers/block/xen-blkfront.c |   96 ++++++++++++++++++++++++++++++++----=
------
>  1 files changed, 73 insertions(+), 23 deletions(-)
>=20
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index b536a9c..c22fb07 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -98,6 +98,9 @@ struct blkfront_info
>  	unsigned long shadow_free;
>  	unsigned int feature_flush;
>  	unsigned int flush_op;
> +	unsigned int feature_trim;
> +	unsigned int discard_granularity;
> +	unsigned int discard_alignment;
>  	int is_ready;
>  };
> =20
> @@ -302,29 +305,36 @@ static int blkif_queue_request(struct request =
*req)
>  		ring_req->operation =3D info->flush_op;
>  	}
> =20
> -	ring_req->nr_segments =3D blk_rq_map_sg(req->q, req, info->sg);
> -	BUG_ON(ring_req->nr_segments > BLKIF_MAX_SEGMENTS_PER_REQUEST);
> +	if (unlikely(req->cmd_flags & REQ_DISCARD)) {
> +		/* id, sector_number and handle are set above. */
> +		ring_req->operation =3D BLKIF_OP_TRIM;
> +		ring_req->nr_segments =3D 0;
> +		ring_req->u.trim.nr_sectors =3D blk_rq_sectors(req);
> +	} else {
> +		ring_req->nr_segments =3D blk_rq_map_sg(req->q, req, =
info->sg);
> +		BUG_ON(ring_req->nr_segments > BLKIF_MAX_SEGMENTS_PER_REQUE=
ST);
> =20
> -	for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
> -		buffer_mfn =3D pfn_to_mfn(page_to_pfn(sg_page(sg)));
> -		fsect =3D sg->offset >> 9;
> -		lsect =3D fsect + (sg->length >> 9) - 1;
> -		/* install a grant reference. */
> -		ref =3D gnttab_claim_grant_reference(&gref_head);
> -		BUG_ON(ref =3D=3D -ENOSPC);
> +		for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
> +			buffer_mfn =3D pfn_to_mfn(page_to_pfn(sg_page(sg)))=
;
> +			fsect =3D sg->offset >> 9;
> +			lsect =3D fsect + (sg->length >> 9) - 1;
> +			/* install a grant reference. */
> +			ref =3D gnttab_claim_grant_reference(&gref_head);
> +			BUG_ON(ref =3D=3D -ENOSPC);
> =20
> -		gnttab_grant_foreign_access_ref(
> -				ref,
> -				info->xbdev->otherend_id,
> -				buffer_mfn,
> -				rq_data_dir(req) );
> -
> -		info->shadow[id].frame[i] =3D mfn_to_pfn(buffer_mfn);
> -		ring_req->u.rw.seg[i] =3D
> -				(struct blkif_request_segment) {
> -					.gref       =3D ref,
> -					.first_sect =3D fsect,
> -					.last_sect  =3D lsect };
> +			gnttab_grant_foreign_access_ref(
> +					ref,
> +					info->xbdev->otherend_id,
> +					buffer_mfn,
> +					rq_data_dir(req));
> +
> +			info->shadow[id].frame[i] =3D mfn_to_pfn(buffer_mfn=
);
> +			ring_req->u.rw.seg[i] =3D
> +					(struct blkif_request_segment) {
> +						.gref       =3D ref,
> +						.first_sect =3D fsect,
> +						.last_sect  =3D lsect };
> +		}
>  	}
> =20
>  	info->ring.req_prod_pvt++;
> @@ -399,6 +409,7 @@ wait:
>  static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size)
>  {
>  	struct request_queue *rq;
> +	struct blkfront_info *info =3D gd->private_data;
> =20
>  	rq =3D blk_init_queue(do_blkif_request, &blkif_io_lock);
>  	if (rq =3D=3D NULL)
> @@ -406,6 +417,13 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, =
u16=20
> sector_size)
> =20
>  	queue_flag_set_unlocked(QUEUE_FLAG_VIRT, rq);
> =20
> +	if (info->feature_trim) {
> +		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, rq);
> +		blk_queue_max_discard_sectors(rq, get_capacity(gd));
> +		rq->limits.discard_granularity =3D info->discard_granularit=
y;
> +		rq->limits.discard_alignment =3D 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=20
> *dev_id)
> =20
>  		error =3D (bret->status =3D=3D BLKIF_RSP_OKAY) ? 0 : -EIO;
>  		switch (bret->operation) {
> +		case BLKIF_OP_TRIM:
> +			if (unlikely(bret->status =3D=3D BLKIF_RSP_EOPNOTSU=
PP)) {
> +				struct request_queue *rq =3D info->rq;
> +				printk(KERN_WARNING "blkfront: %s: trim op =
failed\n",
> +					   info->gd->disk_name);
> +				error =3D -EOPNOTSUPP;
> +				info->feature_trim =3D 0;
> +				spin_lock_irq(rq->queue_lock);
> +				queue_flag_clear(QUEUE_FLAG_DISCARD, rq);
> +				spin_unlock_irq(rq->queue_lock);

Are you sure you want to re-enable interrupts here unconditionally?

> +			}
> +			__blk_end_request_all(req, error);
> +			break;
>  		case BLKIF_OP_FLUSH_DISKCACHE:
>  		case BLKIF_OP_WRITE_BARRIER:
>  			if (unlikely(bret->status =3D=3D BLKIF_RSP_EOPNOTSU=
PP)) {
> @@ -1108,7 +1139,9 @@ static void blkfront_connect(struct blkfront_info=
=20
> *info)
>  	unsigned long sector_size;
>  	unsigned int binfo;
>  	int err;
> -	int barrier, flush;
> +	unsigned int discard_granularity;
> +	unsigned int discard_alignment;
> +	int barrier, flush, trim;
> =20
>  	switch (info->connected) {
>  	case BLKIF_STATE_CONNECTED:
> @@ -1178,7 +1211,24 @@ static void blkfront_connect(struct blkfront_info=
=20
> *info)
>  		info->feature_flush =3D REQ_FLUSH;
>  		info->flush_op =3D BLKIF_OP_FLUSH_DISKCACHE;
>  	}
> -	=09
> +
> +	err =3D xenbus_gather(XBT_NIL, info->xbdev->otherend,
> +			    "feature-trim", "%d", &trim,
> +			    NULL);

So you switched this to use "trim", but ...

> +
> +	if (!err && trim) {
> +		info->feature_trim =3D 1;
> +		err =3D xenbus_gather(XBT_NIL, info->xbdev->otherend,
> +			"discard_granularity", "%u", &discard_granularity,
> +			"discard_alignment", "%u", &discard_alignment,

... you kept these using "discard" - quite inconsistent.

Also, I think (but I may be wrong) that dashes are preferred over
underscores in xenstore node names.

> +			NULL);
> +		if (!err) {
> +			info->discard_granularity =3D discard_granularity;
> +			info->discard_alignment =3D discard_alignment;

I think you should set info->feature_trim only here (and then you can
eliminate the local variables).

Jan

> +		}
> +	} else
> +		info->feature_trim =3D 0;
> +
>  	err =3D xlvbd_alloc_gendisk(sectors, info, binfo, sector_size);
>  	if (err) {
>  		xenbus_dev_fatal(info->xbdev, err, "xlvbd_add at %s",