Re: DISABLE_CLUSTERING in scsi drivers
On 11/21/18 1:41 AM, Christoph Hellwig wrote:
> Hi all,
>
> you in the To list maintain or wrote SCSI drivers that set the
> DISABLE_CLUSTERING flag, which basically disable merges of any
> bio segments. We already have the actual max_segment size limit
> to say which length a segment should have, independent of merged
> or originally created, so this limit generally should rarely if
> ever be required, and mostly is an old cut an paste error.
>
> Can you go over your drivers and check if it could be removed?
>
I have tested changing this to ENABLE_CLUSTERING in iscsi_tcp.c. This
code is used for both the iscsi initiator and the target.
On the initiator side, there is no problem with enabling clustering of bios.
But on the target side, the code seems to assume that IOs do not cross
page boundaries, resulting in WARN_ON messages and failed IO when
ENABLE_CLUSTERING is set:
> [ 78.644595] RIP: 0010:copy_page_to_iter+0x1a6/0x2e0
> [ 78.644596] Code: 0c 24 48 98 49 89 ce 48 89 c2 49 29 c6 48 29 ca 4d 01 f4
> 48 01 d5 48 85 c0 0f 85 71 ff ff ff 48 85 ed 0f 84 68 ff ff ff eb b2 <0f> 0b
> 45 31 ed eb 8d bf 01 00 00 00 e8 d9 1e c5 ff 65 4c 8b 34 25
> [ 78.644596] RSP: 0018:a04b41eefbe0 EFLAGS: 00010206
> [ 78.644597] RAX: f7acc40de180 RBX: a04b41eefd80 RCX:
> 0028a014
> [ 78.644598] RDX: 2000 RSI: 1000 RDI:
> f7acc000
> [ 78.644598] RBP: f7acc40de180 R08: 8f1703786000 R09:
> 2000
> [ 78.644599] R10: 02c0 R11: 8f16b3f31400 R12:
>
> [ 78.644600] R13: 2000 R14: 0008 R15:
> 3800
> [ 78.644601] FS: () GS:8f1739c0()
> knlGS:
> [ 78.644601] CS: 0010 DS: ES: CR0: 80050033
> [ 78.644602] CR2: 7f0f54772000 CR3: 00012e670004 CR4:
> 003606f0
> [ 78.644624] DR0: DR1: DR2:
>
> [ 78.644625] DR3: DR6: fffe0ff0 DR7:
> 0400
> [ 78.644625] Call Trace:
> [ 78.644631] skb_copy_datagram_iter+0x16f/0x270
> [ 78.644633] tcp_recvmsg+0x223/0xc30
> [ 78.644637] inet_recvmsg+0x4b/0xe0
> [ 78.644644] iscsit_do_rx_data.constprop.9+0x89/0x110 [iscsi_target_mod]
> [ 78.644650] rx_data+0x58/0x70 [iscsi_target_mod]
> [ 78.644654] iscsit_get_rx_pdu+0xa7b/0xd20 [iscsi_target_mod]
> [ 78.644657] ? preempt_count_sub+0x43/0x50
> [ 78.644659] ? _raw_spin_unlock_irq+0x22/0x40
> [ 78.644663] ? iscsi_target_tx_thread+0x1d0/0x1d0 [iscsi_target_mod]
> [ 78.644667] ? iscsi_target_rx_thread+0x71/0xd0 [iscsi_target_mod]
> [ 78.644670] iscsi_target_rx_thread+0x71/0xd0 [iscsi_target_mod]
> [ 78.644672] kthread+0x116/0x130
> [ 78.644673] ? kthread_create_worker_on_cpu+0x40/0x40
> [ 78.644675] ret_from_fork+0x24/0x50
> [ 78.644678] ---[ end trace a0d6d5ab0e8625ee ]---
>
The problem seems to be in iov_iter.c:copy_page_to_iter(), where it
first calls page_copy_sane(), which makes sure the copy does not cross a
page boundary:
> static inline bool page_copy_sane(struct page *page, size_t offset, size_t n)
> {
> struct page *head = compound_head(page);
> size_t v = n + offset + page_address(page) - page_address(head);
>
> if (likely(n <= v && v <= (PAGE_SIZE << compound_order(head
> return true;
> WARN_ON(1);
> return false;
> }
I will submit a separate BUG email about this to the linux-scsi and
target-devel mailing lists, since it's not clear to me how to fix this.
In the mean time, the iscsi_tcp.c file still needs DISABLE_CLUSTERING.
--
Lee
"Deviants will be sacrificed to ensure group solidarity."
Re: DISABLE_CLUSTERING in scsi drivers
On Mon, 3 Dec 2018, Hannes Reinecke wrote: > As I said: I need to do PIO for the last two bytes of the data buffer. > For everything else DMA works nicely, it's just the last two bytes which > might be left over in the FIFO buffer under certain circumstances. I read the driver a few times already, thanks. > If you have an alternative suggestion without scsi_kmap_atomic_sg() I'd > be happy to convert it. I'm not trying to avoid scsi_kmap_atomic_sg(). Nevermind. -- > > Cheers, > > Hannes >
Re: DISABLE_CLUSTERING in scsi drivers
On 12/2/18 11:13 PM, Finn Thain wrote: On Sun, 2 Dec 2018, Hannes Reinecke wrote: On 12/2/18 10:21 PM, Finn Thain wrote: On Sun, 2 Dec 2018, Hannes Reinecke wrote: Well, that lone 'kmap' is due to a quirk/errata in the datasheet; essentially we have to PIO a lone byte out of the FIFO to clear it up. And this byte is technically still part of the SCSI data, so we need to stuff it onto the end of the actual data sg list. Which is what the kmap() thingie does. So it really shouldn't be affected by the clustering algorithm. Sorry, I don't follow. If it's dead code, can it be removed Oh, it's not dead code. It's required as per datasheet. If it's not, does it require DISABLE_CLUSTERING? No, not really. It just affects the very last byte of the sglist, so I can't really see how it should be affected by clustering. AIUI, the scsi_kmap_atomic_sg() call which you added to esp_scsi.c assumes that the sg list elements are page sized and page aligned. DISABLE_CLUSTERING provides that guarantee, but am53c974.c doesn't use it. Is this not a bug? What am I missing? As I said: I need to do PIO for the last two bytes of the data buffer. For everything else DMA works nicely, it's just the last two bytes which might be left over in the FIFO buffer under certain circumstances. If you have an alternative suggestion without scsi_kmap_atomic_sg() I'd be happy to convert it. Cheers, Hannes -- Dr. Hannes ReineckezSeries & Storage h...@suse.com +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
Re: DISABLE_CLUSTERING in scsi drivers
On Sun, 2 Dec 2018, Hannes Reinecke wrote: > On 12/2/18 10:21 PM, Finn Thain wrote: > > On Sun, 2 Dec 2018, Hannes Reinecke wrote: > > > > > Well, that lone 'kmap' is due to a quirk/errata in the datasheet; > > > essentially > > > we have to PIO a lone byte out of the FIFO to clear it up. > > > And this byte is technically still part of the SCSI data, so we need to > > > stuff it onto the end of the actual data sg list. Which is what the kmap() > > > thingie does. > > > So it really shouldn't be affected by the clustering algorithm. > > > > > > > Sorry, I don't follow. > > > > If it's dead code, can it be removed > > > Oh, it's not dead code. It's required as per datasheet. > > > If it's not, does it require DISABLE_CLUSTERING? > > > No, not really. It just affects the very last byte of the sglist, > so I can't really see how it should be affected by clustering. > AIUI, the scsi_kmap_atomic_sg() call which you added to esp_scsi.c assumes that the sg list elements are page sized and page aligned. DISABLE_CLUSTERING provides that guarantee, but am53c974.c doesn't use it. Is this not a bug? What am I missing? BTW, Documentation/block/biodoc.txt suggests a bounce buffer as an alternative. There are some situations when pages from high memory may need to be kmapped, even if bounce buffers are not necessary. For example a device may need to abort DMA operations and revert to PIO for the transfer, in which case a virtual mapping of the page is required. For SCSI it is also done in some scenarios where the low level driver cannot be trusted to handle a single sg entry correctly. The driver is expected to perform the kmaps as needed on such occasions as appropriate. A driver could also use the blk_queue_bounce() routine on its own to bounce highmem i/o to low memory for specific requests if so desired. -- > Cheers, > > Hannes > >
Re: DISABLE_CLUSTERING in scsi drivers
On 12/2/18 10:21 PM, Finn Thain wrote: On Sun, 2 Dec 2018, Hannes Reinecke wrote: Well, that lone 'kmap' is due to a quirk/errata in the datasheet; essentially we have to PIO a lone byte out of the FIFO to clear it up. And this byte is technically still part of the SCSI data, so we need to stuff it onto the end of the actual data sg list. Which is what the kmap() thingie does. So it really shouldn't be affected by the clustering algorithm. Sorry, I don't follow. If it's dead code, can it be removed Oh, it's not dead code. It's required as per datasheet. If it's not, does it require DISABLE_CLUSTERING? No, not really. It just affects the very last byte of the sglist, so I can't really see how it should be affected by clustering. Cheers, Hannes
Re: DISABLE_CLUSTERING in scsi drivers
On Sun, 2 Dec 2018, Hannes Reinecke wrote: > Well, that lone 'kmap' is due to a quirk/errata in the datasheet; essentially > we have to PIO a lone byte out of the FIFO to clear it up. > And this byte is technically still part of the SCSI data, so we need to > stuff it onto the end of the actual data sg list. Which is what the kmap() > thingie does. > So it really shouldn't be affected by the clustering algorithm. > Sorry, I don't follow. If it's dead code, can it be removed? If it's not, does it require DISABLE_CLUSTERING? -- > Cheers, > > Hannes >
Re: DISABLE_CLUSTERING in scsi drivers
On 11/26/18 10:46 AM, Finn Thain wrote: On Mon, 26 Nov 2018, Christoph Hellwig wrote: On Thu, Nov 22, 2018 at 09:02:13AM +1100, Finn Thain wrote: you in the To list maintain or wrote SCSI drivers that set the DISABLE_CLUSTERING flag, which basically disable merges of any bio segments. We already have the actual max_segment size limit to say which length a segment should have, independent of merged or originally created, so this limit generally should rarely if ever be required, and mostly is an old cut an paste error. Are you referring to blk_queue_max_segment_size(q, dma_get_max_seg_size(dev)); in drivers/scsi/scsi_lib.c? Is the segment size limitation of the DMA controller the only reason to want DISABLE_CLUSTERING? DISABLE_CLUSTERING mixes up two not really related things: 1) limit the size of each segment to a single page size 2) limit each segment to not actually span a page boundary. Both could be valid limit for DMA engines, but also might be particularly relevant for pio, if you e.g. kmap each page of a scatterlist do do pio you'd want to see both limits. I looked through all the drivers based on esp_scsi.c and NCR5380.c which use DISABLE_CLUSTERING. There is one driver that uses DMA and sometimes kmap too, which is am53c974.c. It defaults to ENABLE_CLUSTERING, which would seem to be a bug if kmap isn't compatible with that (Hannes?). Well, that lone 'kmap' is due to a quirk/errata in the datasheet; essentially we have to PIO a lone byte out of the FIFO to clear it up. And this byte is technically still part of the SCSI data, so we need to stuff it onto the end of the actual data sg list. Which is what the kmap() thingie does. So it really shouldn't be affected by the clustering algorithm. Cheers, Hannes
Re: DISABLE_CLUSTERING in scsi drivers
On Mon, Nov 26, 2018 at 08:46:39PM +1100, Finn Thain wrote: > On Mon, 26 Nov 2018, Christoph Hellwig wrote: > > > On Thu, Nov 22, 2018 at 09:02:13AM +1100, Finn Thain wrote: > > > > you in the To list maintain or wrote SCSI drivers that set the > > > > DISABLE_CLUSTERING flag, which basically disable merges of any > > > > bio segments. We already have the actual max_segment size limit > > > > to say which length a segment should have, independent of merged > > > > or originally created, so this limit generally should rarely if > > > > ever be required, and mostly is an old cut an paste error. > > > > > > > > > > Are you referring to > > > blk_queue_max_segment_size(q, dma_get_max_seg_size(dev)); > > > in drivers/scsi/scsi_lib.c? > > > > > > Is the segment size limitation of the DMA controller the only reason to > > > want DISABLE_CLUSTERING? > > > > DISABLE_CLUSTERING mixes up two not really related things: > > > > 1) limit the size of each segment to a single page size > > 2) limit each segment to not actually span a page boundary. > > > > Both could be valid limit for DMA engines, but also might be particularly > > relevant for pio, if you e.g. kmap each page of a scatterlist do do > > pio you'd want to see both limits. > > > > I looked through all the drivers based on esp_scsi.c and NCR5380.c which > use DISABLE_CLUSTERING. > > There is one driver that uses DMA and sometimes kmap too, which is > am53c974.c. It defaults to ENABLE_CLUSTERING, which would seem to be a bug > if kmap isn't compatible with that (Hannes?). > > For g_NCR5380.c (ISA bus) and dmx3191d.c (PCI bus) these drivers need > DISABLE_CLUSTERING because apparently they don't use kmap or DMA. This > will need to be addressed if you remove DISABLE_CLUSTERING. > > Then we have the ARM drivers, cumana_1.c and oak.c. I believe that ecard > drivers like these are only used with CONFIG_ARCH_RPC, and I think that > would imply !CONFIG_HIGHMEM (Russell?). > > m68k that doesn't seem to have CONFIG_HIGHMEM either, so no need for kmap > or DISABLE_CLUSTERING when doing PIO or PDMA. That includes mac_esp.c and > mac_scsi.c. Michael has already answered for atari_scsi.c. > > That leaves the Sun 3 drivers, sun3_scsi.c and sun3_scsi_vme.c, which may > have DMA limitations I don't know about (Sam?). I don't believe the Sun3 DMA has limitations which require DISABLE_CLUSTERING as described above. I'm going to go with the "old cut and paste error" theory as being most likely. -- Sam
Re: DISABLE_CLUSTERING in scsi drivers
On Mon, 26 Nov 2018, Christoph Hellwig wrote: > On Thu, Nov 22, 2018 at 09:02:13AM +1100, Finn Thain wrote: > > > you in the To list maintain or wrote SCSI drivers that set the > > > DISABLE_CLUSTERING flag, which basically disable merges of any > > > bio segments. We already have the actual max_segment size limit > > > to say which length a segment should have, independent of merged > > > or originally created, so this limit generally should rarely if > > > ever be required, and mostly is an old cut an paste error. > > > > > > > Are you referring to > > blk_queue_max_segment_size(q, dma_get_max_seg_size(dev)); > > in drivers/scsi/scsi_lib.c? > > > > Is the segment size limitation of the DMA controller the only reason to > > want DISABLE_CLUSTERING? > > DISABLE_CLUSTERING mixes up two not really related things: > > 1) limit the size of each segment to a single page size > 2) limit each segment to not actually span a page boundary. > > Both could be valid limit for DMA engines, but also might be particularly > relevant for pio, if you e.g. kmap each page of a scatterlist do do > pio you'd want to see both limits. > I looked through all the drivers based on esp_scsi.c and NCR5380.c which use DISABLE_CLUSTERING. There is one driver that uses DMA and sometimes kmap too, which is am53c974.c. It defaults to ENABLE_CLUSTERING, which would seem to be a bug if kmap isn't compatible with that (Hannes?). For g_NCR5380.c (ISA bus) and dmx3191d.c (PCI bus) these drivers need DISABLE_CLUSTERING because apparently they don't use kmap or DMA. This will need to be addressed if you remove DISABLE_CLUSTERING. Then we have the ARM drivers, cumana_1.c and oak.c. I believe that ecard drivers like these are only used with CONFIG_ARCH_RPC, and I think that would imply !CONFIG_HIGHMEM (Russell?). m68k that doesn't seem to have CONFIG_HIGHMEM either, so no need for kmap or DISABLE_CLUSTERING when doing PIO or PDMA. That includes mac_esp.c and mac_scsi.c. Michael has already answered for atari_scsi.c. That leaves the Sun 3 drivers, sun3_scsi.c and sun3_scsi_vme.c, which may have DMA limitations I don't know about (Sam?). --
Re: DISABLE_CLUSTERING in scsi drivers
Am 26.11.2018 um 20:50 schrieb Christoph Hellwig: On Thu, Nov 22, 2018 at 10:11:33AM +1300, Michael Schmitz wrote: Christoph, for Atari SCSI, commands can only be merged if the physical addresses of all buffers are contiguous (limitation of the Falcon DMA engine). Documentation/scsi/scsi_mid_low_api.tx does not spell out whether that is the case. Atari SCSI disables scatter/gather, so if that's sufficient to cue midlevel or bio to not undertake any merging, the flag is no longer needed. Yes, if scatter/gather is disable (sg_tablesize == 1 or 0), there will just be a single, contiguos segment up to .max_sectors, which might straddle a page boundary if it is larger than PAGE_SIZE. If that is ok for the ata SCSI hardware we can remove the DISABLE_CLUSTERING setting. No requirements for DMA segments to be page aligned or not cross a page boundary - max. 255 sectors for Falcon (and the driver will fall back to PIO if that's exceeded IIRC). No problem removing the setting - probably best if Finn queues that change for all NCR5380 drivers that can cope with it. Cheers, Michael
Re: DISABLE_CLUSTERING in scsi drivers
On Thu, Nov 22, 2018 at 10:11:33AM +1300, Michael Schmitz wrote: > Christoph, > for Atari SCSI, commands can only be merged if the physical addresses > of all buffers are contiguous (limitation of the Falcon DMA engine). > Documentation/scsi/scsi_mid_low_api.tx does not spell out whether that > is the case. > > Atari SCSI disables scatter/gather, so if that's sufficient to cue > midlevel or bio to not undertake any merging, the flag is no longer > needed. Yes, if scatter/gather is disable (sg_tablesize == 1 or 0), there will just be a single, contiguos segment up to .max_sectors, which might straddle a page boundary if it is larger than PAGE_SIZE. If that is ok for the ata SCSI hardware we can remove the DISABLE_CLUSTERING setting.
Re: DISABLE_CLUSTERING in scsi drivers
On Thu, Nov 22, 2018 at 09:02:13AM +1100, Finn Thain wrote: > > you in the To list maintain or wrote SCSI drivers that set the > > DISABLE_CLUSTERING flag, which basically disable merges of any > > bio segments. We already have the actual max_segment size limit > > to say which length a segment should have, independent of merged > > or originally created, so this limit generally should rarely if > > ever be required, and mostly is an old cut an paste error. > > > > Are you referring to > blk_queue_max_segment_size(q, dma_get_max_seg_size(dev)); > in drivers/scsi/scsi_lib.c? > > Is the segment size limitation of the DMA controller the only reason to > want DISABLE_CLUSTERING? DISABLE_CLUSTERING mixes up two not really related things: 1) limit the size of each segment to a single page size 2) limit each segment to not actually span a page boundary. Both could be valid limit for DMA engines, but also might be particularly relevant for pio, if you e.g. kmap each page of a scatterlist do do pio you'd want to see both limits.
Re: DISABLE_CLUSTERING in scsi drivers
On Fri, Nov 23, 2018 at 09:09:49AM +0100, Juergen Gross wrote: > On 21/11/2018 10:41, Christoph Hellwig wrote: > > Hi all, > > > > you in the To list maintain or wrote SCSI drivers that set the > > DISABLE_CLUSTERING flag, which basically disable merges of any > > bio segments. We already have the actual max_segment size limit > > to say which length a segment should have, independent of merged > > or originally created, so this limit generally should rarely if > > ever be required, and mostly is an old cut an paste error. > > > > Can you go over your drivers and check if it could be removed? > > > > xen-scsifront.c doesn't need it. Do you want me to remove it at once > or are you doing it when removing the support for it? If it is a plain removal please queue it up yourself. Eventually I plan to do a bulk replacement, but that will take a while.
Re: DISABLE_CLUSTERING in scsi drivers
On 21/11/2018 10:41, Christoph Hellwig wrote: > Hi all, > > you in the To list maintain or wrote SCSI drivers that set the > DISABLE_CLUSTERING flag, which basically disable merges of any > bio segments. We already have the actual max_segment size limit > to say which length a segment should have, independent of merged > or originally created, so this limit generally should rarely if > ever be required, and mostly is an old cut an paste error. > > Can you go over your drivers and check if it could be removed? > xen-scsifront.c doesn't need it. Do you want me to remove it at once or are you doing it when removing the support for it? Juergen
Re: DISABLE_CLUSTERING in scsi drivers
On Wed, 21 Nov 2018, Christoph Hellwig wrote: > Hi all, > > you in the To list maintain or wrote SCSI drivers that set the > DISABLE_CLUSTERING flag, which basically disable merges of any > bio segments. We already have the actual max_segment size limit > to say which length a segment should have, independent of merged > or originally created, so this limit generally should rarely if > ever be required, and mostly is an old cut an paste error. > Are you referring to blk_queue_max_segment_size(q, dma_get_max_seg_size(dev)); in drivers/scsi/scsi_lib.c? Is the segment size limitation of the DMA controller the only reason to want DISABLE_CLUSTERING? > Can you go over your drivers and check if it could be removed? > Adding Sun 3 maintainer to Cc. Besides sun3_scsi.c and atari_scsi.c, the SCSI drivers I take an interest in don't actually use a DMA controller. If my question about DMA controllers gets a "yes" then I guess I can send a patch for those drivers. --
Re: DISABLE_CLUSTERING in scsi drivers
Christoph, for Atari SCSI, commands can only be merged if the physical addresses of all buffers are contiguous (limitation of the Falcon DMA engine). Documentation/scsi/scsi_mid_low_api.tx does not spell out whether that is the case. Atari SCSI disables scatter/gather, so if that's sufficient to cue midlevel or bio to not undertake any merging, the flag is no longer needed. Cheers, Michael On Wed, Nov 21, 2018 at 10:41 PM Christoph Hellwig wrote: > > Hi all, > > you in the To list maintain or wrote SCSI drivers that set the > DISABLE_CLUSTERING flag, which basically disable merges of any > bio segments. We already have the actual max_segment size limit > to say which length a segment should have, independent of merged > or originally created, so this limit generally should rarely if > ever be required, and mostly is an old cut an paste error. > > Can you go over your drivers and check if it could be removed?
DISABLE_CLUSTERING in scsi drivers
Hi all, you in the To list maintain or wrote SCSI drivers that set the DISABLE_CLUSTERING flag, which basically disable merges of any bio segments. We already have the actual max_segment size limit to say which length a segment should have, independent of merged or originally created, so this limit generally should rarely if ever be required, and mostly is an old cut an paste error. Can you go over your drivers and check if it could be removed?
