| Message ID | 20190606000209.26086-1-sagi@grimberg.me (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IB/iser: explicitly set shost max_segment_size | expand |
On Wed, Jun 05, 2019 at 05:02:09PM -0700, Sagi Grimberg wrote: > if the lld does not explicitly sets this, scsi takes BLK_MAX_SEGMENT_SIZE > and sets it using dma_set_max_seg_size(). In our case, this will affect > all the rdma device consumers. > > Fix it by setting shost max_segment_size according to the device > capability. > > Reported-by: Jason Gunthorpe <jgg@ziepe.ca> > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > This goes on top of hch patchset: > "properly communicate queue limits to the DMA layer" > > Normally this should go through the rdma tree, so we can > either get it through jens with hch patchset. Alternatively > this is a fix that should go to rc anyways? If CH's series goes to -rc we can take this through rdma after it gets to -rc4 or so.. Otherwise Jens should probably take it, I'm not expecting conflicts in this area so far. Thanks, Jason
On Wed, Jun 05, 2019 at 05:02:09PM -0700, Sagi Grimberg wrote: > if the lld does not explicitly sets this, scsi takes BLK_MAX_SEGMENT_SIZE > and sets it using dma_set_max_seg_size(). In our case, this will affect > all the rdma device consumers. > > Fix it by setting shost max_segment_size according to the device > capability. > > Reported-by: Jason Gunthorpe <jgg@ziepe.ca> > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > --- > This goes on top of hch patchset: > "properly communicate queue limits to the DMA layer" > > Normally this should go through the rdma tree, so we can > either get it through jens with hch patchset. Alternatively > this is a fix that should go to rc anyways? I don't think this make sense. > > drivers/infiniband/ulp/iser/iscsi_iser.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c > index 56848232eb81..2984a366dd7d 100644 > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > @@ -653,6 +653,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, > SHOST_DIX_GUARD_CRC); > } > > + shost->max_segment_size = ib_dma_max_seg_size(ib_dev); > if (!(ib_dev->attrs.device_cap_flags & IB_DEVICE_SG_GAPS_REG)) > shost->virt_boundary_mask = ~MASK_4K; We only really need this settings in the IB_DEVICE_SG_GAPS_REG case, as the segement size is unlimited on the PRP-like scheme used by the other MR types anyway, and set as such by the block layer. I.e.g this should become: if (ib_dev->attrs.device_cap_flags & IB_DEVICE_SG_GAPS_REG) shost->max_segment_size = ib_dma_max_seg_size(ib_dev); else shost->virt_boundary_mask = ~MASK_4K;
>> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c >> index 56848232eb81..2984a366dd7d 100644 >> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c >> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c >> @@ -653,6 +653,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, >> SHOST_DIX_GUARD_CRC); >> } >> >> + shost->max_segment_size = ib_dma_max_seg_size(ib_dev); >> if (!(ib_dev->attrs.device_cap_flags & IB_DEVICE_SG_GAPS_REG)) >> shost->virt_boundary_mask = ~MASK_4K; > > We only really need this settings in the IB_DEVICE_SG_GAPS_REG case, > as the segement size is unlimited on the PRP-like scheme used by the > other MR types anyway, and set as such by the block layer. I.e.g this > should become: > > if (ib_dev->attrs.device_cap_flags & IB_DEVICE_SG_GAPS_REG) > shost->max_segment_size = ib_dma_max_seg_size(ib_dev); > else > shost->virt_boundary_mask = ~MASK_4K; Not sure I understand. max_segment_size and virt_boundary_mask are related how?
On Wed, Jun 05, 2019 at 11:47:23PM -0700, Sagi Grimberg wrote: > Not sure I understand. > > max_segment_size and virt_boundary_mask are related how? virt_boundary_devices has hardware segment size of a single page (device page as identified by the boundary, not necessarily linux PAGE_SIZE). So we don't need a max_segment_size in the Linux size, as any amount of hardware segments fitting the virt boundary can be merged into a 'Linux segment'. Because of the accouting for the merges is hard and was partially broken anyway we've now dropped the accounting and force the max_segment_size to be unlimited in the block layer if a device sets the virt_boundary_mask.
>> Not sure I understand. >> >> max_segment_size and virt_boundary_mask are related how? > > virt_boundary_devices has hardware segment size of a single page (device > page as identified by the boundary, not necessarily linux PAGE_SIZE). > So we don't need a max_segment_size in the Linux size, as any amount of > hardware segments fitting the virt boundary can be merged into a 'Linux > segment'. Because of the accouting for the merges is hard and was > partially broken anyway we've now dropped the accounting and force the > max_segment_size to be unlimited in the block layer if a device sets > the virt_boundary_mask. OK, want me to respin? or you want to wait to see if you have a v2 and fold it in if you do?
On 6/6/19 12:04 AM, Christoph Hellwig wrote: > On Wed, Jun 05, 2019 at 11:47:23PM -0700, Sagi Grimberg wrote: >> Not sure I understand. >> >> max_segment_size and virt_boundary_mask are related how? > > virt_boundary_devices has hardware segment size of a single page (device > page as identified by the boundary, not necessarily linux PAGE_SIZE). > So we don't need a max_segment_size in the Linux size, as any amount of > hardware segments fitting the virt boundary can be merged into a 'Linux > segment'. Because of the accouting for the merges is hard and was > partially broken anyway we've now dropped the accounting and force the > max_segment_size to be unlimited in the block layer if a device sets > the virt_boundary_mask. Hi Christoph, What if that accounting would ever be implemented in the block layer? Would that mean that v2 of this patch becomes incorrect and has to be changed back into v1 of this patch? Is v1 incorrect today? Thank you, Bart.
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c index 56848232eb81..2984a366dd7d 100644 --- a/drivers/infiniband/ulp/iser/iscsi_iser.c +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c @@ -653,6 +653,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, SHOST_DIX_GUARD_CRC); } + shost->max_segment_size = ib_dma_max_seg_size(ib_dev); if (!(ib_dev->attrs.device_cap_flags & IB_DEVICE_SG_GAPS_REG)) shost->virt_boundary_mask = ~MASK_4K;
if the lld does not explicitly sets this, scsi takes BLK_MAX_SEGMENT_SIZE and sets it using dma_set_max_seg_size(). In our case, this will affect all the rdma device consumers. Fix it by setting shost max_segment_size according to the device capability. Reported-by: Jason Gunthorpe <jgg@ziepe.ca> Signed-off-by: Sagi Grimberg <sagi@grimberg.me> --- This goes on top of hch patchset: "properly communicate queue limits to the DMA layer" Normally this should go through the rdma tree, so we can either get it through jens with hch patchset. Alternatively this is a fix that should go to rc anyways? drivers/infiniband/ulp/iser/iscsi_iser.c | 1 + 1 file changed, 1 insertion(+)