| Message ID | 20210602121037.11083-4-nanich.lee@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bio: control bio max size | expand |
On 2021/06/02 21:29, Changheun Lee wrote: > Set max_bio_bytes same with queue max sectors. It will lead to fast bio > submit when bio size is over than queue max sectors. And it might be helpful > to align submit bio size in some case. > > Signed-off-by: Changheun Lee <nanich.lee@samsung.com> > --- > drivers/scsi/scsi_lib.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 532304d42f00..f6269268b0e0 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1837,6 +1837,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q) > blk_queue_virt_boundary(q, shost->virt_boundary_mask); > dma_set_max_seg_size(dev, queue_max_segment_size(q)); > > + blk_queue_max_bio_bytes(q, queue_max_sectors(q)); Doing this unconditionally for all scsi block devices is probably not a good idea. Cannot this be moved to the LLD handling the devices that actually need it ? > + > /* > * Set a reasonable default alignment: The larger of 32-byte (dword), > * which is a common minimum for HBAs, and the minimum DMA alignment, >
On 6/2/21 5:10 AM, Changheun Lee wrote: > Set max_bio_bytes same with queue max sectors. It will lead to fast bio > submit when bio size is over than queue max sectors. And it might be helpful > to align submit bio size in some case. > > Signed-off-by: Changheun Lee <nanich.lee@samsung.com> > --- > drivers/scsi/scsi_lib.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 532304d42f00..f6269268b0e0 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1837,6 +1837,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q) > blk_queue_virt_boundary(q, shost->virt_boundary_mask); > dma_set_max_seg_size(dev, queue_max_segment_size(q)); > > + blk_queue_max_bio_bytes(q, queue_max_sectors(q)); > + > /* > * Set a reasonable default alignment: The larger of 32-byte (dword), > * which is a common minimum for HBAs, and the minimum DMA alignment, Has this patch been tested with dm-crypt on top of a SCSI device? I'm concerned that this patch will trigger data corruption with dm-crypt on top because the above change will make the following dm-crypt code fail for a sufficiently large bio: bio_add_page(clone, page, len, 0); When testing dm-crypt on top of this patch series, please change the above dm-crypt code into the following before running any tests: WARN_ON(bio_add_page(clone, page, len, 0) < len); Thanks, Bart.
> On 2021/06/02 21:29, Changheun Lee wrote: > > Set max_bio_bytes same with queue max sectors. It will lead to fast bio > > submit when bio size is over than queue max sectors. And it might be helpful > > to align submit bio size in some case. > > > > Signed-off-by: Changheun Lee <nanich.lee@samsung.com> > > --- > > drivers/scsi/scsi_lib.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > > index 532304d42f00..f6269268b0e0 100644 > > --- a/drivers/scsi/scsi_lib.c > > +++ b/drivers/scsi/scsi_lib.c > > @@ -1837,6 +1837,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q) > > blk_queue_virt_boundary(q, shost->virt_boundary_mask); > > dma_set_max_seg_size(dev, queue_max_segment_size(q)); > > > > + blk_queue_max_bio_bytes(q, queue_max_sectors(q)); > > Doing this unconditionally for all scsi block devices is probably not a good > idea. Cannot this be moved to the LLD handling the devices that actually need it ? OK, I'll try to check more nice location in LLD. > > > + > > /* > > * Set a reasonable default alignment: The larger of 32-byte (dword), > > * which is a common minimum for HBAs, and the minimum DMA alignment, > > > > > -- > Damien Le Moal > Western Digital Research Thank you, Changheun Lee
> On 6/2/21 5:10 AM, Changheun Lee wrote: > > Set max_bio_bytes same with queue max sectors. It will lead to fast bio > > submit when bio size is over than queue max sectors. And it might be helpful > > to align submit bio size in some case. > > > > Signed-off-by: Changheun Lee <nanich.lee@samsung.com> > > --- > > drivers/scsi/scsi_lib.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > > index 532304d42f00..f6269268b0e0 100644 > > --- a/drivers/scsi/scsi_lib.c > > +++ b/drivers/scsi/scsi_lib.c > > @@ -1837,6 +1837,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q) > > blk_queue_virt_boundary(q, shost->virt_boundary_mask); > > dma_set_max_seg_size(dev, queue_max_segment_size(q)); > > > > + blk_queue_max_bio_bytes(q, queue_max_sectors(q)); > > + > > /* > > * Set a reasonable default alignment: The larger of 32-byte (dword), > > * which is a common minimum for HBAs, and the minimum DMA alignment, > > Has this patch been tested with dm-crypt on top of a SCSI device? I'm > concerned that this patch will trigger data corruption with dm-crypt on > top because the above change will make the following dm-crypt code fail > for a sufficiently large bio: > > bio_add_page(clone, page, len, 0); > > When testing dm-crypt on top of this patch series, please change the > above dm-crypt code into the following before running any tests: > > WARN_ON(bio_add_page(clone, page, len, 0) < len); > > Thanks, > > Bart. I think it will be OK if nr_iovecs is not over BIO_MAX_VECS in crypt_alloc_buffer(). Because page is added to bio in PAGE_SIZE as maximum, and the minimum of max_bio_bytes is "BIO_MAX_VECS * PAGE_SIZE". for (i = 0; i < nr_iovecs; i++) { page = mempool_alloc(&cc->page_pool, gfp_mask); if (!page) { crypt_free_buffer_pages(cc, clone); bio_put(clone); gfp_mask |= __GFP_DIRECT_RECLAIM; goto retry; } len = (remaining_size > PAGE_SIZE) ? PAGE_SIZE : remaining_size; bio_add_page(clone, page, len, 0); remaining_size -= len; } Anyway as your advise, I'm checking it in my test environment. There are no problem by now yet. I'm testing with 1MB max_bio_bytes of SCSI, and UINT_MAX max_bio_bytes of dm-crypt. Thank you, Changheun Lee.
> > On 2021/06/02 21:29, Changheun Lee wrote: > > > Set max_bio_bytes same with queue max sectors. It will lead to fast bio > > > submit when bio size is over than queue max sectors. And it might be helpful > > > to align submit bio size in some case. > > > > > > Signed-off-by: Changheun Lee <nanich.lee@samsung.com> > > > --- > > > drivers/scsi/scsi_lib.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > > > index 532304d42f00..f6269268b0e0 100644 > > > --- a/drivers/scsi/scsi_lib.c > > > +++ b/drivers/scsi/scsi_lib.c > > > @@ -1837,6 +1837,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q) > > > blk_queue_virt_boundary(q, shost->virt_boundary_mask); > > > dma_set_max_seg_size(dev, queue_max_segment_size(q)); > > > > > > + blk_queue_max_bio_bytes(q, queue_max_sectors(q)); > > > > Doing this unconditionally for all scsi block devices is probably not a good > > idea. Cannot this be moved to the LLD handling the devices that actually need it ? > > OK, I'll try to check more nice location in LLD. > > > > > > + > > > /* > > > * Set a reasonable default alignment: The larger of 32-byte (dword), > > > * which is a common minimum for HBAs, and the minimum DMA alignment, > > > > > > > > > -- > > Damien Le Moal > > Western Digital Research > > Thank you, > Changheun Lee I think below location might be good. feedback to me please. diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 3eb54937f1d8..0f97b7d275ee 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -4831,6 +4831,9 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev) ufshcd_get_lu_power_on_wp_status(hba, sdev); + blk_queue_max_bio_bytes(sdev->request_queue, + queue_max_sectors(sdev->request_queue)); + return 0; } Thank you, Changheun Lee
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 532304d42f00..f6269268b0e0 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1837,6 +1837,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q) blk_queue_virt_boundary(q, shost->virt_boundary_mask); dma_set_max_seg_size(dev, queue_max_segment_size(q)); + blk_queue_max_bio_bytes(q, queue_max_sectors(q)); + /* * Set a reasonable default alignment: The larger of 32-byte (dword), * which is a common minimum for HBAs, and the minimum DMA alignment,
Set max_bio_bytes same with queue max sectors. It will lead to fast bio submit when bio size is over than queue max sectors. And it might be helpful to align submit bio size in some case. Signed-off-by: Changheun Lee <nanich.lee@samsung.com> --- drivers/scsi/scsi_lib.c | 2 ++ 1 file changed, 2 insertions(+)