[PATCH V3 0/3] xen-blkfront/xen-blkback trim support

[PATCH V3 0/3] xen-blkfront/xen-blkback trim support

[Li Dongyang]

Dear list,
this is the V3 of the trim support for xen-blkfront/blkback,
thanks for all your reviews.
and when I looked back at Owen's patch in Dec 2010,
http://lists.xensource.com/archives/html/xen-devel/2010-12/msg00299.html
this patch above also add the trim union to blkif_x86_{32|64}_request,
and take care of trim request in blkif_get_x86{32|64}_req(),
however, in the later versions, the part is just gone. I wonder if it is
needed here? Thanks.

Changelog V3:
    rebased on linus's tree
    enum backend types in blkif instead of flags in the interface header
    more reasonable names in xenstore
    move trim requesting handling to a separate function
    do not re-enable interrupts unconditionally when handling response
    set info->feature-trim only when we have all info needed for request queue
Changelog V2:
    rebased on Jeremy's tree
    fixes according to Jan Beulich's comments

[PATCH V3 1/3] xen-blkback: add BLKIF_OP_TRIM and backend types

[Li Dongyang]

This adds the BLKIF_OP_TRIM for blkfront and blkback, also 2 enums telling
us the type of the backend, used in blkback to determine what to do when we
see a trim request.
The BLKIF_OP_TRIM part is just taken from Owen Smith, Thanks

Signed-off-by: Owen Smith <owen.smith@citrix.com>
Signed-off-by: Li Dongyang <lidongyang@novell.com>
---
 drivers/block/xen-blkback/common.h |   10 +++++++++-
 include/xen/interface/io/blkif.h   |   19 +++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
index 9e40b28..146d325 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -113,6 +113,11 @@ enum blkif_protocol {
 	BLKIF_PROTOCOL_X86_64 = 3,
 };
 
