Re: [PATCH 05/17] IB/iser: Embed ib_scatterlist into iser_data_buf

[Sagi Grimberg <sagi@xxxxxxxxxxx>]

> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
> index 120b40829560..2fc79ea897a3 100644
> --- a/drivers/infiniband/ulp/iser/iscsi_iser.h
> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
> @@ -188,16 +188,14 @@ enum iser_data_dir {
>   /**
>    * struct iser_data_buf - iSER data buffer
>    *
> - * @sg:           pointer to the sg list
> - * @size:         num entries of this sg
> - * @data_len:     total beffer byte len
> - * @dma_nents:    returned by dma_map_sg
> + * @ib_sg:        ib scatterlist
> + * @size:         num entries of the unmaped sg list
> + * @data_len:     total buffer byte len

If you already introduced ib_scatterlist (still not sure if its the best
way to go), I think that it should also keep track of the original
number of entries before dma mapping. I guess all implementations keep
both anyways right?

>    */
>   struct iser_data_buf {
> -	struct scatterlist *sg;
> -	int                size;
> -	unsigned long      data_len;
> -	unsigned int       dma_nents;
> +	struct ib_scatterlist ib_sg;
> +	int                   size;
> +	unsigned long         data_len;
>   };
>   
>   /* fwd declarations */
> diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c b/drivers/infiniband/ulp/iser/iser_initiator.c
> index 96af06cfe0af..9556ec55dec2 100644
> --- a/drivers/infiniband/ulp/iser/iser_initiator.c
> +++ b/drivers/infiniband/ulp/iser/iser_initiator.c
> @@ -388,16 +388,18 @@ int iser_send_command(struct iscsi_conn *conn,
>   	}
>   
>   	if (scsi_sg_count(sc)) { /* using a scatter list */
> -		data_buf->sg = scsi_sglist(sc);
> +		data_buf->ib_sg.sg = scsi_sglist(sc);
>   		data_buf->size = scsi_sg_count(sc);
>   	}
>   	data_buf->data_len = scsi_bufflen(sc);
> +	data_buf->ib_sg.offset = 0;
>   
>   	if (scsi_prot_sg_count(sc)) {
> -		prot_buf->sg  = scsi_prot_sglist(sc);
> +		prot_buf->ib_sg.sg = scsi_prot_sglist(sc);
>   		prot_buf->size = scsi_prot_sg_count(sc);
>   		prot_buf->data_len = (data_buf->data_len >>
>   				     ilog2(sc->device->sector_size)) * 8;
> +		prot_buf->ib_sg.offset = 0;
>   	}
>   
>   	if (hdr->flags & ISCSI_FLAG_CMD_READ) {
> diff --git a/drivers/infiniband/ulp/iser/iser_memory.c b/drivers/infiniband/ulp/iser/iser_memory.c
> index da3f6f7cbc72..8fd3df94ec4b 100644
> --- a/drivers/infiniband/ulp/iser/iser_memory.c
> +++ b/drivers/infiniband/ulp/iser/iser_memory.c
> @@ -142,7 +142,7 @@ static void iser_data_buf_dump(struct iser_data_buf *data,
>   	struct scatterlist *sg;
>   	int i;
>   
> -	for_each_sg(data->sg, sg, data->dma_nents, i)
> +	for_each_sg(data->ib_sg.sg, sg, data->ib_sg.dma_nents, i)
>   		iser_dbg("sg[%d] dma_addr:0x%lX page:0x%p "
>   			 "off:0x%x sz:0x%x dma_len:0x%x\n",
>   			 i, (unsigned long)ib_sg_dma_address(ibdev, sg),
> @@ -170,8 +170,9 @@ int iser_dma_map_task_data(struct iscsi_iser_task *iser_task,
>   	iser_task->dir[iser_dir] = 1;
>   	dev = iser_task->iser_conn->ib_conn.device->ib_device;
>   
> -	data->dma_nents = ib_dma_map_sg(dev, data->sg, data->size, dma_dir);
> -	if (data->dma_nents == 0) {
> +	data->ib_sg.dma_nents = ib_dma_map_sg(dev, data->ib_sg.sg, data->size,
> +					      dma_dir);
> +	if (data->ib_sg.dma_nents == 0) {
>   		iser_err("dma_map_sg failed!!!\n");
>   		return -EINVAL;
>   	}
> @@ -185,14 +186,14 @@ void iser_dma_unmap_task_data(struct iscsi_iser_task *iser_task,
>   	struct ib_device *dev;
>   
>   	dev = iser_task->iser_conn->ib_conn.device->ib_device;
> -	ib_dma_unmap_sg(dev, data->sg, data->size, dir);
> +	ib_dma_unmap_sg(dev, data->ib_sg.sg, data->size, dir);
>   }
>   
>   static int
>   iser_reg_dma(struct iser_device *device, struct iser_data_buf *mem,
>   	     struct iser_mem_reg *reg)
>   {
> -	struct scatterlist *sg = mem->sg;
> +	struct scatterlist *sg = mem->ib_sg.sg;
>   
>   	reg->sge.lkey = device->pd->local_dma_lkey;
>   	/*
> @@ -235,16 +236,12 @@ int iser_fast_reg_fmr(struct iscsi_iser_task *iser_task,
>   	struct iser_page_vec *page_vec = rsc->page_vec;
>   	struct ib_fmr_pool *fmr_pool = rsc->fmr_pool;
>   	struct ib_pool_fmr *fmr;
> -	struct ib_scatterlist ib_sg;
>   	int ret, plen;
>   
>   	page_vec->npages = 0;
>   	page_vec->fake_mr.page_size = SIZE_4K;
> -	ib_sg.sg = mem->sg;
> -	ib_sg.dma_nents = mem->dma_nents;
> -	ib_sg.offset = 0;
> -	plen = ib_sg_to_pages(&page_vec->fake_mr, &ib_sg, iser_set_page);
> -	if (unlikely(plen < mem->dma_nents)) {
> +	plen = ib_sg_to_pages(&page_vec->fake_mr, &mem->ib_sg, iser_set_page);
> +	if (unlikely(plen < mem->ib_sg.dma_nents)) {

We really should get rid of fmrs, I'll send a patch on top of this once
it settles.