On Mon, Aug 31, 2020 at 08:08:26PM -0700, Sagi Grimberg wrote:
> Currently we allocate rx buffers in a single contiguous buffers for
> headers (iser and iscsi) and data trailer. This means that most likely
> the data starting offset is aligned to 76 bytes (size of both headers).
>
> This worked fine for years, but at some point this broke, resulting in
> data corruptions in isert when a command comes with immediate data
> and the underlying backend device assumes 512 bytes buffer alignment.
>
> We assume a hard-requirement for all direct I/O buffers to be 512 bytes
> aligned. To fix this, we should avoid passing unaligned buffers for I/O.
>
> Instead, we allocate our recv buffers with some extra space such that we
> can have the data portion align to 512 byte boundary. This also means
> that we cannot reference headers or data using structure but rather
> accessors (as they may move based on alignment). Also, get rid of the
> wrong __packed annotation from iser_rx_desc as this has only harmful
> effects (not aligned to anything).
>
> This affects the rx descriptors for iscsi login and data plane.
>
> Reported-by: Stephen Rust <srust@xxxxxxxxxxxxxxx>
> Tested-by: Doug Dumitru <doug@xxxxxxxxxxx>
> Signed-off-by: Sagi Grimberg <sagi@xxxxxxxxxxx>
> ---
> Changes from v1:
> - revised change log
>
> drivers/infiniband/ulp/isert/ib_isert.c | 93 +++++++++++++------------
> drivers/infiniband/ulp/isert/ib_isert.h | 41 ++++++++---
> 2 files changed, 79 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 61e2f7fc513d..5b6a0ad9faaa 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -140,15 +140,16 @@ isert_alloc_rx_descriptors(struct isert_conn *isert_conn)
> rx_desc = isert_conn->rx_descs;
>
> for (i = 0; i < ISERT_QP_MAX_RECV_DTOS; i++, rx_desc++) {
> - dma_addr = ib_dma_map_single(ib_dev, (void *)rx_desc,
> - ISER_RX_PAYLOAD_SIZE, DMA_FROM_DEVICE);
> + dma_addr = ib_dma_map_single(ib_dev,
> + rx_desc->buf,
Nit: no real need to the line break here.
> + ib_dma_unmap_single(ib_dev, isert_conn->login_desc->dma_addr,
> + ISER_RX_SIZE,
> DMA_FROM_DEVICE);
Same here.
> + * RX size is default of 8k plus headers, but data needs to align to
> + * 512 boundary, so use 1024 to have the extra space for alignment.
> + */
> +#define ISER_RX_SIZE (ISCSI_DEF_MAX_RECV_SEG_LEN + 1024)
A 512 byte alignment is not the correct for e.g. the 4k Xen block
device case. Any reason we don't just separate allocations for headers
vs the data and use another ib_sge?
