Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer
On 10/18/18 8:06 PM, Ming Lei wrote: > On Thu, Oct 18, 2018 at 07:52:59PM -0600, Jens Axboe wrote: >> On 10/18/18 7:39 PM, Ming Lei wrote: >>> On Thu, Oct 18, 2018 at 07:33:50PM -0600, Jens Axboe wrote: On 10/18/18 7:28 PM, Ming Lei wrote: > On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote: >> On 10/18/18 7:18 AM, Ming Lei wrote: >>> Now we only check if DMA IO buffer is aligned to queue_dma_alignment() >>> for pass-through request, and it isn't done for normal IO request. >>> >>> Given the check has to be done on each bvec, it isn't efficient to add >>> the >>> check in generic_make_request_checks(). >>> >>> This patch addes one WARN in blk_queue_split() for capturing this issue. >> >> I don't want to do this, because then we are forever doomed to >> have something that fully loops a bio at submission time. I >> absolutely hate the splitting we have and the need for it, >> hopefully it can go away for a subset of IOs at some point. >> >> In many ways, this seems to be somewhat of a made-up problem, I don't >> recall a single bug report for something like this over decades of >> working with the IO stack. 512b alignment restrictions for DMA seems >> absolutely insane. I know people claim they exist, but clearly that >> isn't a hard requirement or we would have been boned years ago. > > There are still some drivers with this requirement: > > drivers/ata/libata-scsi.c:1308: blk_queue_update_dma_alignment(q, > sdev->sector_size - 1); > drivers/ata/pata_macio.c:812: > blk_queue_update_dma_alignment(sdev->request_queue, 31); > drivers/ata/pata_macio.c:827: > blk_queue_update_dma_alignment(sdev->request_queue, 15); > drivers/block/ps3disk.c:470:blk_queue_dma_alignment(queue, > dev->blk_size-1); > drivers/block/rsxx/dev.c:282: > blk_queue_dma_alignment(card->queue, blk_size - 1); > drivers/block/xen-blkfront.c:957: blk_queue_dma_alignment(rq, 511); > drivers/ide/ide-cd.c:1512: blk_queue_dma_alignment(q, 31); > drivers/message/fusion/mptscsih.c:2388: blk_queue_dma_alignment > (sdev->request_queue, 512 - 1); > drivers/staging/rts5208/rtsx.c:94: > blk_queue_dma_alignment(sdev->request_queue, (512 - 1)); > drivers/usb/image/microtek.c:329: > blk_queue_dma_alignment(s->request_queue, (512 - 1)); > drivers/usb/storage/scsiglue.c:92: > blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); > drivers/usb/storage/uas.c:818: > blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); Of course, I too can grep :-) My point is that these settings might not match reality. And the WARN_ON(), as implemented, is going to trigger on any device that DOESN'T set the alignment, as Bart pointed out. >>> >>> It is just a WARN_ON_ONCE() which exactly shows something which need >>> further attention, then related people may take a look and we can move >>> on. >>> >>> So I think it is correct thing to do. >> >> It most certainly is NOT the right thing to do, when we know that: >> >> 1) We currently have drivers setting an alignment that we don't meet >> 2) We have drivers not setting an alignment, and getting 512 by default > > The 512 default should have been removed given it isn't respected at > all in normal io path, but it is included from the beginning of 2.6.12 > >> 3) We have drivers setting an alignment that seems incorrect > > Then WARN_ON() is helpful for both 1) and 2) after the default 512 > limit is removed. I'm not saying it's not useful (though even that is doubtful), I'm saying it's exactly the wrong order. You cut the very paragraph where I stated that. Drop this patch, focus on the other bits. Once that is done, then we can debate whether it's useful or not. Right now it definitely isn't. -- Jens Axboe
Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer
On Thu, Oct 18, 2018 at 07:52:59PM -0600, Jens Axboe wrote: > On 10/18/18 7:39 PM, Ming Lei wrote: > > On Thu, Oct 18, 2018 at 07:33:50PM -0600, Jens Axboe wrote: > >> On 10/18/18 7:28 PM, Ming Lei wrote: > >>> On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote: > On 10/18/18 7:18 AM, Ming Lei wrote: > > Now we only check if DMA IO buffer is aligned to queue_dma_alignment() > > for pass-through request, and it isn't done for normal IO request. > > > > Given the check has to be done on each bvec, it isn't efficient to add > > the > > check in generic_make_request_checks(). > > > > This patch addes one WARN in blk_queue_split() for capturing this issue. > > I don't want to do this, because then we are forever doomed to > have something that fully loops a bio at submission time. I > absolutely hate the splitting we have and the need for it, > hopefully it can go away for a subset of IOs at some point. > > In many ways, this seems to be somewhat of a made-up problem, I don't > recall a single bug report for something like this over decades of > working with the IO stack. 512b alignment restrictions for DMA seems > absolutely insane. I know people claim they exist, but clearly that > isn't a hard requirement or we would have been boned years ago. > >>> > >>> There are still some drivers with this requirement: > >>> > >>> drivers/ata/libata-scsi.c:1308: blk_queue_update_dma_alignment(q, > >>> sdev->sector_size - 1); > >>> drivers/ata/pata_macio.c:812: > >>> blk_queue_update_dma_alignment(sdev->request_queue, 31); > >>> drivers/ata/pata_macio.c:827: > >>> blk_queue_update_dma_alignment(sdev->request_queue, 15); > >>> drivers/block/ps3disk.c:470:blk_queue_dma_alignment(queue, > >>> dev->blk_size-1); > >>> drivers/block/rsxx/dev.c:282: > >>> blk_queue_dma_alignment(card->queue, blk_size - 1); > >>> drivers/block/xen-blkfront.c:957: blk_queue_dma_alignment(rq, 511); > >>> drivers/ide/ide-cd.c:1512: blk_queue_dma_alignment(q, 31); > >>> drivers/message/fusion/mptscsih.c:2388: blk_queue_dma_alignment > >>> (sdev->request_queue, 512 - 1); > >>> drivers/staging/rts5208/rtsx.c:94: > >>> blk_queue_dma_alignment(sdev->request_queue, (512 - 1)); > >>> drivers/usb/image/microtek.c:329: > >>> blk_queue_dma_alignment(s->request_queue, (512 - 1)); > >>> drivers/usb/storage/scsiglue.c:92: > >>> blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); > >>> drivers/usb/storage/uas.c:818: > >>> blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); > >> > >> Of course, I too can grep :-) > >> > >> My point is that these settings might not match reality. And the > >> WARN_ON(), as implemented, is going to trigger on any device that > >> DOESN'T set the alignment, as Bart pointed out. > > > > It is just a WARN_ON_ONCE() which exactly shows something which need > > further attention, then related people may take a look and we can move > > on. > > > > So I think it is correct thing to do. > > It most certainly is NOT the right thing to do, when we know that: > > 1) We currently have drivers setting an alignment that we don't meet > 2) We have drivers not setting an alignment, and getting 512 by default The 512 default should have been removed given it isn't respected at all in normal io path, but it is included from the beginning of 2.6.12 > 3) We have drivers setting an alignment that seems incorrect Then WARN_ON() is helpful for both 1) and 2) after the default 512 limit is removed. Thanks, Ming
Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer
On 10/18/18 7:39 PM, Ming Lei wrote: > On Thu, Oct 18, 2018 at 07:33:50PM -0600, Jens Axboe wrote: >> On 10/18/18 7:28 PM, Ming Lei wrote: >>> On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote: On 10/18/18 7:18 AM, Ming Lei wrote: > Now we only check if DMA IO buffer is aligned to queue_dma_alignment() > for pass-through request, and it isn't done for normal IO request. > > Given the check has to be done on each bvec, it isn't efficient to add the > check in generic_make_request_checks(). > > This patch addes one WARN in blk_queue_split() for capturing this issue. I don't want to do this, because then we are forever doomed to have something that fully loops a bio at submission time. I absolutely hate the splitting we have and the need for it, hopefully it can go away for a subset of IOs at some point. In many ways, this seems to be somewhat of a made-up problem, I don't recall a single bug report for something like this over decades of working with the IO stack. 512b alignment restrictions for DMA seems absolutely insane. I know people claim they exist, but clearly that isn't a hard requirement or we would have been boned years ago. >>> >>> There are still some drivers with this requirement: >>> >>> drivers/ata/libata-scsi.c:1308: blk_queue_update_dma_alignment(q, >>> sdev->sector_size - 1); >>> drivers/ata/pata_macio.c:812: >>> blk_queue_update_dma_alignment(sdev->request_queue, 31); >>> drivers/ata/pata_macio.c:827: >>> blk_queue_update_dma_alignment(sdev->request_queue, 15); >>> drivers/block/ps3disk.c:470:blk_queue_dma_alignment(queue, >>> dev->blk_size-1); >>> drivers/block/rsxx/dev.c:282: >>> blk_queue_dma_alignment(card->queue, blk_size - 1); >>> drivers/block/xen-blkfront.c:957: blk_queue_dma_alignment(rq, 511); >>> drivers/ide/ide-cd.c:1512: blk_queue_dma_alignment(q, 31); >>> drivers/message/fusion/mptscsih.c:2388: blk_queue_dma_alignment >>> (sdev->request_queue, 512 - 1); >>> drivers/staging/rts5208/rtsx.c:94: >>> blk_queue_dma_alignment(sdev->request_queue, (512 - 1)); >>> drivers/usb/image/microtek.c:329: >>> blk_queue_dma_alignment(s->request_queue, (512 - 1)); >>> drivers/usb/storage/scsiglue.c:92: >>> blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); >>> drivers/usb/storage/uas.c:818: >>> blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); >> >> Of course, I too can grep :-) >> >> My point is that these settings might not match reality. And the >> WARN_ON(), as implemented, is going to trigger on any device that >> DOESN'T set the alignment, as Bart pointed out. > > It is just a WARN_ON_ONCE() which exactly shows something which need > further attention, then related people may take a look and we can move > on. > > So I think it is correct thing to do. It most certainly is NOT the right thing to do, when we know that: 1) We currently have drivers setting an alignment that we don't meet 2) We have drivers not setting an alignment, and getting 512 by default 3) We have drivers setting an alignment that seems incorrect It's something that can be debated once everything else has been done, it's most certainly not something that should be done upfront when we KNOW it'll trigger for cases. WARN_ON_ONCE() can be useful to figure out if an invalid condition triggers, it's pointless to add it for cases that we know exactly how they will trigger. -- Jens Axboe
Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer
On Thu, Oct 18, 2018 at 07:33:50PM -0600, Jens Axboe wrote: > On 10/18/18 7:28 PM, Ming Lei wrote: > > On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote: > >> On 10/18/18 7:18 AM, Ming Lei wrote: > >>> Now we only check if DMA IO buffer is aligned to queue_dma_alignment() > >>> for pass-through request, and it isn't done for normal IO request. > >>> > >>> Given the check has to be done on each bvec, it isn't efficient to add the > >>> check in generic_make_request_checks(). > >>> > >>> This patch addes one WARN in blk_queue_split() for capturing this issue. > >> > >> I don't want to do this, because then we are forever doomed to > >> have something that fully loops a bio at submission time. I > >> absolutely hate the splitting we have and the need for it, > >> hopefully it can go away for a subset of IOs at some point. > >> > >> In many ways, this seems to be somewhat of a made-up problem, I don't > >> recall a single bug report for something like this over decades of > >> working with the IO stack. 512b alignment restrictions for DMA seems > >> absolutely insane. I know people claim they exist, but clearly that > >> isn't a hard requirement or we would have been boned years ago. > > > > There are still some drivers with this requirement: > > > > drivers/ata/libata-scsi.c:1308: blk_queue_update_dma_alignment(q, > > sdev->sector_size - 1); > > drivers/ata/pata_macio.c:812: > > blk_queue_update_dma_alignment(sdev->request_queue, 31); > > drivers/ata/pata_macio.c:827: > > blk_queue_update_dma_alignment(sdev->request_queue, 15); > > drivers/block/ps3disk.c:470:blk_queue_dma_alignment(queue, > > dev->blk_size-1); > > drivers/block/rsxx/dev.c:282: > > blk_queue_dma_alignment(card->queue, blk_size - 1); > > drivers/block/xen-blkfront.c:957: blk_queue_dma_alignment(rq, 511); > > drivers/ide/ide-cd.c:1512: blk_queue_dma_alignment(q, 31); > > drivers/message/fusion/mptscsih.c:2388: blk_queue_dma_alignment > > (sdev->request_queue, 512 - 1); > > drivers/staging/rts5208/rtsx.c:94: > > blk_queue_dma_alignment(sdev->request_queue, (512 - 1)); > > drivers/usb/image/microtek.c:329: > > blk_queue_dma_alignment(s->request_queue, (512 - 1)); > > drivers/usb/storage/scsiglue.c:92: > > blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); > > drivers/usb/storage/uas.c:818: > > blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); > > Of course, I too can grep :-) > > My point is that these settings might not match reality. And the > WARN_ON(), as implemented, is going to trigger on any device that > DOESN'T set the alignment, as Bart pointed out. It is just a WARN_ON_ONCE() which exactly shows something which need further attention, then related people may take a look and we can move on. So I think it is correct thing to do. Thanks, Ming
Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer
On 10/18/18 7:28 PM, Ming Lei wrote: > On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote: >> On 10/18/18 7:18 AM, Ming Lei wrote: >>> Now we only check if DMA IO buffer is aligned to queue_dma_alignment() >>> for pass-through request, and it isn't done for normal IO request. >>> >>> Given the check has to be done on each bvec, it isn't efficient to add the >>> check in generic_make_request_checks(). >>> >>> This patch addes one WARN in blk_queue_split() for capturing this issue. >> >> I don't want to do this, because then we are forever doomed to >> have something that fully loops a bio at submission time. I >> absolutely hate the splitting we have and the need for it, >> hopefully it can go away for a subset of IOs at some point. >> >> In many ways, this seems to be somewhat of a made-up problem, I don't >> recall a single bug report for something like this over decades of >> working with the IO stack. 512b alignment restrictions for DMA seems >> absolutely insane. I know people claim they exist, but clearly that >> isn't a hard requirement or we would have been boned years ago. > > There are still some drivers with this requirement: > > drivers/ata/libata-scsi.c:1308: blk_queue_update_dma_alignment(q, > sdev->sector_size - 1); > drivers/ata/pata_macio.c:812: > blk_queue_update_dma_alignment(sdev->request_queue, 31); > drivers/ata/pata_macio.c:827: > blk_queue_update_dma_alignment(sdev->request_queue, 15); > drivers/block/ps3disk.c:470:blk_queue_dma_alignment(queue, > dev->blk_size-1); > drivers/block/rsxx/dev.c:282: blk_queue_dma_alignment(card->queue, > blk_size - 1); > drivers/block/xen-blkfront.c:957: blk_queue_dma_alignment(rq, 511); > drivers/ide/ide-cd.c:1512: blk_queue_dma_alignment(q, 31); > drivers/message/fusion/mptscsih.c:2388: blk_queue_dma_alignment > (sdev->request_queue, 512 - 1); > drivers/staging/rts5208/rtsx.c:94: > blk_queue_dma_alignment(sdev->request_queue, (512 - 1)); > drivers/usb/image/microtek.c:329: > blk_queue_dma_alignment(s->request_queue, (512 - 1)); > drivers/usb/storage/scsiglue.c:92: > blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); > drivers/usb/storage/uas.c:818: > blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); Of course, I too can grep :-) My point is that these settings might not match reality. And the WARN_ON(), as implemented, is going to trigger on any device that DOESN'T set the alignment, as Bart pointed out. -- Jens Axboe
Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer
On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote: > On 10/18/18 7:18 AM, Ming Lei wrote: > > Now we only check if DMA IO buffer is aligned to queue_dma_alignment() > > for pass-through request, and it isn't done for normal IO request. > > > > Given the check has to be done on each bvec, it isn't efficient to add the > > check in generic_make_request_checks(). > > > > This patch addes one WARN in blk_queue_split() for capturing this issue. > > I don't want to do this, because then we are forever doomed to > have something that fully loops a bio at submission time. I > absolutely hate the splitting we have and the need for it, > hopefully it can go away for a subset of IOs at some point. > > In many ways, this seems to be somewhat of a made-up problem, I don't > recall a single bug report for something like this over decades of > working with the IO stack. 512b alignment restrictions for DMA seems > absolutely insane. I know people claim they exist, but clearly that > isn't a hard requirement or we would have been boned years ago. There are still some drivers with this requirement: drivers/ata/libata-scsi.c:1308: blk_queue_update_dma_alignment(q, sdev->sector_size - 1); drivers/ata/pata_macio.c:812: blk_queue_update_dma_alignment(sdev->request_queue, 31); drivers/ata/pata_macio.c:827: blk_queue_update_dma_alignment(sdev->request_queue, 15); drivers/block/ps3disk.c:470:blk_queue_dma_alignment(queue, dev->blk_size-1); drivers/block/rsxx/dev.c:282: blk_queue_dma_alignment(card->queue, blk_size - 1); drivers/block/xen-blkfront.c:957: blk_queue_dma_alignment(rq, 511); drivers/ide/ide-cd.c:1512: blk_queue_dma_alignment(q, 31); drivers/message/fusion/mptscsih.c:2388: blk_queue_dma_alignment (sdev->request_queue, 512 - 1); drivers/staging/rts5208/rtsx.c:94: blk_queue_dma_alignment(sdev->request_queue, (512 - 1)); drivers/usb/image/microtek.c:329: blk_queue_dma_alignment(s->request_queue, (512 - 1)); drivers/usb/storage/scsiglue.c:92: blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); drivers/usb/storage/uas.c:818: blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); Thanks, Ming
Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer
On 10/18/18 8:43 AM, Christoph Hellwig wrote: > On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote: >> On 10/18/18 7:18 AM, Ming Lei wrote: >>> Now we only check if DMA IO buffer is aligned to queue_dma_alignment() >>> for pass-through request, and it isn't done for normal IO request. >>> >>> Given the check has to be done on each bvec, it isn't efficient to add the >>> check in generic_make_request_checks(). >>> >>> This patch addes one WARN in blk_queue_split() for capturing this issue. >> >> I don't want to do this, because then we are forever doomed to >> have something that fully loops a bio at submission time. I >> absolutely hate the splitting we have and the need for it, >> hopefully it can go away for a subset of IOs at some point. > > It is just a WARN_ON - no one should rely on it, but it is a good > debug aid. And then we'll have it start triggering on random things because some drivers set random limits, that don't reflect reality at all... We've basically had this setting that some drivers set, but that we don't really look at except for mapping in user data. Those should be audited first before adding something like this. -- Jens Axboe
Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer
On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote: > On 10/18/18 7:18 AM, Ming Lei wrote: > > Now we only check if DMA IO buffer is aligned to queue_dma_alignment() > > for pass-through request, and it isn't done for normal IO request. > > > > Given the check has to be done on each bvec, it isn't efficient to add the > > check in generic_make_request_checks(). > > > > This patch addes one WARN in blk_queue_split() for capturing this issue. > > I don't want to do this, because then we are forever doomed to > have something that fully loops a bio at submission time. I > absolutely hate the splitting we have and the need for it, > hopefully it can go away for a subset of IOs at some point. It is just a WARN_ON - no one should rely on it, but it is a good debug aid.
Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 42a46744c11b..d2dbd508cb6d 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -174,6 +174,8 @@ static struct bio *blk_bio_segment_split(struct
> request_queue *q,
> const unsigned max_sectors = get_max_io_size(q, bio);
>
> bio_for_each_segment(bv, bio, iter) {
> + WARN_ON_ONCE(queue_dma_alignment(q) & bv.bv_offset);
I'd write this the other way around, although I have no good argument
for that.
Otherwise this looks fine:
Reviewed-by: Christoph Hellwig
Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer
On 10/18/18 7:18 AM, Ming Lei wrote: > Now we only check if DMA IO buffer is aligned to queue_dma_alignment() > for pass-through request, and it isn't done for normal IO request. > > Given the check has to be done on each bvec, it isn't efficient to add the > check in generic_make_request_checks(). > > This patch addes one WARN in blk_queue_split() for capturing this issue. I don't want to do this, because then we are forever doomed to have something that fully loops a bio at submission time. I absolutely hate the splitting we have and the need for it, hopefully it can go away for a subset of IOs at some point. In many ways, this seems to be somewhat of a made-up problem, I don't recall a single bug report for something like this over decades of working with the IO stack. 512b alignment restrictions for DMA seems absolutely insane. I know people claim they exist, but clearly that isn't a hard requirement or we would have been boned years ago. -- Jens Axboe
[PATCH 1/5] block: warn on un-aligned DMA IO buffer
Now we only check if DMA IO buffer is aligned to queue_dma_alignment()
for pass-through request, and it isn't done for normal IO request.
Given the check has to be done on each bvec, it isn't efficient to add the
check in generic_make_request_checks().
This patch addes one WARN in blk_queue_split() for capturing this issue.
Cc: Vitaly Kuznetsov
Cc: Dave Chinner
Cc: Linux FS Devel
Cc: Darrick J. Wong
Cc: x...@vger.kernel.org
Cc: Dave Chinner
Cc: Christoph Hellwig
Cc: Bart Van Assche
Cc: Matthew Wilcox
Signed-off-by: Ming Lei
---
block/blk-merge.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 42a46744c11b..d2dbd508cb6d 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -174,6 +174,8 @@ static struct bio *blk_bio_segment_split(struct
request_queue *q,
const unsigned max_sectors = get_max_io_size(q, bio);
bio_for_each_segment(bv, bio, iter) {
+ WARN_ON_ONCE(queue_dma_alignment(q) & bv.bv_offset);
+
/*
* If the queue doesn't support SG gaps and adding this
* offset would create a gap, disallow it.
--
2.9.5