+enum blkif_backend_type {
+	BLKIF_BACKEND_PHY  = 1,
+	BLKIF_BACKEND_FILE = 2,
+};
+
 struct xen_vbd {
 	/* What the domain refers to this vbd as. */
 	blkif_vdev_t		handle;
@@ -138,6 +143,7 @@ struct xen_blkif {
 	unsigned int		irq;
 	/* Comms information. */
 	enum blkif_protocol	blk_protocol;
+	enum blkif_backend_type blk_backend_type;
 	union blkif_back_rings	blk_rings;
 	struct vm_struct	*blk_ring_area;
 	/* The VBD attached to this interface. */
@@ -159,8 +165,10 @@ struct xen_blkif {
 	int			st_wr_req;
 	int			st_oo_req;
 	int			st_f_req;
+	int			st_tr_req;
 	int			st_rd_sect;
 	int			st_wr_sect;
+	int			st_tr_sect;
 
 	wait_queue_head_t	waiting_to_free;
 
@@ -182,7 +190,7 @@ struct xen_blkif {
 
 struct phys_req {
 	unsigned short		dev;
-	unsigned short		nr_sects;
+	blkif_sector_t		nr_sects;
 	struct block_device	*bdev;
 	blkif_sector_t		sector_number;
 };
diff --git a/include/xen/interface/io/blkif.h b/include/xen/interface/io/blkif.h
index 3d5d6db..43762dd 100644
--- a/include/xen/interface/io/blkif.h
+++ b/include/xen/interface/io/blkif.h
@@ -57,6 +57,19 @@ typedef uint64_t blkif_sector_t;
  * "feature-flush-cache" node!
  */
 #define BLKIF_OP_FLUSH_DISKCACHE   3
+
+/*
+ * Recognised only if "feature-trim" is present in backend xenbus info.
+ * The "feature-trim" node contains a boolean indicating whether trim
+ * requests are likely to succeed or fail. Either way, a trim request
+ * may fail at any time with BLKIF_RSP_EOPNOTSUPP if it is unsupported by
+ * the underlying block-device hardware. The boolean simply indicates whether
+ * or not it is worthwhile for the frontend to attempt trim requests.
+ * If a backend does not recognise BLKIF_OP_TRIM, it should *not*
+ * create the "feature-trim" node!
+ */
+#define BLKIF_OP_TRIM              5
+
 /*
  * Maximum scatter/gather segments per request.
  * This is carefully chosen so that sizeof(struct blkif_ring) <= PAGE_SIZE.
@@ -74,6 +87,11 @@ struct blkif_request_rw {
 	} seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 };
 
+struct blkif_request_trim {
+	blkif_sector_t sector_number;
+	uint64_t nr_sectors;
+};
+
 struct blkif_request {
 	uint8_t        operation;    /* BLKIF_OP_???                         */
 	uint8_t        nr_segments;  /* number of segments                   */
@@ -81,6 +99,7 @@ struct blkif_request {
 	uint64_t       id;           /* private guest value, echoed in resp  */
 	union {
 		struct blkif_request_rw rw;
+		struct blkif_request_trim trim;
 	} u;
 };
 
-- 
1.7.6

Re: [PATCH V3 1/3] xen-blkback: add BLKIF_OP_TRIM and backend types

[Jan Beulich]

>>> On 24.08.11 at 11:23, Li Dongyang <lidongyang@novell.com> wrote:
> This adds the BLKIF_OP_TRIM for blkfront and blkback, also 2 enums telling
> us the type of the backend, used in blkback to determine what to do when we
> see a trim request.
> The BLKIF_OP_TRIM part is just taken from Owen Smith, Thanks
> 
> Signed-off-by: Owen Smith <owen.smith@citrix.com>
> Signed-off-by: Li Dongyang <lidongyang@novell.com>
> ---
>  drivers/block/xen-blkback/common.h |   10 +++++++++-
>  include/xen/interface/io/blkif.h   |   19 +++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/common.h 
> b/drivers/block/xen-blkback/common.h
> index 9e40b28..146d325 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -113,6 +113,11 @@ enum blkif_protocol {
>  	BLKIF_PROTOCOL_X86_64 = 3,
>  };
>  
> +enum blkif_backend_type {
> +	BLKIF_BACKEND_PHY  = 1,
> +	BLKIF_BACKEND_FILE = 2,
> +};
> +
>  struct xen_vbd {
>  	/* What the domain refers to this vbd as. */
>  	blkif_vdev_t		handle;
> @@ -138,6 +143,7 @@ struct xen_blkif {
>  	unsigned int		irq;
>  	/* Comms information. */
>  	enum blkif_protocol	blk_protocol;
> +	enum blkif_backend_type blk_backend_type;
>  	union blkif_back_rings	blk_rings;
>  	struct vm_struct	*blk_ring_area;
>  	/* The VBD attached to this interface. */
> @@ -159,8 +165,10 @@ struct xen_blkif {
>  	int			st_wr_req;
>  	int			st_oo_req;
>  	int			st_f_req;
> +	int			st_tr_req;
>  	int			st_rd_sect;
>  	int			st_wr_sect;
> +	int			st_tr_sect;

Just to repeat - I don't think this piece of statistic is very useful, the
more that you use "int" here while ...

>  
>  	wait_queue_head_t	waiting_to_free;
>  
> @@ -182,7 +190,7 @@ struct xen_blkif {
>  
>  struct phys_req {
>  	unsigned short		dev;
> -	unsigned short		nr_sects;
> +	blkif_sector_t		nr_sects;

... you specifically widen the field to 64 bits here.

Also, all of the changes to this header look somewhat misplaced, they
should rather be part of the backend patch.

Jan

>  	struct block_device	*bdev;
>  	blkif_sector_t		sector_number;
>  };
> diff --git a/include/xen/interface/io/blkif.h 
> b/include/xen/interface/io/blkif.h
> index 3d5d6db..43762dd 100644
> --- a/include/xen/interface/io/blkif.h
> +++ b/include/xen/interface/io/blkif.h
> @@ -57,6 +57,19 @@ typedef uint64_t blkif_sector_t;
>   * "feature-flush-cache" node!
>   */
>  #define BLKIF_OP_FLUSH_DISKCACHE   3
> +
> +/*
> + * Recognised only if "feature-trim" is present in backend xenbus info.
> + * The "feature-trim" node contains a boolean indicating whether trim
> + * requests are likely to succeed or fail. Either way, a trim request
> + * may fail at any time with BLKIF_RSP_EOPNOTSUPP if it is unsupported by
> + * the underlying block-device hardware. The boolean simply indicates 
> whether
> + * or not it is worthwhile for the frontend to attempt trim requests.
> + * If a backend does not recognise BLKIF_OP_TRIM, it should *not*
> + * create the "feature-trim" node!
> + */
> +#define BLKIF_OP_TRIM              5
> +
>  /*
>   * Maximum scatter/gather segments per request.
>   * This is carefully chosen so that sizeof(struct blkif_ring) <= PAGE_SIZE.
> @@ -74,6 +87,11 @@ struct blkif_request_rw {
>  	} seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
>  };
>  
> +struct blkif_request_trim {
> +	blkif_sector_t sector_number;
> +	uint64_t nr_sectors;
> +};
> +
>  struct blkif_request {
>  	uint8_t        operation;    /* BLKIF_OP_???                         */
>  	uint8_t        nr_segments;  /* number of segments                   */
> @@ -81,6 +99,7 @@ struct blkif_request {
>  	uint64_t       id;           /* private guest value, echoed in resp  */
>  	union {
>  		struct blkif_request_rw rw;
> +		struct blkif_request_trim trim;
>  	} u;
>  };
>  

Re: [PATCH V3 1/3] xen-blkback: add BLKIF_OP_TRIM and backend types

[Li Dongyang]

On Wed, Aug 24, 2011 at 6:26 PM, Jan Beulich <JBeulich@novell.com> wrote:
>>>> On 24.08.11 at 11:23, Li Dongyang <lidongyang@novell.com> wrote:
>> This adds the BLKIF_OP_TRIM for blkfront and blkback, also 2 enums telling
>> us the type of the backend, used in blkback to determine what to do when we
>> see a trim request.
>> The BLKIF_OP_TRIM part is just taken from Owen Smith, Thanks
>>
>> Signed-off-by: Owen Smith <owen.smith@citrix.com>
>> Signed-off-by: Li Dongyang <lidongyang@novell.com>
>> ---
>>  drivers/block/xen-blkback/common.h |   10 +++++++++-
>>  include/xen/interface/io/blkif.h   |   19 +++++++++++++++++++
>>  2 files changed, 28 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/common.h
>> b/drivers/block/xen-blkback/common.h
>> index 9e40b28..146d325 100644
>> --- a/drivers/block/xen-blkback/common.h
>> +++ b/drivers/block/xen-blkback/common.h
>> @@ -113,6 +113,11 @@ enum blkif_protocol {
>>       BLKIF_PROTOCOL_X86_64 = 3,
>>  };
>>
>> +enum blkif_backend_type {
>> +     BLKIF_BACKEND_PHY  = 1,
>> +     BLKIF_BACKEND_FILE = 2,
>> +};
>> +
>>  struct xen_vbd {
>>       /* What the domain refers to this vbd as. */
>>       blkif_vdev_t            handle;
>> @@ -138,6 +143,7 @@ struct xen_blkif {
>>       unsigned int            irq;
>>       /* Comms information. */
>>       enum blkif_protocol     blk_protocol;
>> +     enum blkif_backend_type blk_backend_type;
>>       union blkif_back_rings  blk_rings;
>>       struct vm_struct        *blk_ring_area;
>>       /* The VBD attached to this interface. */
>> @@ -159,8 +165,10 @@ struct xen_blkif {
>>       int                     st_wr_req;
>>       int                     st_oo_req;
>>       int                     st_f_req;
>> +     int                     st_tr_req;
>>       int                     st_rd_sect;
>>       int                     st_wr_sect;
>> +     int                     st_tr_sect;
>
> Just to repeat - I don't think this piece of statistic is very useful, the
> more that you use "int" here while ...
>
>>
>>       wait_queue_head_t       waiting_to_free;
>>
>> @@ -182,7 +190,7 @@ struct xen_blkif {
>>
>>  struct phys_req {
>>       unsigned short          dev;
>> -     unsigned short          nr_sects;
>> +     blkif_sector_t          nr_sects;
>
> ... you specifically widen the field to 64 bits here.
>
sounds reasonable, gonna kill the stat stuff
> Also, all of the changes to this header look somewhat misplaced, they
> should rather be part of the backend patch.
>
> Jan
>
>>       struct block_device     *bdev;
>>       blkif_sector_t          sector_number;
>>  };
>> diff --git a/include/xen/interface/io/blkif.h
>> b/include/xen/interface/io/blkif.h
>> index 3d5d6db..43762dd 100644
>> --- a/include/xen/interface/io/blkif.h
>> +++ b/include/xen/interface/io/blkif.h
>> @@ -57,6 +57,19 @@ typedef uint64_t blkif_sector_t;
>>   * "feature-flush-cache" node!
>>   */
>>  #define BLKIF_OP_FLUSH_DISKCACHE   3
>> +
>> +/*
>> + * Recognised only if "feature-trim" is present in backend xenbus info.
>> + * The "feature-trim" node contains a boolean indicating whether trim
>> + * requests are likely to succeed or fail. Either way, a trim request
>> + * may fail at any time with BLKIF_RSP_EOPNOTSUPP if it is unsupported by
>> + * the underlying block-device hardware. The boolean simply indicates
>> whether
>> + * or not it is worthwhile for the frontend to attempt trim requests.
>> + * If a backend does not recognise BLKIF_OP_TRIM, it should *not*
>> + * create the "feature-trim" node!
>> + */
>> +#define BLKIF_OP_TRIM              5
>> +
>>  /*
>>   * Maximum scatter/gather segments per request.
>>   * This is carefully chosen so that sizeof(struct blkif_ring) <= PAGE_SIZE.
>> @@ -74,6 +87,11 @@ struct blkif_request_rw {
>>       } seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
>>  };
>>
>> +struct blkif_request_trim {
>> +     blkif_sector_t sector_number;
>> +     uint64_t nr_sectors;
>> +};
>> +
>>  struct blkif_request {
>>       uint8_t        operation;    /* BLKIF_OP_???                         */
>>       uint8_t        nr_segments;  /* number of segments                   */
>> @@ -81,6 +99,7 @@ struct blkif_request {
>>       uint64_t       id;           /* private guest value, echoed in resp  */
>>       union {
>>               struct blkif_request_rw rw;
>> +             struct blkif_request_trim trim;
>>       } u;
>>  };
>>
>
>
>
>

Re: [PATCH V3 1/3] xen-blkback: add BLKIF_OP_TRIM and backend types

[Jan Beulich]

>>> On 25.08.11 at 08:34, Li Dongyang <lidongyang@novell.com> wrote:
> On Wed, Aug 24, 2011 at 6:26 PM, Jan Beulich <JBeulich@novell.com> wrote:
>>>>> On 24.08.11 at 11:23, Li Dongyang <lidongyang@novell.com> wrote:
>>> This adds the BLKIF_OP_TRIM for blkfront and blkback, also 2 enums telling
>>> us the type of the backend, used in blkback to determine what to do when we
>>> see a trim request.
>>> The BLKIF_OP_TRIM part is just taken from Owen Smith, Thanks
>>>
>>> Signed-off-by: Owen Smith <owen.smith@citrix.com>
>>> Signed-off-by: Li Dongyang <lidongyang@novell.com>
>>> ---
>>>  drivers/block/xen-blkback/common.h |   10 +++++++++-
>>>  include/xen/interface/io/blkif.h   |   19 +++++++++++++++++++
>>>  2 files changed, 28 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/block/xen-blkback/common.h
>>> b/drivers/block/xen-blkback/common.h
>>> index 9e40b28..146d325 100644
>>> --- a/drivers/block/xen-blkback/common.h
>>> +++ b/drivers/block/xen-blkback/common.h
>>> @@ -113,6 +113,11 @@ enum blkif_protocol {
>>>       BLKIF_PROTOCOL_X86_64 = 3,
>>>  };
>>>
>>> +enum blkif_backend_type {
>>> +     BLKIF_BACKEND_PHY  = 1,
>>> +     BLKIF_BACKEND_FILE = 2,
>>> +};
>>> +
>>>  struct xen_vbd {
>>>       /* What the domain refers to this vbd as. */
>>>       blkif_vdev_t            handle;
>>> @@ -138,6 +143,7 @@ struct xen_blkif {
>>>       unsigned int            irq;
>>>       /* Comms information. */
>>>       enum blkif_protocol     blk_protocol;
>>> +     enum blkif_backend_type blk_backend_type;
>>>       union blkif_back_rings  blk_rings;
>>>       struct vm_struct        *blk_ring_area;
>>>       /* The VBD attached to this interface. */
>>> @@ -159,8 +165,10 @@ struct xen_blkif {
>>>       int                     st_wr_req;
>>>       int                     st_oo_req;
>>>       int                     st_f_req;
>>> +     int                     st_tr_req;
>>>       int                     st_rd_sect;
>>>       int                     st_wr_sect;
>>> +     int                     st_tr_sect;
>>
>> Just to repeat - I don't think this piece of statistic is very useful, the
>> more that you use "int" here while ...
>>
>>>
>>>       wait_queue_head_t       waiting_to_free;
>>>
>>> @@ -182,7 +190,7 @@ struct xen_blkif {
>>>
>>>  struct phys_req {
>>>       unsigned short          dev;
>>> -     unsigned short          nr_sects;
>>> +     blkif_sector_t          nr_sects;
>>
>> ... you specifically widen the field to 64 bits here.
>>
> sounds reasonable, gonna kill the stat stuff

I didn't mean to say kill it all - the st_tr_req is quite reasonable to
have (in line with st_f_req).

Jan

[PATCH V3 2/3] xen-blkfront: teach blkfront driver handle trim request

[Li Dongyang]

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.

Signed-off-by: Li Dongyang <lidongyang@novell.com>
---
 drivers/block/xen-blkfront.c |  111 +++++++++++++++++++++++++++++++++---------
 1 files changed, 88 insertions(+), 23 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 9ea8c25..aa3cede 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;
 };
 
@@ -302,29 +305,36 @@ static int blkif_queue_request(struct request *req)
 		ring_req->operation = info->flush_op;
 	}
 
-	ring_req->nr_segments = 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 = BLKIF_OP_TRIM;
+		ring_req->nr_segments = 0;
+		ring_req->u.trim.nr_sectors = blk_rq_sectors(req);
+	} else {
+		ring_req->nr_segments = blk_rq_map_sg(req->q, req, info->sg);
+		BUG_ON(ring_req->nr_segments > BLKIF_MAX_SEGMENTS_PER_REQUEST);
 
-	for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
-		buffer_mfn = pfn_to_mfn(page_to_pfn(sg_page(sg)));
-		fsect = sg->offset >> 9;
-		lsect = fsect + (sg->length >> 9) - 1;
-		/* install a grant reference. */
-		ref = gnttab_claim_grant_reference(&gref_head);
-		BUG_ON(ref == -ENOSPC);
+		for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
+			buffer_mfn = pfn_to_mfn(page_to_pfn(sg_page(sg)));
+			fsect = sg->offset >> 9;
+			lsect = fsect + (sg->length >> 9) - 1;
+			/* install a grant reference. */
+			ref = gnttab_claim_grant_reference(&gref_head);
+			BUG_ON(ref == -ENOSPC);
 
-		gnttab_grant_foreign_access_ref(
-				ref,
-				info->xbdev->otherend_id,
-				buffer_mfn,
-				rq_data_dir(req) );
-
-		info->shadow[id].frame[i] = mfn_to_pfn(buffer_mfn);
-		ring_req->u.rw.seg[i] =
-				(struct blkif_request_segment) {
-					.gref       = ref,
-					.first_sect = fsect,
-					.last_sect  = lsect };
+			gnttab_grant_foreign_access_ref(
+					ref,
+					info->xbdev->otherend_id,
+					buffer_mfn,
+					rq_data_dir(req));
+
+			info->shadow[id].frame[i] = mfn_to_pfn(buffer_mfn);
+			ring_req->u.rw.seg[i] =
+					(struct blkif_request_segment) {
+						.gref       = ref,
+						.first_sect = fsect,
+						.last_sect  = lsect };
+		}
 	}
 
 	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 = gd->private_data;
 
 	rq = blk_init_queue(do_blkif_request, &blkif_io_lock);
 	if (rq == NULL)
@@ -406,6 +417,13 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size)
 
 	queue_flag_set_unlocked(QUEUE_FLAG_VIRT, rq);
 
+	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 = 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_TRIM:
+			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
+				struct request_queue *rq = info->rq;
+				printk(KERN_WARNING "blkfront: %s: trim op failed\n",
+					   info->gd->disk_name);
+				error = -EOPNOTSUPP;
+				info->feature_trim = 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_trim(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_trim = 1;
+			info->discard_granularity = discard_granularity;
+			info->discard_alignment = discard_alignment;
+		}
+	} else if (strncmp(type, "file", 4) == 0)
+		info->feature_trim = 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, trim;
 
 	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-trim", "%d", &trim,
+			    NULL);
+
+	if (!err && trim)
+		blkfront_setup_trim(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

Re: [PATCH V3 2/3] xen-blkfront: teach blkfront driver handle trim request

[Jan Beulich]

>>> On 24.08.11 at 11:23, Li Dongyang <lidongyang@novell.com> wrote:
> 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.
> 
> Signed-off-by: Li Dongyang <lidongyang@novell.com>
> ---
>  drivers/block/xen-blkfront.c |  111 +++++++++++++++++++++++++++++++++---------
>  1 files changed, 88 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 9ea8c25..aa3cede 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;
>  };
>  
> @@ -302,29 +305,36 @@ static int blkif_queue_request(struct request *req)
>  		ring_req->operation = info->flush_op;
>  	}
>  
> -	ring_req->nr_segments = 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 = BLKIF_OP_TRIM;
> +		ring_req->nr_segments = 0;
> +		ring_req->u.trim.nr_sectors = blk_rq_sectors(req);
> +	} else {
> +		ring_req->nr_segments = blk_rq_map_sg(req->q, req, info->sg);
> +		BUG_ON(ring_req->nr_segments > BLKIF_MAX_SEGMENTS_PER_REQUEST);
>  
> -	for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
> -		buffer_mfn = pfn_to_mfn(page_to_pfn(sg_page(sg)));
> -		fsect = sg->offset >> 9;
> -		lsect = fsect + (sg->length >> 9) - 1;
> -		/* install a grant reference. */
> -		ref = gnttab_claim_grant_reference(&gref_head);
> -		BUG_ON(ref == -ENOSPC);
> +		for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
> +			buffer_mfn = pfn_to_mfn(page_to_pfn(sg_page(sg)));
> +			fsect = sg->offset >> 9;
> +			lsect = fsect + (sg->length >> 9) - 1;
> +			/* install a grant reference. */
> +			ref = gnttab_claim_grant_reference(&gref_head);
> +			BUG_ON(ref == -ENOSPC);
>  
> -		gnttab_grant_foreign_access_ref(
> -				ref,
> -				info->xbdev->otherend_id,
> -				buffer_mfn,
> -				rq_data_dir(req) );
> -
> -		info->shadow[id].frame[i] = mfn_to_pfn(buffer_mfn);
> -		ring_req->u.rw.seg[i] =
> -				(struct blkif_request_segment) {
> -					.gref       = ref,
> -					.first_sect = fsect,
> -					.last_sect  = lsect };
> +			gnttab_grant_foreign_access_ref(
> +					ref,
> +					info->xbdev->otherend_id,
> +					buffer_mfn,
> +					rq_data_dir(req));
> +
> +			info->shadow[id].frame[i] = mfn_to_pfn(buffer_mfn);
> +			ring_req->u.rw.seg[i] =
> +					(struct blkif_request_segment) {
> +						.gref       = ref,
> +						.first_sect = fsect,
> +						.last_sect  = lsect };
> +		}
>  	}
>  
>  	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 = gd->private_data;
>  
>  	rq = blk_init_queue(do_blkif_request, &blkif_io_lock);
>  	if (rq == NULL)
> @@ -406,6 +417,13 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 
> sector_size)
>  
>  	queue_flag_set_unlocked(QUEUE_FLAG_VIRT, rq);
>  
> +	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 = info->discard_granularity;
> +		rq->limits.discard_alignment = info->discard_alignment;

Don't you also need to set rq->limits.max_discard_sectors here (since
when zero blkdev_issue_discard() doesn't do anything)? And wouldn't
that need to be propagated from the backend, too?

> +	}
> +
>  	/* 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_TRIM:
> +			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
> +				struct request_queue *rq = info->rq;
> +				printk(KERN_WARNING "blkfront: %s: trim op failed\n",
> +					   info->gd->disk_name);
> +				error = -EOPNOTSUPP;
> +				info->feature_trim = 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_trim(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);

Let me repeat my wish to have these nodes start with "trim-" rather
than "discard-", so they can be easily associated with the "feature-trim"
one.

Jan

> +		if (!err) {
> +			info->feature_trim = 1;
> +			info->discard_granularity = discard_granularity;
> +			info->discard_alignment = discard_alignment;
> +		}
> +	} else if (strncmp(type, "file", 4) == 0)
> +		info->feature_trim = 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, trim;
>  
>  	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-trim", "%d", &trim,
> +			    NULL);
> +
> +	if (!err && trim)
> +		blkfront_setup_trim(info);
> +
>  	err = xlvbd_alloc_gendisk(sectors, info, binfo, sector_size);
>  	if (err) {
>  		xenbus_dev_fatal(info->xbdev, err, "xlvbd_add at %s",

Re: [PATCH V3 2/3] xen-blkfront: teach blkfront driver handle trim request

[Li Dongyang]

On Wed, Aug 24, 2011 at 6:42 PM, Jan Beulich <JBeulich@novell.com> wrote:
>>>> On 24.08.11 at 11:23, Li Dongyang <lidongyang@novell.com> wrote:
>> 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.
>>
>> Signed-off-by: Li Dongyang <lidongyang@novell.com>
>> ---
>>  drivers/block/xen-blkfront.c |  111 +++++++++++++++++++++++++++++++++---------
>>  1 files changed, 88 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>> index 9ea8c25..aa3cede 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;
>>  };
>>
>> @@ -302,29 +305,36 @@ static int blkif_queue_request(struct request *req)
>>               ring_req->operation = info->flush_op;
>>       }
>>
>> -     ring_req->nr_segments = 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 = BLKIF_OP_TRIM;
>> +             ring_req->nr_segments = 0;
>> +             ring_req->u.trim.nr_sectors = blk_rq_sectors(req);
>> +     } else {
>> +             ring_req->nr_segments = blk_rq_map_sg(req->q, req, info->sg);
>> +             BUG_ON(ring_req->nr_segments > BLKIF_MAX_SEGMENTS_PER_REQUEST);
>>
>> -     for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
>> -             buffer_mfn = pfn_to_mfn(page_to_pfn(sg_page(sg)));
>> -             fsect = sg->offset >> 9;
>> -             lsect = fsect + (sg->length >> 9) - 1;
>> -             /* install a grant reference. */
>> -             ref = gnttab_claim_grant_reference(&gref_head);
>> -             BUG_ON(ref == -ENOSPC);
>> +             for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
>> +                     buffer_mfn = pfn_to_mfn(page_to_pfn(sg_page(sg)));
>> +                     fsect = sg->offset >> 9;
>> +                     lsect = fsect + (sg->length >> 9) - 1;
>> +                     /* install a grant reference. */
>> +                     ref = gnttab_claim_grant_reference(&gref_head);
>> +                     BUG_ON(ref == -ENOSPC);
>>
>> -             gnttab_grant_foreign_access_ref(
>> -                             ref,
>> -                             info->xbdev->otherend_id,
>> -                             buffer_mfn,
>> -                             rq_data_dir(req) );
>> -
>> -             info->shadow[id].frame[i] = mfn_to_pfn(buffer_mfn);
>> -             ring_req->u.rw.seg[i] =
>> -                             (struct blkif_request_segment) {
>> -                                     .gref       = ref,
>> -                                     .first_sect = fsect,
>> -                                     .last_sect  = lsect };
>> +                     gnttab_grant_foreign_access_ref(
>> +                                     ref,
>> +                                     info->xbdev->otherend_id,
>> +                                     buffer_mfn,
>> +                                     rq_data_dir(req));
>> +
>> +                     info->shadow[id].frame[i] = mfn_to_pfn(buffer_mfn);
>> +                     ring_req->u.rw.seg[i] =
>> +                                     (struct blkif_request_segment) {
>> +                                             .gref       = ref,
>> +                                             .first_sect = fsect,
>> +                                             .last_sect  = lsect };
>> +             }
>>       }
>>
>>       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 = gd->private_data;
>>
>>       rq = blk_init_queue(do_blkif_request, &blkif_io_lock);
>>       if (rq == NULL)
>> @@ -406,6 +417,13 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16
>> sector_size)
>>
>>       queue_flag_set_unlocked(QUEUE_FLAG_VIRT, rq);
>>
>> +     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 = info->discard_granularity;
>> +             rq->limits.discard_alignment = info->discard_alignment;
>
> Don't you also need to set rq->limits.max_discard_sectors here (since
> when zero blkdev_issue_discard() doesn't do anything)? And wouldn't
> that need to be propagated from the backend, too?
the max_discard_sectors are set by blk_queue_max_discard_sectors() above ;-)
rq->limits.max_discard_sectors is the full phy device size, and if we
only assign
a partition to guest, the number is incorrect for guest, so the
max_discard_sectors should
be the capacity the guest will see, Thanks
>
>> +     }
>> +
>>       /* 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_TRIM:
>> +                     if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
>> +                             struct request_queue *rq = info->rq;
>> +                             printk(KERN_WARNING "blkfront: %s: trim op failed\n",
>> +                                        info->gd->disk_name);
>> +                             error = -EOPNOTSUPP;
>> +                             info->feature_trim = 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_trim(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);
>
> Let me repeat my wish to have these nodes start with "trim-" rather
> than "discard-", so they can be easily associated with the "feature-trim"
> one.
>
> Jan
>
>> +             if (!err) {
>> +                     info->feature_trim = 1;
>> +                     info->discard_granularity = discard_granularity;
>> +                     info->discard_alignment = discard_alignment;
>> +             }
>> +     } else if (strncmp(type, "file", 4) == 0)
>> +             info->feature_trim = 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, trim;
>>
>>       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-trim", "%d", &trim,
>> +                         NULL);
>> +
>> +     if (!err && trim)
>> +             blkfront_setup_trim(info);
>> +
>>       err = xlvbd_alloc_gendisk(sectors, info, binfo, sector_size);
>>       if (err) {
>>               xenbus_dev_fatal(info->xbdev, err, "xlvbd_add at %s",
>
>
>
>

Re: [PATCH V3 2/3] xen-blkfront: teach blkfront driver handle trim request

[Jan Beulich]

>>> On 25.08.11 at 08:37, Li Dongyang <lidongyang@novell.com> wrote:
> On Wed, Aug 24, 2011 at 6:42 PM, Jan Beulich <JBeulich@novell.com> wrote:
>>>>> On 24.08.11 at 11:23, Li Dongyang <lidongyang@novell.com> wrote:
>>> 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.
>>>
>>> Signed-off-by: Li Dongyang <lidongyang@novell.com>
>>> ---
>>>  drivers/block/xen-blkfront.c |  111 +++++++++++++++++++++++++++++++++---------
>>>  1 files changed, 88 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>> index 9ea8c25..aa3cede 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;
>>>  };
>>>
>>> @@ -302,29 +305,36 @@ static int blkif_queue_request(struct request *req)
>>>               ring_req->operation = info->flush_op;
>>>       }
>>>
>>> -     ring_req->nr_segments = 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 = BLKIF_OP_TRIM;
>>> +             ring_req->nr_segments = 0;
>>> +             ring_req->u.trim.nr_sectors = blk_rq_sectors(req);
>>> +     } else {
>>> +             ring_req->nr_segments = blk_rq_map_sg(req->q, req, info->sg);
>>> +             BUG_ON(ring_req->nr_segments > BLKIF_MAX_SEGMENTS_PER_REQUEST);
>>>
>>> -     for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
>>> -             buffer_mfn = pfn_to_mfn(page_to_pfn(sg_page(sg)));
>>> -             fsect = sg->offset >> 9;
>>> -             lsect = fsect + (sg->length >> 9) - 1;
>>> -             /* install a grant reference. */
>>> -             ref = gnttab_claim_grant_reference(&gref_head);
>>> -             BUG_ON(ref == -ENOSPC);
>>> +             for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
>>> +                     buffer_mfn = pfn_to_mfn(page_to_pfn(sg_page(sg)));
>>> +                     fsect = sg->offset >> 9;
>>> +                     lsect = fsect + (sg->length >> 9) - 1;
>>> +                     /* install a grant reference. */
>>> +                     ref = gnttab_claim_grant_reference(&gref_head);
>>> +                     BUG_ON(ref == -ENOSPC);
>>>
>>> -             gnttab_grant_foreign_access_ref(
>>> -                             ref,
>>> -                             info->xbdev->otherend_id,
>>> -                             buffer_mfn,
>>> -                             rq_data_dir(req) );
>>> -
>>> -             info->shadow[id].frame[i] = mfn_to_pfn(buffer_mfn);
>>> -             ring_req->u.rw.seg[i] =
>>> -                             (struct blkif_request_segment) {
>>> -                                     .gref       = ref,
>>> -                                     .first_sect = fsect,
>>> -                                     .last_sect  = lsect };
>>> +                     gnttab_grant_foreign_access_ref(
>>> +                                     ref,
>>> +                                     info->xbdev->otherend_id,
>>> +                                     buffer_mfn,
>>> +                                     rq_data_dir(req));
>>> +
>>> +                     info->shadow[id].frame[i] = mfn_to_pfn(buffer_mfn);
>>> +                     ring_req->u.rw.seg[i] =
>>> +                                     (struct blkif_request_segment) {
>>> +                                             .gref       = ref,
>>> +                                             .first_sect = fsect,
>>> +                                             .last_sect  = lsect };
>>> +             }
>>>       }
>>>
>>>       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 = gd->private_data;
>>>
>>>       rq = blk_init_queue(do_blkif_request, &blkif_io_lock);
>>>       if (rq == NULL)
>>> @@ -406,6 +417,13 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16
>>> sector_size)
>>>
>>>       queue_flag_set_unlocked(QUEUE_FLAG_VIRT, rq);
>>>
>>> +     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 = info->discard_granularity;
>>> +             rq->limits.discard_alignment = info->discard_alignment;
>>
>> Don't you also need to set rq->limits.max_discard_sectors here (since
>> when zero blkdev_issue_discard() doesn't do anything)? And wouldn't
>> that need to be propagated from the backend, too?
> the max_discard_sectors are set by blk_queue_max_discard_sectors() above ;-)

Oh, silly me to overlook that.

> rq->limits.max_discard_sectors is the full phy device size, and if we
> only assign
> a partition to guest, the number is incorrect for guest, so the
> max_discard_sectors should
> be the capacity the guest will see, Thanks

Using the capacity seems right only for the file: case; I'd still think
the backend should pass the underlying disk's setting for the phy:
one.

Jan

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

[Li Dongyang]

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 |   88 +++++++++++++++++++++++++++++------
 drivers/block/xen-blkback/xenbus.c  |   62 ++++++++++++++++++++++++
 2 files changed, 136 insertions(+), 14 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 2330a9a..caa33ee 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)
@@ -410,6 +416,45 @@ static int xen_blkbk_map(struct blkif_request *req,
 	return ret;
 }
 
+static void xen_blk_trim(struct xen_blkif *blkif, struct blkif_request *req)
+{
+	int err = 0;
+	int status = BLKIF_RSP_OKAY;
+	struct block_device *bdev = blkif->vbd.bdev;
+
+	if (blkif->blk_backend_type == BLKIF_BACKEND_PHY)
+		/* just forward the trim request */
+		err = blkdev_issue_discard(bdev,
+				req->u.trim.sector_number,
+				req->u.trim.nr_sectors,
+				GFP_KERNEL, 0);
+	else if (blkif->blk_backend_type == BLKIF_BACKEND_FILE) {
+		/* 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,
+				req->u.trim.sector_number << 9,
+				req->u.trim.nr_sectors << 9);
+		else
+			err = -EOPNOTSUPP;
+	} else
+		err = -EOPNOTSUPP;
+
+	if (err == -EOPNOTSUPP) {
+		pr_debug(DRV_PFX "blkback: discard op failed, "
+				"not supported\n");
+		status = BLKIF_RSP_EOPNOTSUPP;
+	} else if (err)
+		status = BLKIF_RSP_ERROR;
+
+	if (status == BLKIF_RSP_OKAY)
+		blkif->st_tr_sect += req->u.trim.nr_sectors;
+	make_response(blkif, req->id, req->operation, status);
+}
+
 /*
  * Completion callback on the bio's. Called as bh->b_end_io()
  */
@@ -563,6 +608,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 +621,8 @@ 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 &&
+				operation != REQ_DISCARD) ||
 	    unlikely(nseg > BLKIF_MAX_SEGMENTS_PER_REQUEST)) {
 		pr_debug(DRV_PFX "Bad number of segments in request (%d)\n",
 			 nseg);
@@ -627,10 +677,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 +707,25 @@ 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 && operation != 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) {
+			xen_blk_trim(blkif, req);
+			xen_blkif_put(blkif);
+			free_req(pending_req);
+			return 0;
+		}
 	}
 
 	/*
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 3f129b4..84ee731 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -272,16 +272,20 @@ VBD_SHOW(oo_req,  "%d\n", be->blkif->st_oo_req);
 VBD_SHOW(rd_req,  "%d\n", be->blkif->st_rd_req);
 VBD_SHOW(wr_req,  "%d\n", be->blkif->st_wr_req);
 VBD_SHOW(f_req,  "%d\n", be->blkif->st_f_req);
+VBD_SHOW(tr_req, "%d\n", be->blkif->st_tr_req);
 VBD_SHOW(rd_sect, "%d\n", be->blkif->st_rd_sect);
 VBD_SHOW(wr_sect, "%d\n", be->blkif->st_wr_sect);
+VBD_SHOW(tr_sect, "%d\n", be->blkif->st_tr_sect);
 
 static struct attribute *xen_vbdstat_attrs[] = {
 	&dev_attr_oo_req.attr,
 	&dev_attr_rd_req.attr,
 	&dev_attr_wr_req.attr,
 	&dev_attr_f_req.attr,
+	&dev_attr_tr_req.attr,
 	&dev_attr_rd_sect.attr,
 	&dev_attr_wr_sect.attr,
+	&dev_attr_tr_sect.attr,
 	NULL
 };
 
@@ -419,6 +423,60 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 	return err;
 }
 
+int xen_blkbk_trim(struct xenbus_transaction xbt, struct backend_info *be)
+{
+	struct xenbus_device *dev = be->dev;
+	struct xen_blkif *blkif = be->blkif;
+	char *type;
+	int err;
+	int state = 0;
+
+	type = xenbus_read(XBT_NIL, dev->nodename, "type", NULL);
+	if (!IS_ERR(type)) {
+		if (strncmp(type, "file", 4) == 0) {
+			state = 1;
+			blkif->blk_backend_type = BLKIF_BACKEND_FILE;
+		}
+		if (strncmp(type, "phy", 3) == 0) {
+			struct block_device *bdev = be->blkif->vbd.bdev;
+			struct request_queue *q = bdev_get_queue(bdev);
+			if (blk_queue_discard(q)) {
+				err = xenbus_printf(xbt, dev->nodename,
+					"discard-granularity", "%u",
+					q->limits.discard_granularity);
+				if (err) {
+					xenbus_dev_fatal(dev, err,
+						"writing discard-granularity");
+					goto kfree;
+				}
+				err = xenbus_printf(xbt, dev->nodename,
+					"discard-alignment", "%u",
+					q->limits.discard_alignment);
+				if (err) {
+					xenbus_dev_fatal(dev, err,
+						"writing discard-alignment");
+					goto kfree;
+				}
+				state = 1;
+				blkif->blk_backend_type = BLKIF_BACKEND_PHY;
+			}
+		}
+	} else {
+		err = PTR_ERR(type);
+		xenbus_dev_fatal(dev, err, "reading type");
+		goto out;
+	}
+
+	err = xenbus_printf(xbt, dev->nodename, "feature-trim",
+			    "%d", state);
+	if (err)
+		xenbus_dev_fatal(dev, err, "writing feature-trim");
+kfree:
+	kfree(type);
+out:
+	return err;
+}
+
 /*
  * Entry point to this code when a new device is created.  Allocate the basic
  * structures, and watch the store waiting for the hotplug scripts to tell us
@@ -650,6 +708,10 @@ again:
 	if (err)
 		goto abort;
 
+	err = xen_blkbk_trim(xbt, be);
+	if (err)
+		goto abort;
+
 	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",
 			    (unsigned long long)vbd_sz(&be->blkif->vbd));
 	if (err) {
-- 
1.7.6

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

[Konrad Rzeszutek Wilk]

> +static void xen_blk_trim(struct xen_blkif *blkif, struct blkif_request *req)

Just call it discard: s/trim/discard/ through all the patches.

Also make sure you do that for the functions. And for the 'tr' - change it to
'discard' maybe?

.. snip..
> +int xen_blkbk_trim(struct xenbus_transaction xbt, struct backend_info *be)

Call it discard.

.. snip ..
> +	err = xenbus_printf(xbt, dev->nodename, "feature-trim",

>From one hand it would be really nice to call this 'feature-discard'
but the Citrix frontends (and perhaps the SuSE ones too?) expect
it as 'feature-trim'. Maybe we should just call it 'feature-discard'
here and add a backwards compatible patch that will call it
'feature-trim'?

Sadly, the 'feature-trim' has been enumareted in the blkif.h so it
kind of is written in stone.

Keir, what is your feeling on this?

Can we change the name to 'feature-discard'?


> +			    "%d", state);
> +	if (err)
> +		xenbus_dev_fatal(dev, err, "writing feature-trim");
> +kfree:
> +	kfree(type);
> +out:
> +	return err;
> +}
> +
>  /*
>   * Entry point to this code when a new device is created.  Allocate the basic
>   * structures, and watch the store waiting for the hotplug scripts to tell us
> @@ -650,6 +708,10 @@ again:
>  	if (err)
>  		goto abort;
>  
> +	err = xen_blkbk_trim(xbt, be);
> +	if (err)
> +		goto abort;
> +

No need really. We don't need to abort b/c we can't establish discard
support.

>  	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",
>  			    (unsigned long long)vbd_sz(&be->blkif->vbd));
>  	if (err) {
> -- 
> 1.7.6
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH V3 0/3] xen-blkfront/xen-blkback trim support

[Pasi Kärkkäinen]

On Wed, Aug 24, 2011 at 05:23:42PM +0800, Li Dongyang wrote:
> Dear list,
> this is the V3 of the trim support for xen-blkfront/blkback,

Isn't the generic name for this functionality "discard" in Linux?

and "trim" being the ATA specific discard-implementation,
and "scsi unmap" the SAS/SCSI specific discard-implementation?

Just wondering..

-- Pasi

> thanks for all your reviews.
> and when I looked back at Owen's patch in Dec 2010,
> http://lists.xensource.com/archives/html/xen-devel/2010-12/msg00299.html
> this patch above also add the trim union to blkif_x86_{32|64}_request,
> and take care of trim request in blkif_get_x86{32|64}_req(),
> however, in the later versions, the part is just gone. I wonder if it is
> needed here? Thanks.
> 
> Changelog V3:
>     rebased on linus's tree
>     enum backend types in blkif instead of flags in the interface header
>     more reasonable names in xenstore
>     move trim requesting handling to a separate function
>     do not re-enable interrupts unconditionally when handling response
>     set info->feature-trim only when we have all info needed for request queue
> Changelog V2:
>     rebased on Jeremy's tree
>     fixes according to Jan Beulich's comments
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH V3 0/3] xen-blkfront/xen-blkback trim support

[Konrad Rzeszutek Wilk]

On Wed, Aug 24, 2011 at 05:17:54PM +0300, Pasi Kärkkäinen wrote:
> On Wed, Aug 24, 2011 at 05:23:42PM +0800, Li Dongyang wrote:
> > Dear list,
> > this is the V3 of the trim support for xen-blkfront/blkback,
> 
> Isn't the generic name for this functionality "discard" in Linux?

> 
> and "trim" being the ATA specific discard-implementation,
> and "scsi unmap" the SAS/SCSI specific discard-implementation?

Yeah. I think you are right. The 'feature-discard' sounds much much
more generic than the 'trim'. Since we are still implementing this
and I think we can take the liberty of making it 'feature-discard'.

Albeit it would mean that the Citrix folks would have to rewrite
their drivers.
> 
> Just wondering..
> 
> -- Pasi
> 
> > thanks for all your reviews.
> > and when I looked back at Owen's patch in Dec 2010,
> > http://lists.xensource.com/archives/html/xen-devel/2010-12/msg00299.html
> > this patch above also add the trim union to blkif_x86_{32|64}_request,
> > and take care of trim request in blkif_get_x86{32|64}_req(),
> > however, in the later versions, the part is just gone. I wonder if it is
> > needed here? Thanks.
> > 
> > Changelog V3:
> >     rebased on linus's tree
> >     enum backend types in blkif instead of flags in the interface header
> >     more reasonable names in xenstore
> >     move trim requesting handling to a separate function
> >     do not re-enable interrupts unconditionally when handling response
> >     set info->feature-trim only when we have all info needed for request queue
> > Changelog V2:
> >     rebased on Jeremy's tree
> >     fixes according to Jan Beulich's comments
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH V3 0/3] xen-blkfront/xen-blkback trim support

[Li Dongyang]

On Sat, Aug 27, 2011 at 1:10 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Wed, Aug 24, 2011 at 05:17:54PM +0300, Pasi Kärkkäinen wrote:
>> On Wed, Aug 24, 2011 at 05:23:42PM +0800, Li Dongyang wrote:
>> > Dear list,
>> > this is the V3 of the trim support for xen-blkfront/blkback,
>>
>> Isn't the generic name for this functionality "discard" in Linux?
>
>>
>> and "trim" being the ATA specific discard-implementation,
>> and "scsi unmap" the SAS/SCSI specific discard-implementation?
>
> Yeah. I think you are right. The 'feature-discard' sounds much much
> more generic than the 'trim'. Since we are still implementing this
> and I think we can take the liberty of making it 'feature-discard'.
yeah, that's why I use "feature-discard" in the patch at the first time.
but since the header has already BLKIF_OP_TRIM so I changed that in
later versions.
and agree with u feature-discard makes more sense, maybe we also need to rename
BLKIF_OP_TRIM to BLKIF_OP_DISCARD?
>
> Albeit it would mean that the Citrix folks would have to rewrite
> their drivers.
>>
>> Just wondering..
>>
>> -- Pasi
>>
>> > thanks for all your reviews.
>> > and when I looked back at Owen's patch in Dec 2010,
>> > http://lists.xensource.com/archives/html/xen-devel/2010-12/msg00299.html
>> > this patch above also add the trim union to blkif_x86_{32|64}_request,
>> > and take care of trim request in blkif_get_x86{32|64}_req(),
>> > however, in the later versions, the part is just gone. I wonder if it is
>> > needed here? Thanks.
>> >
>> > Changelog V3:
>> >     rebased on linus's tree
>> >     enum backend types in blkif instead of flags in the interface header
>> >     more reasonable names in xenstore
>> >     move trim requesting handling to a separate function
>> >     do not re-enable interrupts unconditionally when handling response
>> >     set info->feature-trim only when we have all info needed for request queue
>> > Changelog V2:
>> >     rebased on Jeremy's tree
>> >     fixes according to Jan Beulich's comments
>> >
>> >
>> > _______________________________________________
>> > Xen-devel mailing list
>> > Xen-devel@lists.xensource.com
>> > http://lists.xensource.com/xen-devel
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>

Re: [PATCH V3 0/3] xen-blkfront/xen-blkback trim support

[Konrad Rzeszutek Wilk]

On Mon, Aug 29, 2011 at 03:47:37PM +0800, Li Dongyang wrote:
> On Sat, Aug 27, 2011 at 1:10 AM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > On Wed, Aug 24, 2011 at 05:17:54PM +0300, Pasi Kärkkäinen wrote:
> >> On Wed, Aug 24, 2011 at 05:23:42PM +0800, Li Dongyang wrote:
> >> > Dear list,
> >> > this is the V3 of the trim support for xen-blkfront/blkback,
> >>
> >> Isn't the generic name for this functionality "discard" in Linux?
> >
> >>
> >> and "trim" being the ATA specific discard-implementation,
> >> and "scsi unmap" the SAS/SCSI specific discard-implementation?
> >
> > Yeah. I think you are right. The 'feature-discard' sounds much much
> > more generic than the 'trim'. Since we are still implementing this
> > and I think we can take the liberty of making it 'feature-discard'.
> yeah, that's why I use "feature-discard" in the patch at the first time.
> but since the header has already BLKIF_OP_TRIM so I changed that in
> later versions.
> and agree with u feature-discard makes more sense, maybe we also need to rename
> BLKIF_OP_TRIM to BLKIF_OP_DISCARD?

Lets get Keir and Ian's input on this.

Re: [PATCH V3 0/3] xen-blkfront/xen-blkback trim support

[Konrad Rzeszutek Wilk]

On Wed, Aug 24, 2011 at 05:23:42PM +0800, Li Dongyang wrote:
> Dear list,
> this is the V3 of the trim support for xen-blkfront/blkback,
> thanks for all your reviews.
> and when I looked back at Owen's patch in Dec 2010,
> http://lists.xensource.com/archives/html/xen-devel/2010-12/msg00299.html
> this patch above also add the trim union to blkif_x86_{32|64}_request,
> and take care of trim request in blkif_get_x86{32|64}_req(),
> however, in the later versions, the part is just gone. I wonder if it is
> needed here? Thanks.

Are you referring to git commit 51de69523ffe1c17994dc2f260369f29dfdce71c
xen: Union the blkif_request request specific fields
    
    Prepare for extending the block device ring to allow request
    specific fields, by moving the request specific fields for
    reads, writes and barrier requests to a union member.
?

> 
> Changelog V3:
>     rebased on linus's tree
>     enum backend types in blkif instead of flags in the interface header
>     more reasonable names in xenstore
>     move trim requesting handling to a separate function
>     do not re-enable interrupts unconditionally when handling response
>     set info->feature-trim only when we have all info needed for request queue
> Changelog V2:
>     rebased on Jeremy's tree
>     fixes according to Jan Beulich's comments
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH V3 0/3] xen-blkfront/xen-blkback trim support

[Li Dongyang]

On Sat, Aug 27, 2011 at 12:56 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Wed, Aug 24, 2011 at 05:23:42PM +0800, Li Dongyang wrote:
>> Dear list,
>> this is the V3 of the trim support for xen-blkfront/blkback,
>> thanks for all your reviews.
>> and when I looked back at Owen's patch in Dec 2010,
>> http://lists.xensource.com/archives/html/xen-devel/2010-12/msg00299.html
>> this patch above also add the trim union to blkif_x86_{32|64}_request,
>> and take care of trim request in blkif_get_x86{32|64}_req(),
>> however, in the later versions, the part is just gone. I wonder if it is
>> needed here? Thanks.
>
> Are you referring to git commit 51de69523ffe1c17994dc2f260369f29dfdce71c
> xen: Union the blkif_request request specific fields
that's the patch merged, the link I gave above was the previous
version cooked up by
Owen, and the patch in the link has changes to struct
blkif_x86_{32|64}_request related stuffs,
but that;s gone in the merged version, so I'm not sure if the gone
part is needed here, Thanks
>
>    Prepare for extending the block device ring to allow request
>    specific fields, by moving the request specific fields for
>    reads, writes and barrier requests to a union member.
> ?
>
>>
>> Changelog V3:
>>     rebased on linus's tree
>>     enum backend types in blkif instead of flags in the interface header
>>     more reasonable names in xenstore
>>     move trim requesting handling to a separate function
>>     do not re-enable interrupts unconditionally when handling response
>>     set info->feature-trim only when we have all info needed for request queue
>> Changelog V2:
>>     rebased on Jeremy's tree
>>     fixes according to Jan Beulich's comments
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>

Re: [PATCH V3 0/3] xen-blkfront/xen-blkback trim support

[Konrad Rzeszutek Wilk]

On Mon, Aug 29, 2011 at 03:50:28PM +0800, Li Dongyang wrote:
> On Sat, Aug 27, 2011 at 12:56 AM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > On Wed, Aug 24, 2011 at 05:23:42PM +0800, Li Dongyang wrote:
> >> Dear list,
> >> this is the V3 of the trim support for xen-blkfront/blkback,
> >> thanks for all your reviews.
> >> and when I looked back at Owen's patch in Dec 2010,
> >> http://lists.xensource.com/archives/html/xen-devel/2010-12/msg00299.html
> >> this patch above also add the trim union to blkif_x86_{32|64}_request,
> >> and take care of trim request in blkif_get_x86{32|64}_req(),
> >> however, in the later versions, the part is just gone. I wonder if it is
> >> needed here? Thanks.
> >
> > Are you referring to git commit 51de69523ffe1c17994dc2f260369f29dfdce71c
> > xen: Union the blkif_request request specific fields
> that's the patch merged, the link I gave above was the previous
> version cooked up by
> Owen, and the patch in the link has changes to struct
> blkif_x86_{32|64}_request related stuffs,
> but that;s gone in the merged version, so I'm not sure if the gone
> part is needed here, Thanks

Well, I presume that you tested this patchset you are posting. If it
works properly (and you do see the discard operations in dom0) then
you do not need the extra parts.

You are testing this patchset on SSDs, right?

Re: [PATCH V3 0/3] xen-blkfront/xen-blkback trim support

[Li Dongyang]

On Mon, Aug 29, 2011 at 11:31 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Mon, Aug 29, 2011 at 03:50:28PM +0800, Li Dongyang wrote:
>> On Sat, Aug 27, 2011 at 12:56 AM, Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com> wrote:
>> > On Wed, Aug 24, 2011 at 05:23:42PM +0800, Li Dongyang wrote:
>> >> Dear list,
>> >> this is the V3 of the trim support for xen-blkfront/blkback,
>> >> thanks for all your reviews.
>> >> and when I looked back at Owen's patch in Dec 2010,
>> >> http://lists.xensource.com/archives/html/xen-devel/2010-12/msg00299.html
>> >> this patch above also add the trim union to blkif_x86_{32|64}_request,
>> >> and take care of trim request in blkif_get_x86{32|64}_req(),
>> >> however, in the later versions, the part is just gone. I wonder if it is
>> >> needed here? Thanks.
>> >
>> > Are you referring to git commit 51de69523ffe1c17994dc2f260369f29dfdce71c
>> > xen: Union the blkif_request request specific fields
>> that's the patch merged, the link I gave above was the previous
>> version cooked up by
>> Owen, and the patch in the link has changes to struct
>> blkif_x86_{32|64}_request related stuffs,
>> but that;s gone in the merged version, so I'm not sure if the gone
>> part is needed here, Thanks
>
> Well, I presume that you tested this patchset you are posting. If it
> works properly (and you do see the discard operations in dom0) then
> you do not need the extra parts.
sorry forgot to mention that the patch has been tested on a x86-64 host,
with both 32bit and 64bit guests, and it won't work for 32bit guests
without this part.
gonna post a V4.
>
> You are testing this patchset on SSDs, right?
I only tested on file backend cause I don't have handy SSD right now, and
the disk space of the image did reduce if we discard in the guest, Thanks
>

Re: [PATCH V3 2/3] xen-blkfront: teach blkfront driver handle trim request

[Dong Yang Li]

as I've said, if we propagate the rq->limits.max_discard_sectors of the trim device in the host to xenstore,
it's the number of the sectors we have for the whole device, and if we are exporting just a partition to guest,
the number is just wrong, so we should also use the capacity for device case, Thanks


Li Dongyang

>>> Jan Beulich 08/25/11 3:02 PM >>>
>>> On 25.08.11 at 08:37, Li Dongyang  wrote:
> On Wed, Aug 24, 2011 at 6:42 PM, Jan Beulich  wrote:
>>>>> On 24.08.11 at 11:23, Li Dongyang  wrote:
>>> 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.
>>>
>>> Signed-off-by: Li Dongyang 
>>> ---
>>>  drivers/block/xen-blkfront.c |  111 +++++++++++++++++++++++++++++++++---------
>>>  1 files changed, 88 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>> index 9ea8c25..aa3cede 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;
>>>  };
>>>
>>> @@ -302,29 +305,36 @@ static int blkif_queue_request(struct request *req)
>>>               ring_req->operation = info->flush_op;
>>>       }
>>>
>>> -     ring_req->nr_segments = 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 = BLKIF_OP_TRIM;
>>> +             ring_req->nr_segments = 0;
>>> +             ring_req->u.trim.nr_sectors = blk_rq_sectors(req);
>>> +     } else {
>>> +             ring_req->nr_segments = blk_rq_map_sg(req->q, req, info->sg);
>>> +             BUG_ON(ring_req->nr_segments > BLKIF_MAX_SEGMENTS_PER_REQUEST);
>>>
>>> -     for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
>>> -             buffer_mfn = pfn_to_mfn(page_to_pfn(sg_page(sg)));
>>> -             fsect = sg->offset >> 9;
>>> -             lsect = fsect + (sg->length >> 9) - 1;
>>> -             /* install a grant reference. */
>>> -             ref = gnttab_claim_grant_reference(&gref_head);
>>> -             BUG_ON(ref == -ENOSPC);
>>> +             for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
>>> +                     buffer_mfn = pfn_to_mfn(page_to_pfn(sg_page(sg)));
>>> +                     fsect = sg->offset >> 9;
>>> +                     lsect = fsect + (sg->length >> 9) - 1;
>>> +                     /* install a grant reference. */
>>> +                     ref = gnttab_claim_grant_reference(&gref_head);
>>> +                     BUG_ON(ref == -ENOSPC);
>>>
>>> -             gnttab_grant_foreign_access_ref(
>>> -                             ref,
>>> -                             info->xbdev->otherend_id,
>>> -                             buffer_mfn,
>>> -                             rq_data_dir(req) );
>>> -
>>> -             info->shadow[id].frame[i] = mfn_to_pfn(buffer_mfn);
>>> -             ring_req->u.rw.seg[i] =
>>> -                             (struct blkif_request_segment) {
>>> -                                     .gref       = ref,
>>> -                                     .first_sect = fsect,
>>> -                                     .last_sect  = lsect };
>>> +                     gnttab_grant_foreign_access_ref(
>>> +                                     ref,
>>> +                                     info->xbdev->otherend_id,
>>> +                                     buffer_mfn,
>>> +                                     rq_data_dir(req));
>>> +
>>> +                     info->shadow[id].frame[i] = mfn_to_pfn(buffer_mfn);
>>> +                     ring_req->u.rw.seg[i] =
>>> +                                     (struct blkif_request_segment) {
>>> +                                             .gref       = ref,
>>> +                                             .first_sect = fsect,
>>> +                                             .last_sect  = lsect };
>>> +             }
>>>       }
>>>
>>>       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 = gd->private_data;
>>>
>>>       rq = blk_init_queue(do_blkif_request, &blkif_io_lock);
>>>       if (rq == NULL)
>>> @@ -406,6 +417,13 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16
>>> sector_size)
>>>
>>>       queue_flag_set_unlocked(QUEUE_FLAG_VIRT, rq);
>>>
>>> +     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 = info->discard_granularity;
>>> +             rq->limits.discard_alignment = info->discard_alignment;
>>
>> Don't you also need to set rq->limits.max_discard_sectors here (since
>> when zero blkdev_issue_discard() doesn't do anything)? And wouldn't
>> that need to be propagated from the backend, too?
> the max_discard_sectors are set by blk_queue_max_discard_sectors() above ;-)

Oh, silly me to overlook that.

> rq->limits.max_discard_sectors is the full phy device size, and if we
> only assign
> a partition to guest, the number is incorrect for guest, so the
> max_discard_sectors should
> be the capacity the guest will see, Thanks

Using the capacity seems right only for the file: case; I'd still think
the backend should pass the underlying disk's setting for the phy:
one.

Jan