Re: ips.c broken since 2.6.23 on x86_64?
On Feb 18, 2008 4:11 PM, FUJITA Tomonori <[EMAIL PROTECTED]> wrote: > Can you please help me just once more? 2.6.25-rc2 fixed this bug in a > bit different way by chance. Please test 2.6.25-rc2 with the attached > patch to make sure that ips in 2.6.25 works well. Confirmed...the patch below against 2.6.25-rc2 also works for me. Thank you again Fujita-san! > > diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c > index bb152fb..429592a 100644 > --- a/drivers/scsi/ips.c > +++ b/drivers/scsi/ips.c > @@ -1576,7 +1576,7 @@ ips_make_passthru(ips_ha_t *ha, struct scsi_cmnd *SC, > ips_scb_t *scb, int intr) > METHOD_TRACE("ips_make_passthru", 1); > > scsi_for_each_sg(SC, sg, scsi_sg_count(SC), i) > -length += sg[i].length; > +length += sg->length; > > if (length < sizeof (ips_passthru_t)) { > /* wrong size */ > @@ -3510,15 +3510,16 @@ ips_scmd_buf_write(struct scsi_cmnd *scmd, void > *data, unsigned int count) > struct scatterlist *sg = scsi_sglist(scmd); > > for (i = 0, xfer_cnt = 0; > - (i < scsi_sg_count(scmd)) && (xfer_cnt < count); i++) { > -min_cnt = min(count - xfer_cnt, sg[i].length); > + (i < scsi_sg_count(scmd)) && (xfer_cnt < count); > + i++, sg = sg_next(sg)) { > +min_cnt = min(count - xfer_cnt, sg->length); > > /* kmap_atomic() ensures addressability of the data buffer.*/ > /* local_irq_save() protects the KM_IRQ0 address slot. */ > local_irq_save(flags); > -buffer = kmap_atomic(sg_page(&sg[i]), KM_IRQ0) + > sg[i].offset; > +buffer = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset; > memcpy(buffer, &cdata[xfer_cnt], min_cnt); > -kunmap_atomic(buffer - sg[i].offset, KM_IRQ0); > +kunmap_atomic(buffer - sg->offset, KM_IRQ0); > local_irq_restore(flags); > > xfer_cnt += min_cnt; > @@ -3543,15 +3544,16 @@ ips_scmd_buf_read(struct scsi_cmnd *scmd, void *data, > unsigned int count) > struct scatterlist *sg = scsi_sglist(scmd); > > for (i = 0, xfer_cnt = 0; > - (i < scsi_sg_count(scmd)) && (xfer_cnt < count); i++) { > -min_cnt = min(count - xfer_cnt, sg[i].length); > + (i < scsi_sg_count(scmd)) && (xfer_cnt < count); > + i++, sg = sg_next(sg)) { > +min_cnt = min(count - xfer_cnt, sg->length); > > /* kmap_atomic() ensures addressability of the data buffer.*/ > /* local_irq_save() protects the KM_IRQ0 address slot. */ > local_irq_save(flags); > -buffer = kmap_atomic(sg_page(&sg[i]), KM_IRQ0) + > sg[i].offset; > +buffer = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset; > memcpy(&cdata[xfer_cnt], buffer, min_cnt); > -kunmap_atomic(buffer - sg[i].offset, KM_IRQ0); > +kunmap_atomic(buffer - sg->offset, KM_IRQ0); > local_irq_restore(flags); > > xfer_cnt += min_cnt; - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ips.c broken since 2.6.23 on x86_64?
On Mon, 18 Feb 2008 15:30:58 -0800 Tim Pepper <[EMAIL PROTECTED]> wrote: > On Mon 18 Feb at 22:32:46 +0900 [EMAIL PROTECTED] said: > > > > diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c > > index 05bb6ea..39cdd68 100644 > > --- a/drivers/scsi/ips.c > > +++ b/drivers/scsi/ips.c > > @@ -6906,7 +6906,7 @@ ips_register_scsi(int index) > > sh->max_channel = ha->nbus - 1; > > sh->can_queue = ha->max_cmds - 1; > > > > - scsi_add_host(sh, NULL); > > + scsi_add_host(sh, &ha->pcidev->dev); > > scsi_scan_host(sh); > > > > return 0; > > Fujita-san, > > This applies and runs well on top of your 0005 patch! The rest of the > patches also then apply in order and run successfully. Great, thanks a lot! > Just to confirm, I applied the above alone to a clean 2.6.24 and things > again build and run successfully. For completeness I also reproduced > the problem against 2.6.23.16 and verified the above patch fixes on that > kernel version as well. Nice. There is another bug on 2.6.24 but we rarely hit this so 2.6.24 works most of the time: http://marc.info/?l=linux-scsi&m=120303487528875&w=2 > Assuming this patch is accepted for 2.6.25, please also queue it for > the 2.6.23/24 stable trees. Yes, I will take care about it. Can you please help me just once more? 2.6.25-rc2 fixed this bug in a bit different way by chance. Please test 2.6.25-rc2 with the attached patch to make sure that ips in 2.6.25 works well. > Thank you very much for your help in tracking this issue down! No problem. I should have fixed it long time ago. Really sorry about it. diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c index bb152fb..429592a 100644 --- a/drivers/scsi/ips.c +++ b/drivers/scsi/ips.c @@ -1576,7 +1576,7 @@ ips_make_passthru(ips_ha_t *ha, struct scsi_cmnd *SC, ips_scb_t *scb, int intr) METHOD_TRACE("ips_make_passthru", 1); scsi_for_each_sg(SC, sg, scsi_sg_count(SC), i) -length += sg[i].length; +length += sg->length; if (length < sizeof (ips_passthru_t)) { /* wrong size */ @@ -3510,15 +3510,16 @@ ips_scmd_buf_write(struct scsi_cmnd *scmd, void *data, unsigned int count) struct scatterlist *sg = scsi_sglist(scmd); for (i = 0, xfer_cnt = 0; - (i < scsi_sg_count(scmd)) && (xfer_cnt < count); i++) { -min_cnt = min(count - xfer_cnt, sg[i].length); + (i < scsi_sg_count(scmd)) && (xfer_cnt < count); + i++, sg = sg_next(sg)) { +min_cnt = min(count - xfer_cnt, sg->length); /* kmap_atomic() ensures addressability of the data buffer.*/ /* local_irq_save() protects the KM_IRQ0 address slot. */ local_irq_save(flags); -buffer = kmap_atomic(sg_page(&sg[i]), KM_IRQ0) + sg[i].offset; +buffer = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset; memcpy(buffer, &cdata[xfer_cnt], min_cnt); -kunmap_atomic(buffer - sg[i].offset, KM_IRQ0); +kunmap_atomic(buffer - sg->offset, KM_IRQ0); local_irq_restore(flags); xfer_cnt += min_cnt; @@ -3543,15 +3544,16 @@ ips_scmd_buf_read(struct scsi_cmnd *scmd, void *data, unsigned int count) struct scatterlist *sg = scsi_sglist(scmd); for (i = 0, xfer_cnt = 0; - (i < scsi_sg_count(scmd)) && (xfer_cnt < count); i++) { -min_cnt = min(count - xfer_cnt, sg[i].length); + (i < scsi_sg_count(scmd)) && (xfer_cnt < count); + i++, sg = sg_next(sg)) { +min_cnt = min(count - xfer_cnt, sg->length); /* kmap_atomic() ensures addressability of the data buffer.*/ /* local_irq_save() protects the KM_IRQ0 address slot. */ local_irq_save(flags); -buffer = kmap_atomic(sg_page(&sg[i]), KM_IRQ0) + sg[i].offset; +buffer = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset; memcpy(&cdata[xfer_cnt], buffer, min_cnt); -kunmap_atomic(buffer - sg[i].offset, KM_IRQ0); +kunmap_atomic(buffer - sg->offset, KM_IRQ0); local_irq_restore(flags); xfer_cnt += min_cnt; - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ips.c broken since 2.6.23 on x86_64?
On Mon 18 Feb at 06:57:14 -0800 [EMAIL PROTECTED] said: > The path needs to be triggered, it is the path to handle spoofing > of the Adapter's inquiry. > > You need more printk instrumentation to determine *why* it is not > reaching that code path. What is the result of scb->scsi_cmd. > scb->bus, ips_is_passthru(scb->scsi_cmd)? > > The sg breakup issue may need to be looked at, but keep in mind > the driver is going down a path that was not intended. Mark, Fujita Tomonori has noted the following: - scsi_add_host(sh, NULL); + scsi_add_host(sh, &ha->pcidev->dev); which appears to fix things for me. -- Tim Pepper <[EMAIL PROTECTED]> IBM Linux Technology Center - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ips.c broken since 2.6.23 on x86_64?
On Mon 18 Feb at 22:32:46 +0900 [EMAIL PROTECTED] said: > > diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c > index 05bb6ea..39cdd68 100644 > --- a/drivers/scsi/ips.c > +++ b/drivers/scsi/ips.c > @@ -6906,7 +6906,7 @@ ips_register_scsi(int index) > sh->max_channel = ha->nbus - 1; > sh->can_queue = ha->max_cmds - 1; > > - scsi_add_host(sh, NULL); > + scsi_add_host(sh, &ha->pcidev->dev); > scsi_scan_host(sh); > > return 0; Fujita-san, This applies and runs well on top of your 0005 patch! The rest of the patches also then apply in order and run successfully. Just to confirm, I applied the above alone to a clean 2.6.24 and things again build and run successfully. For completeness I also reproduced the problem against 2.6.23.16 and verified the above patch fixes on that kernel version as well. Assuming this patch is accepted for 2.6.25, please also queue it for the 2.6.23/24 stable trees. Thank you very much for your help in tracking this issue down! -- Tim Pepper <[EMAIL PROTECTED]> IBM Linux Technology Center - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Device rescan does not find all new devices
Markus Naeher <[EMAIL PROTECTED]> wrote: > The missing disk is always the first one (LUN 000). I have tested this by > changing > the order of the disks on ESS 2. > I have also repeated the test scenario with only one path per disk. > In this testcase, I have divided the ESS's on the two Adapters (Adapter 1 > accesses > the disks on ESS 1, Adapter 2 those on ESS 2). > Sadly, with no change in the result. > > I think the first thing I would need help with is how to further analyze > the problem. > Output from dmesg with SCSI scan logging on would be helpful. You can use ./scsi_logging_level -s --scan 7 with the script (let me know if you cannot locate the script) or you can adjust the logging by hand. -andmike -- Michael Anderson [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gdth new set of patches for 2.6.24 stable
On Mon, Feb 18, 2008 at 04:57:36AM -0800, Andrew Morton wrote: > On Sun, 17 Feb 2008 18:46:03 +0200 Boaz Harrosh <[EMAIL PROTECTED]> wrote: > > > ... > > > > All my testers have reported back that with these 5 patches applied they can > > now run with a 2.6.24 kernel the same way they ran before. However there is > > that reported issue, with the dma_free_coherent WARN_ON (above). The code > > was > > like that from day one and it is a very old issue, however it is a > > regression > > because 2.6.24 introduced that new WARN_ON. > > (infamous commit aa24886e379d2b641c5117e178b15ce1d5d366ba) > > >From posts on lkml and even recent one in linux-scsi about the arcmsr > > >driver > > it looks that all a driver can do is work around it with different kernel > > mechanisms > > and driver rewrites. I'm afraid I need your help here. I'm not sure I > > understand > > why does the gdth driver uses the pci_{alloc,free}_consistent() API's, and > > what > > is needed to replace it. Could you please have a look in gdth_proc.c and > > also in > > gdth.c for all the places that call gdth_ioctl_alloc/gdth_ioctl_free, and > > advise > > what can I do in it's place. Please bear in mind that we need it for > > 2.6.24, as > > a bugfix. > > > > Apart from the above issue, please accept patches 3,4,5 above they have now > > been tested and are reported to bring broken system back to production. > > (Given that you approve off course). And mark them for inclusion to the > > 2.6.24 stable releases. (Or is there some thing that I should do) > > > > --- > > Meanwhile on x86 systems I understand the WARN_ON is cosmetic, and does not > > pose any harm. Some people have reported stability with temporarily > > disabling > > it. For testers that want to try, here it is below. At your own risk. > > > > --- > > >From 50d3657bf6a138ee63ad1ce00052380edc75ace7 Mon Sep 17 00:00:00 2001 > > From: Boaz Harrosh <[EMAIL PROTECTED]> > > Date: Sun, 17 Feb 2008 12:49:35 +0200 > > Subject: [PATCH] gdth: Hack to remove WARN_ON in > > arch/x86/kernel/pci-dma_32.c > > > > gdth uses dma_free_coherent() with interrupts disabled. Which > > is not portable, but is safe on the HW that supports gdth. > > > > NOT Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]> > > --- > > arch/x86/kernel/pci-dma_32.c |2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/arch/x86/kernel/pci-dma_32.c b/arch/x86/kernel/pci-dma_32.c > > index 5133032..350dcfd 100644 > > --- a/arch/x86/kernel/pci-dma_32.c > > +++ b/arch/x86/kernel/pci-dma_32.c > > @@ -63,7 +63,7 @@ void dma_free_coherent(struct device *dev, size_t size, > > struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL; > > int order = get_order(size); > > > > - WARN_ON(irqs_disabled()); /* for portability */ > > +/* WARN_ON(irqs_disabled());*/ /* for portability */ > > if (mem && vaddr >= mem->virt_base && vaddr < (mem->virt_base + > > (mem->size << PAGE_SHIFT))) { > > int page = (vaddr - mem->virt_base) >> PAGE_SHIFT; > > > > Yes. Let's reprise aa24886e379d2b641c5117e178b15ce1d5d366ba: > > : commit aa24886e379d2b641c5117e178b15ce1d5d366ba > : Author: David Brownell <[EMAIL PROTECTED]> > : Date: Fri Aug 10 13:10:27 2007 -0700 > : > : dma_free_coherent() needs irqs enabled (sigh) > : > : On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish > : call context requirement: unlike its dma_alloc_coherent() sibling, it > may > : not be called with IRQs disabled. (This was new behavior on ARM as of > late > : 2005, caused by ARM SMP updates.) This little surprise can be annoyingly > : driver-visible. > : > : Since it looks like that restriction won't be removed, this patch > changes > : the definition of the API to include that requirement. Also, to help > catch > : nonportable drivers, it updates the x86 and swiotlb versions to include > the > : relevant warnings. (I already observed that it trips on the > : bus_reset_tasklet of the new firewire_ohci driver.) > : > > In general, all Linux memory-freeing functions can be called from all > contexts. (vfree is an irritating exception). This is good, and provides > maximum usefulness to callees, as all utility functions should seek to do. > It would be best to fix arm and mips. > > But arm and mips require enabled local irqs because their > dma_free_coherent() needs to do a cross-cpu IPI call. Presumably because > of certain unusual TLB protocols. > > I'm not sure what we should do about this. Presumably the gdth-on-arm > usage base is, umm, zero, so we could lamely add > CONFIG_DMA_FREE_COHERENT_WITH_LOCAL_IRQS_DISABLED_IS_OK and then use that > to disable gdth (and similar) on arm amd mips. But ugh. > > Russell, Ralf: is there something we can do here to relax this requirement? The current MIPS implementation of dma_alloc_coherent / dma_free_coherent is a glorified wrapper around __get
Re: gdth new set of patches for 2.6.24 stable
On Monday 18 February 2008, Andrew Morton wrote: > Russell, Ralf: is there something we can do here to relax this requirement? > > I'm thinking that perhaps we can do some rcu/refcounting tricks: launch the > IPI from within dma_free_coherent(), but don't wait for it to complete. > When all CPUs have handled the IPI then (and only then) the virtual address > becomes recyclable, or something like that? Or the trick some drivers had to do: just defer the actual work of dma_free_coherent() into a tasklet. Better have one such tasklet in arch code than N of them in drivers. To be clear: I never thought that API restriction was a good idea. (Although this discussion now seems moot for the gdth driver.) > > > Actually I think David might have been wrong about mips. afaict its > dma_free_coherent() is callable under local_irq_disable(), so ARM SMP is > the sole exception? All I recall at this point was getting some arch-specific patches in that area; I thought it was MIPS, maybe it was PPC. The arch code may have changed since then too. - Dave - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3 ver2] iscsi bidi & varlen support
On Mon, Feb 18 2008 at 19:22 +0200, James Bottomley <[EMAIL PROTECTED]> wrote: > On Mon, 2008-02-18 at 17:08 +0200, Boaz Harrosh wrote: >> But ... James? is >> there any chance these can go into scsi-rc-fixes for the 2.6.25 >> kernel? The reason they are so late was mainly because of a fallout >> in the merge process and a bug that was introduced because of that, >> but they were intended to go together with bidi into 2.6.25. Also >> as an important client code to the bidi-api that is introduced in >> 2.6.25 kernel. > > Well, I think you know the answer to that one under Linus' rules for non > merge window submission. It's not a bug fix; we haven't even put it > into -mm for testing and it's a pretty invasive change. > > James > > It was extensively tested by all iscsi people. It has the Sign-off-by of the iscsi maintainer. They are not new patches. But, yes you are right. I now remember the trouble we had with Linus last time. So it's 2.6.26 then. :-( . People that need it for 2.6.25 will just get it off the git tree. Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bsg: bidi bio map failure fix
On Mon, Feb 18 2008, James Bottomley wrote: > On Mon, 2008-02-18 at 15:46 +0100, Jens Axboe wrote: > > Seems symmetric to me now, either we fail and everything is cleaned up, > > or return success. What remains? > > My main symmetry complaint was the API: The map takes a request, the > unmap takes a bio. Oh that, well that was done to make it easier for existing users. Feel free to send a patch changing that, however it was done that way on purpose at the time being. -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3 ver2] iscsi bidi & varlen support
On Mon, 2008-02-18 at 17:08 +0200, Boaz Harrosh wrote: > But ... James? is > there any chance these can go into scsi-rc-fixes for the 2.6.25 > kernel? The reason they are so late was mainly because of a fallout > in the merge process and a bug that was introduced because of that, > but they were intended to go together with bidi into 2.6.25. Also > as an important client code to the bidi-api that is introduced in > 2.6.25 kernel. Well, I think you know the answer to that one under Linus' rules for non merge window submission. It's not a bug fix; we haven't even put it into -mm for testing and it's a pretty invasive change. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 2.6.25-rc2-mm1 (cciss build error)
On Sat, 16 Feb 2008 00:25:22 -0800 Andrew Morton wrote: > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.25-rc2/2.6.25-rc2-mm1/ cciss driver has a bad macro definition: #else /* no CONFIG_CISS_SCSI_TAPE */ /* If no tape support, then these become defined out of existence */ #define cciss_scsi_setup(cntl_num) #define cciss_unregister_scsi(ctlr) #define cciss_register_scsi(ctlr) #define cciss_seq_tape_report(struct seq_file *seq, int ctlr) #endif /* CONFIG_CISS_SCSI_TAPE */ which causes this error: In file included from /local/linsrc/linux-2.6.25-rc2-mm1/drivers/block/cciss.c:231: /local/linsrc/linux-2.6.25-rc2-mm1/drivers/block/cciss_scsi.c:1498:38: error: macro parameters must be comma-separated make[3]: *** [drivers/block/cciss.o] Error 1 --- ~Randy - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: aic94xx: failing on high load (another data point)
On Mon, 2008-02-18 at 22:26 +0800, Keith Hopkins wrote: > Well, that made life interesting > but didn't seem to fix anything. > > The behavior is about the same as before, but with more verbose > errors. I failed one member of the raid and had it rebuild as a > test...which hangs for a while and the drive falls off-line. Actually, it now finds the task and tries to do error handling for it ... so we've now uncovered bugs in the error handler. It may not look like it, but this is actually progress. Although, I'm afraid it's going to be a bit like peeling an onion: every time one error gets fixed, you just get to the next layer of errors. > Please grab the dmesg output in all its gory glory from here: > http://wiki.hopnet.net/dokuwiki/lib/exe/fetch.php?media=myit:sas:dmesg-20080218-wpatch-fail.txt.gz > > The drive is a Dell OEM drive, but it's not in a Dell system. There > is at least one firmware (S527) upgrade for it, but the Dell loader > refuses to load it (because it isn't in a Dell system...) > Does anyone know a generic way to load a new firmware onto a SAS drive? The firmware upgrade tools are usually vendor specific, though because the format of the firmware file is vendor specific. Could you just put it in a dell box to upgrade? James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] iscsi bidi & varlen support
[EMAIL PROTECTED] wrote on Mon, 18 Feb 2008 17:39 +0200: > On Tue, Feb 12 2008 at 22:17 +0200, Pete Wyckoff <[EMAIL PROTECTED]> wrote: > > From: Pete Wyckoff <[EMAIL PROTECTED]> > > Subject: [PATCH] iscsi iser: varlen > > > > Handle variable-length CDBs in iSER. > > > > Signed-off-by: Pete Wyckoff <[EMAIL PROTECTED]> > > --- > > drivers/infiniband/ulp/iser/iscsi_iser.c |5 +++-- > > drivers/infiniband/ulp/iser/iscsi_iser.h |2 +- > > drivers/infiniband/ulp/iser/iser_initiator.c | 16 ++-- > > 3 files changed, 14 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c > > b/drivers/infiniband/ulp/iser/iscsi_iser.c > > index 5f2284d..9dfc310 100644 > > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > > @@ -401,7 +401,8 @@ iscsi_iser_session_create(struct iscsi_transport > > *iscsit, > > ctask = session->cmds[i]; > > iser_ctask = ctask->dd_data; > > ctask->hdr = (struct iscsi_cmd *)&iser_ctask->desc.iscsi_header; > > - ctask->hdr_max = sizeof(iser_ctask->desc.iscsi_header); > > + ctask->hdr_max = sizeof(iser_ctask->desc.iscsi_header) + > > +sizeof(iser_ctask->desc.hdrextbuf); > > } > > > > for (i = 0; i < session->mgmtpool_max; i++) { > > @@ -604,7 +605,7 @@ static struct iscsi_transport iscsi_iser_transport = { > > .host_template = &iscsi_iser_sht, > > .conndata_size = sizeof(struct iscsi_conn), > > .max_lun= ISCSI_ISER_MAX_LUN, > > - .max_cmd_len= ISCSI_ISER_MAX_CMD_LEN, > > + .max_cmd_len= 260, > > Same bug I had. .max_cmd_len is still char, before the varlen patch to > scsi-ml. > So it must be at most 252, Until that patch is introduced and it can return to > the correct 260 or better yet SCSI_MAX_VARLEN_CDB_SIZE. That also is only > defined in the scsi-ml varlen patch. Ah, that is unfortunate. > I'm afraid the varlen patches to block and scsi-ml are waiting because of > me. There are more things I need to check, before they can get approved. > > Once I do that, and varlen gets accepted, iSER and iscsi_tcp can go up > to 260 for the .max_cmd_len as they should. I will sit on these iser changes until we get core varlen resolved, then. Or you can just sequence it all cleverly through the various maintainers. -- Pete - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] iscsi bidi & varlen support
On Tue, Feb 12 2008 at 22:17 +0200, Pete Wyckoff <[EMAIL PROTECTED]> wrote: > [EMAIL PROTECTED] wrote on Tue, 12 Feb 2008 15:12 -0500: >> [EMAIL PROTECTED] wrote on Thu, 31 Jan 2008 20:08 +0200: >>> Cheers after 1.3 years these can go in. >>> >>> [PATCH 1/3] iscsi: extended cdb support >>>The varlen support is not yet in mainline for >>> block and scsi-ml. But the API for drivers will >>> not change. All LLD need to do is max_command to >>> the it's maximum and be ready for bigger commands. >>> This is what's done here. Once these commands start >>> coming iscsi will be ready for them. >>> >>> [PATCH 2/3] iscsi: bidi support - libiscsi >>> [PATCH 3/3] iscsi: bidi support - iscsi_tcp >>> bidirectional commands support in iscsi. >>> iSER is not yet ready, but it will not break. >>> There is already a mechanism in libiscsi that will >>> return error if bidi commands are sent iSER way. >>> >>> Pete please send me the iSER bits so we can port them >>> to this latest version. >>> >>> Mike these patches are ontop of iscs branch of the iscsi >>> git tree, they will apply but for compilation you will need >>> to sync with Linus mainline. The patches are for the in-tree >>> iscsi code. I own you the compat patch for the out-off-tree >>> code, but this I will only be Sunday. >> Here's the patch to add bidi support to iSER too. It works >> with my setup, but could use more testing. Note that this does >> rely on the 3 patches quoted above. > > Similar, for varlen support to iSER. Probably apply this one before > the bidi one I just sent. > > -- Pete > > > From: Pete Wyckoff <[EMAIL PROTECTED]> > Subject: [PATCH] iscsi iser: varlen > > Handle variable-length CDBs in iSER. > > Signed-off-by: Pete Wyckoff <[EMAIL PROTECTED]> > --- > drivers/infiniband/ulp/iser/iscsi_iser.c |5 +++-- > drivers/infiniband/ulp/iser/iscsi_iser.h |2 +- > drivers/infiniband/ulp/iser/iser_initiator.c | 16 ++-- > 3 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c > b/drivers/infiniband/ulp/iser/iscsi_iser.c > index 5f2284d..9dfc310 100644 > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > @@ -401,7 +401,8 @@ iscsi_iser_session_create(struct iscsi_transport *iscsit, > ctask = session->cmds[i]; > iser_ctask = ctask->dd_data; > ctask->hdr = (struct iscsi_cmd *)&iser_ctask->desc.iscsi_header; > - ctask->hdr_max = sizeof(iser_ctask->desc.iscsi_header); > + ctask->hdr_max = sizeof(iser_ctask->desc.iscsi_header) + > + sizeof(iser_ctask->desc.hdrextbuf); > } > > for (i = 0; i < session->mgmtpool_max; i++) { > @@ -604,7 +605,7 @@ static struct iscsi_transport iscsi_iser_transport = { > .host_template = &iscsi_iser_sht, > .conndata_size = sizeof(struct iscsi_conn), > .max_lun= ISCSI_ISER_MAX_LUN, > - .max_cmd_len= ISCSI_ISER_MAX_CMD_LEN, > + .max_cmd_len= 260, Same bug I had. .max_cmd_len is still char, before the varlen patch to scsi-ml. So it must be at most 252, Until that patch is introduced and it can return to the correct 260 or better yet SCSI_MAX_VARLEN_CDB_SIZE. That also is only defined in the scsi-ml varlen patch. > /* session management */ > .create_session = iscsi_iser_session_create, > .destroy_session= iscsi_session_teardown, > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h > b/drivers/infiniband/ulp/iser/iscsi_iser.h > index db8f81a..66905df 100644 > --- a/drivers/infiniband/ulp/iser/iscsi_iser.h > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.h > @@ -90,7 +90,6 @@ > /* FMR space for 1 MB of 4k-page transfers, plus 1 if not page aligned */ > #define ISCSI_ISER_SG_TABLESIZE (((1<<20) >> SHIFT_4K) + 1) > #define ISCSI_ISER_MAX_LUN 256 > -#define ISCSI_ISER_MAX_CMD_LEN 16 > > /* QP settings */ > /* Maximal bounds on received asynchronous PDUs */ > @@ -217,6 +216,7 @@ enum iser_desc_type { > struct iser_desc { > struct iser_hdr iser_header; > struct iscsi_hdr iscsi_header; > + char hdrextbuf[ISCSI_MAX_AHS_SIZE]; > struct iser_regd_buf hdr_regd_buf; > void *data; /* used by RX & TX_CONTROL > */ > struct iser_regd_buf data_regd_buf; /* used by RX & TX_CONTROL > */ > diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c > b/drivers/infiniband/ulp/iser/iser_initiator.c > index 83247f1..ea3f5dc 100644 > --- a/drivers/infiniband/ulp/iser/iser_initiator.c > +++ b/drivers/infiniband/ulp/iser/iser_initiator.c > @@ -246,9 +246,13 @@ post_rx_kmalloc_failure: > return err; > } > > -/* creates a new tx descriptor and adds header regd buffer */ > +/** >
[PATCH 3/3 ver2] iscsi: bidi support - iscsi_tcp
bidi support for iscsi_tcp - access the right scsi_in() and/or scsi_out() side of things. also for resid Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]> Reviewed-by: Pete Wyckoff <[EMAIL PROTECTED]> Signed-off-by: Mike Christie <[EMAIL PROTECTED]> --- drivers/scsi/iscsi_tcp.c | 31 +-- 1 files changed, 17 insertions(+), 14 deletions(-) diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index 8a17867..72b9b2a 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -528,6 +528,7 @@ iscsi_data_rsp(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask) struct iscsi_session *session = conn->session; struct scsi_cmnd *sc = ctask->sc; int datasn = be32_to_cpu(rhdr->datasn); + unsigned total_in_length = scsi_in(sc)->length; iscsi_update_cmdsn(session, (struct iscsi_nopin*)rhdr); if (tcp_conn->in.datalen == 0) @@ -542,10 +543,10 @@ iscsi_data_rsp(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask) tcp_ctask->exp_datasn++; tcp_ctask->data_offset = be32_to_cpu(rhdr->offset); - if (tcp_ctask->data_offset + tcp_conn->in.datalen > scsi_bufflen(sc)) { + if (tcp_ctask->data_offset + tcp_conn->in.datalen > total_in_length) { debug_tcp("%s: data_offset(%d) + data_len(%d) > total_length_in(%d)\n", __FUNCTION__, tcp_ctask->data_offset, - tcp_conn->in.datalen, scsi_bufflen(sc)); + tcp_conn->in.datalen, total_in_length); return ISCSI_ERR_DATA_OFFSET; } @@ -558,8 +559,8 @@ iscsi_data_rsp(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask) if (res_count > 0 && (rhdr->flags & ISCSI_FLAG_CMD_OVERFLOW || -res_count <= scsi_bufflen(sc))) - scsi_set_resid(sc, res_count); +res_count <= total_in_length)) + scsi_in(sc)->resid = res_count; else sc->result = (DID_BAD_TARGET << 16) | rhdr->cmd_status; @@ -670,11 +671,11 @@ iscsi_r2t_rsp(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask) r2t->data_length, session->max_burst); r2t->data_offset = be32_to_cpu(rhdr->data_offset); - if (r2t->data_offset + r2t->data_length > scsi_bufflen(ctask->sc)) { + if (r2t->data_offset + r2t->data_length > scsi_out(ctask->sc)->length) { iscsi_conn_printk(KERN_ERR, conn, "invalid R2T with data len %u at offset %u " "and total length %d\n", r2t->data_length, - r2t->data_offset, scsi_bufflen(ctask->sc)); + r2t->data_offset, scsi_out(ctask->sc)->length); __kfifo_put(tcp_ctask->r2tpool.queue, (void*)&r2t, sizeof(void*)); return ISCSI_ERR_DATALEN; @@ -771,6 +772,7 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr) if (tcp_conn->in.datalen) { struct iscsi_tcp_cmd_task *tcp_ctask = ctask->dd_data; struct hash_desc *rx_hash = NULL; + struct scsi_data_buffer *sdb = scsi_in(ctask->sc); /* * Setup copy of Data-In into the Scsi_Cmnd @@ -788,8 +790,8 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr) tcp_ctask->data_offset, tcp_conn->in.datalen); return iscsi_segment_seek_sg(&tcp_conn->in.segment, -scsi_sglist(ctask->sc), -scsi_sg_count(ctask->sc), +sdb->table.sgl, +sdb->table.nents, tcp_ctask->data_offset, tcp_conn->in.datalen, iscsi_tcp_process_data_in, @@ -1332,7 +1334,8 @@ iscsi_tcp_ctask_init(struct iscsi_cmd_task *ctask) return 0; /* If we have immediate data, attach a payload */ - err = iscsi_tcp_send_data_prep(conn, scsi_sglist(sc), scsi_sg_count(sc), + err = iscsi_tcp_send_data_prep(conn, scsi_out(sc)->table.sgl, + scsi_out(sc)->table.nents, 0, ctask->imm_count); if (err) return err; @@ -1386,6 +1389,7 @@ iscsi_tcp_ctask_xmit(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask) { struct iscsi_tcp_
Re: [PATCH] bsg: bidi bio map failure fix
On Mon, 18 Feb 2008 09:09:07 -0600 James Bottomley <[EMAIL PROTECTED]> wrote: > On Mon, 2008-02-18 at 15:46 +0100, Jens Axboe wrote: > > Seems symmetric to me now, either we fail and everything is cleaned up, > > or return success. What remains? > > My main symmetry complaint was the API: The map takes a request, the > unmap takes a bio. Yeah, it would be nice if we avoid such code: blk_rq_map_user(q, rq, ...) bio = rq->bio blk_execute_rq(q, ... blk_rq_unmap_user(bio); I think that none of the users of blk_rq_map_user is interested in bio, the details of how kernel manage I/Os. At least, we can remove bio stuff in bsg if blk_rq_map_user and blk_rq_unmap_user take requests. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3 ver2] iscsi: bidi support - libiscsi
iscsi bidi support at the generic libiscsi level - prepare the additional bidi_read rlength header. - access the right scsi_in() and/or scsi_out() side of things. also for resid. - Handle BIDI underflow overflow from target Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]> Reviewed-by: Pete Wyckoff <[EMAIL PROTECTED]> Signed-off-by: Mike Christie <[EMAIL PROTECTED]> --- drivers/scsi/libiscsi.c | 85 ++ 1 files changed, 70 insertions(+), 15 deletions(-) diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index a43b8ee..9c12915 100644 --- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -176,6 +176,31 @@ static int iscsi_prep_ecdb_ahs(struct iscsi_cmd_task *ctask) return 0; } +static int iscsi_prep_bidi_ahs(struct iscsi_cmd_task *ctask) +{ + struct scsi_cmnd *sc = ctask->sc; + struct iscsi_rlength_ahdr *rlen_ahdr; + int rc; + + rlen_ahdr = iscsi_next_hdr(ctask); + rc = iscsi_add_hdr(ctask, sizeof(*rlen_ahdr)); + if (rc) + return rc; + + rlen_ahdr->ahslength = + cpu_to_be16(sizeof(rlen_ahdr->read_length) + + sizeof(rlen_ahdr->reserved)); + rlen_ahdr->ahstype = ISCSI_AHSTYPE_RLENGTH; + rlen_ahdr->reserved = 0; + rlen_ahdr->read_length = cpu_to_be32(scsi_in(sc)->length); + + debug_scsi("bidi-in rlen_ahdr->read_length(%d) " + "rlen_ahdr->ahslength(%d)\n", + be32_to_cpu(rlen_ahdr->read_length), + be16_to_cpu(rlen_ahdr->ahslength)); + return 0; +} + /** * iscsi_prep_scsi_cmd_pdu - prep iscsi scsi cmd pdu * @ctask: iscsi cmd task @@ -200,7 +225,6 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask) hdr->flags = ISCSI_ATTR_SIMPLE; int_to_scsilun(sc->device->lun, (struct scsi_lun *)hdr->lun); hdr->itt = build_itt(ctask->itt, session->age); - hdr->data_length = cpu_to_be32(scsi_bufflen(sc)); hdr->cmdsn = cpu_to_be32(session->cmdsn); session->cmdsn++; hdr->exp_statsn = cpu_to_be32(conn->exp_statsn); @@ -216,7 +240,15 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask) memcpy(hdr->cdb, sc->cmnd, cmd_len); ctask->imm_count = 0; + if (scsi_bidi_cmnd(sc)) { + hdr->flags |= ISCSI_FLAG_CMD_READ; + rc = iscsi_prep_bidi_ahs(ctask); + if (rc) + return rc; + } if (sc->sc_data_direction == DMA_TO_DEVICE) { + unsigned out_len = scsi_out(sc)->length; + hdr->data_length = cpu_to_be32(out_len); hdr->flags |= ISCSI_FLAG_CMD_WRITE; /* * Write counters: @@ -237,19 +269,19 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask) ctask->unsol_datasn = 0; if (session->imm_data_en) { - if (scsi_bufflen(sc) >= session->first_burst) + if (out_len >= session->first_burst) ctask->imm_count = min(session->first_burst, conn->max_xmit_dlength); else - ctask->imm_count = min(scsi_bufflen(sc), + ctask->imm_count = min(out_len, conn->max_xmit_dlength); hton24(hdr->dlength, ctask->imm_count); } else zero_data(hdr->dlength); if (!session->initial_r2t_en) { - ctask->unsol_count = min((session->first_burst), - (scsi_bufflen(sc))) - ctask->imm_count; + ctask->unsol_count = min(session->first_burst, out_len) +- ctask->imm_count; ctask->unsol_offset = ctask->imm_count; } @@ -259,6 +291,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask) } else { hdr->flags |= ISCSI_FLAG_CMD_FINAL; zero_data(hdr->dlength); + hdr->data_length = cpu_to_be32(scsi_in(sc)->length); if (sc->sc_data_direction == DMA_FROM_DEVICE) hdr->flags |= ISCSI_FLAG_CMD_READ; @@ -277,10 +310,12 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask) return EIO; conn->scsicmd_pdus_cnt++; - debug_scsi("iscsi prep [%s cid %d sc %p cdb 0x%x itt 0x%x len %d " - "cmdsn %d win %d]\n", - sc->sc_data_direction == DMA_TO_DEVICE ? "write" : "read", - conn->id, sc, sc->cmnd[0], ctask->itt, scsi_bufflen(sc), + debug_scsi("iscsi prep [%s cid %d sc %p cdb 0x%x itt 0x%x " + "
[PATCH 1/3 ver2] iscsi: extended cdb support
Support for extended CDBs in iscsi. All we need is to check if command spills over 16 bytes then allocate an iscsi-extended-header for the leftovers. Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]> Reviewed-by: Pete Wyckoff <[EMAIL PROTECTED]> Signed-off-by: Mike Christie <[EMAIL PROTECTED]> --- drivers/scsi/libiscsi.c| 55 include/scsi/iscsi_proto.h |6 +++- 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index 59f8445..a43b8ee 100644 --- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -137,6 +137,45 @@ static int iscsi_add_hdr(struct iscsi_cmd_task *ctask, unsigned len) return 0; } +/* + * make an extended cdb AHS + */ +static int iscsi_prep_ecdb_ahs(struct iscsi_cmd_task *ctask) +{ + struct scsi_cmnd *cmd = ctask->sc; + unsigned rlen, pad_len; + unsigned short ahslength; + struct iscsi_ecdb_ahdr *ecdb_ahdr; + int rc; + + ecdb_ahdr = iscsi_next_hdr(ctask); + rlen = cmd->cmd_len - ISCSI_CDB_SIZE; + + BUG_ON(rlen > sizeof(ecdb_ahdr->ecdb)); + ahslength = rlen + sizeof(ecdb_ahdr->reserved); + + pad_len = iscsi_padding(rlen); + + rc = iscsi_add_hdr(ctask, sizeof(ecdb_ahdr->ahslength) + + sizeof(ecdb_ahdr->ahstype) + ahslength + pad_len); + if (rc) + return rc; + + if (pad_len) + memset(&ecdb_ahdr->ecdb[rlen], 0, pad_len); + + ecdb_ahdr->ahslength = cpu_to_be16(ahslength); + ecdb_ahdr->ahstype = ISCSI_AHSTYPE_CDB; + ecdb_ahdr->reserved = 0; + memcpy(ecdb_ahdr->ecdb, cmd->cmnd + ISCSI_CDB_SIZE, rlen); + + debug_scsi("iscsi_prep_ecdb_ahs: varlen_cdb_len %d " + "rlen %d pad_len %d ahs_length %d iscsi_headers_size %u\n", + cmd->cmd_len, rlen, pad_len, ahslength, ctask->hdr_len); + + return 0; +} + /** * iscsi_prep_scsi_cmd_pdu - prep iscsi scsi cmd pdu * @ctask: iscsi cmd task @@ -150,7 +189,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask) struct iscsi_session *session = conn->session; struct iscsi_cmd *hdr = ctask->hdr; struct scsi_cmnd *sc = ctask->sc; - unsigned hdrlength; + unsigned hdrlength, cmd_len; int rc; ctask->hdr_len = 0; @@ -165,10 +204,16 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask) hdr->cmdsn = cpu_to_be32(session->cmdsn); session->cmdsn++; hdr->exp_statsn = cpu_to_be32(conn->exp_statsn); - memcpy(hdr->cdb, sc->cmnd, sc->cmd_len); - if (sc->cmd_len < MAX_COMMAND_SIZE) - memset(&hdr->cdb[sc->cmd_len], 0, - MAX_COMMAND_SIZE - sc->cmd_len); + cmd_len = sc->cmd_len; + if (cmd_len < ISCSI_CDB_SIZE) + memset(&hdr->cdb[cmd_len], 0, ISCSI_CDB_SIZE - cmd_len); + else if (cmd_len > ISCSI_CDB_SIZE) { + rc = iscsi_prep_ecdb_ahs(ctask); + if (rc) + return rc; + cmd_len = ISCSI_CDB_SIZE; + } + memcpy(hdr->cdb, sc->cmnd, cmd_len); ctask->imm_count = 0; if (sc->sc_data_direction == DMA_TO_DEVICE) { diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h index 5ffec8a..e0593bf 100644 --- a/include/scsi/iscsi_proto.h +++ b/include/scsi/iscsi_proto.h @@ -112,6 +112,7 @@ struct iscsi_ahs_hdr { #define ISCSI_AHSTYPE_CDB 1 #define ISCSI_AHSTYPE_RLENGTH 2 +#define ISCSI_CDB_SIZE 16 /* iSCSI PDU Header */ struct iscsi_cmd { @@ -125,7 +126,7 @@ struct iscsi_cmd { __be32 data_length; __be32 cmdsn; __be32 exp_statsn; - uint8_t cdb[16];/* SCSI Command Block */ + uint8_t cdb[ISCSI_CDB_SIZE];/* SCSI Command Block */ /* Additional Data (Command Dependent) */ }; @@ -154,7 +155,8 @@ struct iscsi_ecdb_ahdr { __be16 ahslength; /* CDB length - 15, including reserved byte */ uint8_t ahstype; uint8_t reserved; - uint8_t ecdb[260 - 16]; /* 4-byte aligned extended CDB spillover */ + /* 4-byte aligned extended CDB spillover */ + uint8_t ecdb[260 - ISCSI_CDB_SIZE]; }; /* SCSI Response Header */ -- 1.5.3.3 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bsg: bidi bio map failure fix
On Mon, 2008-02-18 at 15:46 +0100, Jens Axboe wrote: > Seems symmetric to me now, either we fail and everything is cleaned up, > or return success. What remains? My main symmetry complaint was the API: The map takes a request, the unmap takes a bio. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3 ver2] iscsi bidi & varlen support
On Thu, Jan 31 2008 at 20:08 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote: > Cheers after 1.3 years these can go in. > > [PATCH 1/3] iscsi: extended cdb support >The varlen support is not yet in mainline for > block and scsi-ml. But the API for drivers will > not change. All LLD need to do is max_command to > the it's maximum and be ready for bigger commands. > This is what's done here. Once these commands start > coming iscsi will be ready for them. > > [PATCH 2/3] iscsi: bidi support - libiscsi > [PATCH 3/3] iscsi: bidi support - iscsi_tcp > bidirectional commands support in iscsi. > iSER is not yet ready, but it will not break. > There is already a mechanism in libiscsi that will > return error if bidi commands are sent iSER way. > > Pete please send me the iSER bits so we can port them > to this latest version. > > Mike these patches are ontop of iscs branch of the iscsi > git tree, they will apply but for compilation you will need > to sync with Linus mainline. The patches are for the in-tree > iscsi code. I own you the compat patch for the out-off-tree > code, but this I will only be Sunday. > > If we do it fast it might get accepted to 2.6.25 merge window > > Everybody is invited to a party at Shila ben-yhuda 52 Tel-Aviv > 9:45 pm. Drinks and wonderful see-food on us :) > > Boaz > > - Everything the same as before. But working this time. Also Pete's comment about second patch, was correct and code is now fixed. I have got Mike's Signed-off-by, on these they were tested and approved by him. So they are for scsi-misc. But ... James? is there any chance these can go into scsi-rc-fixes for the 2.6.25 kernel? The reason they are so late was mainly because of a fallout in the merge process and a bug that was introduced because of that, but they were intended to go together with bidi into 2.6.25. Also as an important client code to the bidi-api that is introduced in 2.6.25 kernel. Thanks Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: ips.c broken since 2.6.23 on x86_64?
The path needs to be triggered, it is the path to handle spoofing of the Adapter's inquiry. You need more printk instrumentation to determine *why* it is not reaching that code path. What is the result of scb->scsi_cmd. scb->bus, ips_is_passthru(scb->scsi_cmd)? The sg breakup issue may need to be looked at, but keep in mind the driver is going down a path that was not intended. Sincerely -- Mark Salyzyn > -Original Message- > From: Tim Pepper [mailto:[EMAIL PROTECTED] > Sent: Wednesday, February 13, 2008 7:04 PM > To: linux-scsi@vger.kernel.org > Cc: FUJITA Tomonori; Salyzyn, Mark > Subject: Re: ips.c broken since 2.6.23 on x86_64? > > (replaced missing cc's including linux-scsi) > > On Wed 13 Feb at 14:39:07 -0800 [EMAIL PROTECTED] said: > > > > - Is the command path code even reaching the controller's > processor inquiry > > spoofing section? > > > > if (scb->scsi_cmd->cmnd[0] == INQUIRY) { > > IPS_SCSI_INQ_DATA inquiry; > > > > memset(&inquiry, 0, > >sizeof (IPS_SCSI_INQ_DATA)); > > > > inquiry.DeviceType = > > IPS_SCSI_INQ_TYPE_PROCESSOR; > > I just added printk's in ips_send_cmd()'s INQUIRY case just before > the above condition test and just within the conditional block. > Neither showed on the console. > > > The target_id check may be flawed? It is not using the > > scmd/sdev accessors and could be the wrong value as a > result. Wild goose > > since arrays are working correctly (?) > > In the original case the arrays appeared to be working > correctly although > we were worried. In the repro case we don't actually have any disk > attached...Forgot to mention that in the original email. > > > - There appears to be a missing 'break' statement for the > START_STOP command > > immediately preceding the TEST_UNIT_READY/INQUIRY cases. > What are the side > > effects of that? It appears to be innocuous. > > No change noted with the missing break added. > > > - ips_scmd_buf_write is used for array capacity and other > fiddly bits and > > must be functioning correctly (?), so I can NOT harm it's > functionality > > except to question if the kmap_atomic returned a non-null > value, it's > > return value apparently is not checked. It's failure > could speak of > > problem(s) elsewhere. > > I've got a printk there and haven't seen any output from it. > Haven't seen > anything from any of the printk's I've tried actually before: > > In the run above to check if ips_send_cmd() hits the > condition you were > curious about...I did capture the following from printk's I added in > ips_allocatescbs(): > > pci_alloc_consistent returns ha->scbs@ 0x18446604437762007040 > pci_alloc_consistent returns ips_sg.list @ 0x18446604437762002944 > pci_alloc_consistent returns ha->scbs@ 0x18446604437698871296 > pci_alloc_consistent returns ips_sg.list @ 0x18446604437756837888 > > I take that as ips_init_phase2() being called and presumable returning > SUCCESS. > > -- > Tim Pepper <[EMAIL PROTECTED]> > IBM Linux Technology Center > - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bsg: bidi bio map failure fix
On Mon, 2008-02-18 at 23:37 +0900, FUJITA Tomonori wrote: > On Mon, 18 Feb 2008 13:55:08 +0100 > Jens Axboe <[EMAIL PROTECTED]> wrote: > > > On Tue, Feb 12 2008, James Bottomley wrote: > > > On Tue, 2008-02-12 at 15:40 -0500, Pete Wyckoff wrote: > > > > If blk_rq_map_user requires more than one bio, and fails mapping > > > > somewhere after the first bio, it will return with rq->bio set to > > > > non-NULL, but it will have already unmapped the partial bio. The > > > > "out:" error exit section will see the non-null bio and try to unmap > > > > it again, triggering a mapcount bug via bad_page(). > > > > > > > > Signed-off-by: Pete Wyckoff <[EMAIL PROTECTED]> > > > > --- > > > > block/bsg.c |4 +++- > > > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > > > > > diff --git a/block/bsg.c b/block/bsg.c > > > > index 3337125..bba7154 100644 > > > > --- a/block/bsg.c > > > > +++ b/block/bsg.c > > > > @@ -295,8 +295,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 > > > > *hdr) > > > > > > > > dxferp = (void*)(unsigned long)hdr->din_xferp; > > > > ret = blk_rq_map_user(q, next_rq, dxferp, > > > > hdr->din_xfer_len); > > > > - if (ret) > > > > + if (ret) { > > > > + next_rq->bio = NULL; /* do not unmap twice */ > > > > > > Nice ... that's a nasty asymmetry of the blk_rq_map_user API. The map > > > takes a request gets a ref and fills in the bio. The unmap has to be > > > called on the bio leaving you to clear the now removed bio reference > > > manually. > > > > It is nasty, how about fixing that instead? > > Yeah, looks better for me though the blk_rq_map_user API is still a > bit hacky, as James said. > > James, Pete's patch is still in scsi-fixes, so how about dropping it > and sending this patch via the block? Yes, sure. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gdth new set of patches for 2.6.24 stable
On Mon, 2008-02-18 at 14:02 +, Russell King wrote: > Another solution jejb suggested was to make dma_free_coherent() lazy, > but (a) I'm unconvinced that this'll work with drivers which > constantly > alloc+free in IRQ context since there's generally only 2MB of VM space > for such mappings, and it probably won't take long to eat through that > limited space with such a scheme. I didn't say make the alloc/free lazy, I said make the mapping setup teardown lazy. So on alloc, you first look for existing space, if none, you map some more. On free you *don't* teardown the mappings (which is what requires the IPI) but simply free to the existing space pool ready for reuse. You map and free in page size chunks, since the coherent memory users that want > PAGE_SIZE usually all do boot time allocation (and don't free until release time). The reuse of the pages should keep even the drivers that do persistent alloc/free of dma coherent memory happy, and it should reach a steady state fairly quickly. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bsg: bidi bio map failure fix
On Mon, Feb 18 2008, FUJITA Tomonori wrote: > On Mon, 18 Feb 2008 13:55:08 +0100 > Jens Axboe <[EMAIL PROTECTED]> wrote: > > > On Tue, Feb 12 2008, James Bottomley wrote: > > > On Tue, 2008-02-12 at 15:40 -0500, Pete Wyckoff wrote: > > > > If blk_rq_map_user requires more than one bio, and fails mapping > > > > somewhere after the first bio, it will return with rq->bio set to > > > > non-NULL, but it will have already unmapped the partial bio. The > > > > "out:" error exit section will see the non-null bio and try to unmap > > > > it again, triggering a mapcount bug via bad_page(). > > > > > > > > Signed-off-by: Pete Wyckoff <[EMAIL PROTECTED]> > > > > --- > > > > block/bsg.c |4 +++- > > > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > > > > > diff --git a/block/bsg.c b/block/bsg.c > > > > index 3337125..bba7154 100644 > > > > --- a/block/bsg.c > > > > +++ b/block/bsg.c > > > > @@ -295,8 +295,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 > > > > *hdr) > > > > > > > > dxferp = (void*)(unsigned long)hdr->din_xferp; > > > > ret = blk_rq_map_user(q, next_rq, dxferp, > > > > hdr->din_xfer_len); > > > > - if (ret) > > > > + if (ret) { > > > > + next_rq->bio = NULL; /* do not unmap twice */ > > > > > > Nice ... that's a nasty asymmetry of the blk_rq_map_user API. The map > > > takes a request gets a ref and fills in the bio. The unmap has to be > > > called on the bio leaving you to clear the now removed bio reference > > > manually. > > > > It is nasty, how about fixing that instead? > > Yeah, looks better for me though the blk_rq_map_user API is still a > bit hacky, as James said. Seems symmetric to me now, either we fail and everything is cleaned up, or return success. What remains? -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bsg: bidi bio map failure fix
On Mon, 18 Feb 2008 13:55:08 +0100 Jens Axboe <[EMAIL PROTECTED]> wrote: > On Tue, Feb 12 2008, James Bottomley wrote: > > On Tue, 2008-02-12 at 15:40 -0500, Pete Wyckoff wrote: > > > If blk_rq_map_user requires more than one bio, and fails mapping > > > somewhere after the first bio, it will return with rq->bio set to > > > non-NULL, but it will have already unmapped the partial bio. The > > > "out:" error exit section will see the non-null bio and try to unmap > > > it again, triggering a mapcount bug via bad_page(). > > > > > > Signed-off-by: Pete Wyckoff <[EMAIL PROTECTED]> > > > --- > > > block/bsg.c |4 +++- > > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > > > diff --git a/block/bsg.c b/block/bsg.c > > > index 3337125..bba7154 100644 > > > --- a/block/bsg.c > > > +++ b/block/bsg.c > > > @@ -295,8 +295,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 > > > *hdr) > > > > > > dxferp = (void*)(unsigned long)hdr->din_xferp; > > > ret = blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len); > > > - if (ret) > > > + if (ret) { > > > + next_rq->bio = NULL; /* do not unmap twice */ > > > > Nice ... that's a nasty asymmetry of the blk_rq_map_user API. The map > > takes a request gets a ref and fills in the bio. The unmap has to be > > called on the bio leaving you to clear the now removed bio reference > > manually. > > It is nasty, how about fixing that instead? Yeah, looks better for me though the blk_rq_map_user API is still a bit hacky, as James said. James, Pete's patch is still in scsi-fixes, so how about dropping it and sending this patch via the block? > diff --git a/block/blk-map.c b/block/blk-map.c > index 955d75c..bc5ce60 100644 > --- a/block/blk-map.c > +++ b/block/blk-map.c > @@ -143,6 +143,7 @@ int blk_rq_map_user(struct request_queue *q, struct > request *rq, > return 0; > unmap_rq: > blk_rq_unmap_user(bio); > + rq->bio = NULL; > return ret; > } > EXPORT_SYMBOL(blk_rq_map_user); > > -- > Jens Axboe > > - > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: aic94xx: failing on high load (another data point)
On 02/15/2008 11:28 PM, James Bottomley wrote: > On Fri, 2008-02-15 at 00:11 +0800, Keith Hopkins wrote: >> On 01/31/2008 03:29 AM, Darrick J. Wong wrote: >>> On Wed, Jan 30, 2008 at 06:59:34PM +0800, Keith Hopkins wrote: >>>> V28. My controller functions well with a single drive (low-medium load). >>>> Unfortunately, all attempts to get the mirrors in sync fail and usually >>>> hang the whole box. >>> Adaptec posted a V30 sequencer on their website; does that fix the >>> problems? >>> >>> http://www.adaptec.com/en-US/speed/scsi/linux/aic94xx-seq-30-1_tar_gz.htm >>> >> I lost connectivity to the drive again, and had to reboot to recover >> the drive, so it seemed a good time to try out the V30 firmware. >> Unfortunately, it didn't work any better. Details are in the >> attachment. > > Well, I can offer some hope. The errors you report: > >> aic94xx: escb_tasklet_complete: REQ_TASK_ABORT, reason=0x6 >> aic94xx: escb_tasklet_complete: Can't find task (tc=6) to abort! > > Are requests by the sequencer to abort a task because of a protocol > error. IBM did some extensive testing with seagate drives and found > that the protocol errors were genuine and the result of drive firmware > problems. IBM released a version of seagate firmware (BA17) to correct > these. Unfortunately, your drive identifies its firmware as S513 which > is likely OEM firmware from another vendor ... however, that vendor may > have an update which corrects the problem. > > Of course, the other issue is this: > >> aic94xx: escb_tasklet_complete: Can't find task (tc=6) to abort! > > This is a bug in the driver. It's not finding the task in the > outstanding list. The problem seems to be that it's taking the task > from the escb which, by definition, is always NULL. It should be taking > the task from the ascb it finds by looping over the pending queue. > > If you're willing, could you try this patch which may correct the > problem? It's sort of like falling off a cliff: if you never go near > the edge (i.e. you upgrade the drive fw) you never fall off; > alternatively, it would be nice if you could help me put up guard rails > just in case. > Well, that made life interesting but didn't seem to fix anything. The behavior is about the same as before, but with more verbose errors. I failed one member of the raid and had it rebuild as a test...which hangs for a while and the drive falls off-line. Please grab the dmesg output in all its gory glory from here: http://wiki.hopnet.net/dokuwiki/lib/exe/fetch.php?media=myit:sas:dmesg-20080218-wpatch-fail.txt.gz The drive is a Dell OEM drive, but it's not in a Dell system. There is at least one firmware (S527) upgrade for it, but the Dell loader refuses to load it (because it isn't in a Dell system...) Does anyone know a generic way to load a new firmware onto a SAS drive? --Keith - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[Bug 9769] CONFIG_SCSI_WAIT_SCAN configure error
http://bugzilla.kernel.org/show_bug.cgi?id=9769 [EMAIL PROTECTED] changed: What|Removed |Added CC||[EMAIL PROTECTED] Status|REOPENED|REJECTED Resolution||INVALID --- Comment #5 from [EMAIL PROTECTED] 2008-02-18 06:19 --- Sorry, it is an invalid bug. This has been discussed a few times on linux-scsi. If you have any SCSI driver built as a module, you (or the distro you are running) may need it. We can't tell in Kconfig -- and you might choose to build another driver as a module later. Or out of tree. If you're sure you don't need it, then don't run 'make modules_install'. It's about 2500 bytes ... why waste so much energy being angry? -- Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ps3rom: disable clustering
On Sun, 17 Feb 2008, FUJITA Tomonori wrote: > ps3rom does: > > scsi_for_each_sg(cmd, sgpnt, scsi_sg_count(cmd), k) { > kaddr = kmap_atomic(sg_page(sgpnt), KM_IRQ0); > > We cannot do something like that with the clustering enabled (or we > can use scsi_kmap_atomic_sg). > > Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]> > Cc: Geert Uytterhoeven <[EMAIL PROTECTED]> > Cc: James Bottomley <[EMAIL PROTECTED]> Seems to (still) work fine.. James: as you're scripts seem to be waiting for an ack from me, and it's not really obvious to me whether this patch is needed or not, I'll leave it up to you. > --- > drivers/scsi/ps3rom.c |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/scsi/ps3rom.c b/drivers/scsi/ps3rom.c > index 0cd614a..90985cd 100644 > --- a/drivers/scsi/ps3rom.c > +++ b/drivers/scsi/ps3rom.c > @@ -427,7 +427,7 @@ static struct scsi_host_template ps3rom_host_template = { > .cmd_per_lun = 1, > .emulated = 1, /* only sg driver uses this */ > .max_sectors = PS3ROM_MAX_SECTORS, > - .use_clustering = ENABLE_CLUSTERING, > + .use_clustering = DISABLE_CLUSTERING, > .module = THIS_MODULE, > }; With kind regards, Geert Uytterhoeven Software Architect Sony Network and Software Technology Center Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone:+32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: [EMAIL PROTECTED] Internet: http://www.sony-europe.com/ Sony Network and Software Technology Center Europe A division of Sony Service Centre (Europe) N.V. Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium VAT BE 0413.825.160 · RPR Brussels Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: gdth new set of patches for 2.6.24 stable
On Mon, Feb 18, 2008 at 04:57:36AM -0800, Andrew Morton wrote: > But arm and mips require enabled local irqs because their > dma_free_coherent() needs to do a cross-cpu IPI call. Presumably because > of certain unusual TLB protocols. Consider that TLB flushing needs to call a function on another CPU. Now look at x86's implementation to do cross calls: /** * smp_call_function_mask(): Run a function on a set of other CPUs. * @mask: The set of cpus to run on. Must not include the current cpu. * @func: The function to run. This must be fast and non-blocking. * @info: An arbitrary pointer to pass to the function. * @wait: If true, wait (atomically) until function has completed on other CPUs. * * Returns 0 on success, else a negative status code. * * If @wait is true, then returns once @func has returned; otherwise * it returns just before the target cpu calls @func. * * You must not call this function with disabled interrupts or from a * hardware interrupt handler or from a bottom half handler. */ static int native_smp_call_function_mask(cpumask_t mask, void (*func)(void *), void *info, int wait) You can not call functions on other CPUs without having IRQs enabled, otherwise this functionality deadlocks. That restriction is on all smp_call_function() implementations. Unfortunately, to flush the TLB on other CPUs on ARM, we need to do a cross call, which means using smp_call_function(), which introduces the same sodding restrictions that smp_call_function() has on the functions which call it. > Russell, Ralf: is there something we can do here to relax this requirement? Do what x86 people have so far been unable to resolve and find some way to allow smp_call_function() to operate with IRQs disabled without deadlocking? Another solution jejb suggested was to make dma_free_coherent() lazy, but (a) I'm unconvinced that this'll work with drivers which constantly alloc+free in IRQ context since there's generally only 2MB of VM space for such mappings, and it probably won't take long to eat through that limited space with such a scheme. Also, I don't have the facility to really test out these issues... -- Russell King Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ips.c broken since 2.6.23 on x86_64?
On Sun, 17 Feb 2008 15:37:02 -0800 Tim Pepper <[EMAIL PROTECTED]> wrote: > On Mon 19 Feb at 07:31:56 +0900 [EMAIL PROTECTED] said: > > > > Can you apply the 0001 and 0002 against 2.6.24 and see how it works? > > If it works well, then please apply the 0001, 0002 and 0003. > > Fujita-san, > > I've started through the patches in order, cumulatively and after applying > 0005 things break. I wont be able to test anything else until tomorrow > when I can phycisally reset the machine... Great, thanks a lot! Can you apply this patch after the 0005 patch and see how it works? If it works, then please continue to test 0006, 0007 ... diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c index 05bb6ea..39cdd68 100644 --- a/drivers/scsi/ips.c +++ b/drivers/scsi/ips.c @@ -6906,7 +6906,7 @@ ips_register_scsi(int index) sh->max_channel = ha->nbus - 1; sh->can_queue = ha->max_cmds - 1; - scsi_add_host(sh, NULL); + scsi_add_host(sh, &ha->pcidev->dev); scsi_scan_host(sh); return 0; -- 1.5.3.7 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gdth new set of patches for 2.6.24 stable
On Mon, Feb 18 2008 at 14:57 +0200, Andrew Morton <[EMAIL PROTECTED]> wrote: > On Sun, 17 Feb 2008 18:46:03 +0200 Boaz Harrosh <[EMAIL PROTECTED]> wrote: > >> ... >> >> All my testers have reported back that with these 5 patches applied they can >> now run with a 2.6.24 kernel the same way they ran before. However there is >> that reported issue, with the dma_free_coherent WARN_ON (above). The code >> was >> like that from day one and it is a very old issue, however it is a >> regression >> because 2.6.24 introduced that new WARN_ON. >> (infamous commit aa24886e379d2b641c5117e178b15ce1d5d366ba) >> >From posts on lkml and even recent one in linux-scsi about the arcmsr driver >> it looks that all a driver can do is work around it with different kernel >> mechanisms >> and driver rewrites. I'm afraid I need your help here. I'm not sure I >> understand >> why does the gdth driver uses the pci_{alloc,free}_consistent() API's, and >> what >> is needed to replace it. Could you please have a look in gdth_proc.c and >> also in >> gdth.c for all the places that call gdth_ioctl_alloc/gdth_ioctl_free, and >> advise >> what can I do in it's place. Please bear in mind that we need it for 2.6.24, >> as >> a bugfix. >> >> Apart from the above issue, please accept patches 3,4,5 above they have now >> been tested and are reported to bring broken system back to production. >> (Given that you approve off course). And mark them for inclusion to the >> 2.6.24 stable releases. (Or is there some thing that I should do) >> >> --- >> Meanwhile on x86 systems I understand the WARN_ON is cosmetic, and does not >> pose any harm. Some people have reported stability with temporarily disabling >> it. For testers that want to try, here it is below. At your own risk. >> >> --- >> >From 50d3657bf6a138ee63ad1ce00052380edc75ace7 Mon Sep 17 00:00:00 2001 >> From: Boaz Harrosh <[EMAIL PROTECTED]> >> Date: Sun, 17 Feb 2008 12:49:35 +0200 >> Subject: [PATCH] gdth: Hack to remove WARN_ON in arch/x86/kernel/pci-dma_32.c >> >> gdth uses dma_free_coherent() with interrupts disabled. Which >> is not portable, but is safe on the HW that supports gdth. >> >> NOT Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]> >> --- >> arch/x86/kernel/pci-dma_32.c |2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/arch/x86/kernel/pci-dma_32.c b/arch/x86/kernel/pci-dma_32.c >> index 5133032..350dcfd 100644 >> --- a/arch/x86/kernel/pci-dma_32.c >> +++ b/arch/x86/kernel/pci-dma_32.c >> @@ -63,7 +63,7 @@ void dma_free_coherent(struct device *dev, size_t size, >> struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL; >> int order = get_order(size); >> >> -WARN_ON(irqs_disabled()); /* for portability */ >> +/* WARN_ON(irqs_disabled());*/ /* for portability */ >> if (mem && vaddr >= mem->virt_base && vaddr < (mem->virt_base + >> (mem->size << PAGE_SHIFT))) { >> int page = (vaddr - mem->virt_base) >> PAGE_SHIFT; >> > > Yes. Let's reprise aa24886e379d2b641c5117e178b15ce1d5d366ba: > > : commit aa24886e379d2b641c5117e178b15ce1d5d366ba > : Author: David Brownell <[EMAIL PROTECTED]> > : Date: Fri Aug 10 13:10:27 2007 -0700 > : > : dma_free_coherent() needs irqs enabled (sigh) > : > : On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish > : call context requirement: unlike its dma_alloc_coherent() sibling, it > may > : not be called with IRQs disabled. (This was new behavior on ARM as of > late > : 2005, caused by ARM SMP updates.) This little surprise can be annoyingly > : driver-visible. > : > : Since it looks like that restriction won't be removed, this patch > changes > : the definition of the API to include that requirement. Also, to help > catch > : nonportable drivers, it updates the x86 and swiotlb versions to include > the > : relevant warnings. (I already observed that it trips on the > : bus_reset_tasklet of the new firewire_ohci driver.) > : > > In general, all Linux memory-freeing functions can be called from all > contexts. (vfree is an irritating exception). This is good, and provides > maximum usefulness to callees, as all utility functions should seek to do. > It would be best to fix arm and mips. > > But arm and mips require enabled local irqs because their > dma_free_coherent() needs to do a cross-cpu IPI call. Presumably because > of certain unusual TLB protocols. > > I'm not sure what we should do about this. Presumably the gdth-on-arm > usage base is, umm, zero, so we could lamely add > CONFIG_DMA_FREE_COHERENT_WITH_LOCAL_IRQS_DISABLED_IS_OK and then use that > to disable gdth (and similar) on arm amd mips. But ugh. > > Russell, Ralf: is there something we can do here to relax this requirement? > > I'm thinking that perhaps we can do some rcu/refcounting tricks: launch the > IPI from within dma_free_coherent(), but don't wait for it to complete. > When all CPUs have
[Bug 9769] CONFIG_SCSI_WAIT_SCAN configure error
http://bugzilla.kernel.org/show_bug.cgi?id=9769 [EMAIL PROTECTED] changed: What|Removed |Added Status|REJECTED|REOPENED Resolution|INVALID | --- Comment #4 from [EMAIL PROTECTED] 2008-02-18 05:16 --- This is NOT an invalid bug! In all previous kernels, if # CONFIG_SCSI_SCAN_ASYNC is not set then CONFIG_SCSI_WAIT_SCAN=m does not even appear in the .config file, and the module scsi_wait_scan.ko is not built. The module is NOT built, only in 2.6.24 is it getting built regardless. For 2.6.23.x and all earlier kernels, the module is not built. Ditto when doing 'make menuconfig', in all previous kernels, if CONFIG_SCSI_SCAN_ASYNC is turned off then the option to build the module is not even offered. However in 2.6.24 it continues to be offered regardless. So, I repeat, this is a BUG! Or, if you have made a change that requires the module to be built regardless, can you please explain why it is needed in the case of '.CONFIG_SCSI_SCAN_ASYNC is not set' and why this change from all previous kernels. Barry Kauler -- Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gdth new set of patches for 2.6.24 stable
On Sun, 17 Feb 2008 18:46:03 +0200 Boaz Harrosh <[EMAIL PROTECTED]> wrote: > > ... > > All my testers have reported back that with these 5 patches applied they can > now run with a 2.6.24 kernel the same way they ran before. However there is > that reported issue, with the dma_free_coherent WARN_ON (above). The code was > like that from day one and it is a very old issue, however it is a regression > because 2.6.24 introduced that new WARN_ON. > (infamous commit aa24886e379d2b641c5117e178b15ce1d5d366ba) > >From posts on lkml and even recent one in linux-scsi about the arcmsr driver > it looks that all a driver can do is work around it with different kernel > mechanisms > and driver rewrites. I'm afraid I need your help here. I'm not sure I > understand > why does the gdth driver uses the pci_{alloc,free}_consistent() API's, and > what > is needed to replace it. Could you please have a look in gdth_proc.c and also > in > gdth.c for all the places that call gdth_ioctl_alloc/gdth_ioctl_free, and > advise > what can I do in it's place. Please bear in mind that we need it for 2.6.24, > as > a bugfix. > > Apart from the above issue, please accept patches 3,4,5 above they have now > been tested and are reported to bring broken system back to production. > (Given that you approve off course). And mark them for inclusion to the > 2.6.24 stable releases. (Or is there some thing that I should do) > > --- > Meanwhile on x86 systems I understand the WARN_ON is cosmetic, and does not > pose any harm. Some people have reported stability with temporarily disabling > it. For testers that want to try, here it is below. At your own risk. > > --- > >From 50d3657bf6a138ee63ad1ce00052380edc75ace7 Mon Sep 17 00:00:00 2001 > From: Boaz Harrosh <[EMAIL PROTECTED]> > Date: Sun, 17 Feb 2008 12:49:35 +0200 > Subject: [PATCH] gdth: Hack to remove WARN_ON in arch/x86/kernel/pci-dma_32.c > > gdth uses dma_free_coherent() with interrupts disabled. Which > is not portable, but is safe on the HW that supports gdth. > > NOT Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]> > --- > arch/x86/kernel/pci-dma_32.c |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/kernel/pci-dma_32.c b/arch/x86/kernel/pci-dma_32.c > index 5133032..350dcfd 100644 > --- a/arch/x86/kernel/pci-dma_32.c > +++ b/arch/x86/kernel/pci-dma_32.c > @@ -63,7 +63,7 @@ void dma_free_coherent(struct device *dev, size_t size, > struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL; > int order = get_order(size); > > - WARN_ON(irqs_disabled()); /* for portability */ > +/* WARN_ON(irqs_disabled());*/ /* for portability */ > if (mem && vaddr >= mem->virt_base && vaddr < (mem->virt_base + > (mem->size << PAGE_SHIFT))) { > int page = (vaddr - mem->virt_base) >> PAGE_SHIFT; > Yes. Let's reprise aa24886e379d2b641c5117e178b15ce1d5d366ba: : commit aa24886e379d2b641c5117e178b15ce1d5d366ba : Author: David Brownell <[EMAIL PROTECTED]> : Date: Fri Aug 10 13:10:27 2007 -0700 : : dma_free_coherent() needs irqs enabled (sigh) : : On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish : call context requirement: unlike its dma_alloc_coherent() sibling, it may : not be called with IRQs disabled. (This was new behavior on ARM as of late : 2005, caused by ARM SMP updates.) This little surprise can be annoyingly : driver-visible. : : Since it looks like that restriction won't be removed, this patch changes : the definition of the API to include that requirement. Also, to help catch : nonportable drivers, it updates the x86 and swiotlb versions to include the : relevant warnings. (I already observed that it trips on the : bus_reset_tasklet of the new firewire_ohci driver.) : In general, all Linux memory-freeing functions can be called from all contexts. (vfree is an irritating exception). This is good, and provides maximum usefulness to callees, as all utility functions should seek to do. It would be best to fix arm and mips. But arm and mips require enabled local irqs because their dma_free_coherent() needs to do a cross-cpu IPI call. Presumably because of certain unusual TLB protocols. I'm not sure what we should do about this. Presumably the gdth-on-arm usage base is, umm, zero, so we could lamely add CONFIG_DMA_FREE_COHERENT_WITH_LOCAL_IRQS_DISABLED_IS_OK and then use that to disable gdth (and similar) on arm amd mips. But ugh. Russell, Ralf: is there something we can do here to relax this requirement? I'm thinking that perhaps we can do some rcu/refcounting tricks: launch the IPI from within dma_free_coherent(), but don't wait for it to complete. When all CPUs have handled the IPI then (and only then) the virtual address becomes recyclable, or something like that? Actually I think David might have been wrong about mips. afaict its dma_free_coherent() is callable under local_irq_disable
Re: [PATCH] bsg: bidi bio map failure fix
On Tue, Feb 12 2008, James Bottomley wrote: > On Tue, 2008-02-12 at 15:40 -0500, Pete Wyckoff wrote: > > If blk_rq_map_user requires more than one bio, and fails mapping > > somewhere after the first bio, it will return with rq->bio set to > > non-NULL, but it will have already unmapped the partial bio. The > > "out:" error exit section will see the non-null bio and try to unmap > > it again, triggering a mapcount bug via bad_page(). > > > > Signed-off-by: Pete Wyckoff <[EMAIL PROTECTED]> > > --- > > block/bsg.c |4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/block/bsg.c b/block/bsg.c > > index 3337125..bba7154 100644 > > --- a/block/bsg.c > > +++ b/block/bsg.c > > @@ -295,8 +295,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 > > *hdr) > > > > dxferp = (void*)(unsigned long)hdr->din_xferp; > > ret = blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len); > > - if (ret) > > + if (ret) { > > + next_rq->bio = NULL; /* do not unmap twice */ > > Nice ... that's a nasty asymmetry of the blk_rq_map_user API. The map > takes a request gets a ref and fills in the bio. The unmap has to be > called on the bio leaving you to clear the now removed bio reference > manually. It is nasty, how about fixing that instead? diff --git a/block/blk-map.c b/block/blk-map.c index 955d75c..bc5ce60 100644 --- a/block/blk-map.c +++ b/block/blk-map.c @@ -143,6 +143,7 @@ int blk_rq_map_user(struct request_queue *q, struct request *rq, return 0; unmap_rq: blk_rq_unmap_user(bio); + rq->bio = NULL; return ret; } EXPORT_SYMBOL(blk_rq_map_user); -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Device rescan does not find all new devices
Hello people ! I have an x86 SLES 10 system that is connected to some disks on two EnterpriseStorageSystems (ESS) via two QLogic FibreChannel SCSI Adapters. Each adapter has one path to each disk, so I have a real multipathing environment. Now, I have the need to re-configure the available disks during the system is up and running. Up to now (and hopefully in future also), the only kind of re-configuration is to add more disks. To be more exact, the system is started wth only the disks of ESS 1 connected, and the disks on ESS 2 need to be added later on. The disk layouts on both ESS's are identical. To make the new disks known to the system, I followed these instructions: http://www-941.ibm.com/collaboration/wiki/display/LinuxP/SCSI+-+Hot+add,+remove,+rescan+of+SCSI+devices And this is the script I have created, acording to the above mentioned instructions: for HOST in /sys/class/scsi_host/host? do echo "- - -" > $HOST/scan done When I run this script, all new disks except one are detected. All new disks (except the missing one of course) are found by both pathes. In the syslog, there are many messages about the disks that were found, but none (not even error messages) about the disk that was not found. When I then rebot the system, all disks are there, including the one that was not found by the rescan. Therefore I think my configuration is OK. The missing disk is always the first one (LUN 000). I have tested this by changing the order of the disks on ESS 2. I have also repeated the test scenario with only one path per disk. In this testcase, I have divided the ESS's on the two Adapters (Adapter 1 accesses the disks on ESS 1, Adapter 2 those on ESS 2). Sadly, with no change in the result. I think the first thing I would need help with is how to further analyze the problem. Thanks and Regards, Markus - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Integration of SCST in the mainstream Linux kernel
On Feb 18, 2008 10:43 AM, Erez Zilber <[EMAIL PROTECTED]> wrote: > If you use a high value for FirstBurstLength, all (or most) of your data > will be sent as unsolicited data-out PDUs. These PDUs don't use the RDMA > engine, so you miss the advantage of IB. Hello Erez, Did you notice the e-mail Roland Dreier wrote on Februari 6, 2008 ? This is what Roland wrote: > I think the confusion here is caused by a slight misuse of the term > "RDMA". It is true that all data is always transported over an > InfiniBand connection when iSER is used, but not all such transfers > are one-sided RDMA operations; some data can be transferred using > send/receive operations. Or: data sent during the first burst is not transferred via one-sided remote memory reads or writes but via two-sided send/receive operations. At least on my setup, these operations are as fast as one-sided remote memory reads or writes. As an example, I obtained the following numbers on my setup (SDR 4x network); ib_write_bw: 933 MB/s. ib_read_bw: 905 MB/s. ib_send_bw: 931 MB/s. Bart Van Assche. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Integration of SCST in the mainstream Linux kernel
Bart Van Assche wrote: > On Feb 5, 2008 6:01 PM, Erez Zilber <[EMAIL PROTECTED]> wrote: > >> Using such large values for FirstBurstLength will give you poor >> performance numbers for WRITE commands (with iSER). FirstBurstLength >> means how much data should you send as unsolicited data (i.e. without >> RDMA). It means that your WRITE commands were sent without RDMA. >> > > Sorry, but I'm afraid you got this wrong. When the iSER transport is > used instead of TCP, all data is sent via RDMA, including unsolicited > data. If you have look at the iSER implementation in the Linux kernel > (source files under drivers/infiniband/ulp/iser), you will see that > all data is transferred via RDMA and not via TCP/IP. > > When you execute WRITE commands with iSCSI, it works like this: EDTL (Expected data length) - the data length of your command FirstBurstLength - the length of data that will be sent as unsolicited data (i.e. as immediate data with the SCSI command and as unsolicited data-out PDUs) If you use a high value for FirstBurstLength, all (or most) of your data will be sent as unsolicited data-out PDUs. These PDUs don't use the RDMA engine, so you miss the advantage of IB. If you use a lower value for FirstBurstLength, EDTL - FirstBurstLength bytes will be sent as solicited data-out PDUs. With iSER, solicited data-out PDUs are RDMA operations. I hope that I'm more clear now. Erez - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gdth new set of patches for 2.6.24 stable
On Sun, Feb 17 2008 at 19:24 +0200, James Bottomley <[EMAIL PROTECTED]> wrote: > On Sun, 2008-02-17 at 18:46 +0200, Boaz Harrosh wrote: >> On Thu, Feb 14 2008 at 20:47 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote: >>> Submitted are a new set of patches, that fix lots of problems >>> with the gdth driver. >>> >>> It fixes the following problems: >>> - scan for drives on hosts. (Already in mainline) >>> - truly fixes the exit/reboot problems but does call flush() before >>> reboot. >>> - fix crash when accessing array with icpcon management application. >>> - fix crash when doing $ cat /proc/sys/gdth/0. >>> This one still has the below WARN_ON in messages (see below) >>> So there is one more thing hiding in there. >>> - use pci_get_device >>> One of the testers requested if we can also put the move to >>> pci_get_device >>> patch with removal of dependency on PCI_LEGACY, to the stable release. >>> >>> The patches are for and based on Linux-2.6.24. here is the list of patches: >>> [PATCH 1/5] gdth: update deprecated pci_find_device >>> [PATCH 2/5] gdth: scan for scsi devices >>> [PATCH 3/5] gdth: bugfix for the at-exit problems >>> [PATCH 4/5] gdth: fix to internal commands execution >>> [PATCH 5/5] gdth: remove gdth cooked up command accessors >>> >>> Please all test and report your findings. >>> >>> Thanks in advance >>> Boaz >>> >>> --- >>> >>> WARNING: at arch/x86/kernel/pci-dma_32.c:66 dma_free_coherent() >>> Pid: 5501, comm: cat Not tainted 2.6.24 #43 >>>[] dma_free_coherent+0x93/0x95 >>>[] gdth_ioctl_free+0x4c/0x69 >>>[] gdth_proc_info+0x165f/0x182c >>>[] update_curr+0xeb/0xf2 >>>[] task_rq_lock+0x29/0x50 >>>[] try_to_wake_up+0x42/0x342 >>>[] try_to_wake_up+0x42/0x342 >>>[] __wake_up_common+0x46/0x6d >>>[] __wake_up+0x32/0x42 >>>[] n_tty_receive_buf+0x2e8/0xe97 >>>[] n_tty_receive_buf+0x2e8/0xe97 >>>[] update_curr+0x7b/0xf2 >>>[] enqueue_task_fair+0x27/0x30 >>>[] enqueue_task+0xa/0x14 >>>[] proc_scsi_read+0x29/0x3d >>>[] proc_scsi_read+0x0/0x3d >>>[] proc_file_read+0x1c6/0x279 >>>[] proc_file_read+0x0/0x279 >>>[] proc_reg_read+0x53/0x71 >>>[] proc_reg_read+0x0/0x71 >>>[] vfs_read+0x85/0x11b >>>[] sys_read+0x41/0x6a >>>[] sysenter_past_esp+0x5f/0x85 >>> >>> - >> James hi. >> >> All my testers have reported back that with these 5 patches applied they can >> now run with a 2.6.24 kernel the same way they ran before. However there is >> that reported issue, with the dma_free_coherent WARN_ON (above). The code >> was >> like that from day one and it is a very old issue, however it is a >> regression >> because 2.6.24 introduced that new WARN_ON. >> (infamous commit aa24886e379d2b641c5117e178b15ce1d5d366ba) >> >From posts on lkml and even recent one in linux-scsi about the arcmsr driver >> it looks that all a driver can do is work around it with different kernel >> mechanisms >> and driver rewrites. I'm afraid I need your help here. I'm not sure I >> understand >> why does the gdth driver uses the pci_{alloc,free}_consistent() API's, and >> what >> is needed to replace it. Could you please have a look in gdth_proc.c and >> also in >> gdth.c for all the places that call gdth_ioctl_alloc/gdth_ioctl_free, and >> advise >> what can I do in it's place. Please bear in mind that we need it for 2.6.24, >> as >> a bugfix. >> >> Apart from the above issue, please accept patches 3,4,5 above they have now >> been tested and are reported to bring broken system back to production. >> (Given that you approve off course). And mark them for inclusion to the >> 2.6.24 stable releases. (Or is there some thing that I should do) >> >> --- >> Meanwhile on x86 systems I understand the WARN_ON is cosmetic, and does not >> pose any harm. Some people have reported stability with temporarily disabling >> it. For testers that want to try, here it is below. At your own risk. > > Isn't this the correct fix? pscratch is a permanent address (it's > allocated at boot time and never changes). All you need the smp lock > for is mediating the scratch in use flag. > > James > > diff --git a/drivers/scsi/gdth_proc.c b/drivers/scsi/gdth_proc.c > index de57734..ce0228e 100644 > --- a/drivers/scsi/gdth_proc.c > +++ b/drivers/scsi/gdth_proc.c > @@ -694,15 +694,13 @@ static void gdth_ioctl_free(gdth_ha_str *ha, int size, > char *buf, ulong64 paddr) > { > ulong flags; > > -spin_lock_irqsave(&ha->smp_lock, flags); > - > if (buf == ha->pscratch) { > + spin_lock_irqsave(&ha->smp_lock, flags); > ha->scratch_busy = FALSE; > + spin_unlock_irqrestore(&ha->smp_lock, flags); > } else { > pci_free_consistent(ha->pdev, size, buf, paddr); > } > - > -spin_unlock_irqrestore(&ha->smp_lock, flags); > } > > #ifdef GDTH_IOCTL_PROC > > > - James You are bung on the money. It was tested and it works. So simple, I was thinking it was accessed by DMA and freed at interrupt