[PATCH 0 of 2] Patches to alter BLKIF_OP_TRIM to BLKIF_OP_DISCARD (v3)

[PATCH 0 of 2] Patches to alter BLKIF_OP_TRIM to BLKIF_OP_DISCARD (v3)

[Konrad Rzeszutek Wilk]

This is the v3 of the patches. I've split the patch in two to cover just
the documentation, and then the structure change. The structure change
is the same, except the name of the 'secure' flag is BLKIF_DISCARD_SECURE
now.

I've also taken the liberty of applying Acked-by: Jan Beulich on both
of the patches.

Please apply these two patches to the tree at your convience. Thanks.

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

[Konrad Rzeszutek Wilk]

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

The name 'trim' is specific to the ATA discard implementation, but the
operation is not specific to ATA. We could name it on the 'scsi unmap'
for the SCSI discard implementation but that would be more confusing.

Instead lets use a generic name, which also Linux choose, and that
is 'discard'. Furthermore, we flesh out the description to include
extra details on what is expected of 'feature-flush' and what are
some of the extra parameters that the frontend can read from the
backend. Those extra parameters are: : discard-aligment,
discard-granularity, and discard-secure.


Acked-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff -r 72f339bc600d -r c8ee79dee3fe 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	Wed Oct 12 11:18:37 2011 -0400
@@ -82,26 +82,48 @@
  */
 #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. To use this feature, the flag
+ * BLKIF_DISCARD_SECURE must be set in the blkif_request_trim.
  */
-#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.

[PATCH 2 of 2] interface: blkif_request_trim->blkif_request_discard

[Konrad Rzeszutek Wilk]

# HG changeset patch
# User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
# Date 1318432717 14400
# Node ID de77bf1b00c20d22e0f9976c7f35ae1404994342
# Parent  c8ee79dee3fe29c5fb9d211ed46c4c65ec24430b
interface: blkif_request_trim->blkif_request_discard

Change the naming of the structure and as well alter the 'reserved'
uint8_t to be used a 'flag'. We use only for one flag: BLKIF_DISCARD_SECURE.

That flag can only be set if the backend has set 'discard-secure' to one.
If backend has not set 'discard-secure' to one, that flag will have no
effect.

Acked-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff -r c8ee79dee3fe -r de77bf1b00c2 xen/include/public/io/blkif.h
--- a/xen/include/public/io/blkif.h	Wed Oct 12 11:18:37 2011 -0400
+++ b/xen/include/public/io/blkif.h	Wed Oct 12 11:18:37 2011 -0400
@@ -157,17 +157,19 @@ typedef struct blkif_request blkif_reque
 
 /*
  * Cast to this structure when blkif_request.operation == BLKIF_OP_TRIM
- * sizeof(struct blkif_request_trim) <= sizeof(struct blkif_request)
+ * sizeof(struct blkif_request_discard) <= sizeof(struct blkif_request)
  */
-struct blkif_request_trim {
+struct blkif_request_discard {
     uint8_t        operation;    /* BLKIF_OP_TRIM                        */
-    uint8_t        reserved;     /*                                      */
+    uint8_t        flag;         /* BLKIF_DISCARD_SECURE or 0            */
+                                 /* ignored if 'discard-secure=0'        */
+#define BLKIF_DISCARD_SECURE       (1<<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 */
+    uint64_t       nr_sectors;   /* number of contiguous sectors to discard */
 };
-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 0 of 2] Patches to alter BLKIF_OP_TRIM to BLKIF_OP_DISCARD (v3)

[Ian Campbell]

On Wed, 2011-10-12 at 16:18 +0100, Konrad Rzeszutek Wilk wrote:
> This is the v3 of the patches. I've split the patch in two to cover just
> the documentation, and then the structure change. The structure change
> is the same, except the name of the 'secure' flag is BLKIF_DISCARD_SECURE
> now.

What I meant was to post the s/trim/discard/ (of both the docs and the
struct) as a separate patch with no other non-mechanical changes first.
As it stands the docs patch is unreadable because basically every line
has changed _and_ you've simultaneously added new content. The stuff
which has been added is the interesting bit WRT reviewing it, the rename
itself can be reviewed just by understanding the old and new names.

Once you've done the rename then you can add the docs and datastructures
relating to secure discard as a single patch, the stuff to do with
granularity as a patch etc etc. Currently you modify the datastructure
in your 2/2 patch but document the meaning of the new fields all mixed
up in patch 1/2.

> 
> I've also taken the liberty of applying Acked-by: Jan Beulich on both
> of the patches.
> 
> Please apply these two patches to the tree at your convience. Thanks.
> 
>