Re: [Xen-devel] [RFC PATCH] xen-block: introduces extra request to pass-through SCSI commands

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Bob Liu [mailto:bob.liu@xxxxxxxxxx]
> Sent: 29 February 2016 03:37
> To: xen-devel@xxxxxxxxxxxxx
> Cc: Ian Jackson; jbeulich@xxxxxxxx; Roger Pau Monne; jgross@xxxxxxxx;
> Paul Durrant; konrad.wilk@xxxxxxxxxx; Bob Liu
> Subject: [RFC PATCH] xen-block: introduces extra request to pass-through
> SCSI commands
> 
> 1) What is this patch about?
> This patch introduces an new block operation (BLKIF_OP_EXTRA_FLAG).
> A request with BLKIF_OP_EXTRA_FLAG set means the following request is an
> extra request which is used to pass through SCSI commands.
> This is like a simplified version of XEN_NETIF_EXTRA_* in netif.h.
> It can be extended easily to transmit other per-request/bio data from
> frontend
> to backend e.g Data Integrity Field per bio.
> 
> 2) Why we need this?
> Currently only raw data segments are transmitted from blkfront to blkback,
> which
> means some advanced features are lost.
>  * Guest knows nothing about features of the real backend storage.
>       For example, on bare-metal environment INQUIRY SCSI command
> can be used
>       to query storage device information. If it's a SSD or flash device we
>       can have the option to use the device as a fast cache.
>       But this can't happen in current domU guests, because blkfront only
>       knows it's just a normal virtual disk
> 

That's the sort of information that should be advertised via xenstore then. 
There already feature flags for specific things but if some form of 
throughput/latency information is meaningful to a frontend stack then perhaps 
that can be advertised too.

>  * Failover Clusters in Windows
>       Failover clusters require SCSI-3 persistent reservation target disks,
>       but now this can't work in domU.
> 

That's true but allowing arbitrary SCSI messages through is not the way forward 
IMO. Just because Windows thinks every HBA is SCSI doesn't mean other OS do so 
I think reservation/release should have dedicated messages in the blkif 
protocol if it's desirable to support clustering in the frontend.

> 3) Known issues:
>  * Security issues, how to 'validate' this extra request payload.
>    E.g SCSI operates on LUN bases (the whole disk) while we really just want
> to
>    operate on partitions
> 
>  * Can't pass SCSI commands through if the backend storage driver is bio-
> based
>    instead of request-based.
> 
> 4) Alternative approach: Using PVSCSI instead:
>  * Doubt PVSCSI can support as many type of backend storage devices as
> Xen-block.
> 

LIO can interface to any block device in much the same way blkback does IIRC.

>  * Much longer path:
>    ioctl() -> SCSI upper layer -> Middle layer -> PVSCSI-frontend -> PVSCSI-
> backend -> Target framework(LIO?) ->
> 
>    With xen-block we only need:
>    ioctl() -> blkfront -> blkback ->
> 

...and what happens if the block device that blkback is talking to is a SCSI 
LUN?

That latter path is also not true for Windows. You've got all the SCSI 
translation logic in the frontend when using blkif so that first path would 
collapse to:

Disk driver -> (SCSI) HBA Driver -> xen-scsiback -> LIO -> backstore -> XXX

>  * xen-block has been existed for many years, widely used and more stable.
> 

It's definitely widely used, but it has had stability issues in recent times.

  Paul

> Welcome any input, thank you!
> 
> Signed-off-by: Bob Liu <bob.liu@xxxxxxxxxx>
> ---
>  xen/include/public/io/blkif.h |   73
> +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 99f0326..7c10bce 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -635,6 +635,28 @@
>  #define BLKIF_OP_INDIRECT          6
> 
>  /*
> + * Recognised 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 SCSI command, DIF/DIX and other per-
> request/bio
> + * data 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,10 +725,61 @@ struct blkif_request_indirect {
>  };
>  typedef struct blkif_request_indirect blkif_request_indirect_t;
> 
> +enum blkif_extra_request_type {
> +     BLKIF_EXTRA_TYPE_SCSI_CMD = 1,          /* Transmit SCSI
> command.  */
> +};
> +
> +struct scsi_cmd_req {
> +     /*
> +      * Grant mapping for transmiting SCSI command to backend, and
> +      * also receive sense data from backend.
> +      * One 4KB page is enough.
> +      */
> +     grant_ref_t cmd_gref;
> +     /* Length of SCSI command in the grant mapped page. */
> +     unsigned int cmd_len;
> +
> +     /*
> +      * SCSI command may require transmiting data segment length less
> +      * than a sector(512 bytes).
> +      * Record num_sg and last segment length in extra request so that
> +      * backend can know about them.
> +      */
> +     unsigned int num_sg;
> +     unsigned int last_sg_len;
> +};
> +
> +/*
> + * Extra request, must follow a normal-request and a normal-request can
> + * only be followed by one extra request.
> + */
> +struct blkif_request_extra {
> +     uint8_t type;           /* BLKIF_EXTRA_TYPE_* */
> +     uint16_t _pad1;
> +#ifndef CONFIG_X86_32
> +     uint32_t _pad2;         /* offsetof(blkif_...,u.extra.id) == 8
> */
> +#endif
> +     uint64_t id;
> +     struct scsi_cmd_req scsi_cmd;
> +} __attribute__((__packed__));
> +typedef struct blkif_request_extra blkif_request_extra_t;
> +
> +struct scsi_cmd_res {
> +     unsigned int resid_len;
> +     /* Length of sense data returned in grant mapped page. */
> +     unsigned int sense_len;
> +};
> +
> +struct blkif_response_extra {
> +     uint8_t type;  /* BLKIF_EXTRA_TYPE_* */
> +     struct scsi_cmd_res scsi_cmd;
> +} __attribute__((__packed__));
> +
>  struct blkif_response {
>      uint64_t        id;              /* copied from request */
>      uint8_t         operation;       /* copied from request */
>      int16_t         status;          /* BLKIF_RSP_???       */
> +    struct blkif_response_extra extra;
>  };
>  typedef struct blkif_response blkif_response_t;
> 
> --
> 1.7.10.4


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