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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.10.11 at 20:27, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 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@xxxxxxxxxx 
> CC: owen.smith@xxxxxxxxxx 
> CC: Pasi KÃrkkÃinen <pasik@xxxxxx>
> CC: JBeulich@xxxxxxxxxx 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> 
> 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@xxxxxxxx>

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



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