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

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

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 */