[PATCH 0 of 1] Patch to alter BLKIF_OP_TRIM to BLKIF_OP_DISCARD (v1).

[PATCH 0 of 1] Patch to alter BLKIF_OP_TRIM to BLKIF_OP_DISCARD (v1).

[Konrad Rzeszutek Wilk]

Hey guys,

Pasi mentioned on Li's (and Owen's) patches which provide TRIM/UNMAP support
to the Linux backend/frontend that:
  "
  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..
  "

and it sounds right to me. The problem is of course that the 'feature-trim'
is already in the interface - but I was wondering how much it is set
in stone? Could it be feasible to modify the backend/frontend in Citrix
product to use 'feature-discard' instead? And naturally also the Novell
drivers.

[PATCH 1 of 1] interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

[Konrad Rzeszutek Wilk]

[-- Attachment #1: Type: text/plain, Size: 3245 bytes --]

# HG changeset patch
# User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
# Date 1314761891 14400
# Node ID d2538ec1df2b9f4c39978d96ebe3bccccb068ad0
# Parent  ac9aa65050e9abc8f1c12c8603acf3b99e22cddc
interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

The name 'trim' is specific to the ATA discard implementation.
The name 'scsi unmap' is specific to the SCSI discard implementation.

We should really use a generic name - and the name 'discard'
looks to be the most generic of them all.

CC: lidongyang@novell.com
CC: owen.smith@citrix.com
CC: Pasi Kärkkäinen <pasik@iki.fi>
CC: JBeulich@novell.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff -r ac9aa65050e9 -r d2538ec1df2b xen/include/public/io/blkif.h
--- a/xen/include/public/io/blkif.h	Tue Aug 30 11:46:58 2011 +0100
+++ b/xen/include/public/io/blkif.h	Tue Aug 30 23:38:11 2011 -0400
@@ -82,25 +82,29 @@
  */
 #define BLKIF_OP_RESERVED_1        4
 /*
- * 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
+ * Recognised only if "feature-discard" is present in backend xenbus info.
+ * The "feature-discard" node contains a boolean indicating whether trim
+ * (ATA) or unmap (SCSI) - conviently called discard requests are likely
+ * to succeed or fail. Either way, a discard 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!
- * 
- * Trim operation is a request for the underlying block device to mark
- * extents to be erased. Trim operations are passed with sector_number as the
- * sector index to begin trim operations at and nr_sectors as the number of
- * sectors to be trimmed. The specified sectors should be trimmed if the
- * underlying block device supports trim operations, or a BLKIF_RSP_EOPNOTSUPP
- * should be returned. More information about trim operations at:
+ * or not it is worthwhile for the frontend to attempt discard requests.
+ * If a backend does not recognise BLKIF_OP_DISCARD, it should *not*
+ * create the "feature-discard" node!
+ *
+ * Discard operation is a request for the underlying block device to mark
+ * extents to be erased. Discard operations are passed with sector_number as the
+ * sector index to begin discard operations at and nr_sectors as the number of
+ * sectors to be discarded. The specified sectors should be discarded if the
+ * underlying block device supports trim (ATA) or unmap (SCSI) operations,
+ * or a BLKIF_RSP_EOPNOTSUPP  should be returned.
+ * More information about trim/unmap operations at:
  * http://t13.org/Documents/UploadedDocuments/docs2008/
  *     e07154r6-Data_Set_Management_Proposal_for_ATA-ACS2.doc
+ * http://www.seagate.com/staticfiles/support/disc/manuals/
+ *     Interface%20manuals/100293068c.pdf
  */
-#define BLKIF_OP_TRIM              5
+#define BLKIF_OP_DISCARD          5
 
 /*
  * Maximum scatter/gather segments per request.

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 1 of 1] interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

[Li Dongyang]

On Wed, Aug 31, 2011 at 11:40 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> # HG changeset patch
> # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> # Date 1314761891 14400
> # Node ID d2538ec1df2b9f4c39978d96ebe3bccccb068ad0
> # Parent  ac9aa65050e9abc8f1c12c8603acf3b99e22cddc
> interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD
>
> The name 'trim' is specific to the ATA discard implementation.
> The name 'scsi unmap' is specific to the SCSI discard implementation.
>
> We should really use a generic name - and the name 'discard'
> looks to be the most generic of them all.
>
> CC: lidongyang@novell.com
> CC: owen.smith@citrix.com
> CC: Pasi Kärkkäinen <pasik@iki.fi>
> CC: JBeulich@novell.com
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> diff -r ac9aa65050e9 -r d2538ec1df2b xen/include/public/io/blkif.h
> --- a/xen/include/public/io/blkif.h     Tue Aug 30 11:46:58 2011 +0100
> +++ b/xen/include/public/io/blkif.h     Tue Aug 30 23:38:11 2011 -0400
> @@ -82,25 +82,29 @@
>  */
>  #define BLKIF_OP_RESERVED_1        4
>  /*
> - * 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
> + * Recognised only if "feature-discard" is present in backend xenbus info.
> + * The "feature-discard" node contains a boolean indicating whether trim
> + * (ATA) or unmap (SCSI) - conviently called discard requests are likely
> + * to succeed or fail. Either way, a discard 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!
> - *
> - * Trim operation is a request for the underlying block device to mark
> - * extents to be erased. Trim operations are passed with sector_number as the
> - * sector index to begin trim operations at and nr_sectors as the number of
> - * sectors to be trimmed. The specified sectors should be trimmed if the
> - * underlying block device supports trim operations, or a BLKIF_RSP_EOPNOTSUPP
> - * should be returned. More information about trim operations at:
> + * or not it is worthwhile for the frontend to attempt discard requests.
> + * If a backend does not recognise BLKIF_OP_DISCARD, it should *not*
> + * create the "feature-discard" node!
> + *
> + * Discard operation is a request for the underlying block device to mark
> + * extents to be erased. Discard operations are passed with sector_number as the
well I think we need to change the comments here, discard doesn't
guarantee the blocks
will be erased from the device, it's just a hint to the device
controller these blocks are no longer
used, and what to do with these blocks are left to the device
controller, on an SSD the controller
might erase them and use the blank blocks to do some optimization like
gc and better wear leveling.

and for the novell driver, we are free to change to BLKIF_OP_DISCARD,
cause we have nothing depend
on BLKIF_OP_TRIM, and Jan should know better than me on this, correct
me if am wrong, Thanks.
> + * sector index to begin discard operations at and nr_sectors as the number of
> + * sectors to be discarded. The specified sectors should be discarded if the
> + * underlying block device supports trim (ATA) or unmap (SCSI) operations,
> + * or a BLKIF_RSP_EOPNOTSUPP  should be returned.
> + * More information about trim/unmap operations at:
>  * http://t13.org/Documents/UploadedDocuments/docs2008/
>  *     e07154r6-Data_Set_Management_Proposal_for_ATA-ACS2.doc
> + * http://www.seagate.com/staticfiles/support/disc/manuals/
> + *     Interface%20manuals/100293068c.pdf
>  */
> -#define BLKIF_OP_TRIM              5
> +#define BLKIF_OP_DISCARD          5
>
>  /*
>  * Maximum scatter/gather segments per request.
>

RE: [PATCH 0 of 1] Patch to alter BLKIF_OP_TRIM to BLKIF_OP_DISCARD (v1).

[Paul Durrant]

The windows frontends in current XenServer product don't have the trim code enabled since the backends don't really do anything useful with it yet so I have no objection to a name change if it really matters that much.

  Paul

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: 31 August 2011 04:41
> To: xen-devel@lists.xensource.com; Ian Jackson; Stefano Stabellini;
> Ian Campbell; lidongyang@novell.com; Owen Smith; Paul Durrant;
> pasik@iki.fi; JBeulich@novell.com
> Cc: konrad.wilk@oracle.com
> Subject: [PATCH 0 of 1] Patch to alter BLKIF_OP_TRIM to
> BLKIF_OP_DISCARD (v1).
> 
> Hey guys,
> 
> Pasi mentioned on Li's (and Owen's) patches which provide TRIM/UNMAP
> support to the Linux backend/frontend that:
>   "
>   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..
>   "
> 
> and it sounds right to me. The problem is of course that the
> 'feature-trim'
> is already in the interface - but I was wondering how much it is set
> in stone? Could it be feasible to modify the backend/frontend in
> Citrix product to use 'feature-discard' instead? And naturally also
> the Novell drivers.
> 

Re: Re: [PATCH 1 of 1] interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

[Konrad Rzeszutek Wilk]

> well I think we need to change the comments here, discard doesn't
> guarantee the blocks
> will be erased from the device, it's just a hint to the device
> controller these blocks are no longer
> used, and what to do with these blocks are left to the device
> controller, on an SSD the controller

How about this one below:

# HG changeset patch
# Parent ac9aa65050e9abc8f1c12c8603acf3b99e22cddc
interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

The name 'trim' is specific to the ATA discard implementation.
The name 'scsi unmap' is specific to the SCSI discard implementation.

We should really use a generic name - and the name 'discard'
looks to be the most generic of them all.

CC: lidongyang@novell.com
CC: owen.smith@citrix.com
CC: Pasi Kärkkäinen <pasik@iki.fi>
CC: JBeulich@novell.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff -r ac9aa65050e9 xen/include/public/io/blkif.h
--- a/xen/include/public/io/blkif.h	Tue Aug 30 11:46:58 2011 +0100
+++ b/xen/include/public/io/blkif.h	Wed Aug 31 11:22:57 2011 -0400
@@ -82,26 +82,33 @@
  */
 #define BLKIF_OP_RESERVED_1        4
 /*
- * 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
+ * Recognised only if "feature-discard" is present in backend xenbus info.
+ * The "feature-discard" node contains a boolean indicating whether trim
+ * (ATA) or unmap (SCSI) - conviently called discard requests are likely
+ * to succeed or fail. Either way, a discard 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!
- * 
- * Trim operation is a request for the underlying block device to mark
- * extents to be erased. Trim operations are passed with sector_number as the
- * sector index to begin trim operations at and nr_sectors as the number of
- * sectors to be trimmed. The specified sectors should be trimmed if the
- * underlying block device supports trim operations, or a BLKIF_RSP_EOPNOTSUPP
- * should be returned. More information about trim operations at:
+ * or not it is worthwhile for the frontend to attempt discard requests.
+ * If a backend does not recognise BLKIF_OP_DISCARD, it should *not*
+ * create the "feature-discard" node!
+ *
+ * Discard operation is a request for the underlying block device to mark
+ * extents to be erased. However, discard does not guarantee that the blocks
+ * will be erased from the device - it is just a hint to the device
+ * controller that these blocks are no longer in use. What the device
+ * controller does with that information is left to the controller.
+ * Discard operations are passed with sector_number as the
+ * sector index to begin discard operations at and nr_sectors as the number of
+ * sectors to be discarded. The specified sectors should be discarded if the
+ * underlying block device supports trim (ATA) or unmap (SCSI) operations,
+ * or a BLKIF_RSP_EOPNOTSUPP  should be returned.
+ * More information about trim/unmap operations at:
  * http://t13.org/Documents/UploadedDocuments/docs2008/
  *     e07154r6-Data_Set_Management_Proposal_for_ATA-ACS2.doc
+ * http://www.seagate.com/staticfiles/support/disc/manuals/
+ *     Interface%20manuals/100293068c.pdf
  */
-#define BLKIF_OP_TRIM              5
-
+#define BLKIF_OP_DISCARD          5
 /*
  * Maximum scatter/gather segments per request.
  * This is carefully chosen so that sizeof(blkif_ring_t) <= PAGE_SIZE.

Re: [PATCH 1 of 1] interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

[Konrad Rzeszutek Wilk]

On Wed, Oct 12, 2011 at 11:40:04AM +0100, Ian Campbell wrote:
> On Tue, 2011-10-11 at 19:27 +0100, Konrad Rzeszutek Wilk wrote:
> > On Mon, Oct 10, 2011 at 03:58:42PM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Mon, Oct 10, 2011 at 01:50:11PM -0400, Konrad Rzeszutek Wilk wrote:
> > 
> > Per Ian and Jan's suggestion (note, the structure is 4-byte aligned
> > so we do not need to pad it):
> > 
> > # HG changeset patch
> > # Parent 72f339bc600d7a9629d3f9eb8a279fbf8be25b12
> > interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD
> > 
> > The name 'trim' is specific to the ATA discard implementation.
> > The name 'scsi unmap' is specific to the SCSI discard implementation.
> > 
> > We should really use a generic name - and the name 'discard'
> > looks to be the most generic of them all. Also update the description
> > to mention the other parameters that the frontend can query the
> > backend for: discard-aligment, discard-granularity, and
> > discard-secure. We also utilize per Jan Beulich keen suggestion,
> > the 8-bit reserved field to use as a flag value. Currently the only
> > flag that can be passed for a discard operation is secure delete:
> > BLKIF_OP_DISCARD_FLAG_SECURE.
> 
> This change seems to be conflating a large rename with new data fields,
> changes to the documentation in the comments and other bits and bobs,
> which makes it pretty hard to review it's actual impact. Can we get them
> separately?

Of course. Sent them along with Jan's Acked-by flag.

Re: [PATCH 1 of 1] interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

[Ian Campbell]

On Tue, 2011-10-11 at 19:27 +0100, Konrad Rzeszutek Wilk wrote:
> On Mon, Oct 10, 2011 at 03:58:42PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Mon, Oct 10, 2011 at 01:50:11PM -0400, Konrad Rzeszutek Wilk wrote:
> 
> Per Ian and Jan's suggestion (note, the structure is 4-byte aligned
> so we do not need to pad it):
> 
> # HG changeset patch
> # Parent 72f339bc600d7a9629d3f9eb8a279fbf8be25b12
> interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD
> 
> The name 'trim' is specific to the ATA discard implementation.
> The name 'scsi unmap' is specific to the SCSI discard implementation.
> 
> We should really use a generic name - and the name 'discard'
> looks to be the most generic of them all. Also update the description
> to mention the other parameters that the frontend can query the
> backend for: discard-aligment, discard-granularity, and
> discard-secure. We also utilize per Jan Beulich keen suggestion,
> the 8-bit reserved field to use as a flag value. Currently the only
> flag that can be passed for a discard operation is secure delete:
> BLKIF_OP_DISCARD_FLAG_SECURE.

This change seems to be conflating a large rename with new data fields,
changes to the documentation in the comments and other bits and bobs,
which makes it pretty hard to review it's actual impact. Can we get them
separately?

Ian.

> 
> CC: lidongyang@novell.com
> CC: owen.smith@citrix.com
> CC: Pasi Kärkkäinen <pasik@iki.fi>
> CC: JBeulich@novell.com
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> diff -r 72f339bc600d xen/include/public/io/blkif.h
> --- a/xen/include/public/io/blkif.h	Mon Oct 10 11:21:51 2011 +0100
> +++ b/xen/include/public/io/blkif.h	Tue Oct 11 14:10:33 2011 -0400
> @@ -82,26 +82,47 @@
>   */
>  #define BLKIF_OP_RESERVED_1        4
>  /*
> - * 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
> + * Recognised only if "feature-discard" is present in backend xenbus info.
> + * The "feature-discard" node contains a boolean indicating whether trim
> + * (ATA) or unmap (SCSI) - conviently called discard requests are likely
> + * to succeed or fail. Either way, a discard 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!
> - * 
> - * Trim operation is a request for the underlying block device to mark
> - * extents to be erased. Trim operations are passed with sector_number as the
> - * sector index to begin trim operations at and nr_sectors as the number of
> - * sectors to be trimmed. The specified sectors should be trimmed if the
> - * underlying block device supports trim operations, or a BLKIF_RSP_EOPNOTSUPP
> - * should be returned. More information about trim operations at:
> + * or not it is worthwhile for the frontend to attempt discard requests.
> + * If a backend does not recognise BLKIF_OP_DISCARD, it should *not*
> + * create the "feature-discard" node!
> + *
> + * Discard operation is a request for the underlying block device to mark
> + * extents to be erased. However, discard does not guarantee that the blocks
> + * will be erased from the device - it is just a hint to the device
> + * controller that these blocks are no longer in use. What the device
> + * controller does with that information is left to the controller.
> + * Discard operations are passed with sector_number as the
> + * sector index to begin discard operations at and nr_sectors as the number of
> + * sectors to be discarded. The specified sectors should be discarded if the
> + * underlying block device supports trim (ATA) or unmap (SCSI) operations,
> + * or a BLKIF_RSP_EOPNOTSUPP  should be returned.
> + * More information about trim/unmap operations at:
>   * http://t13.org/Documents/UploadedDocuments/docs2008/
>   *     e07154r6-Data_Set_Management_Proposal_for_ATA-ACS2.doc
> + * http://www.seagate.com/staticfiles/support/disc/manuals/
> + *     Interface%20manuals/100293068c.pdf
> + * The backend can optionally provide three extra XenBus attributes to
> + * further optimize the discard functionality:
> + * 'discard-aligment' - Devices that support discard functionality may
> + * internally allocate space in units that are bigger than the exported
> + * logical block size. The discard-alignment parameter indicates how many bytes
> + * the beginning of the partition is offset from the internal allocation unit's
> + * natural alignment.
> + * 'discard-granularity'  - Devices that support discard functionality may
> + * internally allocate space using units that are bigger than the logical block
> + * size. The discard-granularity parameter indicates the size of the internal
> + * allocation unit in bytes if reported by the device. Otherwise the
> + * discard-granularity will be set to match the device's physical block size.
> + * 'discard-secure' - All copies of the discarded sectors (potentially created by
> + * garbage collection) must also be erased.
>   */
> -#define BLKIF_OP_TRIM              5
> -
> +#define BLKIF_OP_DISCARD           5
>  /*
>   * Maximum scatter/gather segments per request.
>   * This is carefully chosen so that sizeof(blkif_ring_t) <= PAGE_SIZE.
> @@ -134,18 +155,20 @@ struct blkif_request {
>  typedef struct blkif_request blkif_request_t;
>  
>  /*
> - * Cast to this structure when blkif_request.operation == BLKIF_OP_TRIM
> - * sizeof(struct blkif_request_trim) <= sizeof(struct blkif_request)
> + * Cast to this structure when blkif_request.operation == BLKIF_OP_DISCARD
> + * sizeof(struct blkif_request_discard) <= sizeof(struct blkif_request)
>   */
> -struct blkif_request_trim {
> -    uint8_t        operation;    /* BLKIF_OP_TRIM                        */
> -    uint8_t        reserved;     /*                                      */
> +struct blkif_request_discard {
> +    uint8_t        operation;    /* BLKIF_OP_DISCARD                     */
> +                                 /* ignored if 'discard-secure=0'        */
> +#define BLKIF_OP_DISCARD_FLAG_SECURE (1<<0)
> +    uint8_t        flag;         /* BLKIF_OP_DISCARD_FLAG_SECURE or 0    */
>      blkif_vdev_t   handle;       /* same as for read/write requests      */
>      uint64_t       id;           /* private guest value, echoed in resp  */
>      blkif_sector_t sector_number;/* start sector idx on disk             */
>      uint64_t       nr_sectors;   /* number of contiguous sectors to trim */
>  };
> -typedef struct blkif_request_trim blkif_request_trim_t;
> +typedef struct blkif_request_discard blkif_request_discard_t;
>  
>  struct blkif_response {
>      uint64_t        id;              /* copied from request */

Re: [PATCH 1 of 1] interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

[Jan Beulich]

>>> On 11.10.11 at 20:27, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Oct 10, 2011 at 03:58:42PM -0400, Konrad Rzeszutek Wilk wrote:
>> On Mon, Oct 10, 2011 at 01:50:11PM -0400, Konrad Rzeszutek Wilk wrote:
> 
> Per Ian and Jan's suggestion (note, the structure is 4-byte aligned
> so we do not need to pad it):
> 
> # HG changeset patch
> # Parent 72f339bc600d7a9629d3f9eb8a279fbf8be25b12
> interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD
> 
> The name 'trim' is specific to the ATA discard implementation.
> The name 'scsi unmap' is specific to the SCSI discard implementation.
> 
> We should really use a generic name - and the name 'discard'
> looks to be the most generic of them all. Also update the description
> to mention the other parameters that the frontend can query the
> backend for: discard-aligment, discard-granularity, and
> discard-secure. We also utilize per Jan Beulich keen suggestion,
> the 8-bit reserved field to use as a flag value. Currently the only
> flag that can be passed for a discard operation is secure delete:
> BLKIF_OP_DISCARD_FLAG_SECURE.
> 
> CC: lidongyang@novell.com 
> CC: owen.smith@citrix.com 
> CC: Pasi Kärkkäinen <pasik@iki.fi>
> CC: JBeulich@novell.com 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> diff -r 72f339bc600d xen/include/public/io/blkif.h
> --- a/xen/include/public/io/blkif.h	Mon Oct 10 11:21:51 2011 +0100
> +++ b/xen/include/public/io/blkif.h	Tue Oct 11 14:10:33 2011 -0400
> @@ -82,26 +82,47 @@
>   */
>  #define BLKIF_OP_RESERVED_1        4
>  /*
> - * 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
> + * Recognised only if "feature-discard" is present in backend xenbus info.
> + * The "feature-discard" node contains a boolean indicating whether trim
> + * (ATA) or unmap (SCSI) - conviently called discard requests are likely
> + * to succeed or fail. Either way, a discard 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!
> - * 
> - * Trim operation is a request for the underlying block device to mark
> - * extents to be erased. Trim operations are passed with sector_number as 
> the
> - * sector index to begin trim operations at and nr_sectors as the number of
> - * sectors to be trimmed. The specified sectors should be trimmed if the
> - * underlying block device supports trim operations, or a 
> BLKIF_RSP_EOPNOTSUPP
> - * should be returned. More information about trim operations at:
> + * or not it is worthwhile for the frontend to attempt discard requests.
> + * If a backend does not recognise BLKIF_OP_DISCARD, it should *not*
> + * create the "feature-discard" node!
> + *
> + * Discard operation is a request for the underlying block device to mark
> + * extents to be erased. However, discard does not guarantee that the 
> blocks
> + * will be erased from the device - it is just a hint to the device
> + * controller that these blocks are no longer in use. What the device
> + * controller does with that information is left to the controller.
> + * Discard operations are passed with sector_number as the
> + * sector index to begin discard operations at and nr_sectors as the number 
> of
> + * sectors to be discarded. The specified sectors should be discarded if 
> the
> + * underlying block device supports trim (ATA) or unmap (SCSI) operations,
> + * or a BLKIF_RSP_EOPNOTSUPP  should be returned.
> + * More information about trim/unmap operations at:
>   * http://t13.org/Documents/UploadedDocuments/docs2008/ 
>   *     e07154r6-Data_Set_Management_Proposal_for_ATA-ACS2.doc
> + * http://www.seagate.com/staticfiles/support/disc/manuals/ 
> + *     Interface%20manuals/100293068c.pdf
> + * The backend can optionally provide three extra XenBus attributes to
> + * further optimize the discard functionality:
> + * 'discard-aligment' - Devices that support discard functionality may
> + * internally allocate space in units that are bigger than the exported
> + * logical block size. The discard-alignment parameter indicates how many 
> bytes
> + * the beginning of the partition is offset from the internal allocation 
> unit's
> + * natural alignment.
> + * 'discard-granularity'  - Devices that support discard functionality may
> + * internally allocate space using units that are bigger than the logical 
> block
> + * size. The discard-granularity parameter indicates the size of the 
> internal
> + * allocation unit in bytes if reported by the device. Otherwise the
> + * discard-granularity will be set to match the device's physical block 
> size.
> + * 'discard-secure' - All copies of the discarded sectors (potentially 
> created by
> + * garbage collection) must also be erased.
>   */
> -#define BLKIF_OP_TRIM              5
> -
> +#define BLKIF_OP_DISCARD           5
>  /*
>   * Maximum scatter/gather segments per request.
>   * This is carefully chosen so that sizeof(blkif_ring_t) <= PAGE_SIZE.
> @@ -134,18 +155,20 @@ struct blkif_request {
>  typedef struct blkif_request blkif_request_t;
>  
>  /*
> - * Cast to this structure when blkif_request.operation == BLKIF_OP_TRIM
> - * sizeof(struct blkif_request_trim) <= sizeof(struct blkif_request)
> + * Cast to this structure when blkif_request.operation == BLKIF_OP_DISCARD
> + * sizeof(struct blkif_request_discard) <= sizeof(struct blkif_request)
>   */
> -struct blkif_request_trim {
> -    uint8_t        operation;    /* BLKIF_OP_TRIM                        */
> -    uint8_t        reserved;     /*                                      */
> +struct blkif_request_discard {
> +    uint8_t        operation;    /* BLKIF_OP_DISCARD                     */
> +                                 /* ignored if 'discard-secure=0'        */
> +#define BLKIF_OP_DISCARD_FLAG_SECURE (1<<0)

I'd like to have the _OP in here dropped - BLKIF_OP_* should be
"reserved" to express actual operations.

With that change,
Acked-by: Jan Beulich <jbeulich@suse.com>

(I'd also consider the _FLAG part to be sort of redundant.)

Jan

> +    uint8_t        flag;         /* BLKIF_OP_DISCARD_FLAG_SECURE or 0    */
>      blkif_vdev_t   handle;       /* same as for read/write requests      */
>      uint64_t       id;           /* private guest value, echoed in resp  */
>      blkif_sector_t sector_number;/* start sector idx on disk             */
>      uint64_t       nr_sectors;   /* number of contiguous sectors to trim */
>  };
> -typedef struct blkif_request_trim blkif_request_trim_t;
> +typedef struct blkif_request_discard blkif_request_discard_t;
>  
>  struct blkif_response {
>      uint64_t        id;              /* copied from request */

Re: [PATCH 1 of 1] interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

[Konrad Rzeszutek Wilk]

On Mon, Oct 10, 2011 at 03:58:42PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Oct 10, 2011 at 01:50:11PM -0400, Konrad Rzeszutek Wilk wrote:

Per Ian and Jan's suggestion (note, the structure is 4-byte aligned
so we do not need to pad it):

# HG changeset patch
# Parent 72f339bc600d7a9629d3f9eb8a279fbf8be25b12
interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

The name 'trim' is specific to the ATA discard implementation.
The name 'scsi unmap' is specific to the SCSI discard implementation.

We should really use a generic name - and the name 'discard'
looks to be the most generic of them all. Also update the description
to mention the other parameters that the frontend can query the
backend for: discard-aligment, discard-granularity, and
discard-secure. We also utilize per Jan Beulich keen suggestion,
the 8-bit reserved field to use as a flag value. Currently the only
flag that can be passed for a discard operation is secure delete:
BLKIF_OP_DISCARD_FLAG_SECURE.

CC: lidongyang@novell.com
CC: owen.smith@citrix.com
CC: Pasi Kärkkäinen <pasik@iki.fi>
CC: JBeulich@novell.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff -r 72f339bc600d xen/include/public/io/blkif.h
--- a/xen/include/public/io/blkif.h	Mon Oct 10 11:21:51 2011 +0100
+++ b/xen/include/public/io/blkif.h	Tue Oct 11 14:10:33 2011 -0400
@@ -82,26 +82,47 @@
  */
 #define BLKIF_OP_RESERVED_1        4
 /*
- * 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
+ * Recognised only if "feature-discard" is present in backend xenbus info.
+ * The "feature-discard" node contains a boolean indicating whether trim
+ * (ATA) or unmap (SCSI) - conviently called discard requests are likely
+ * to succeed or fail. Either way, a discard 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!
- * 
- * Trim operation is a request for the underlying block device to mark
- * extents to be erased. Trim operations are passed with sector_number as the
- * sector index to begin trim operations at and nr_sectors as the number of
- * sectors to be trimmed. The specified sectors should be trimmed if the
- * underlying block device supports trim operations, or a BLKIF_RSP_EOPNOTSUPP
- * should be returned. More information about trim operations at:
+ * or not it is worthwhile for the frontend to attempt discard requests.
+ * If a backend does not recognise BLKIF_OP_DISCARD, it should *not*
+ * create the "feature-discard" node!
+ *
+ * Discard operation is a request for the underlying block device to mark
+ * extents to be erased. However, discard does not guarantee that the blocks
+ * will be erased from the device - it is just a hint to the device
+ * controller that these blocks are no longer in use. What the device
+ * controller does with that information is left to the controller.
+ * Discard operations are passed with sector_number as the
+ * sector index to begin discard operations at and nr_sectors as the number of
+ * sectors to be discarded. The specified sectors should be discarded if the
+ * underlying block device supports trim (ATA) or unmap (SCSI) operations,
+ * or a BLKIF_RSP_EOPNOTSUPP  should be returned.
+ * More information about trim/unmap operations at:
  * http://t13.org/Documents/UploadedDocuments/docs2008/
  *     e07154r6-Data_Set_Management_Proposal_for_ATA-ACS2.doc
+ * http://www.seagate.com/staticfiles/support/disc/manuals/
+ *     Interface%20manuals/100293068c.pdf
+ * The backend can optionally provide three extra XenBus attributes to
+ * further optimize the discard functionality:
+ * 'discard-aligment' - Devices that support discard functionality may
+ * internally allocate space in units that are bigger than the exported
+ * logical block size. The discard-alignment parameter indicates how many bytes
+ * the beginning of the partition is offset from the internal allocation unit's
+ * natural alignment.
+ * 'discard-granularity'  - Devices that support discard functionality may
+ * internally allocate space using units that are bigger than the logical block
+ * size. The discard-granularity parameter indicates the size of the internal
+ * allocation unit in bytes if reported by the device. Otherwise the
+ * discard-granularity will be set to match the device's physical block size.
+ * 'discard-secure' - All copies of the discarded sectors (potentially created by
+ * garbage collection) must also be erased.
  */
-#define BLKIF_OP_TRIM              5
-
+#define BLKIF_OP_DISCARD           5
 /*
  * Maximum scatter/gather segments per request.
  * This is carefully chosen so that sizeof(blkif_ring_t) <= PAGE_SIZE.
@@ -134,18 +155,20 @@ struct blkif_request {
 typedef struct blkif_request blkif_request_t;
 
 /*
- * Cast to this structure when blkif_request.operation == BLKIF_OP_TRIM
- * sizeof(struct blkif_request_trim) <= sizeof(struct blkif_request)
+ * Cast to this structure when blkif_request.operation == BLKIF_OP_DISCARD
+ * sizeof(struct blkif_request_discard) <= sizeof(struct blkif_request)
  */
-struct blkif_request_trim {
-    uint8_t        operation;    /* BLKIF_OP_TRIM                        */
-    uint8_t        reserved;     /*                                      */
+struct blkif_request_discard {
+    uint8_t        operation;    /* BLKIF_OP_DISCARD                     */
+                                 /* ignored if 'discard-secure=0'        */
+#define BLKIF_OP_DISCARD_FLAG_SECURE (1<<0)
+    uint8_t        flag;         /* BLKIF_OP_DISCARD_FLAG_SECURE or 0    */
     blkif_vdev_t   handle;       /* same as for read/write requests      */
     uint64_t       id;           /* private guest value, echoed in resp  */
     blkif_sector_t sector_number;/* start sector idx on disk             */
     uint64_t       nr_sectors;   /* number of contiguous sectors to trim */
 };
-typedef struct blkif_request_trim blkif_request_trim_t;
+typedef struct blkif_request_discard blkif_request_discard_t;
 
 struct blkif_response {
     uint64_t        id;              /* copied from request */

Re: [PATCH 1 of 1] interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

[Konrad Rzeszutek Wilk]

On Mon, Oct 10, 2011 at 01:50:11PM -0400, Konrad Rzeszutek Wilk wrote:
> # HG changeset patch
> # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> # Date 1318268906 14400
> # Node ID d17ab29f2f9ad29a7b8af02d6f17de7f112a9723
> # Parent  72f339bc600d7a9629d3f9eb8a279fbf8be25b12
> interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD
> 

Per Ian's suggestion:

# HG changeset patch
# Parent 72f339bc600d7a9629d3f9eb8a279fbf8be25b12
interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

The name 'trim' is specific to the ATA discard implementation.
The name 'scsi unmap' is specific to the SCSI discard implementation.

We should really use a generic name - and the name 'discard'
looks to be the most generic of them all.

CC: lidongyang@novell.com
CC: owen.smith@citrix.com
CC: Pasi Kärkkäinen <pasik@iki.fi>
CC: JBeulich@novell.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff -r 72f339bc600d xen/include/public/io/blkif.h
--- a/xen/include/public/io/blkif.h	Mon Oct 10 11:21:51 2011 +0100
+++ b/xen/include/public/io/blkif.h	Mon Oct 10 15:54:51 2011 -0400
@@ -82,26 +82,47 @@
  */
 #define BLKIF_OP_RESERVED_1        4
 /*
- * 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
+ * Recognised only if "feature-discard" is present in backend xenbus info.
+ * The "feature-discard" node contains a boolean indicating whether trim
+ * (ATA) or unmap (SCSI) - conviently called discard requests are likely
+ * to succeed or fail. Either way, a discard 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!
- * 
- * Trim operation is a request for the underlying block device to mark
- * extents to be erased. Trim operations are passed with sector_number as the
- * sector index to begin trim operations at and nr_sectors as the number of
- * sectors to be trimmed. The specified sectors should be trimmed if the
- * underlying block device supports trim operations, or a BLKIF_RSP_EOPNOTSUPP
- * should be returned. More information about trim operations at:
+ * or not it is worthwhile for the frontend to attempt discard requests.
+ * If a backend does not recognise BLKIF_OP_DISCARD, it should *not*
+ * create the "feature-discard" node!
+ *
+ * Discard operation is a request for the underlying block device to mark
+ * extents to be erased. However, discard does not guarantee that the blocks
+ * will be erased from the device - it is just a hint to the device
+ * controller that these blocks are no longer in use. What the device
+ * controller does with that information is left to the controller.
+ * Discard operations are passed with sector_number as the
+ * sector index to begin discard operations at and nr_sectors as the number of
+ * sectors to be discarded. The specified sectors should be discarded if the
+ * underlying block device supports trim (ATA) or unmap (SCSI) operations,
+ * or a BLKIF_RSP_EOPNOTSUPP  should be returned.
+ * More information about trim/unmap operations at:
  * http://t13.org/Documents/UploadedDocuments/docs2008/
  *     e07154r6-Data_Set_Management_Proposal_for_ATA-ACS2.doc
+ * http://www.seagate.com/staticfiles/support/disc/manuals/
+ *     Interface%20manuals/100293068c.pdf
+ * The backend can optionally provide three extra XenBus attributes to
+ * further optimize the discard functionality:
+ * 'discard-aligment' - Devices that support discard functionality may
+ * internally allocate space in units that are bigger than the exported
+ * logical block size. The discard-alignment parameter indicates how many bytes
+ * the beginning of the partition is offset from the internal allocation unit's
+ * natural alignment.
+ * 'discard-granularity'  - Devices that support discard functionality may
+ * internally allocate space using units that are bigger than the logical block
+ * size. The discard-granularity parameter indicates the size of the internal
+ * allocation unit in bytes if reported by the device. Otherwise the
+ * discard-granularity will be set to match the device's physical block size.
+ * 'discard-secure' - All copies of the discarded sectors (potentially created by
+ * garbage collection) must also be erased.
  */
-#define BLKIF_OP_TRIM              5
-
+#define BLKIF_OP_DISCARD           5
 /*
  * Maximum scatter/gather segments per request.
  * This is carefully chosen so that sizeof(blkif_ring_t) <= PAGE_SIZE.
@@ -134,18 +155,22 @@ struct blkif_request {
 typedef struct blkif_request blkif_request_t;
 
 /*
- * Cast to this structure when blkif_request.operation == BLKIF_OP_TRIM
- * sizeof(struct blkif_request_trim) <= sizeof(struct blkif_request)
+ * Cast to this structure when blkif_request.operation == BLKIF_OP_DISCARD
+ * sizeof(struct blkif_request_discard) <= sizeof(struct blkif_request)
  */
-struct blkif_request_trim {
-    uint8_t        operation;    /* BLKIF_OP_TRIM                        */
+struct blkif_request_discard {
+    uint8_t        operation;    /* BLKIF_OP_DISCARD                     */
     uint8_t        reserved;     /*                                      */
     blkif_vdev_t   handle;       /* same as for read/write requests      */
     uint64_t       id;           /* private guest value, echoed in resp  */
     blkif_sector_t sector_number;/* start sector idx on disk             */
     uint64_t       nr_sectors;   /* number of contiguous sectors to trim */
+                                 /* ignored if 'discard-secure=0'        */
+#define BLKIF_OP_DISCARD_FLAG_SECURE (1<<0)
+    uint32_t       flag;
+    uint32_t       pad;
 };
-typedef struct blkif_request_trim blkif_request_trim_t;
+typedef struct blkif_request_discard blkif_request_discard_t;
 
 struct blkif_response {
     uint64_t        id;              /* copied from request */

[PATCH 1 of 1] interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

[Konrad Rzeszutek Wilk]

[-- Attachment #1: Type: text/plain, Size: 5231 bytes --]

# HG changeset patch
# User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
# Date 1318268906 14400
# Node ID d17ab29f2f9ad29a7b8af02d6f17de7f112a9723
# Parent  72f339bc600d7a9629d3f9eb8a279fbf8be25b12
interface: BLKIF_OP_TRIM -> BLKIF_OP_DISCARD

The name 'trim' is specific to the ATA discard implementation.
The name 'scsi unmap' is specific to the SCSI discard implementation.

We should really use a generic name - and the name 'discard'
looks to be the most generic of them all.

CC: lidongyang@novell.com
CC: owen.smith@citrix.com
CC: Pasi Kärkkäinen <pasik@iki.fi>
CC: JBeulich@novell.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff -r 72f339bc600d -r d17ab29f2f9a xen/include/public/io/blkif.h
--- a/xen/include/public/io/blkif.h	Mon Oct 10 11:21:51 2011 +0100
+++ b/xen/include/public/io/blkif.h	Mon Oct 10 13:48:26 2011 -0400
@@ -82,26 +82,37 @@
  */
 #define BLKIF_OP_RESERVED_1        4
 /*
- * 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
+ * Recognised only if "feature-discard" is present in backend xenbus info.
+ * The "feature-discard" node contains a boolean indicating whether trim
+ * (ATA) or unmap (SCSI) - conviently called discard requests are likely
+ * to succeed or fail. Either way, a discard 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!
- * 
- * Trim operation is a request for the underlying block device to mark
- * extents to be erased. Trim operations are passed with sector_number as the
- * sector index to begin trim operations at and nr_sectors as the number of
- * sectors to be trimmed. The specified sectors should be trimmed if the
- * underlying block device supports trim operations, or a BLKIF_RSP_EOPNOTSUPP
- * should be returned. More information about trim operations at:
+ * or not it is worthwhile for the frontend to attempt discard requests.
+ * If a backend does not recognise BLKIF_OP_DISCARD, it should *not*
+ * create the "feature-discard" node!
+ *
+ * Discard operation is a request for the underlying block device to mark
+ * extents to be erased. However, discard does not guarantee that the blocks
+ * will be erased from the device - it is just a hint to the device
+ * controller that these blocks are no longer in use. What the device
+ * controller does with that information is left to the controller.
+ * Discard operations are passed with sector_number as the
+ * sector index to begin discard operations at and nr_sectors as the number of
+ * sectors to be discarded. The specified sectors should be discarded if the
+ * underlying block device supports trim (ATA) or unmap (SCSI) operations,
+ * or a BLKIF_RSP_EOPNOTSUPP  should be returned.
+ * More information about trim/unmap operations at:
  * http://t13.org/Documents/UploadedDocuments/docs2008/
  *     e07154r6-Data_Set_Management_Proposal_for_ATA-ACS2.doc
+ * http://www.seagate.com/staticfiles/support/disc/manuals/
+ *     Interface%20manuals/100293068c.pdf
+ * We also provide three extra XenBus options to the discard operation:
+ * 'discard-granularity' - Max amount of sectors that can be discarded.
+ * 'discard-alignment' - 4K, 128K, etc aligment on sectors to erased.
+ * 'discard-secure' - whether the discard can also securely erase data.
  */
-#define BLKIF_OP_TRIM              5
-
+#define BLKIF_OP_DISCARD           5
 /*
  * Maximum scatter/gather segments per request.
  * This is carefully chosen so that sizeof(blkif_ring_t) <= PAGE_SIZE.
@@ -134,18 +145,21 @@ struct blkif_request {
 typedef struct blkif_request blkif_request_t;
 
 /*
- * Cast to this structure when blkif_request.operation == BLKIF_OP_TRIM
- * sizeof(struct blkif_request_trim) <= sizeof(struct blkif_request)
+ * Cast to this structure when blkif_request.operation == BLKIF_OP_DISCARD
+ * sizeof(struct blkif_request_discard) <= sizeof(struct blkif_request)
  */
-struct blkif_request_trim {
-    uint8_t        operation;    /* BLKIF_OP_TRIM                        */
+struct blkif_request_discard {
+    uint8_t        operation;    /* BLKIF_OP_DISCARD                     */
     uint8_t        reserved;     /*                                      */
     blkif_vdev_t   handle;       /* same as for read/write requests      */
     uint64_t       id;           /* private guest value, echoed in resp  */
     blkif_sector_t sector_number;/* start sector idx on disk             */
     uint64_t       nr_sectors;   /* number of contiguous sectors to trim */
+                                 /* ignored if 'discard-secure=0'        */
+#define BLKIF_OP_DISCARD_FLAG_SECURE (1<<1)
+    uint32_t       flag;
 };
-typedef struct blkif_request_trim blkif_request_trim_t;
+typedef struct blkif_request_discard blkif_request_discard_t;
 
 struct blkif_response {
     uint64_t        id;              /* copied from request */

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel