Re: [Xen-devel] [RFC PATCH] Data integrity extension support for xen-block

[Juergen Gross <jgross@xxxxxxxx>]

On 13/04/16 14:22, Bob Liu wrote:
> 
> On 04/07/2016 06:00 PM, Bob Liu wrote:
>> * What's data integrity extension and why?
>> Modern filesystems feature checksumming of data and metadata to protect 
>> against
>> data corruption.  However, the detection of the corruption is done at read 
>> time
>> which could potentially be months after the data was written.  At that point 
>> the
>> original data that the application tried to write is most likely lost.
>>
>> The solution in Linux is the data integrity framework which enables 
>> protection
>> information to be pinned to I/Os and sent to/received from controllers that
>> support it. struct bio has been extended with a pointer to a struct bip which
>> in turn contains the integrity metadata. The bip is essentially a trimmed 
>> down
>> bio with a bio_vec and some housekeeping.
>>
>> * Issues when xen-block get involved.
>> xen-blkfront only transmits the normal data of struct bio while the integrity
>> metadata buffer(struct bio_integrity_payload in each bio) is ignored.
>>
>> * Proposal of transmitting bio integrity payload.
>> Adding an extra request following the normal data request, this extra request
>> contains the integrity payload.
>> The xen-blkback will reconstruct an new bio with both received normal data 
>> and
>> integrity metadata.
>>
>> Welcome any better ideas, thank you!
>>
> 
> A simpler possible solution:
> 
> bob@boliuliu:~/xen$ git diff xen/include/public/io/blkif.h
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 3d8d39f..34581a5 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -689,6 +689,11 @@ struct blkif_request_segment {
>  struct blkif_request {
>      uint8_t        operation;    /* BLKIF_OP_???                         */
>      uint8_t        nr_segments;  /* number of segments                   */
> +    /*
> +     * Recording how many segments are data integrity segments.
> +     * raw data_segments + dix_segments = nr_segments
> +     */
> +    uint8_t       dix_segments;
>      blkif_vdev_t   handle;       /* only for read/write requests         */
>      uint64_t       id;           /* private guest value, echoed in resp  */
>      blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
> @@ -715,6 +720,11 @@ struct blkif_request_indirect {
>      uint8_t        operation;    /* BLKIF_OP_INDIRECT                    */
>      uint8_t        indirect_op;  /* BLKIF_OP_{READ/WRITE}                */
>      uint16_t       nr_segments;  /* number of segments                   */
> +    /*
> +     * Recording how many segments are data integrity segments.
> +     * raw data_segments + dix_segments = nr_segments
> +     */
> +    uint16_t       dix_segments;
>      uint64_t       id;           /* private guest value, echoed in resp  */
>      blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
>      blkif_vdev_t   handle;       /* same as for read/write requests      */
> 

Without having checked whether there were padding holes where you
introduced the new elements: this looks much better. As Ian already
pointed out: you should mention somewhere what the new segments are
containing (data layout description, possibly just a reference to a
hardware spec?).

Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel