On Fri, 8 Apr 2016, Bob Liu wrote: > > On 04/07/2016 11:55 PM, Juergen Gross wrote: > > On 07/04/16 12:00, 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! > >> > >> [1] http://lwn.net/Articles/280023/ > >> [2] https://www.kernel.org/doc/Documentation/block/data-integrity.txt > >> > >> Signed-off-by: Bob Liu <bob....@oracle.com> > >> --- > >> xen/include/public/io/blkif.h | 50 > >> +++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 50 insertions(+) > >> > >> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h > >> index 99f0326..3d8d39f 100644 > >> --- a/xen/include/public/io/blkif.h > >> +++ b/xen/include/public/io/blkif.h > >> @@ -635,6 +635,28 @@ > >> #define BLKIF_OP_INDIRECT 6 > >> > >> /* > >> + * Recognized only if "feature-extra-request" is present in backend > >> xenbus info. > >> + * A request with BLKIF_OP_EXTRA_FLAG indicates an extra request is > >> followed > >> + * in the shared ring buffer. > >> + * > >> + * By this way, extra data like bio integrity payload can be transmitted > >> from > >> + * frontend to backend. > >> + * > >> + * The 'wire' format is like: > >> + * Request 1: xen_blkif_request > >> + * [Request 2: xen_blkif_extra_request] (only if request 1 has > >> BLKIF_OP_EXTRA_FLAG) > >> + * Request 3: xen_blkif_request > >> + * Request 4: xen_blkif_request > >> + * [Request 5: xen_blkif_extra_request] (only if request 4 has > >> BLKIF_OP_EXTRA_FLAG) > >> + * ... > >> + * Request N: xen_blkif_request > >> + * > >> + * If a backend does not recognize BLKIF_OP_EXTRA_FLAG, it should *not* > >> create the > >> + * "feature-extra-request" node! > >> + */ > >> +#define BLKIF_OP_EXTRA_FLAG (0x80) > >> + > >> +/* > >> * Maximum scatter/gather segments per request. > >> * This is carefully chosen so that sizeof(blkif_ring_t) <= PAGE_SIZE. > >> * NB. This could be 12 if the ring indexes weren't stored in the same > >> page. > >> @@ -703,6 +725,34 @@ struct blkif_request_indirect { > >> }; > >> typedef struct blkif_request_indirect blkif_request_indirect_t; > >> > >> +enum blkif_extra_request_type { > >> + BLKIF_EXTRA_TYPE_DIX = 1, /* Data integrity extension > >> payload. */ > >> +}; > >> + > >> +struct bio_integrity_req { > >> + /* > >> + * Grant mapping for transmitting bio integrity payload to backend. > >> + */ > >> + grant_ref_t *gref; > >> + unsigned int nr_grefs; > >> + unsigned int len; > >> +}; > > > > How does the payload look like? It's structure should be defined here > > or a reference to it's definition in case it is a standard should be > > given. > > > > The payload is also described using struct bio_vec(the same as bio). > > /* > * bio integrity payload > */ > struct bio_integrity_payload { > struct bio *bip_bio; /* parent bio */ > > struct bvec_iter bip_iter; > > bio_end_io_t *bip_end_io; /* saved I/O completion fn */ > > unsigned short bip_slab; /* slab the bip came from */ > unsigned short bip_vcnt; /* # of integrity bio_vecs */ > unsigned short bip_max_vcnt; /* integrity bio_vec slots */ > unsigned short bip_flags; /* control flags */ > > struct work_struct bip_work; /* I/O completion */ > > struct bio_vec *bip_vec; > struct bio_vec bip_inline_vecs[0];/* embedded bvec array */ > };
There's no way we are going to embed such a Linux specific payload into the PV block protocol. Also, I have the feeling there are a lot of fields in this struct that make no sense to transmit on the ring (work_struct?). TBH, I don't know much about this integrity thing. Why does the frontend needs to create and pass this bio_integrity_payload around? Couldn't this be created from blkback before sending the request down to the disk? Then blkback would check the result and either return BLKIF_RSP_OKAY or BLKIF_RSP_ERROR if the metadata doesn't match? Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel