Re: status of drivers/scsi/scsi_tgt_lib.c

[FUJITA Tomonori]

On Sun, 26 Jan 2014 23:50:38 -0800
Christoph Hellwig  wrote:

>> > Do you know if either SRP or ibmvscsi are used regularly these days?
>> > Does tgtd userspace still support this interface?
>> 
>> It doesn't. I think that ibm pseries switched to virtual fc driver
>> from virtual srp long ago. Only very old pseries could use the
>> driver. I'm not sure there are any users of the driver. I guess that
>> you could try to remove the driver to see if someone would complain.
> 
> Should we try to throw in a deprecation warnings for this merge window
> and see if anyone cares, or go ahead and remove it entirely?

Either is fine by me.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: status of drivers/scsi/scsi_tgt_lib.c

[FUJITA Tomonori]

Hey,

On Fri, 24 Jan 2014 05:15:25 -0800
Christoph Hellwig  wrote:

> Do you know if either SRP or ibmvscsi are used regularly these days?
> Does tgtd userspace still support this interface?

It doesn't. I think that ibm pseries switched to virtual fc driver
from virtual srp long ago. Only very old pseries could use the
driver. I'm not sure there are any users of the driver. I guess that
you could try to remove the driver to see if someone would complain.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [SCSI] bnx2fc: zero out sense buffer properly

[FUJITA Tomonori]

On Sat, 18 Aug 2012 11:46:37 +0300
Dan Carpenter  wrote:

> ->sense_buffer used to be an array but it changed to pointer in
> de25deb180 "[SCSI] use dynamically allocated sense buffer".  This call
> to memset() needs to be updated as well.
> 
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
> index 73f231c..8d4626c 100644
> --- a/drivers/scsi/bnx2fc/bnx2fc_io.c
> +++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
> @@ -1807,7 +1807,7 @@ static void bnx2fc_parse_fcp_rsp(struct bnx2fc_cmd 
> *io_req,
>   fcp_sns_len = SCSI_SENSE_BUFFERSIZE;
>   }
>  
> - memset(sc_cmd->sense_buffer, 0, sizeof(sc_cmd->sense_buffer));
> + memset(sc_cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);

I guess that you can remove the line instead. IIRC, scsi-ml does it
for LLDs.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ps3rom: Simplify fill_from_dev_buffer()

[FUJITA Tomonori]

On Mon, 25 Feb 2008 14:24:31 +0100 (CET)
Geert Uytterhoeven <[EMAIL PROTECTED]> wrote:

> Subject: [PATCH] ps3rom: Simplify fill_from_dev_buffer()
> 
> As we no longer need to calculate the data length of the whole scatterlist,
> we can abort the loop earlier and coalesce req_len and act_len into one
> variable, making fill_from_dev_buffer() more similar to fetch_to_dev_buffer().

I'll add new APIs to copy data between a sg list and a buffer soon
after cleaning up (and fixing) some drivers on this area. I plan to
remove fill_from_dev_buffer and fetch_to_dev_buffer in ps3rom. As you
know, they are same functions that scsi_debug uses. There are other
drivers that need similar functions. I expect my ps3rom patch to be
applied to the scsi-fixes tree so I didn't change much as you did in
this patch.
-
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] ps3rom: fix wrong resid calculation bug

[FUJITA Tomonori]

sg driver rounds up the length in struct scatterlist to be a multiple
of 512 in some conditions. So LLDs can't use the data length in a sg
list to calculate residual. Instead, the length in struct scsi_cmnd
should be used.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
Cc: Geert Uytterhoeven <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
 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..6944bda 100644
--- a/drivers/scsi/ps3rom.c
+++ b/drivers/scsi/ps3rom.c
@@ -124,7 +124,7 @@ static int fill_from_dev_buffer(struct scsi_cmnd *cmd, 
const void *buf)
}
req_len += sgpnt->length;
}
-   scsi_set_resid(cmd, req_len - act_len);
+   scsi_set_resid(cmd, scsi_bufflen(cmd) - act_len);
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

[PATCH] scsi_debug: fix wrong resid calculation bug

[FUJITA Tomonori]

sg driver rounds up the length in struct scatterlist to be a multiple
of 512 in some conditions. So LLDs can't use the data length in a sg
list to calculate residual. Instead, the length in struct scsi_cmnd
should be used.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
Cc: Douglas Gilbert <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
 drivers/scsi/scsi_debug.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index d1777a9..691efd9 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -651,7 +651,7 @@ static int fill_from_dev_buffer(struct scsi_cmnd * scp, 
unsigned char * arr,
if (sdb->resid)
sdb->resid -= act_len;
else
-   sdb->resid = req_len - act_len;
+   sdb->resid = scsi_bufflen(scp) - act_len;
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

[PATCH] aacraid: READ_CAPACITY_16 shouldn't trust allocation length in cdb

[FUJITA Tomonori]

When aacraid spoofs READ_CAPACITY_16, it assumes that the data length
in the sg list is equal to allocation length in cdb. But sg can put
any value in scb so the driver needs to check both the data length in
the sg list and allocation length in cdb.

If allocation length is larger than the response length that the
driver expects, it clears the data buffer in the sg list to zero but
it doesn't need to do. Just setting resid is fine.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
Cc: Mark Salyzyn <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
 drivers/scsi/aacraid/aachba.c |   22 +++---
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index c05092f..b9fc9b1 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -2047,6 +2047,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
{
u64 capacity;
char cp[13];
+   unsigned int alloc_len;
 
dprintk((KERN_DEBUG "READ CAPACITY_16 command.\n"));
capacity = fsa_dev_ptr[cid].size - 1;
@@ -2063,18 +2064,17 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
cp[10] = 2;
cp[11] = 0;
cp[12] = 0;
-   aac_internal_transfer(scsicmd, cp, 0,
- min_t(size_t, scsicmd->cmnd[13], sizeof(cp)));
-   if (sizeof(cp) < scsicmd->cmnd[13]) {
-   unsigned int len, offset = sizeof(cp);
 
-   memset(cp, 0, offset);
-   do {
-   len = min_t(size_t, scsicmd->cmnd[13] - offset,
-   sizeof(cp));
-   aac_internal_transfer(scsicmd, cp, offset, len);
-   } while ((offset += len) < scsicmd->cmnd[13]);
-   }
+   alloc_len = ((scsicmd->cmnd[10] << 24)
++ (scsicmd->cmnd[11] << 16)
++ (scsicmd->cmnd[12] << 8) + scsicmd->cmnd[13]);
+
+   alloc_len = min_t(size_t, alloc_len, sizeof(cp));
+   aac_internal_transfer(scsicmd, cp, 0, alloc_len);
+
+   if (alloc_len < scsi_bufflen(scsicmd))
+   scsi_set_resid(scsicmd,
+  scsi_bufflen(scsicmd) - alloc_len);
 
/* Do not cache partition table for arrays */
scsicmd->device->removable = 1;
-- 
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: [PATCH] ips: sg chaining support to the path to non I/O commands

[FUJITA Tomonori]

On Tue, 19 Feb 2008 04:39:00 -0800
"Salyzyn, Mark" <[EMAIL PROTECTED]> wrote:

> ACK

Thanks!


> Other RAID drivers (eg: aacraid) makes the assumption that commands
> in these paths (INQUIRY, READ CAPACITY, MODE SENSE etc spoofing) are
> single scatter gather elements and have yet to be bitten. I agree
> with Fujita-san about the practical unlikelihood. The fix does not
> incur any change in code overhead, so it is worth hardening!
>
> Can one create a request via /dev/sg for a buffer smaller than 256
> and manage to create a multi-element scatter gather?

I think that we can do post 2.6.24 since we relaxed the default
alignment requirements (from 511 to 3). So a buffer more than 8 bytes
can leads to multi-element scatter gathers though it rarely happens.

Shortly I will submit the helper functions to copy data between sg
list and a buffer. It can replace aac_internal_transfer but it's not
for scsi-rc-fixes. If you worry that aac_internal_transfer can't
handle multiple sg entries, you can do something like this, I think:


diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index ae5f74f..73fa7c2 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -455,6 +455,12 @@ static int aac_slave_configure(struct scsi_device *sdev)
return 0;
 }
 
+static int aac_slave_alloc(struct scsi_device *sdev)
+{
+   blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
+   return 0;
+}
+
 /**
  * aac_change_queue_depth  -   alter queue depths
  * @sdev:  SCSI device we are considering
@@ -1015,6 +1021,7 @@ static struct scsi_host_template aac_driver_template = {
.queuecommand   = aac_queuecommand,
.bios_param = aac_biosparm,
.shost_attrs= aac_attrs,
+   .slave_alloc= aac_slave_alloc,
.slave_configure= aac_slave_configure,
.change_queue_depth = aac_change_queue_depth,
.sdev_attrs = aac_dev_attrs,



=
Here's a malicious version of sg_inq, which tries to create multiple
sg entries:

=
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

#define INQ_REPLY_LEN 96
#define INQ_CMD_LEN 6

int main(int argc, char **argv)
{
struct sg_io_hdr hdr;
unsigned char scb[INQ_CMD_LEN] = {0x12, 0, 0, 0, INQ_REPLY_LEN, 0};
unsigned char sense[32];
void *buf;
int offset = 4096 - 4;
int fd, ret;

buf = valloc(8192);

memset(&hdr, 0, sizeof(hdr));

hdr.interface_id = 'S';
hdr.cmd_len = sizeof(scb);
hdr.mx_sb_len = sizeof(sense);
hdr.dxfer_direction = SG_DXFER_FROM_DEV;
hdr.dxfer_len = INQ_REPLY_LEN;
hdr.dxferp = buf + offset;
hdr.cmdp = scb;
hdr.sbp = sense;
hdr.flags |= SG_FLAG_DIRECT_IO;

fd = open(argv[1], O_RDONLY);
if (fd < 0) {
fprintf(stderr, "fail to open %s\n", argv[1]);
return fd;
}

ret = ioctl(fd, SG_IO, &hdr);
if (ret < 0) {
fprintf(stderr, "fail to ioctl %d\n", ret);
return ret;
}

return ret;
}
-
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 1/2] stex: stex_direct_copy shouldn't call dma_map_sg

[FUJITA Tomonori]

stex_direct_copy copies an in-kernel buffer to a sg list in order to
spoof some SCSI commands. stex_direct_copy calls dma_map_sg and then
stex_internal_copy with the value that dma_map_sg returned. It calls
scsi_kmap_atomic_sg to copy data.

scsi_kmap_atomic_sg doesn't see sg->dma_length so if dma_map_sg merges
sg entries, stex_internal_copy gets the smaller number of sg entries
than the acutual number, which means it wrongly think that the data
length in the sg list is shorter than the actual length.

stex_direct_copy shouldn't call dma_map_sg and it doesn't need since
this code path doesn't involve dma transfers. This patch removes
stex_direct_copy and simply calls stex_internal_copy with the actual
number of sg entries.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
Cc: Ed Lin <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
 drivers/scsi/stex.c |   34 --
 1 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index 72f6d80..4b6861c 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -461,23 +461,6 @@ static void stex_internal_copy(struct scsi_cmnd *cmd,
}
 }
 
-static int stex_direct_copy(struct scsi_cmnd *cmd,
-   const void *src, size_t count)
-{
-   size_t cp_len = count;
-   int n_elem = 0;
-
-   n_elem = scsi_dma_map(cmd);
-   if (n_elem < 0)
-   return 0;
-
-   stex_internal_copy(cmd, src, &cp_len, n_elem, ST_TO_CMD);
-
-   scsi_dma_unmap(cmd);
-
-   return cp_len == count;
-}
-
 static void stex_controller_info(struct st_hba *hba, struct st_ccb *ccb)
 {
struct st_frame *p;
@@ -569,8 +552,10 @@ stex_queuecommand(struct scsi_cmnd *cmd, void (* 
done)(struct scsi_cmnd *))
unsigned char page;
page = cmd->cmnd[2] & 0x3f;
if (page == 0x8 || page == 0x3f) {
-   stex_direct_copy(cmd, ms10_caching_page,
-   sizeof(ms10_caching_page));
+   size_t cp_len = sizeof(ms10_caching_page);
+   stex_internal_copy(cmd, ms10_caching_page,
+  &cp_len, scsi_sg_count(cmd),
+  ST_TO_CMD);
cmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8;
done(cmd);
} else
@@ -599,8 +584,10 @@ stex_queuecommand(struct scsi_cmnd *cmd, void (* 
done)(struct scsi_cmnd *))
if (id != host->max_id - 1)
break;
if (lun == 0 && (cmd->cmnd[1] & INQUIRY_EVPD) == 0) {
-   stex_direct_copy(cmd, console_inq_page,
-   sizeof(console_inq_page));
+   size_t cp_len = sizeof(console_inq_page);
+   stex_internal_copy(cmd, console_inq_page,
+  &cp_len, scsi_sg_count(cmd),
+  ST_TO_CMD);
cmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8;
done(cmd);
} else
@@ -609,6 +596,7 @@ stex_queuecommand(struct scsi_cmnd *cmd, void (* 
done)(struct scsi_cmnd *))
case PASSTHRU_CMD:
if (cmd->cmnd[1] == PASSTHRU_GET_DRVVER) {
struct st_drvver ver;
+   size_t cp_len = sizeof(ver);
ver.major = ST_VER_MAJOR;
ver.minor = ST_VER_MINOR;
ver.oem = ST_OEM;
@@ -616,7 +604,9 @@ stex_queuecommand(struct scsi_cmnd *cmd, void (* 
done)(struct scsi_cmnd *))
ver.signature[0] = PASSTHRU_SIGNATURE;
ver.console_id = host->max_id - 1;
ver.host_no = hba->host->host_no;
-   cmd->result = stex_direct_copy(cmd, &ver, sizeof(ver)) ?
+   stex_internal_copy(cmd, &ver, &cp_len,
+  scsi_sg_count(cmd), ST_TO_CMD);
+   cmd->result = sizeof(ver) == cp_len ?
DID_OK << 16 | COMMAND_COMPLETE << 8 :
DID_ERROR << 16 | COMMAND_COMPLETE << 8;
done(cmd);
-- 
1.5.3.4

-
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/2] stex: stex_internal_copy should be called with sg_count in struct st_ccb

[FUJITA Tomonori]

stex_internal_copy copies an in-kernel buffer to a sg list by using
scsi_kmap_atomic_sg. Some functions calls stex_internal_copy with
sg_count in struct st_ccb, which is the value that dma_map_sg
returned. However it might be shorter than the actual number of sg
entries (if the IOMMU merged the sg entries).

scsi_kmap_atomic_sg doesn't see sg->dma_length so stex_internal_copy
should be called with the actual number of sg entries
(i.e. scsi_sg_count), because if the sg entries were merged,
stex_direct_copy wrongly think that the data length in the sg list is
shorter than the actual length.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
Cc: Ed Lin <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
 drivers/scsi/stex.c |   10 ++
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index 4b6861c..654430e 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -467,7 +467,8 @@ static void stex_controller_info(struct st_hba *hba, struct 
st_ccb *ccb)
size_t count = sizeof(struct st_frame);
 
p = hba->copy_buffer;
-   stex_internal_copy(ccb->cmd, p, &count, ccb->sg_count, ST_FROM_CMD);
+   stex_internal_copy(ccb->cmd, p, &count, scsi_sg_count(ccb->cmd),
+  ST_FROM_CMD);
memset(p->base, 0, sizeof(u32)*6);
*(unsigned long *)(p->base) = pci_resource_start(hba->pdev, 0);
p->rom_addr = 0;
@@ -485,7 +486,8 @@ static void stex_controller_info(struct st_hba *hba, struct 
st_ccb *ccb)
p->subid =
hba->pdev->subsystem_vendor << 16 | hba->pdev->subsystem_device;
 
-   stex_internal_copy(ccb->cmd, p, &count, ccb->sg_count, ST_TO_CMD);
+   stex_internal_copy(ccb->cmd, p, &count, scsi_sg_count(ccb->cmd),
+  ST_TO_CMD);
 }
 
 static void
@@ -699,7 +701,7 @@ static void stex_copy_data(struct st_ccb *ccb,
if (ccb->cmd == NULL)
return;
stex_internal_copy(ccb->cmd,
-   resp->variable, &count, ccb->sg_count, ST_TO_CMD);
+   resp->variable, &count, scsi_sg_count(ccb->cmd), ST_TO_CMD);
 }
 
 static void stex_ys_commands(struct st_hba *hba,
@@ -724,7 +726,7 @@ static void stex_ys_commands(struct st_hba *hba,
 
count = STEX_EXTRA_SIZE;
stex_internal_copy(ccb->cmd, hba->copy_buffer,
-   &count, ccb->sg_count, ST_FROM_CMD);
+   &count, scsi_sg_count(ccb->cmd), ST_FROM_CMD);
inq_data = (ST_INQ *)hba->copy_buffer;
if (inq_data->DeviceTypeQualifier != 0)
ccb->srb_status = SRB_STATUS_SELECTION_TIMEOUT;
-- 
1.5.3.4

-
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?

[FUJITA Tomonori]

On Tue, 19 Feb 2008 10:06:39 -0600
James Bottomley <[EMAIL PROTECTED]> wrote:

> On Tue, 2008-02-19 at 17:02 +0900, FUJITA Tomonori wrote:
> > ips did scsi_add_host(sh, NULL) so scsi_dma_map uses
> > shost_gendev.parent that isn't initialized properly, then the kernel
> > crashes. 2.6.23 and 2.6.24 have this bug.
> > 
> > We can fix this by calling scsi_add_host with pdev->dev, in the
> > standard way (like the following way) but this bug was fixed in the
> > current Linus tree by:
> > 
> > commit 2551a13e61d3c3df6c2da6de5a3ece78e6d67111
> > Author: Jeff Garzik <[EMAIL PROTECTED]>
> > Date:   Thu Dec 13 16:14:10 2007 -0800
> > 
> > [SCSI] ips: handle scsi_add_host() failure, and other err cleanups
> > 
> > 
> > James, the legitimate way to fix stable trees is sending this commit
> > (not sending a patch that was not committed upstream)?
> 
> Well, the upstream patch doesn't look so bad as a stable candidate to my
> eye.  Just because it's an unintended bugfix doesn't automatically
> invalidate it.
> 
> The reason stable likes backports of existing upstream patches is
> because they've supposedly been well tested in upstream.  Although that
> doesn't apply in this case because the other bug rather prevented
> testing, the principle is still sound.
> 
> So, would there be any problems simply backporting this?

Thanks, I see. There is no problem. Please backport this.
-
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: SCSI RAM driver

[FUJITA Tomonori]

On Tue, 19 Feb 2008 06:31:20 -0700
Matthew Wilcox <[EMAIL PROTECTED]> wrote:

> On Tue, Feb 19, 2008 at 10:14:53PM +0900, FUJITA Tomonori wrote:
> > I see that two drivers have very different objectives but if we add
> > use_thread option to scsi_debug (we can do easily), it seems that
> > scsi_debug can provide all the features that scsi_ram does.
> 
> It's not just use_thread.  It's also discard_read/discard_write.

scsi_debug has a similar option, fake_rw, which discards both read and
write data.


> And scsi_ram has a different data storage model from scsi_debug --
> scsi_debug simulates an arbitrarily sized disc by wrapping around some
> small (virtually) contiguous allocation of pages; scsi_ram actually
> allocates the amount of ram that it's told to.  This can be solved with
> another module parameter, of course.

IIRC, if virtual_gb option is set to zero, scsi_debug allocates the
amount of ram that it's told to.


> I'm in no way opposed to merging the two; it's a question of whether
> Doug will mind me doing some surgery on his driver.
-
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: SCSI RAM driver

[FUJITA Tomonori]

On Sun, 17 Feb 2008 23:36:59 -0700
Matthew Wilcox <[EMAIL PROTECTED]> wrote:

> 
> This is the first release of a driver we've been using internally at Intel
> for a few weeks which is as low-latency as possible.  It's designed to
> let us find latency issues elsewhere in the storage stack (eg filesystem,
> block layer, scsi layer)
> 
> There are a few different options, controlled through module parameters.
> The sector size and disc capacity are load-time parameters, but the
> parameters affecting performance are tweakable at runtime.
> 
> Arguably, this driver should be merged into scsi_debug.  I didn't want to
> trip over Doug's toes while developing this driver, and the two drivers
> do have very different objectives.  scsi_debug is obviously a lot more
> fully-featured and implements lots of bits of the spec I simply haven't
> bothered with.  Like MODE SENSE ;-)

I see that two drivers have very different objectives but if we add
use_thread option to scsi_debug (we can do easily), it seems that
scsi_debug can provide all the features that scsi_ram does.

If we clean up scsi_debug driver, can scsi_debug work as a nice scsi
ram driver?
-
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] ips: sg chaining support to the path to non I/O commands

[FUJITA Tomonori]

Here is another ips patch, but not a bug fix.

=
From: FUJITA Tomonori <[EMAIL PROTECTED]>
Subject: [PATCH] ips: sg chaining support to the path to non I/O commands

I overlooked ips_scmd_buf_write and ips_scmd_buf_read when I converted
ips to use the data buffer accessors.

ips is unlikely to use sg chaining (especially in this path) since a)
this path is used only for non I/O commands (with little data
transfer), b) ips's sg_tablesize is set to just 17.

Thanks to Tim Pepper for testing this patch.


Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
Cc: Salyzyn, Mark <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
 drivers/scsi/ips.c |   18 ++
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index 7ed568f..e5467a4 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -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;
-- 
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: ips.c broken since 2.6.23 on x86_64?

[FUJITA Tomonori]

On Mon, 18 Feb 2008 19:48:49 -0800
"Tim Pepper" <[EMAIL PROTECTED]> wrote:

> 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.

Thanks a lot!

I'll make sure that the ips driver in 2.6.23-stable, 2.6.24-stable
(and 2.6.25) will work well.
-
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] ips: fix data buffer accessors conversion bug

[FUJITA Tomonori]

There is one more bug in ips. I think that this needs to go to
scsi-rc-fixes, 2.6.24-stable, and 2.6.23-stable though we might rarely
hit this bug.

=
From: FUJITA Tomonori <[EMAIL PROTECTED]>
Date: Tue, 19 Feb 2008 16:03:47 +0900
Subject: [PATCH] ips: fix data buffer accessors conversion bug

This fixes a bug that can't handle a passthru command with more than
two sg entries.

Big thanks to Tim Pepper for debugging the problem.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
Cc: Tim Pepper <[EMAIL PROTECTED]>
Cc: Salyzyn, Mark <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
 drivers/scsi/ips.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index bb152fb..7ed568f 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 */
-- 
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: ips.c broken since 2.6.23 on x86_64?

[FUJITA Tomonori]

ips did scsi_add_host(sh, NULL) so scsi_dma_map uses
shost_gendev.parent that isn't initialized properly, then the kernel
crashes. 2.6.23 and 2.6.24 have this bug.

We can fix this by calling scsi_add_host with pdev->dev, in the
standard way (like the following way) but this bug was fixed in the
current Linus tree by:

commit 2551a13e61d3c3df6c2da6de5a3ece78e6d67111
Author: Jeff Garzik <[EMAIL PROTECTED]>
Date:   Thu Dec 13 16:14:10 2007 -0800

[SCSI] ips: handle scsi_add_host() failure, and other err cleanups


James, the legitimate way to fix stable trees is sending this commit
(not sending a patch that was not committed upstream)?


On Mon, 18 Feb 2008 22:32:46 +0900
FUJITA Tomonori <[EMAIL PROTECTED]> wrote:

> 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
-
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?

[FUJITA Tomonori]

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: [PATCH] bsg: bidi bio map failure fix

[FUJITA Tomonori]

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

Re: [PATCH] bsg: bidi bio map failure fix

[FUJITA Tomonori]

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: ips.c broken since 2.6.23 on x86_64?

[FUJITA Tomonori]

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: ips.c broken since 2.6.23 on x86_64?

[FUJITA Tomonori]

On Sun, 17 Feb 2008 13:15:40 -0800
Tim Pepper <[EMAIL PROTECTED]> wrote:

> On Sat 16 Feb at 09:41:48 +0900 [EMAIL PROTECTED] said:
> > 
> > Do you mean that you applied only the following two patches against
> > 2.6.24, and then it doesn't work?
> > 
> > 0001-ips-revert-the-changes-for-the-data-buffer-accessor.patch
> 
> I only applies the 0001 patch and the driver appeared to function fine.

I see, thanks. It would be nice if we can push only the 0001 patch to
mainline, however as I explained, we can't unfortunately. The
0002-0009 patches are necessary for post 2.6.24.


> > If so, the second patch is broken. Did you saw BUG_ON message (I added
> > some BUG_ON to the patch)?
> 
> I did not apply the 0002 patch.

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.

Please the patches in a step-by-step way and tell me which patch
breaks the driver.

Thanks,
-
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] scsi_debug: disable clustering

[FUJITA Tomonori]

On Sun, 17 Feb 2008 09:02:14 -0600
James Bottomley <[EMAIL PROTECTED]> wrote:

> 
> On Sun, 2008-02-17 at 23:52 +0900, FUJITA Tomonori wrote:
> > On Sun, 17 Feb 2008 07:28:48 -0700
> > Matthew Wilcox <[EMAIL PROTECTED]> wrote:
> > 
> > > On Sun, Feb 17, 2008 at 08:18:11AM -0600, James Bottomley wrote:
> > > > No, he means that kmap_atomic can only map a page of data.  This makes
> > > > single page only sg list entries and input assumption into this loop.
> > > > with ENABLE_CLUSTERING, that's potentially not true.   Of course, this
> > > > accidentally works most of the time because of the way kmap functions.
> > > 
> > > Ah, right.  I'm on the verge of releasing a ram-based scsi driver I've
> > > been working on ... this loop should work fine with clustering as it
> > > takes account of the sg potentially having multiple pages:
> > > 
> > > scsi_for_each_sg(cmnd, sg, scsi_sg_count(cmnd), i) {
> > > struct page *sgpage = sg_page(sg);
> > > unsigned int to_off = sg->offset;
> > > unsigned int sg_copy = sg->length;
> > > if (sg_copy > len)
> > > sg_copy = len;
> > > len -= sg_copy;
> > 
> > stex driver has a similar function to copies data between a buffer and
> > a scatter list. I think that scsi_kmap_atomic_sg is a bit primitive
> > (and not very popular).  I'll send a patch to add a helper function to
> > scsi_lib.c that copies data between a buffer and a scatter list. It
> > would be useful for several drivers.
> 
> Actually, if you're going to sweep up them all, libata also does this.

Thanks, I'll look at it.


> However, mapping and copying data isn't a SCSI specific function, it's
> one any virtual block driver should do, so I think block might be the
> correct location for such a function.

I see. It could go to lib/scatterlist.c or block/ I guess.
-
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] scsi_debug: disable clustering

[FUJITA Tomonori]

On Sun, 17 Feb 2008 07:28:48 -0700
Matthew Wilcox <[EMAIL PROTECTED]> wrote:

> On Sun, Feb 17, 2008 at 08:18:11AM -0600, James Bottomley wrote:
> > No, he means that kmap_atomic can only map a page of data.  This makes
> > single page only sg list entries and input assumption into this loop.
> > with ENABLE_CLUSTERING, that's potentially not true.   Of course, this
> > accidentally works most of the time because of the way kmap functions.
> 
> Ah, right.  I'm on the verge of releasing a ram-based scsi driver I've
> been working on ... this loop should work fine with clustering as it
> takes account of the sg potentially having multiple pages:
> 
> scsi_for_each_sg(cmnd, sg, scsi_sg_count(cmnd), i) {
> struct page *sgpage = sg_page(sg);
> unsigned int to_off = sg->offset;
> unsigned int sg_copy = sg->length;
> if (sg_copy > len)
> sg_copy = len;
> len -= sg_copy;

stex driver has a similar function to copies data between a buffer and
a scatter list. I think that scsi_kmap_atomic_sg is a bit primitive
(and not very popular).  I'll send a patch to add a helper function to
scsi_lib.c that copies data between a buffer and a scatter list. It
would be useful for several drivers.
-
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] ps3rom: disable clustering

[FUJITA Tomonori]

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]>
---
 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,
 };
 
-- 
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

[PATCH] scsi_debug: disable clustering

[FUJITA Tomonori]

From: FUJITA Tomonori <[EMAIL PROTECTED]>
Subject: [PATCH] scsi_debug: disable clustering

scsi_debug does at several places:

for_each_sg(sdb->table.sgl, sg, sdb->table.nents, k) {
kaddr = (unsigned char *)
kmap_atomic(sg_page(sg), KM_USER0);


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]>
---
 drivers/scsi/scsi_debug.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 1541c17..d1777a9 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -222,7 +222,7 @@ static struct scsi_host_template sdebug_driver_template = {
.cmd_per_lun =  16,
.max_sectors =  0x,
.unchecked_isa_dma =0,
-   .use_clustering =   ENABLE_CLUSTERING,
+   .use_clustering =   DISABLE_CLUSTERING,
.module =   THIS_MODULE,
 };
 
-- 
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

[PATCH] qla2xxx: fix compilation compile

[FUJITA Tomonori]

scsi/qla2xxx/qla_dfs.c: In function 'qla2x00_dfs_fce_show':
scsi/qla2xxx/qla_dfs.c:26: warning: format '%llx' expects type 'long long 
unsigned int', but argument 3 has type 'uint64_t'

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/qla2xxx/qla_dfs.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_dfs.c b/drivers/scsi/qla2xxx/qla_dfs.c
index 1479c60..2cd899b 100644
--- a/drivers/scsi/qla2xxx/qla_dfs.c
+++ b/drivers/scsi/qla2xxx/qla_dfs.c
@@ -23,7 +23,7 @@ qla2x00_dfs_fce_show(struct seq_file *s, void *unused)
mutex_lock(&ha->fce_mutex);
 
seq_printf(s, "FCE Trace Buffer\n");
-   seq_printf(s, "In Pointer = %llx\n\n", ha->fce_wr);
+   seq_printf(s, "In Pointer = %llx\n\n", (unsigned long long)ha->fce_wr);
seq_printf(s, "Base = %llx\n\n", (unsigned long long) ha->fce_dma);
seq_printf(s, "FCE Enable Registers\n");
seq_printf(s, "%08x %08x %08x %08x %08x %08x\n",
-- 
1.5.3.4

-
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?

[FUJITA Tomonori]

On Fri, 15 Feb 2008 14:50:57 -0800
Tim Pepper <[EMAIL PROTECTED]> wrote:

> On Sat 16 Feb at 01:09:43 +0900 [EMAIL PROTECTED] said:
> > 
> > The first one is just reverting the data buffer accessors
> > conversion. It would be nice if we could just revert it but we
> > can't. These changes are necessary to compile the driver against post
> > 2.6.24.
> 
> Fujita-san,
> 
> Unfortunately (and not too surprisingly given what we've tried so far) with
> only the first of your series reverted the driver is working fine for me
> again.

Do you mean that you applied only the following two patches against
2.6.24, and then it doesn't work?

0001-ips-revert-the-changes-for-the-data-buffer-accessor.patch
0002-ips-kill-the-map_single-path-in-ips_scmd_buf_write.patch

If so, the second patch is broken. Did you saw BUG_ON message (I added
some BUG_ON to the patch)?


> I saw (eg: replies to http://lkml.org/lkml/2007/5/11/132) some possibly
> similar sounding issues with other drivers.  Could there be some memory
> uninitialised?  I did try changing all the ips.c kmalloc's to kzalloc's,
> but that didn't help.  Also that thread ties into pci gart.  The machines
> we've been using are liable to getting pci calgary although given my
> .config has:
> CONFIG_GART_IOMMU=y
> CONFIG_CALGARY_IOMMU=y
> # CONFIG_CALGARY_IOMMU_ENABLED_BY_DEFAULT is not set
> and that when booting this mainline I don't see any Calgary related
> messages like I get from eg: Ubuntu's 2.6.22-14-server...I'm probably not
> actually running the calgary iommu code in these repros.

Yes, probabaly, your machine doesn't use any IOMMU hardware
(nommu_map_sg function was in your crash log).


> Anyway, I greatly appreciate your efforts so far in trying to find what
> could be wrong here!

Really sorry about the troubles and thanks for testing.
-
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?

[FUJITA Tomonori]

On Thu, 14 Feb 2008 17:16:36 -0800
Tim Pepper <[EMAIL PROTECTED]> wrote:

> On Fri 15 Feb at 09:13:16 +0900 [EMAIL PROTECTED] said:
> > 
> > Thanks. So we surely have a bug in the non-breakup part.
> > 
> > I've just found one bug. Can you try this patch against 2.6.24?
> 
> Tested and unfortunately no change.  Behaves same as the breakup-revert patch.

Thanks and sorry about that.

Now I don't have any idea. I split the patch. Can you please apply
them in a step-by-step way and tell me which patch breaks the driver?

There are nine patches against 2.6.24:

http://www.kernel.org/pub/linux/kernel/people/tomo/ips/

The first one is just reverting the data buffer accessors
conversion. It would be nice if we could just revert it but we
can't. These changes are necessary to compile the driver against post
2.6.24.

Really sorry about the troubles,
-
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?

[FUJITA Tomonori]

On Thu, 14 Feb 2008 15:55:49 -0800
Tim Pepper <[EMAIL PROTECTED]> wrote:

> On Thu 14 Feb at 20:48:38 +0900 [EMAIL PROTECTED] said:
> > 
> > I have a slight doubt on the breakup code though I'm not sure you hit
> > the code. Reverting only the breakup part works? The patch is against
> > 2.6.24.
> 
> I've tested this revert you posted.  Essentially the same trace is logged
> (see below).  The machine doesn't die though.  But /proc/scsi/scsi doesn't
> show the hardware and I am getting an additional trace dumped now (see
> futher below).

Thanks. So we surely have a bug in the non-breakup part.

I've just found one bug. Can you try this patch against 2.6.24?

Thanks,

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index 5c5a9b2..d8f5eb5 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -1575,12 +1575,12 @@ ips_make_passthru(ips_ha_t *ha, struct scsi_cmnd *SC, 
ips_scb_t *scb, int intr)
ips_passthru_t *pt;
int length = 0;
int i, ret;
-struct scatterlist *sg = scsi_sglist(SC);
+struct scatterlist *sg;
 
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 */
-
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

[FUJITA Tomonori]

On Tue, 12 Feb 2008 15:40:24 -0500
Pete Wyckoff <[EMAIL PROTECTED]> 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 */
>   goto out;
> + }
>   }
>  
>   if (hdr->dout_xfer_len) {

Thanks!

Acked-by: FUJITA Tomonori <[EMAIL PROTECTED]>

James, please put this to the scsi-fixes tree.
-
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?

[FUJITA Tomonori]

On Wed, 13 Feb 2008 13:43:24 -0800
Tim Pepper <[EMAIL PROTECTED]> wrote:

> We recently upgraded a production x86_64 machine with serveraid
> cards to 2.6.24 and noted that /proc/scsi/scsi showed garbage for our
> serveraid service processors.  sg_inq also returned garbage from the
> service processors' sg devices.  After a few iterations I started seeing
> meaninful stuff in the garbage.  Not sure if it was returning live memory
> or just unzero'd.  Either way not good so we went back to a known good,
> older kernel and tried to repro on a similar machine.  We got different,
> but still bad results in terms of pointing at memory badness.
> 
> FWIW, the original machine had the following hardware:
> scsi0 : IBM PCI ServeRAID 7.12.05  Build 761 
> scsi1 : IBM PCI ServeRAID 7.12.05  Build 761 
> and the repro's have been on a machine with just:
> scsi0 : IBM PCI ServeRAID 7.12.05  Build 761 
> 
> On the repro machine I'm getting a hang on ips driver load with the following
> logged:
> 
> Feb 13 13:16:08 ipstest kernel: [  915.236563] scsi3 : IBM PCI ServeRAID 
> 7.12.05  Build 761 
> Feb 13 13:16:08 ipstest kernel: [  915.236839] Unable to handle kernel NULL 
> pointer dereference at  RIP:
> Feb 13 13:16:08 ipstest kernel: [  915.236863]  [check_addr+16/80] 
> check_addr+0x10/0x50
> Feb 13 13:16:08 ipstest kernel: [  915.237209] PGD 79fff067 PUD 7a898067 PMD 0
> Feb 13 13:16:08 ipstest kernel: [  915.237341] Oops:  [1] SMP
> Feb 13 13:16:08 ipstest kernel: [  915.237463] CPU 1
> Feb 13 13:16:08 ipstest kernel: [  915.239436] Modules linked in: ips aic94xx
> Feb 13 13:16:08 ipstest kernel: [  915.239559] Pid: 5213, comm: scsi_scan_3 
> Not tainted 2.6.23-ips_as_module #3
> Feb 13 13:16:08 ipstest kernel: [  915.239692] RIP: 0010:[check_addr+16/80]  
> [check_addr+16/80] check_addr+0x10/0x50
> Feb 13 13:16:08 ipstest kernel: [  915.239932] RSP: 0018:810076d87900  
> EFLAGS: 00010082
> Feb 13 13:16:08 ipstest kernel: [  915.240059] RAX:  RBX: 
> 81007b636300 RCX: 0024
> Feb 13 13:16:08 ipstest kernel: [  915.240196] RDX: 7b636b00 RSI: 
> 8077cde0 RDI: 806c4ed5
> Feb 13 13:16:08 ipstest kernel: [  915.240332] RBP: 810076d87900 R08: 
> 0500 R09: 
> Feb 13 13:16:08 ipstest kernel: [  915.240468] R10: 81007aa33b40 R11: 
> 0060 R12: 
> Feb 13 13:16:08 ipstest kernel: [  915.240605] R13: 0001 R14: 
> 8077cde0 R15: 81007aa33a80
> Feb 13 13:16:08 ipstest kernel: [  915.240741] FS:  () 
> GS:810001039300() knlGS:
> Feb 13 13:16:08 ipstest kernel: [  915.240981] CS:  0010 DS: 0018 ES: 0018 
> CR0: 8005003b
> Feb 13 13:16:08 ipstest kernel: [  915.24] CR2:  CR3: 
> 78a98000 CR4: 06e0
> Feb 13 13:16:08 ipstest kernel: [  915.241248] DR0:  DR1: 
>  DR2: 
> Feb 13 13:16:08 ipstest kernel: [  915.241384] DR3:  DR6: 
> 0ff0 DR7: 0400
> Feb 13 13:16:08 ipstest kernel: [  915.241520] Process scsi_scan_3 (pid: 
> 5213, threadinfo 810076d86000, task 81007be26720)
> Feb 13 13:16:08 ipstest kernel: [  915.241761] Stack:  810076d87930 
> 802125c3 81007aa33a80 81007480cf50
> Feb 13 13:16:08 ipstest kernel: [  915.242006]   
> 81007ba38ca8 810076d87940 8046fb42
> Feb 13 13:16:08 ipstest kernel: [  915.242248]  810076d879c0 
> 8801c2ee 81007aa33af0 00017aa33af0
> Feb 13 13:16:08 ipstest kernel: [  915.242389] Call Trace:
> Feb 13 13:16:08 ipstest kernel: [  915.242606]  [nommu_map_sg+115/144] 
> nommu_map_sg+0x73/0x90
> Feb 13 13:16:08 ipstest kernel: [  915.242736]  [scsi_dma_map+66/96] 
> scsi_dma_map+0x42/0x60
> Feb 13 13:16:08 ipstest kernel: [  915.242867]  [_end+124884230/2127548952] 
> :ips:ips_next+0x33e/0xc00
> Feb 13 13:16:08 ipstest kernel: [  915.242986]  [scsi_done+0/48] 
> scsi_done+0x0/0x30
> Feb 13 13:16:08 ipstest kernel: [  915.243114]  [_end+124896894/2127548952] 
> :ips:ips_queue+0x106/0x1f0
> Feb 13 13:16:08 ipstest kernel: [  915.243240]  [scsi_dispatch_cmd+498/784] 
> scsi_dispatch_cmd+0x1f2/0x310
> Feb 13 13:16:08 ipstest kernel: [  915.243370]  [scsi_request_fn+491/976] 
> scsi_request_fn+0x1eb/0x3d0
> Feb 13 13:16:08 ipstest kernel: [  915.243500]  
> [__generic_unplug_device+37/48] __generic_unplug_device+0x25/0x30
> Feb 13 13:16:08 ipstest kernel: [  915.243630]  
> [blk_execute_rq_nowait+99/176] blk_execute_rq_nowait+0x63/0xb0
> Feb 13 13:16:08 ipstest kernel: [  915.243761]  [blk_execute_rq+122/224] 
> blk_execute_rq+0x7a/0xe0
> Feb 13 13:16:08 ipstest kernel: [  915.243889]  [scsi_execute+240/288] 
> scsi_execute+0xf0/0x120
> Feb 13 13:16:08 ipstest kernel: [  915.244016]  [scsi_execute_req+134/240] 
> scsi_execute_req+0x86/0xf0
> Feb 13 13:16:08 ipstest kernel: [  915.24414

Re: [GIT PATCH] final SCSI updates for 2.6.24 merge window

[FUJITA Tomonori]

On Thu, 07 Feb 2008 19:37:07 -0600
James Bottomley <[EMAIL PROTECTED]> wrote:

> 
> On Thu, 2008-02-07 at 18:56 -0600, James Bottomley wrote:
> > Quite a bit of this is fixing things broken previously (the advansys fix
> > is still pending resolution, but I'll send it as an -rc fix when we have
> > it).  There's the final elimination of all drivers that are esp based
> > but don't use the scsi_esp core (that's mostly m68k and alpha).  Plus
> > the usual bunch of driver updates and the addition of a new enclosure
> > services driver and the corresponding ULD.
> 
> OK, the advansys fix came in.  I've added it to the patch.
> 
> James
> 
> 
> 
> >From f983323fea178352ed3b69c70561a13825a3ce59 Mon Sep 17 00:00:00 2001
> From: FUJITA Tomonori <[EMAIL PROTECTED]>
> Date: Fri, 8 Feb 2008 09:50:08 +0900
> Subject: [SCSI] advansys: fix overrun_buf aligned bug

Seems that it was a bit late, Linus pulled scsi-misc before the patch
was added.
-
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] advansys: fix overrun_buf aligned bug

[FUJITA Tomonori]

On Thu, 07 Feb 2008 19:01:55 -0600
James Bottomley <[EMAIL PROTECTED]> wrote:

> 
> On Fri, 2008-02-08 at 09:50 +0900, FUJITA Tomonori wrote:
> > struct asc_dvc_var needs overrun buffer to be placed on an 8 byte
> > boundary. advansys defines struct asc_dvc_var:
> > 
> > struct asc_dvc_var {
> > ...
> > uchar overrun_buf[ASC_OVERRUN_BSIZE] __aligned(8);
> > 
> > The problem is that struct asc_dvc_var is placed on
> > shost->hostdata. So if the hostdata is not on an 8 byte boundary, the
> > advansys crashes. The hostdata is placed on a sizeof(unsigned long)
> > boundary so the 8 byte boundary is not garanteed with x86_32.
> > 
> > With 2.6.23 and 2.6.24, the hostdata is on an 8 byte boundary by
> > chance, but with the current git, it's not.
> > 
> > This patch removes overrun_buf static array and use kzalloc.
> 
> It's a bit of a waste of a kmallocs.  The usual way of fixing this type
> of cockup is to float the structure until it becomes aligned, but I
> suppose that involves changing all calls to shost_priv in the driver ...

Yeah, agreed. It's better but I'm not familiar with the driver so I
use kmalloc. It's not so bad as a short-term solution, I think.

Any chance to push it to final SCSI updates for 2.6.24 merge window?
Though we can push it any time since it's a bug fix.

Anyway, I'm fine with dropping it if Matthew will fix the driver in a
better way. I'm happy unless people blame my IOMMU or sense buffer
patch for this bug. :)
-
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] advansys: fix overrun_buf aligned bug

[FUJITA Tomonori]

struct asc_dvc_var needs overrun buffer to be placed on an 8 byte
boundary. advansys defines struct asc_dvc_var:

struct asc_dvc_var {
...
uchar overrun_buf[ASC_OVERRUN_BSIZE] __aligned(8);

The problem is that struct asc_dvc_var is placed on
shost->hostdata. So if the hostdata is not on an 8 byte boundary, the
advansys crashes. The hostdata is placed on a sizeof(unsigned long)
boundary so the 8 byte boundary is not garanteed with x86_32.

With 2.6.23 and 2.6.24, the hostdata is on an 8 byte boundary by
chance, but with the current git, it's not.

This patch removes overrun_buf static array and use kzalloc.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/advansys.c |   13 +++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index ccef891..3c2d688 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -566,7 +566,7 @@ typedef struct asc_dvc_var {
ASC_SCSI_BIT_ID_TYPE unit_not_ready;
ASC_SCSI_BIT_ID_TYPE queue_full_or_busy;
ASC_SCSI_BIT_ID_TYPE start_motor;
-   uchar overrun_buf[ASC_OVERRUN_BSIZE] __aligned(8);
+   uchar *overrun_buf;
dma_addr_t overrun_dma;
uchar scsi_reset_wait;
uchar chip_no;
@@ -13833,6 +13833,12 @@ static int __devinit advansys_board_found(struct 
Scsi_Host *shost,
 */
if (ASC_NARROW_BOARD(boardp)) {
ASC_DBG(2, "AscInitAsc1000Driver()\n");
+
+   asc_dvc_varp->overrun_buf = kzalloc(ASC_OVERRUN_BSIZE, 
GFP_KERNEL);
+   if (!asc_dvc_varp->overrun_buf) {
+   ret = -ENOMEM;
+   goto err_free_wide_mem;
+   }
warn_code = AscInitAsc1000Driver(asc_dvc_varp);
 
if (warn_code || asc_dvc_varp->err_code) {
@@ -13840,8 +13846,10 @@ static int __devinit advansys_board_found(struct 
Scsi_Host *shost,
"warn 0x%x, error 0x%x\n",
asc_dvc_varp->init_state, warn_code,
asc_dvc_varp->err_code);
-   if (asc_dvc_varp->err_code)
+   if (asc_dvc_varp->err_code) {
ret = -ENODEV;
+   kfree(asc_dvc_varp->overrun_buf);
+   }
}
} else {
if (advansys_wide_init_chip(shost))
@@ -13894,6 +13902,7 @@ static int advansys_release(struct Scsi_Host *shost)
dma_unmap_single(board->dev,
board->dvc_var.asc_dvc_var.overrun_dma,
ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
+   kfree(board->dvc_var.asc_dvc_var.overrun_buf);
} else {
iounmap(board->ioremap_addr);
advansys_wide_free_mem(board);
-- 
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: Latest git oopses during boot

[FUJITA Tomonori]

On Thu, 7 Feb 2008 23:24:00 +0100
"Harald Arnesen" <[EMAIL PROTECTED]> wrote:

> On 2/7/08, Linus Torvalds <[EMAIL PROTECTED]> wrote:
> >
> >
> > On Thu, 7 Feb 2008, Harald Arnesen wrote:
> > >
> > > I'll try applying the patch to a freshly downloaded git-tree.
> >
> > Ok, good.
> >
> > > Shall I try another compiler? I have at least these two:
> > >
> > > gcc version 3.4.6 (Ubuntu 3.4.6-6ubuntu2)
> > > gcc version 4.1.3 20070929 (prerelease) (Ubuntu 4.1.2-16ubuntu2)
> >
> > I would suggest a patch mis-application problem first (or possibly even
> > the patch itself being broken - I simply didn't look very closely at the
> > patch, but it *looked* ok).
> >
> > If it's a compiler bug, it's a pretty big one, and quite frankly, I doubt
> > it. Compiler bugs do happen, but they are pretty rare, and they tend to
> > have more subtle effects than the one you see.
> >
> > However:
> >
> > > in addition to the self-compiled 4.2.3 I used for the tests.
> >
> > 4.2.3? Really? That's pretty damn recent, and so almost totally untested.
> > That does make a compiler bug at least more likely.
> >
> > So yes, if you already have other compilers installed, you should try
> > them. If it really is a compiler bug, it's a really bad one, and you would
> > want to let the gcc people know.
> >
> > Still, I'd double-check that the
> >
> > asc_dvc_varp->overrun_buf = kzalloc(ASC_OVERRUN_BSIZE, GFP_KERNEL);
> >
> > line was added properly first. You should see it way after the point where
> > it did
> >
> > asc_dvc_varp = &boardp->dvc_var.asc_dvc_var;
> >
> > to initialize it (and both statements should be inside a
> >
> > if (ASC_NARROW_BOARD(boardp)) {
> >
> > conditional - please check that the source code looks sane too).
> >
> > Linus
> 
> I just re-downloaded an re-patched and re-compiled (with gcc 4.2.3),
> and now the kernel boots. I must have screwed up the previous
> patching.
> 
> It now works, with Fujita's patch applied.

Thanks Harald and Linus,

The bug has been in the advansys driver. 2.6.23 and 2.6.24 works just
because the size of Scsi_Host structure was multiples of 8. After
2.6.24, some patches change Scsi_Host structure and now the size is
not multiples of 8. So we hit this bug.


I'll resend the patch with a proper description.
-
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: Latest git oopses during boot

[FUJITA Tomonori]

On Thu, 7 Feb 2008 12:14:56 +0100
"Harald Arnesen" <[EMAIL PROTECTED]> wrote:

> On 2/7/08, Andrew Morton <[EMAIL PROTECTED]> wrote:
> >
> > (cc's restored, and expanded a bit)
> 
> Ah, sorry, not used to gmail's web interface. Clicked the wrong button.
> 
> > > Seems to be the advansys driver, so I tried to remove it - and indeed,
> > > the kernel now boots. So I guess it's either that driver or my ancient
> > > Nikon Coolscan II that is the only thing attached to the board.
> >
> > Thanks.  I uploaded the oops picture to
> > http://userweb.kernel.org/~akpm/oops.jpg
> >
> > > Cc to the Matthew Wilcox added.
> >
> > mm...  looks like all Matthew's changes were in 2.6.23.  And 2.6.23 worked
> > OK, yes?
> 
> Both 2.6.23 and 2.6.24 are ok.
> 
> > The only recent changes to drivers/scsi/advansys.c are
> >
> > commit b80ca4f7ee36c26d300c5a8f429e73372d153379
> > Author: FUJITA Tomonori <[EMAIL PROTECTED]>
> > Date:   Sun Jan 13 15:46:13 2008 +0900
> >
> > [SCSI] replace sizeof sense_buffer with SCSI_SENSE_BUFFERSIZE
> >
> > This replaces sizeof sense_buffer with SCSI_SENSE_BUFFERSIZE in
> > several LLDs. It's a preparation for the future changes to remove
> > sense_buffer array in scsi_cmnd structure.
> >
> > Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
> > Signed-off-by: James Bottomley <[EMAIL PROTECTED]>
> >
> > :100644 100644 9dd3952... 492702b... M  drivers/scsi/advansys.c
> >
> > commit 747d016e7e25e216b31022fe2b012508d99fb682
> > Author: Randy Dunlap <[EMAIL PROTECTED]>
> > Date:   Mon Jan 14 00:55:18 2008 -0800
> >
> > advansys: fix section mismatch warning
> >
> > Fix section mismatch warning:
> >
> > WARNING: vmlinux.o(.exit.text+0x152a): Section mismatch: reference to 
> > .init.
> >
> > Signed-off-by: Randy Dunlap <[EMAIL PROTECTED]>
> > Cc: Matthew Wilcox <[EMAIL PROTECTED]>
> > Cc: James Bottomley <[EMAIL PROTECTED]>
> > Signed-off-by: Andrew Morton <[EMAIL PROTECTED]>
> > Signed-off-by: Linus Torvalds <[EMAIL PROTECTED]>
> >
> > which seem fairly benign.
> >
> >
> > gcc inlining is going to make it rather a lot of work to find out which
> > statement has actually oopsed there.
> -- 

Can you try this?

Thanks,

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index 374ed02..f5dde12 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -566,7 +566,7 @@ typedef struct asc_dvc_var {
ASC_SCSI_BIT_ID_TYPE unit_not_ready;
ASC_SCSI_BIT_ID_TYPE queue_full_or_busy;
ASC_SCSI_BIT_ID_TYPE start_motor;
-   uchar overrun_buf[ASC_OVERRUN_BSIZE] __aligned(8);
+   uchar *overrun_buf;
dma_addr_t overrun_dma;
uchar scsi_reset_wait;
uchar chip_no;
@@ -13833,6 +13833,12 @@ static int __devinit advansys_board_found(struct 
Scsi_Host *shost,
 */
if (ASC_NARROW_BOARD(boardp)) {
ASC_DBG(2, "AscInitAsc1000Driver()\n");
+
+   asc_dvc_varp->overrun_buf = kzalloc(ASC_OVERRUN_BSIZE, 
GFP_KERNEL);
+   if (!asc_dvc_varp->overrun_buf) {
+   ret = -ENOMEM;
+   goto err_free_wide_mem;
+   }
warn_code = AscInitAsc1000Driver(asc_dvc_varp);
 
if (warn_code || asc_dvc_varp->err_code) {
@@ -13840,8 +13846,10 @@ static int __devinit advansys_board_found(struct 
Scsi_Host *shost,
"warn 0x%x, error 0x%x\n",
asc_dvc_varp->init_state, warn_code,
asc_dvc_varp->err_code);
-   if (asc_dvc_varp->err_code)
+   if (asc_dvc_varp->err_code) {
ret = -ENODEV;
+   kfree(asc_dvc_varp->overrun_buf);
+   }
}
} else {
if (advansys_wide_init_chip(shost))
@@ -13891,9 +13899,11 @@ static int advansys_release(struct Scsi_Host *shost)
free_dma(shost->dma_channel);
}
if (ASC_NARROW_BOARD(board)) {
+   ASC_DVC_VAR *asc_dvc_varp = &board->dvc_var.asc_dvc_var;
dma_unmap_single(board->dev,
board->dvc_var.asc_dvc_var.overrun_dma,
ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
+   kfree(asc_dvc_varp->overrun_buf);
} else {
iounmap(board->ioremap_addr);
advansys_wide_free_mem(board);
-
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: [Bugme-new] [Bug 9901] New: kernel panic in stex modules (?)

[FUJITA Tomonori]

On Wed, 06 Feb 2008 12:26:39 -0600
James Bottomley <[EMAIL PROTECTED]> wrote:

> On Wed, 2008-02-06 at 10:15 -0800, Andrew Morton wrote:
> > On Wed,  6 Feb 2008 09:40:15 -0800 (PST) [EMAIL PROTECTED] wrote:
> > 
> > > http://bugzilla.kernel.org/show_bug.cgi?id=9901
> > > 
> > >Summary: kernel panic in stex modules (?)
> > >Product: IO/Storage
> > >Version: 2.5
> > >  KernelVersion: 2.6.24
> > >   Platform: All
> > > OS/Version: Linux
> > >   Tree: Mainline
> > > Status: NEW
> > >   Severity: normal
> > >   Priority: P1
> > >  Component: Serial ATA
> > > AssignedTo: [EMAIL PROTECTED]
> > > ReportedBy: [EMAIL PROTECTED]
> > > 
> > > 
> > > Latest working kernel version: 2.6.23-r6
> > > Earliest failing kernel version: 2.6.24
> > > Distribution: Gentoo
> > > Hardware Environment: Core2D E6600, Asus p5B Dlx, 2G DDR2 667, Promise ST
> > > EX4350
> > > Software Environment: GCC 4.2.3/4.1.2, CFLAGS="-O2"
> > > 
> > > Problem Description:
> > > The problem is frequent kernel panics within the same module. Can't say 
> > > what it
> > > is, but looks like it is related to dma and promise driver.
> > > The first culprit, the memory, is ok, 8 hours of memtest passed without 
> > > errors.
> > > Before, kernel 2.6.23-gentoo-r6, compiled with GCC 4.1.2 worked just 
> > > fine, then
> > > after upgrade to 4.2.2 th bug appeared. Upgrade to 2.6.24 didn't solve the
> > > problem. Switching back to GCC 4.1.2 made things better for a moment, 
> > > crashes
> > > became less frequent and I thought compiler was the cause. But today 
> > > system
> > > crashed again with same symptoms.
> > > Sorry, but I can't save crash log, so I'll provide screen "shot":
> > > http://img238.imageshack.us/my.php?image=p2030030ki1.jpg
> > > 
> > > Steps to reproduce:
> > > Boot, start FTP-server, load RAID with heavy input, in some hours it will
> > > crash. With pure reads system can run several days, heavy write load 
> > > kills it
> > > much too easier.
> > > 
> > 
> > The supertrak driver has regressed in 2.6.24.  And
> > 
> > commit 9cb83c7529d929c00f37d821daed1942a1b20602
> > Author: FUJITA Tomonori <[EMAIL PROTECTED]>
> > Date:   Tue Oct 16 11:24:32 2007 +0200
> > 
> > [SCSI] add use_sg_chaining option to scsi_host_template
> > 
> > looks a likely candidate.
> > 
> > And this:
> > 
> > commit d3f46f39b7092594b498abc12f0c73b0b9913bde
> > Author: James Bottomley <[EMAIL PROTECTED]>
> > Date:   Tue Jan 15 11:11:46 2008 -0600
> > 
> > [SCSI] remove use_sg_chaining
> > 
> > from 2.6.25 looks to be a likely fix for it.  Should it be backported?
> 
> If the patch you identify is the culprit, mine can't be the fix ... and
> it should also be present in git head.
> 
> The BUG_ON is here: isn't it?
> 
> static inline void
> dma_unmap_sg(struct device *hwdev, struct scatterlist *sg, int nents,
>int direction)
> {
>   BUG_ON(!valid_dma_direction(direction));
> ^^^
>   dma_ops->unmap_sg(hwdev, sg, nents, direction);
> }
> 
> stex only does scsi_dma_unmap(), so something looks to have tampered
> with the cmnd->sc_data_direction somehow ... and I can't see how.

Surely, someone changes the cmnd->sc_data_direction, or else we should
be hit by dma_map_sg before dma_unmap_sg:

static inline int
dma_map_sg(struct device *hwdev, struct scatterlist *sg, int nents, int 
direction)
{
BUG_ON(!valid_dma_direction(direction));
return dma_ops->map_sg(hwdev, sg, nents, direction);
}
-
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: [Scst-devel] Integration of SCST in the mainstream Linux kernel

[FUJITA Tomonori]

On Tue, 05 Feb 2008 18:09:15 +0100
Matteo Tescione <[EMAIL PROTECTED]> wrote:

> On 5-02-2008 14:38, "FUJITA Tomonori" <[EMAIL PROTECTED]> wrote:
> 
> > On Tue, 05 Feb 2008 08:14:01 +0100
> > Tomasz Chmielewski <[EMAIL PROTECTED]> wrote:
> > 
> >> James Bottomley schrieb:
> >> 
> >>> These are both features being independently worked on, are they not?
> >>> Even if they weren't, the combination of the size of SCST in kernel plus
> >>> the problem of having to find a migration path for the current STGT
> >>> users still looks to me to involve the greater amount of work.
> >> 
> >> I don't want to be mean, but does anyone actually use STGT in
> >> production? Seriously?
> >> 
> >> In the latest development version of STGT, it's only possible to stop
> >> the tgtd target daemon using KILL / 9 signal - which also means all
> >> iSCSI initiator connections are corrupted when tgtd target daemon is
> >> started again (kernel upgrade, target daemon upgrade, server reboot etc.).
> > 
> > I don't know what "iSCSI initiator connections are corrupted"
> > mean. But if you reboot a server, how can an iSCSI target
> > implementation keep iSCSI tcp connections?
> > 
> > 
> >> Imagine you have to reboot all your NFS clients when you reboot your NFS
> >> server. Not only that - your data is probably corrupted, or at least the
> >> filesystem deserves checking...
> 
> Don't know if matters, but in my setup (iscsi on top of drbd+heartbeat)
> rebooting the primary server doesn't affect my iscsi traffic, SCST correctly
> manages stop/crash, by sending unit attention to clients on reconnect.
> Drbd+heartbeat correctly manages those things too.
> Still from an end-user POV, i was able to reboot/survive a crash only with
> SCST, IETD still has reconnect problems and STGT are even worst.

Please tell us on stgt-devel mailing list if you see problems. We will
try to fix them.

Thanks,
-
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: new scsi sense handling

[FUJITA Tomonori]

On Tue, 5 Feb 2008 11:43:58 -0800 (PST)
Luben Tuikov <[EMAIL PROTECTED]> wrote:

> --- On Tue, 2/5/08, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> > On Mon, 4 Feb 2008 18:39:22 -0800 (PST)
> > Luben Tuikov <[EMAIL PROTECTED]> wrote:
> > 
> > > --- On Mon, 2/4/08, Boaz Harrosh
> > <[EMAIL PROTECTED]> wrote:
> > > > There are 3 usages of sense handling in drivers
> > > > 
> > > > 1. sense is available in driver internal
> > structure and is
> > > > mem-copied to upper level
> > > > 2. A CHECK_CONDITION status was returned and the
> > driver
> > > > uses the scsi_eh_prep_cmnd()
> > > >for a REQUEST_SENSE invocation to the target.
> > Then
> > > > returning the sense in 
> > > >scsi_eh_return_cmnd(). A variation on this is
> > when the
> > > > driver does nothing the queue
> > > >is frozen an the scsi watchdog timer does the
> > above.
> > > > 3. The underline host adapter does the
> > REQUEST_SENSE and a
> > > > pre-allocated and DMA mapped
> > > >sense buffer receives the sense information
> > from HW.
> > > 
> > > Many years ago when "ACA" had a constructive
> > meaning,
> > > so did "Autosense".  Then about 5 years ago,
> > "Autosense"
> > > disappeared completely since it became the de facto
> > > implementation of the then SCSI Execute Command
> > "RPC",
> > > now just SCSI Execute Command procedure call.
> > > 
> > > At that point in time, the SCSI mid-layer decided
> > > to embrace this model and give the LLDD a scsi command
> > > structure which included the sense data buffer to
> > > a size that the SCSI mid-layer was interested in,
> > > at the moment 96 bytes, macro defined in
> > > include/scsi/scsi_cmnd.h.
> > > 
> > > The concept of "Autosense" was off-loaded to
> > LLDD
> > > to emulate it if the specific target device to
> > > which the command was issued, didn't supply the
> > > sense data on CHECK CONDITION, and more so
> > > relevant to target devices which implemented
> > > queuing, thus the ACA.
> > > 
> > > And the mid-layer would consider extracting
> > > the sense data via REQUEST SENSE command
> > > as a _special case_ if the LLDD/transport layer
> > > didn't implement the "autosense" model.
> > 
> > Only SPI and USB?
> 
> I don't understand this question.

I meant, 'what transport protocols are categorized into the transport
protocol that doesn't implement the "autosense" model?'


> > The most of LLDs using the transport protocol that we care
> > about today
> > uses sense buffer in their own internal structure.
> 
> Yes.
> 
> > 
> > I think that the issue to solve to kill
> > scsi_cmnd:sense_buffer is how
> > to share (or export) such sense buffer with the scsi
> > mid-layer.
> 
> And therein lies the problem.  Sense data is SCSI specific,
> it should be left to SCSI, unless of course you can
> stipulate that _all_ block devices return sense data.

Yeah, sense data is SCSI specific and it should be left to SCSI. But
I'm not sure we need to stipulate that _all_ block devices return
sense data. Today the block layer users (sg, bsg, etc) use it only
when it's appropriate (or only if they want to use it).


> If that's not the case and you move it to the block
> layer, then you get a whole bunch of other problems,
> like does this device want/use it, should we allocate
> it, etc. OTOH, if that _is_ the case, then you don't
> have to worry about this and the model is pretty
> much as the SCSI mid-layer has it, i.e. sense buffer
> always present.  So I guess the question is, can
> you stipulate that _all_ block devices return sense data?
-
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 21/24][RFC] scsi_tgt: use of sense accessors

[FUJITA Tomonori]

On Tue, 5 Feb 2008 11:21:33 -0500
Pete Wyckoff <[EMAIL PROTECTED]> wrote:

> [EMAIL PROTECTED] wrote on Mon, 04 Feb 2008 19:53 +0200:
> >   FIXME: I need help with this driver (Pete?)
> > I used scsi_sense() in a none const way. But since
> > scsi_tgt is the ULD here, it can just access it's own sense
> > buffer directly. I did not use scsi_eh_cpy_sense() because
> > I did not want the extra copy. Pete will want to use a 260
> > bytes buffer here.
> > 
> > Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
> > Need-help-from: Pete Wyckoff <[EMAIL PROTECTED]>
> 
> FYI, I never use scsi_tgt.  Only just pure userspace on the target,
> and a dumb ethernet NIC that does not know it is speaking any form
> of SCSI.

Seems that many people misunderstand STGT iSCSI (and iSER), FCoE, and
SRP (not implemented yet) software target drivers. They don't use the
tgt kernel module. They just run in user space like user-space nfs
daemon.
-
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: [Scst-devel] Integration of SCST in the mainstream Linux kernel

[FUJITA Tomonori]

On Tue, 05 Feb 2008 17:07:07 +0100
Tomasz Chmielewski <[EMAIL PROTECTED]> wrote:

> FUJITA Tomonori schrieb:
> > On Tue, 05 Feb 2008 08:14:01 +0100
> > Tomasz Chmielewski <[EMAIL PROTECTED]> wrote:
> > 
> >> James Bottomley schrieb:
> >>
> >>> These are both features being independently worked on, are they not?
> >>> Even if they weren't, the combination of the size of SCST in kernel plus
> >>> the problem of having to find a migration path for the current STGT
> >>> users still looks to me to involve the greater amount of work.
> >> I don't want to be mean, but does anyone actually use STGT in
> >> production? Seriously?
> >>
> >> In the latest development version of STGT, it's only possible to stop
> >> the tgtd target daemon using KILL / 9 signal - which also means all
> >> iSCSI initiator connections are corrupted when tgtd target daemon is
> >> started again (kernel upgrade, target daemon upgrade, server reboot etc.).
> > 
> > I don't know what "iSCSI initiator connections are corrupted"
> > mean. But if you reboot a server, how can an iSCSI target
> > implementation keep iSCSI tcp connections?
> 
> The problem with tgtd is that you can't start it (configured) in an
> "atomic" way.
> Usually, one will start tgtd and it's configuration in a script (I 
> replaced some parameters with "..." to make it shorter and more readable):

Thanks for the details. So the way to stop the daemon is not related
with your problem.

It's easily fixable. Can you start a new thread about this on
stgt-devel mailing list? When we agree on the interface to start the
daemon, I'll implement it.


> tgtd
> tgtadm --op new ...
> tgtadm --lld iscsi --op new ...

(snip)

> So the only way to start/restart tgtd reliably is to do hacks which are 
> needed with yet another iSCSI kernel implementation (IET): use iptables.
> 
> iptables 
> tgtd
> sleep 1
> tgtadm --op new ...
> tgtadm --lld iscsi --op new ...
> iptables 
> 
> 
> A bit ugly, isn't it?
> Having to tinker with a firewall in order to start a daemon is by no 
> means a sign of a well-tested and mature project.
> 
> That's why I asked how many people use stgt in a production environment 
> - James was worried about a potential migration path for current users.

I don't know how many people use stgt in a production environment but
I'm not sure that this problem prevents many people from using it in a
production environment.

You want to reboot a server running target devices while initiators
connect to it. Rebooting the target server behind the initiators
seldom works. System adminstorators in my workplace reboot storage
devices once a year and tell us to shut down the initiator machines
that use them before that.
-
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: [Scst-devel] Integration of SCST in the mainstream Linux kernel

[FUJITA Tomonori]

On Mon, 4 Feb 2008 20:07:01 -0600
"Chris Weiss" <[EMAIL PROTECTED]> wrote:

> On Feb 4, 2008 11:30 AM, Douglas Gilbert <[EMAIL PROTECTED]> wrote:
> > Alan Cox wrote:
> > >> better. So for example, I personally suspect that ATA-over-ethernet is 
> > >> way
> > >> better than some crazy SCSI-over-TCP crap, but I'm biased for simple and
> > >> low-level, and against those crazy SCSI people to begin with.
> > >
> > > Current ATAoE isn't. It can't support NCQ. A variant that did NCQ and IP
> > > would probably trash iSCSI for latency if nothing else.
> >
> > And a variant that doesn't do ATA or IP:
> > http://www.fcoe.com/
> >
> 
> however, and interestingly enough, the open-fcoe software target
> depends on scst (for now anyway)

STGT also supports software FCoE target driver though it's still
experimental stuff.

http://www.mail-archive.com/linux-scsi@vger.kernel.org/msg12705.html

It works in user space like STGT's iSCSI (and iSER) target driver
(i.e. no kernel/user space interaction).
-
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: new scsi sense handling

[FUJITA Tomonori]

On Mon, 4 Feb 2008 18:39:22 -0800 (PST)
Luben Tuikov <[EMAIL PROTECTED]> wrote:

> --- On Mon, 2/4/08, Boaz Harrosh <[EMAIL PROTECTED]> wrote:
> > There are 3 usages of sense handling in drivers
> > 
> > 1. sense is available in driver internal structure and is
> > mem-copied to upper level
> > 2. A CHECK_CONDITION status was returned and the driver
> > uses the scsi_eh_prep_cmnd()
> >for a REQUEST_SENSE invocation to the target. Then
> > returning the sense in 
> >scsi_eh_return_cmnd(). A variation on this is when the
> > driver does nothing the queue
> >is frozen an the scsi watchdog timer does the above.
> > 3. The underline host adapter does the REQUEST_SENSE and a
> > pre-allocated and DMA mapped
> >sense buffer receives the sense information from HW.
> 
> Many years ago when "ACA" had a constructive meaning,
> so did "Autosense".  Then about 5 years ago, "Autosense"
> disappeared completely since it became the de facto
> implementation of the then SCSI Execute Command "RPC",
> now just SCSI Execute Command procedure call.
> 
> At that point in time, the SCSI mid-layer decided
> to embrace this model and give the LLDD a scsi command
> structure which included the sense data buffer to
> a size that the SCSI mid-layer was interested in,
> at the moment 96 bytes, macro defined in
> include/scsi/scsi_cmnd.h.
> 
> The concept of "Autosense" was off-loaded to LLDD
> to emulate it if the specific target device to
> which the command was issued, didn't supply the
> sense data on CHECK CONDITION, and more so
> relevant to target devices which implemented
> queuing, thus the ACA.
> 
> And the mid-layer would consider extracting
> the sense data via REQUEST SENSE command
> as a _special case_ if the LLDD/transport layer
> didn't implement the "autosense" model.

Only SPI and USB?

The most of LLDs using the transport protocol that we care about today
uses sense buffer in their own internal structure.

I think that the issue to solve to kill scsi_cmnd:sense_buffer is how
to share (or export) such sense buffer with the scsi mid-layer.

For the old transport protocols, we could do something that James said
in this thread to to kill scsi_cmnd:sense_buffer.
-
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: [Scst-devel] Integration of SCST in the mainstream Linux kernel

[FUJITA Tomonori]

On Tue, 05 Feb 2008 05:43:10 +0100
Matteo Tescione <[EMAIL PROTECTED]> wrote:

> Hi all,
> And sorry for intrusion, i am not a developer but i work everyday with iscsi
> and i found it fantastic.
> Altough Aoe, Fcoe and so on could be better, we have to look in real world
> implementations what is needed *now*, and if we look at vmware world,
> virtual iron, microsoft clustering etc, the answer is iSCSI.
> And now, SCST is the best open-source iSCSI target. So, from an end-user
> point of view, what are the really problems to not integrate scst in the
> mainstream kernel?

Currently, the best open-source iSCSI target implemenation in Linux is
Nicholas's LIO, I guess.
-
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: [Scst-devel] Integration of SCST in the mainstream Linux kernel

[FUJITA Tomonori]

On Tue, 05 Feb 2008 08:14:01 +0100
Tomasz Chmielewski <[EMAIL PROTECTED]> wrote:

> James Bottomley schrieb:
> 
> > These are both features being independently worked on, are they not?
> > Even if they weren't, the combination of the size of SCST in kernel plus
> > the problem of having to find a migration path for the current STGT
> > users still looks to me to involve the greater amount of work.
> 
> I don't want to be mean, but does anyone actually use STGT in
> production? Seriously?
> 
> In the latest development version of STGT, it's only possible to stop
> the tgtd target daemon using KILL / 9 signal - which also means all
> iSCSI initiator connections are corrupted when tgtd target daemon is
> started again (kernel upgrade, target daemon upgrade, server reboot etc.).

I don't know what "iSCSI initiator connections are corrupted"
mean. But if you reboot a server, how can an iSCSI target
implementation keep iSCSI tcp connections?


> Imagine you have to reboot all your NFS clients when you reboot your NFS
> server. Not only that - your data is probably corrupted, or at least the
> filesystem deserves checking...
-
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

[FUJITA Tomonori]

On Wed, 30 Jan 2008 14:10:47 +0100
"Bart Van Assche" <[EMAIL PROTECTED]> wrote:

> On Jan 30, 2008 11:56 AM, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> > On Wed, 30 Jan 2008 09:38:04 +0100
> > "Bart Van Assche" <[EMAIL PROTECTED]> wrote:
> > >
> > > Please specify which parameters you are referring to. As you know I
> >
> > Sorry, I can't say. I don't know much about iSER. But seems that Pete
> > and Robin can get the better I/O performance - line speed ratiwo with
> > STGT.
> 
> Robin Humble was using a DDR InfiniBand network, while my tests were
> performed with an SDR InfiniBand network. Robin's results can't be
> directly compared to my results.

I know that you use different hardware. I used 'ratio' word.


BTW, you said the performance difference of dio READ is 38% but I
think it's 27.3 %, though it's still large.


> Pete Wyckoff's results
> (http://www.osc.edu/~pw/papers/wyckoff-iser-snapi07-talk.pdf) are hard
> to interpret. I have asked Pete which of the numbers in his test can
> be compared with what I measured, but Pete did not reply.
> 
> > The version of OpenIB might matters too. For example, Pete said that
> > STGT reads loses about 100 MB/s for some transfer sizes for some
> > transfer sizes due to the OpenIB version difference or other unclear
> > reasons.
> >
> > http://article.gmane.org/gmane.linux.iscsi.tgt.devel/135
> 
> Pete wrote about a degradation from 600 MB/s to 500 MB/s for reads
> with STGT+iSER. In my tests I measured 589 MB/s for reads (direct
> I/O), which matches with the better result obtained by Pete.

I don't know he used the same benchmark software so I don't think that
we can compare them.

All I tried to say is the OFED version might has big effect on the
performance. So you might need to find the best one.


> Note: the InfiniBand kernel modules I used were those from the
> 2.6.22.9 kernel, not from the OFED distribution.

I'm talking about a target machine (I think that Pete was also talking
about OFED on his target machine). STGT uses OFED libraries, I think.
-
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

[FUJITA Tomonori]

On Wed, 30 Jan 2008 09:38:04 +0100
"Bart Van Assche" <[EMAIL PROTECTED]> wrote:

> On Jan 30, 2008 12:32 AM, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> >
> > iSER has parameters to limit the maximum size of RDMA (it needs to
> > repeat RDMA with a poor configuration)?
> 
> Please specify which parameters you are referring to. As you know I

Sorry, I can't say. I don't know much about iSER. But seems that Pete
and Robin can get the better I/O performance - line speed ratio with
STGT.

The version of OpenIB might matters too. For example, Pete said that
STGT reads loses about 100 MB/s for some transfer sizes for some
transfer sizes due to the OpenIB version difference or other unclear
reasons.

http://article.gmane.org/gmane.linux.iscsi.tgt.devel/135

It's fair to say that it takes long time and need lots of knowledge to
get the maximum performance of SAN, I think.

I think that it would be easier to convince James with the detailed
analysis (e.g. where does it take so long, like Pete did), not just
'dd' performance results.

Pushing iSCSI target code into mainline failed four times: IET, SCST,
STGT (doing I/Os in kernel in the past), and PyX's one (*1). iSCSI
target code is huge. You said SCST comprises 14,000 lines, but it's
not iSCSI target code. The SCSI engine code comprises 14,000
lines. You need another 10,000 lines for the iSCSI driver. Note that
SCST's iSCSI driver provides only basic iSCSI features. PyX's iSCSI
target code implemenents more iSCSI features (like MC/S, ERL2, etc)
and comprises about 60,000 lines and it still lacks some features like
iSER, bidi, etc.

I think that it's reasonable to say that we need more than 'dd'
results before pushing about possible more than 60,000 lines to
mainline.

(*1) http://linux-iscsi.org/
-
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

[FUJITA Tomonori]

On Tue, 29 Jan 2008 13:31:52 -0800
Roland Dreier <[EMAIL PROTECTED]> wrote:

>  > .   .   STGT read SCST read.STGT read  
> SCST read.
>  > .   .  performance   performance   . performance   
>  performance   .
>  > .   .  (0.5K, MB/s)  (0.5K, MB/s)  .   (1 MB, 
> MB/s)   (1 MB, MB/s)  .
>  > . iSER (8 Gb/s network) . 250N/A   .   360 
>   N/A   .
>  > . SRP  (8 Gb/s network) . N/A421   .   N/A 
>   683   .
> 
>  > On the comparable figures, which only seem to be IPoIB they're showing a
>  > 13-18% variance, aren't they?  Which isn't an incredible difference.
> 
> Maybe I'm all wet, but I think iSER vs. SRP should be roughly
> comparable.  The exact formatting of various messages etc. is
> different but the data path using RDMA is pretty much identical.  So
> the big difference between STGT iSER and SCST SRP hints at some big
> difference in the efficiency of the two implementations.

iSER has parameters to limit the maximum size of RDMA (it needs to
repeat RDMA with a poor configuration)?


Anyway, here's the results from Robin Humble:

iSER to 7G ramfs, x86_64, centos4.6, 2.6.22 kernels, git tgtd,
initiator end booted with mem=512M, target with 8G ram

 direct i/o dd
  write/read  800/751 MB/s
dd if=/dev/zero of=/dev/sdc bs=1M count=5000 oflag=direct
dd of=/dev/null if=/dev/sdc bs=1M count=5000 iflag=direct

http://www.mail-archive.com/linux-scsi@vger.kernel.org/msg13502.html

I think that STGT is pretty fast with the fast backing storage. 


I don't think that there is the notable perfornace difference between
kernel-space and user-space SRP (or ISER) implementations about moving
data between hosts. IB is expected to enable user-space applications
to move data between hosts quickly (if not, what can IB provide us?).

I think that the question is how fast user-space applications can do
I/Os ccompared with I/Os in kernel space. STGT is eager for the advent
of good asynchronous I/O and event notification interfances.


One more possible optimization for STGT is zero-copy data
transfer. STGT uses pre-registered buffers and move data between page
cache and thsse buffers, and then does RDMA transfer. If we implement
own caching mechanism to use pre-registered buffers directly with (AIO
and O_DIRECT), then STGT can move data without data copies.
-
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] zfcp: fix sense_buffer access bug

[FUJITA Tomonori]

On Mon, 28 Jan 2008 08:46:25 +0100
Christof Schmitt <[EMAIL PROTECTED]> wrote:

> On Sun, Jan 27, 2008 at 12:41:50PM +0900, FUJITA Tomonori wrote:
> > The commit de25deb18016f66dcdede165d07654559bb332bc changed
> > scsi_cmnd.sense_buffer from a static array to a dynamically allocated
> > buffer. We can't access to sense_buffer in '&cmd->sense_buffer' way.
> > 
> > Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
> > ---
> >  drivers/s390/scsi/zfcp_fsf.c |4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
> > index fe57941..a9a147d 100644
> > --- a/drivers/s390/scsi/zfcp_fsf.c
> > +++ b/drivers/s390/scsi/zfcp_fsf.c
> > @@ -4224,10 +4224,10 @@ zfcp_fsf_send_fcp_command_task_handler(struct 
> > zfcp_fsf_req *fsf_req)
> > 
> > ZFCP_LOG_TRACE("%i bytes sense data provided by FCP\n",
> >fcp_rsp_iu->fcp_sns_len);
> > -   memcpy(&scpnt->sense_buffer,
> > +   memcpy(scpnt->sense_buffer,
> >zfcp_get_fcp_sns_info_ptr(fcp_rsp_iu), sns_len);
> > ZFCP_HEX_DUMP(ZFCP_LOG_LEVEL_TRACE,
> > - (void *) &scpnt->sense_buffer, sns_len);
> > + (void *)scpnt->sense_buffer, sns_len);
> > }
> > 
> > /* check for overrun */
> 
> ACK for fixing the access to the sense buffer.
> 
> We are working internally on cleaning up the zfcp messages. With this
> change, the 'trace' and 'hex dump' messages will disappear. So, could
> you simply remove the ZFCP_HEX_DUMP message above, instead of fixing
> it?

I can but James has already merged the above patch to scsi-misc. So it
would be more convenient for everyone if you could rebase your
patchset on top of scsi-misc?
-
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] zfcp: fix sense_buffer access bug

[FUJITA Tomonori]

The commit de25deb18016f66dcdede165d07654559bb332bc changed
scsi_cmnd.sense_buffer from a static array to a dynamically allocated
buffer. We can't access to sense_buffer in '&cmd->sense_buffer' way.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/s390/scsi/zfcp_fsf.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index fe57941..a9a147d 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -4224,10 +4224,10 @@ zfcp_fsf_send_fcp_command_task_handler(struct 
zfcp_fsf_req *fsf_req)
 
ZFCP_LOG_TRACE("%i bytes sense data provided by FCP\n",
   fcp_rsp_iu->fcp_sns_len);
-   memcpy(&scpnt->sense_buffer,
+   memcpy(scpnt->sense_buffer,
   zfcp_get_fcp_sns_info_ptr(fcp_rsp_iu), sns_len);
ZFCP_HEX_DUMP(ZFCP_LOG_LEVEL_TRACE,
- (void *) &scpnt->sense_buffer, sns_len);
+ (void *)scpnt->sense_buffer, sns_len);
}
 
/* check for overrun */
-- 
1.5.3.4

-
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] ncr53c8xx: fix sense_buffer access bug

[FUJITA Tomonori]

The commit de25deb18016f66dcdede165d07654559bb332bc changed
scsi_cmnd.sense_buffer from a static array to a dynamically allocated
buffer. We can't access to sense_buffer in '&cmd->sense_buffer' way.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/ncr53c8xx.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/ncr53c8xx.c b/drivers/scsi/ncr53c8xx.c
index c02771a..c5ebf01 100644
--- a/drivers/scsi/ncr53c8xx.c
+++ b/drivers/scsi/ncr53c8xx.c
@@ -4967,7 +4967,7 @@ void ncr_complete (struct ncb *np, struct ccb *cp)
 sizeof(cp->sense_buf)));
 
if (DEBUG_FLAGS & (DEBUG_RESULT|DEBUG_TINY)) {
-   u_char * p = (u_char*) & cmd->sense_buffer;
+   u_char *p = cmd->sense_buffer;
int i;
PRINT_ADDR(cmd, "sense data:");
for (i=0; i<14; i++) printk (" %x", *p++);
-- 
1.5.3.4

-
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

aic79xx: fix sense_buffer access bug

[FUJITA Tomonori]

Ah, I overlooked more LLDs...

=
From: FUJITA Tomonori <[EMAIL PROTECTED]>
Subject: [PATCH] aic79xx: fix sense_buffer access bug

The commit de25deb18016f66dcdede165d07654559bb332bc changed
scsi_cmnd.sense_buffer from a static array to a dynamically allocated
buffer. We can't access to sense_buffer in '&cmd->sense_buffer' way.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/aic7xxx/aic79xx_osm.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c 
b/drivers/scsi/aic7xxx/aic79xx_osm.c
index 0e4708f..3c4efa4 100644
--- a/drivers/scsi/aic7xxx/aic79xx_osm.c
+++ b/drivers/scsi/aic7xxx/aic79xx_osm.c
@@ -1922,7 +1922,7 @@ ahd_linux_queue_cmd_complete(struct ahd_softc *ahd, 
struct scsi_cmnd *cmd)
struct scsi_sense_data *sense;

sense = (struct scsi_sense_data *)
-   &cmd->sense_buffer;
+   cmd->sense_buffer;
if (sense->extra_len >= 5 &&
(sense->add_sense_code == 0x47
 || sense->add_sense_code == 0x48))
-- 
1.5.3.4

-
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] hptiop: fix sense_buffer

[FUJITA Tomonori]

Sorry, there was another place that I overlooked in the sense buffer
conversion.

=
From: FUJITA Tomonori <[EMAIL PROTECTED]>
Subject: [PATCH] hptiop: fix sense_buffer access bug

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/hptiop.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/hptiop.c b/drivers/scsi/hptiop.c
index e7b2f35..890f44f 100644
--- a/drivers/scsi/hptiop.c
+++ b/drivers/scsi/hptiop.c
@@ -573,7 +573,7 @@ static void hptiop_finish_scsi_req(struct hptiop_hba *hba, 
u32 tag,
scsi_set_resid(scp,
scsi_bufflen(scp) - le32_to_cpu(req->dataxfer_length));
scp->result = SAM_STAT_CHECK_CONDITION;
-   memcpy(&scp->sense_buffer, &req->sg_list,
+   memcpy(scp->sense_buffer, &req->sg_list,
min_t(size_t, SCSI_SENSE_BUFFERSIZE,
le32_to_cpu(req->dataxfer_length)));
break;
-- 
1.5.3.4

-
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] aic79xx: fix warnings with CONFIG_PM disabled

[FUJITA Tomonori]

  CC [M]  drivers/scsi/aic7xxx/aic79xx_osm_pci.o
drivers/scsi/aic7xxx/aic79xx_osm_pci.c:101: warning: 
'ahd_linux_pci_dev_suspend' defined but not used
drivers/scsi/aic7xxx/aic79xx_osm_pci.c:121: warning: 'ahd_linux_pci_dev_resume' 
defined but not used

This moves aic79xx_pci_driver struct, removes some forward
declarations, and adds some ifdef CONFIG_PM.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/aic7xxx/aic79xx.h |5 +++-
 drivers/scsi/aic7xxx/aic79xx_core.c|2 +
 drivers/scsi/aic7xxx/aic79xx_osm_pci.c |   33 +++
 drivers/scsi/aic7xxx/aic79xx_pci.c |2 +
 4 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/aic7xxx/aic79xx.h b/drivers/scsi/aic7xxx/aic79xx.h
index ce638aa..2f00467 100644
--- a/drivers/scsi/aic7xxx/aic79xx.h
+++ b/drivers/scsi/aic7xxx/aic79xx.h
@@ -1340,8 +1340,10 @@ struct   ahd_pci_identity 
*ahd_find_pci_device(ahd_dev_softc_t);
 int  ahd_pci_config(struct ahd_softc *,
 struct ahd_pci_identity *);
 intahd_pci_test_register_access(struct ahd_softc *);
+#ifdef CONFIG_PM
 void   ahd_pci_suspend(struct ahd_softc *);
 void   ahd_pci_resume(struct ahd_softc *);
+#endif
 
 /** SCB and SCB queue management 
**/
 void   ahd_qinfifo_requeue_tail(struct ahd_softc *ahd,
@@ -1352,8 +1354,10 @@ struct ahd_softc *ahd_alloc(void *platform_arg, char 
*name);
 int ahd_softc_init(struct ahd_softc *);
 voidahd_controller_info(struct ahd_softc *ahd, char *buf);
 int ahd_init(struct ahd_softc *ahd);
+#ifdef CONFIG_PM
 int ahd_suspend(struct ahd_softc *ahd);
 voidahd_resume(struct ahd_softc *ahd);
+#endif
 int ahd_default_config(struct ahd_softc *ahd);
 int ahd_parse_vpddata(struct ahd_softc *ahd,
   struct vpd_config *vpd);
@@ -1361,7 +1365,6 @@ intahd_parse_cfgdata(struct 
ahd_softc *ahd,
   struct seeprom_config *sc);
 voidahd_intr_enable(struct ahd_softc *ahd, int enable);
 voidahd_pause_and_flushwork(struct ahd_softc *ahd);
-int ahd_suspend(struct ahd_softc *ahd); 
 voidahd_set_unit(struct ahd_softc *, int);
 voidahd_set_name(struct ahd_softc *, char *);
 struct scb *ahd_get_scb(struct ahd_softc *ahd, u_int col_idx);
diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c 
b/drivers/scsi/aic7xxx/aic79xx_core.c
index a7dd8cd..ade0fb8 100644
--- a/drivers/scsi/aic7xxx/aic79xx_core.c
+++ b/drivers/scsi/aic7xxx/aic79xx_core.c
@@ -7175,6 +7175,7 @@ ahd_pause_and_flushwork(struct ahd_softc *ahd)
ahd->flags &= ~AHD_ALL_INTERRUPTS;
 }
 
+#ifdef CONFIG_PM
 int
 ahd_suspend(struct ahd_softc *ahd)
 {
@@ -7197,6 +7198,7 @@ ahd_resume(struct ahd_softc *ahd)
ahd_intr_enable(ahd, TRUE); 
ahd_restart(ahd);
 }
+#endif
 
 /** Busy Target Table 
*/
 /*
diff --git a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c 
b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
index 66f0259..4150c8a 100644
--- a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
+++ b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
@@ -43,17 +43,6 @@
 #include "aic79xx_inline.h"
 #include "aic79xx_pci.h"
 
-static int ahd_linux_pci_dev_probe(struct pci_dev *pdev,
-   const struct pci_device_id *ent);
-static int ahd_linux_pci_reserve_io_regions(struct ahd_softc *ahd,
-u_long *base, u_long *base2);
-static int ahd_linux_pci_reserve_mem_region(struct ahd_softc *ahd,
-u_long *bus_addr,
-uint8_t __iomem **maddr);
-static int ahd_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t 
mesg);
-static int ahd_linux_pci_dev_resume(struct pci_dev *pdev);
-static voidahd_linux_pci_dev_remove(struct pci_dev *pdev);
-
 /* Define the macro locally since it's different for different class of chips.
  */
 #define ID(x)\
@@ -85,17 +74,7 @@ static struct pci_device_id ahd_linux_pci_id_table[] = {
 
 MODULE_DEVICE_TABLE(pci, ahd_linux_pci_id_table);
 
-static struct pci_driver aic79xx_pci_driver = {
-   .name   = "aic79xx",
-   .probe  = ahd_linux_pci_dev_probe,
 #ifdef CONFIG_PM
-   .suspend= ahd_linux_pci_dev_suspend,
-   .resume = ahd_linux_pci_dev_resume,
-#endif
-   .remove = ahd_linux_pci_dev_remove,
-   .id_table   = ahd_linux_pci_id_table
-};
-
 static int
 ahd_linux_pci_dev_suspend(struct pci_dev *pdev, pm_me

[PATCH] aic7xxx: fix warnings with CONFIG_PM disabled

[FUJITA Tomonori]

  CC [M]  drivers/scsi/aic7xxx/aic7xxx_osm_pci.o
drivers/scsi/aic7xxx/aic7xxx_osm_pci.c:148: warning: 
'ahc_linux_pci_dev_suspend' defined but not used
drivers/scsi/aic7xxx/aic7xxx_osm_pci.c:166: warning: 'ahc_linux_pci_dev_resume' 
defined but not used

This moves aic7xxx_pci_driver struct, removes some forward declarations,
and adds some ifdef CONFIG_PM.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/aic7xxx/aic7xxx.h |4 +++
 drivers/scsi/aic7xxx/aic7xxx_core.c|3 +-
 drivers/scsi/aic7xxx/aic7xxx_osm_pci.c |   33 +++
 drivers/scsi/aic7xxx/aic7xxx_pci.c |2 +
 4 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/aic7xxx/aic7xxx.h b/drivers/scsi/aic7xxx/aic7xxx.h
index 3d4e42d..c0344e6 100644
--- a/drivers/scsi/aic7xxx/aic7xxx.h
+++ b/drivers/scsi/aic7xxx/aic7xxx.h
@@ -1143,7 +1143,9 @@ struct ahc_pci_identity   
*ahc_find_pci_device(ahc_dev_softc_t);
 int ahc_pci_config(struct ahc_softc *,
struct ahc_pci_identity *);
 int ahc_pci_test_register_access(struct ahc_softc *);
+#ifdef CONFIG_PM
 voidahc_pci_resume(struct ahc_softc *ahc);
+#endif
 
 /*** EISA/VL Front End 
/
 struct aic7770_identity *aic7770_find_device(uint32_t);
@@ -1170,8 +1172,10 @@ int   ahc_chip_init(struct ahc_softc 
*ahc);
 int ahc_init(struct ahc_softc *ahc);
 voidahc_intr_enable(struct ahc_softc *ahc, int enable);
 voidahc_pause_and_flushwork(struct ahc_softc *ahc);
+#ifdef CONFIG_PM
 int ahc_suspend(struct ahc_softc *ahc); 
 int ahc_resume(struct ahc_softc *ahc);
+#endif
 voidahc_set_unit(struct ahc_softc *, int);
 voidahc_set_name(struct ahc_softc *, char *);
 voidahc_alloc_scbs(struct ahc_softc *ahc);
diff --git a/drivers/scsi/aic7xxx/aic7xxx_core.c 
b/drivers/scsi/aic7xxx/aic7xxx_core.c
index f350b5e..6d2ae64 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_core.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_core.c
@@ -5078,6 +5078,7 @@ ahc_pause_and_flushwork(struct ahc_softc *ahc)
ahc->flags &= ~AHC_ALL_INTERRUPTS;
 }
 
+#ifdef CONFIG_PM
 int
 ahc_suspend(struct ahc_softc *ahc)
 {
@@ -5113,7 +5114,7 @@ ahc_resume(struct ahc_softc *ahc)
ahc_restart(ahc);
return (0);
 }
-
+#endif
 /** Busy Target Table 
*/
 /*
  * Return the untagged transaction id for a given target/channel lun.
diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c 
b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
index 4488946..dd6e21d 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
@@ -42,17 +42,6 @@
 #include "aic7xxx_osm.h"
 #include "aic7xxx_pci.h"
 
-static int ahc_linux_pci_dev_probe(struct pci_dev *pdev,
-   const struct pci_device_id *ent);
-static int ahc_linux_pci_reserve_io_region(struct ahc_softc *ahc,
-   u_long *base);
-static int ahc_linux_pci_reserve_mem_region(struct ahc_softc *ahc,
-u_long *bus_addr,
-uint8_t __iomem **maddr);
-static int ahc_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t 
mesg);
-static int ahc_linux_pci_dev_resume(struct pci_dev *pdev);
-static voidahc_linux_pci_dev_remove(struct pci_dev *pdev);
-
 /* Define the macro locally since it's different for different class of chips.
 */
 #define ID(x)  ID_C(x, PCI_CLASS_STORAGE_SCSI)
@@ -132,17 +121,7 @@ static struct pci_device_id ahc_linux_pci_id_table[] = {
 
 MODULE_DEVICE_TABLE(pci, ahc_linux_pci_id_table);
 
-static struct pci_driver aic7xxx_pci_driver = {
-   .name   = "aic7xxx",
-   .probe  = ahc_linux_pci_dev_probe,
 #ifdef CONFIG_PM
-   .suspend= ahc_linux_pci_dev_suspend,
-   .resume = ahc_linux_pci_dev_resume,
-#endif
-   .remove = ahc_linux_pci_dev_remove,
-   .id_table   = ahc_linux_pci_id_table
-};
-
 static int
 ahc_linux_pci_dev_suspend(struct pci_dev *pdev, pm_message_t mesg)
 {
@@ -182,6 +161,7 @@ ahc_linux_pci_dev_resume(struct pci_dev *pdev)
 
return (ahc_resume(ahc));
 }
+#endif
 
 static void
 ahc_linux_pci_dev_remove(struct pci_dev *pdev)
@@ -289,6 +269,17 @@ ahc_linux_pci_dev_probe(struct pci_dev *pdev, const struct 
pci_device_id *ent)
return (0);
 }
 
+static struct pci_driver aic7xxx_pci_driver = {
+   .name   = "aic7xxx",
+   .probe  = ahc_linux_pci_dev_probe,
+#ifdef CONFIG_PM
+   .suspend= ahc_linux_pci_dev_suspen

Re: [PATCH] use the cmd_type of a leading request for scsi_init_sgtable

[FUJITA Tomonori]

On Sat, 26 Jan 2008 11:22:47 +0900
FUJITA Tomonori <[EMAIL PROTECTED]> wrote:

> On Fri, 25 Jan 2008 20:05:55 -0600
> James Bottomley <[EMAIL PROTECTED]> wrote:
> 
> > On Sat, 2008-01-26 at 09:57 +0900, FUJITA Tomonori wrote:
> > > This is against the scsi-bidi tree.
> > > 
> > > We need to use the cmd_type of a leading request for scsi_init_sgtable
> > > to set up scsi_data_buffer:length of a bidi request properly.
> > > 
> > > An alternative approach is setting the cmd_type of a leading request
> > > and its bidi request (*1). But the block layer and scsi-ml don't
> > > expect that the leading request and its sub-requests have the
> > > different command types.
> > > 
> > > Note that scsi_debug's XDWRITEREAD_10 support is fine without this
> > > patch since req->nr_sectors works for it but req->nr_sectors doesn't
> > > work for everyone.
> > > 
> > > (*1)
> > > 
> > > http://www.mail-archive.com/linux-scsi@vger.kernel.org/msg12669.html
> > > 
> > > =
> > > From: FUJITA Tomonori <[EMAIL PROTECTED]>
> > > Subject: [PATCH] use the cmd_type of a leading request for 
> > > scsi_init_sgtable
> > > 
> > > We need to use the cmd_type of a leading request for scsi_init_sgtable
> > > to set up scsi_data_buffer:length of its bidi request properly.
> > 
> > This seems to be a very convoluted work around for the fact that we
> > forgot to set the cmd_type on the subordinate request.
> > 
> > Wouldn't this be a better fix?
> 
> I'm fine with this. I have no big preference in this issue.
> 
> Acked-by: FUJITA Tomonori <[EMAIL PROTECTED]>
> 
> 
> I just thought that the approach (the block layer and scsi-ml look at
> only the type of a leading request) show better how the block layer
> and scsi-ml see a leading request and its subordinate requests (all
> the requests need to have the same command type).

I meant that the block layer and scsi-ml use its subordinate requests
just to hook data buffer.


Anyway, I'm fine with your patch.
-
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] use the cmd_type of a leading request for scsi_init_sgtable

[FUJITA Tomonori]

On Fri, 25 Jan 2008 20:05:55 -0600
James Bottomley <[EMAIL PROTECTED]> wrote:

> On Sat, 2008-01-26 at 09:57 +0900, FUJITA Tomonori wrote:
> > This is against the scsi-bidi tree.
> > 
> > We need to use the cmd_type of a leading request for scsi_init_sgtable
> > to set up scsi_data_buffer:length of a bidi request properly.
> > 
> > An alternative approach is setting the cmd_type of a leading request
> > and its bidi request (*1). But the block layer and scsi-ml don't
> > expect that the leading request and its sub-requests have the
> > different command types.
> > 
> > Note that scsi_debug's XDWRITEREAD_10 support is fine without this
> > patch since req->nr_sectors works for it but req->nr_sectors doesn't
> > work for everyone.
> > 
> > (*1)
> > 
> > http://www.mail-archive.com/linux-scsi@vger.kernel.org/msg12669.html
> > 
> > =
> > From: FUJITA Tomonori <[EMAIL PROTECTED]>
> > Subject: [PATCH] use the cmd_type of a leading request for scsi_init_sgtable
> > 
> > We need to use the cmd_type of a leading request for scsi_init_sgtable
> > to set up scsi_data_buffer:length of its bidi request properly.
> 
> This seems to be a very convoluted work around for the fact that we
> forgot to set the cmd_type on the subordinate request.
> 
> Wouldn't this be a better fix?

I'm fine with this. I have no big preference in this issue.

Acked-by: FUJITA Tomonori <[EMAIL PROTECTED]>


I just thought that the approach (the block layer and scsi-ml look at
only the type of a leading request) show better how the block layer
and scsi-ml see a leading request and its subordinate requests (all
the requests need to have the same command type).


> James
> 
> ---
> 
> diff --git a/block/bsg.c b/block/bsg.c
> index 69b0a9d..8917c51 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -279,6 +279,7 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
>   goto out;
>   }
>   rq->next_rq = next_rq;
> + next_rq->cmd_type = rq->cmd_type;
>  
>   dxferp = (void*)(unsigned long)hdr->din_xferp;
>   ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);
> 
> 
> -
> 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

[PATCH] use the cmd_type of a leading request for scsi_init_sgtable

[FUJITA Tomonori]

This is against the scsi-bidi tree.

We need to use the cmd_type of a leading request for scsi_init_sgtable
to set up scsi_data_buffer:length of a bidi request properly.

An alternative approach is setting the cmd_type of a leading request
and its bidi request (*1). But the block layer and scsi-ml don't
expect that the leading request and its sub-requests have the
different command types.

Note that scsi_debug's XDWRITEREAD_10 support is fine without this
patch since req->nr_sectors works for it but req->nr_sectors doesn't
work for everyone.

(*1)

http://www.mail-archive.com/linux-scsi@vger.kernel.org/msg12669.html

=
From: FUJITA Tomonori <[EMAIL PROTECTED]>
Subject: [PATCH] use the cmd_type of a leading request for scsi_init_sgtable

We need to use the cmd_type of a leading request for scsi_init_sgtable
to set up scsi_data_buffer:length of its bidi request properly.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/scsi_lib.c |   23 ++-
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e1c7eeb..61093ea 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1001,8 +1001,9 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
scsi_end_request(cmd, -EIO, this_count, !result);
 }
 
-static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
-gfp_t gfp_mask)
+static int scsi_init_sgtable(struct request *req,
+enum rq_cmd_type_bits cmd_type,
+struct scsi_data_buffer *sdb, gfp_t gfp_mask)
 {
int count;
 
@@ -1015,12 +1016,12 @@ static int scsi_init_sgtable(struct request *req, 
struct scsi_data_buffer *sdb,
}
 
req->buffer = NULL;
-   if (blk_pc_request(req))
+   if (cmd_type == REQ_TYPE_BLOCK_PC)
sdb->length = req->data_len;
else
sdb->length = req->nr_sectors << 9;
 
-   /* 
+   /*
 * Next, walk the list, and fill in the addresses and sizes of
 * each segment.
 */
@@ -1043,21 +1044,25 @@ static int scsi_init_sgtable(struct request *req, 
struct scsi_data_buffer *sdb,
  */
 int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 {
-   int error = scsi_init_sgtable(cmd->request, &cmd->sdb, gfp_mask);
+   int error;
+   enum rq_cmd_type_bits cmd_type = cmd->request->cmd_type;
+
+   error = scsi_init_sgtable(cmd->request, cmd_type, &cmd->sdb, gfp_mask);
if (error)
goto err_exit;
 
if (blk_bidi_rq(cmd->request)) {
-   struct scsi_data_buffer *bidi_sdb = kmem_cache_zalloc(
-   scsi_bidi_sdb_cache, GFP_ATOMIC);
+   struct scsi_data_buffer *bidi_sdb;
+
+   bidi_sdb = kmem_cache_zalloc(scsi_bidi_sdb_cache, GFP_ATOMIC);
if (!bidi_sdb) {
error = BLKPREP_DEFER;
goto err_exit;
}
 
cmd->request->next_rq->special = bidi_sdb;
-   error = scsi_init_sgtable(cmd->request->next_rq, bidi_sdb,
-   GFP_ATOMIC);
+   error = scsi_init_sgtable(cmd->request->next_rq, cmd_type,
+ bidi_sdb, GFP_ATOMIC);
if (error)
goto err_exit;
}
-- 
1.5.3.4

-
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 1/2] destroy scsi_bidi_sdb_cache in scsi_exit_queue

[FUJITA Tomonori]

This patchset is against the scsi-bidi tree.

=
From: FUJITA Tomonori <[EMAIL PROTECTED]>
Subject: [PATCH 1/2] destroy scsi_bidi_sdb_cache in scsi_exit_queue

Needs to call kmem_cache_destroy for scsi_bidi_sdb_cache in
scsi_exit_queue.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/scsi_lib.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e1c7eeb..7bfec7e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1712,6 +1712,7 @@ void scsi_exit_queue(void)
int i;
 
kmem_cache_destroy(scsi_io_context_cache);
+   kmem_cache_destroy(scsi_bidi_sdb_cache);
 
for (i = 0; i < SG_MEMPOOL_NR; i++) {
struct scsi_host_sg_pool *sgp = scsi_sg_pools + i;
-- 
1.5.3.4

-
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/2] handle scsi_init_queue failure properly

[FUJITA Tomonori]

scsi_init_queue is expected to clean up allocated things when it
fails.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/scsi_lib.c |   18 +-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7bfec7e..b12fb31 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1682,7 +1682,7 @@ int __init scsi_init_queue(void)
0, 0, NULL);
if (!scsi_bidi_sdb_cache) {
printk(KERN_ERR "SCSI: can't init scsi bidi sdb cache\n");
-   return -ENOMEM;
+   goto cleanup_io_context;
}
 
for (i = 0; i < SG_MEMPOOL_NR; i++) {
@@ -1694,6 +1694,7 @@ int __init scsi_init_queue(void)
if (!sgp->slab) {
printk(KERN_ERR "SCSI: can't init sg slab %s\n",
sgp->name);
+   goto cleanup_bidi_sdb;
}
 
sgp->pool = mempool_create_slab_pool(SG_MEMPOOL_SIZE,
@@ -1701,10 +1702,25 @@ int __init scsi_init_queue(void)
if (!sgp->pool) {
printk(KERN_ERR "SCSI: can't init sg mempool %s\n",
sgp->name);
+   goto cleanup_bidi_sdb;
}
}
 
return 0;
+
+cleanup_bidi_sdb:
+   for (i = 0; i < SG_MEMPOOL_NR; i++) {
+   struct scsi_host_sg_pool *sgp = scsi_sg_pools + i;
+   if (sgp->pool)
+   mempool_destroy(sgp->pool);
+   if (sgp->slab)
+   kmem_cache_destroy(sgp->slab);
+   }
+   kmem_cache_destroy(scsi_bidi_sdb_cache);
+cleanup_io_context:
+   kmem_cache_destroy(scsi_io_context_cache);
+
+   return -ENOMEM;
 }
 
 void scsi_exit_queue(void)
-- 
1.5.3.4

-
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 1/2] ch: fix device minor number management bug

[FUJITA Tomonori]

Oops, sorry, I sent the patches to linux-kernel...

=
From: FUJITA Tomonori <[EMAIL PROTECTED]>
Subject: [PATCH 1/2] ch: fix device minor number management bug

ch_probe uses the total number of ch devices as minor.

ch_probe:
ch->minor = ch_devcount;
...
ch_devcount++;

Then ch_remove decreases ch_devcount:

ch_remove:
ch_devcount--;

If you have two ch devices, sch0 and sch1, and remove sch0,
ch_devcount is 1. Then if you add another ch device, ch_probe tries to
create sch1. So you get a warning and fail to create sch1:

Jan 24 16:01:05 nice kernel: sysfs: duplicate filename 'sch1' can not be created
Jan 24 16:01:05 nice kernel: WARNING: at fs/sysfs/dir.c:424 sysfs_add_one()
Jan 24 16:01:05 nice kernel: Pid: 2571, comm: iscsid Not tainted 
2.6.24-rc7-ga3d2c2e8-dirty #1
Jan 24 16:01:05 nice kernel:
Jan 24 16:01:05 nice kernel: Call Trace:
Jan 24 16:01:05 nice kernel:  [] sysfs_add_one+0x54/0xbd
Jan 24 16:01:05 nice kernel:  [] create_dir+0x4f/0x87
Jan 24 16:01:05 nice kernel:  [] sysfs_create_dir+0x35/0x4a
Jan 24 16:01:05 nice kernel:  [] kobject_get+0x12/0x17
Jan 24 16:01:05 nice kernel:  [] kobject_add+0xf3/0x1a6
Jan 24 16:01:05 nice kernel:  [] class_device_add+0xaa/0x39d
Jan 24 16:01:05 nice kernel:  [] class_device_create+0xcb/0xfa
Jan 24 16:01:05 nice kernel:  [] printk+0x4e/0x56
Jan 24 16:01:05 nice kernel:  [] sysfs_ilookup_test+0x0/0xf
Jan 24 16:01:05 nice kernel:  [] :ch:ch_probe+0xbe/0x61a

(snip)

This patch converts ch to use a standard minor number management way,
idr like sg and bsg.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/ch.c |   71 +---
 1 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 765f2fc..2b07014 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -21,6 +21,7 @@
 #include 
 #include /* here are all the ioctls */
 #include 
+#include 
 
 #include 
 #include 
@@ -33,6 +34,7 @@
 
 #define CH_DT_MAX   16
 #define CH_TYPES8
+#define CH_MAX_DEVS 128
 
 MODULE_DESCRIPTION("device driver for scsi media changer devices");
 MODULE_AUTHOR("Gerd Knorr <[EMAIL PROTECTED]>");
@@ -113,9 +115,8 @@ typedef struct {
struct mutexlock;
 } scsi_changer;
 
-static LIST_HEAD(ch_devlist);
-static DEFINE_SPINLOCK(ch_devlist_lock);
-static int ch_devcount;
+static DEFINE_IDR(ch_index_idr);
+static DEFINE_SPINLOCK(ch_index_lock);
 
 static struct scsi_driver ch_template =
 {
@@ -598,20 +599,17 @@ ch_release(struct inode *inode, struct file *file)
 static int
 ch_open(struct inode *inode, struct file *file)
 {
-   scsi_changer *tmp, *ch;
+   scsi_changer *ch;
int minor = iminor(inode);
 
-   spin_lock(&ch_devlist_lock);
-   ch = NULL;
-   list_for_each_entry(tmp,&ch_devlist,list) {
-   if (tmp->minor == minor)
-   ch = tmp;
-   }
+   spin_lock(&ch_index_lock);
+   ch = idr_find(&ch_index_idr, minor);
+
if (NULL == ch || scsi_device_get(ch->device)) {
-   spin_unlock(&ch_devlist_lock);
+   spin_unlock(&ch_index_lock);
return -ENXIO;
}
-   spin_unlock(&ch_devlist_lock);
+   spin_unlock(&ch_index_lock);
 
file->private_data = ch;
return 0;
@@ -914,6 +912,7 @@ static int ch_probe(struct device *dev)
 {
struct scsi_device *sd = to_scsi_device(dev);
struct class_device *class_dev;
+   int minor, ret = -ENOMEM;
scsi_changer *ch;
 
if (sd->type != TYPE_MEDIUM_CHANGER)
@@ -923,7 +922,22 @@ static int ch_probe(struct device *dev)
if (NULL == ch)
return -ENOMEM;
 
-   ch->minor = ch_devcount;
+   if (!idr_pre_get(&ch_index_idr, GFP_KERNEL))
+   goto free_ch;
+
+   spin_lock(&ch_index_lock);
+   ret = idr_get_new(&ch_index_idr, ch, &minor);
+   spin_unlock(&ch_index_lock);
+
+   if (ret)
+   goto free_ch;
+
+   if (minor > CH_MAX_DEVS) {
+   ret = -ENODEV;
+   goto remove_idr;
+   }
+
+   ch->minor = minor;
sprintf(ch->name,"ch%d",ch->minor);
 
class_dev = class_device_create(ch_sysfs_class, NULL,
@@ -932,8 +946,8 @@ static int ch_probe(struct device *dev)
if (IS_ERR(class_dev)) {
printk(KERN_WARNING "ch%d: class_device_create failed\n",
   ch->minor);
-   kfree(ch);
-   return PTR_ERR(class_dev);
+   ret = PTR_ERR(class_dev);
+   goto remove_idr;
}
 
mutex_init(&ch->lock);
@@ -942,35 +956,29 @@ static int ch_probe(struct device *dev)
if (init)
ch_init_elem(ch);
 
+   dev_set_drvdata(dev, ch)

[PATCH 2/2] ch: remove forward declarations

[FUJITA Tomonori]

This moves ch_template and changer_fops structs to the end of file and
removes forward declarations.

This also removes some trailing whitespace.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/ch.c |  120 -
 1 files changed, 54 insertions(+), 66 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 2b07014..7aad154 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -90,16 +90,6 @@ static const char * vendor_labels[CH_TYPES-4] = {
 
 #define MAX_RETRIES   1
 
-static int  ch_probe(struct device *);
-static int  ch_remove(struct device *);
-static int  ch_open(struct inode * inode, struct file * filp);
-static int  ch_release(struct inode * inode, struct file * filp);
-static long ch_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
-#ifdef CONFIG_COMPAT
-static long ch_ioctl_compat(struct file * filp,
-   unsigned int cmd, unsigned long arg);
-#endif
-
 static struct class * ch_sysfs_class;
 
 typedef struct {
@@ -118,27 +108,6 @@ typedef struct {
 static DEFINE_IDR(ch_index_idr);
 static DEFINE_SPINLOCK(ch_index_lock);
 
-static struct scsi_driver ch_template =
-{
-   .owner  = THIS_MODULE,
-   .gendrv = {
-   .name   = "ch",
-   .probe  = ch_probe,
-   .remove = ch_remove,
-   },
-};
-
-static const struct file_operations changer_fops =
-{
-   .owner  = THIS_MODULE,
-   .open   = ch_open,
-   .release= ch_release,
-   .unlocked_ioctl = ch_ioctl,
-#ifdef CONFIG_COMPAT
-   .compat_ioctl   = ch_ioctl_compat,
-#endif
-};
-
 static const struct {
unsigned char  sense;
unsigned char  asc;
@@ -207,7 +176,7 @@ ch_do_scsi(scsi_changer *ch, unsigned char *cmd,
 {
int errno, retries = 0, timeout, result;
struct scsi_sense_hdr sshdr;
-   
+
timeout = (cmd[0] == INITIALIZE_ELEMENT_STATUS)
? timeout_init : timeout_move;
 
@@ -245,7 +214,7 @@ static int
 ch_elem_to_typecode(scsi_changer *ch, u_int elem)
 {
int i;
-   
+
for (i = 0; i < CH_TYPES; i++) {
if (elem >= ch->firsts[i]  &&
elem <  ch->firsts[i] +
@@ -261,15 +230,15 @@ ch_read_element_status(scsi_changer *ch, u_int elem, char 
*data)
u_char  cmd[12];
u_char  *buffer;
int result;
-   
+
buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
if(!buffer)
return -ENOMEM;
-   
+
  retry:
memset(cmd,0,sizeof(cmd));
cmd[0] = READ_ELEMENT_STATUS;
-   cmd[1] = (ch->device->lun << 5) | 
+   cmd[1] = (ch->device->lun << 5) |
(ch->voltags ? 0x10 : 0) |
ch_elem_to_typecode(ch,elem);
cmd[2] = (elem >> 8) & 0xff;
@@ -296,7 +265,7 @@ ch_read_element_status(scsi_changer *ch, u_int elem, char 
*data)
return result;
 }
 
-static int 
+static int
 ch_init_elem(scsi_changer *ch)
 {
int err;
@@ -322,7 +291,7 @@ ch_readconfig(scsi_changer *ch)
buffer = kzalloc(512, GFP_KERNEL | GFP_DMA);
if (!buffer)
return -ENOMEM;
-   
+
memset(cmd,0,sizeof(cmd));
cmd[0] = MODE_SENSE;
cmd[1] = ch->device->lun << 5;
@@ -365,7 +334,7 @@ ch_readconfig(scsi_changer *ch)
} else {
vprintk("reading element address assigment page failed!\n");
}
-   
+
/* vendor specific element types */
for (i = 0; i < 4; i++) {
if (0 == vendor_counts[i])
@@ -443,7 +412,7 @@ static int
 ch_position(scsi_changer *ch, u_int trans, u_int elem, int rotate)
 {
u_char  cmd[10];
-   
+
dprintk("position: 0x%x\n",elem);
if (0 == trans)
trans = ch->firsts[CHET_MT];
@@ -462,7 +431,7 @@ static int
 ch_move(scsi_changer *ch, u_int trans, u_int src, u_int dest, int rotate)
 {
u_char  cmd[12];
-   
+
dprintk("move: 0x%x => 0x%x\n",src,dest);
if (0 == trans)
trans = ch->firsts[CHET_MT];
@@ -484,7 +453,7 @@ ch_exchange(scsi_changer *ch, u_int trans, u_int src,
u_int dest1, u_int dest2, int rotate1, int rotate2)
 {
u_char  cmd[12];
-   
+
dprintk("exchange: 0x%x => 0x%x => 0x%x\n",
src,dest1,dest2);
if (0 == trans)
@@ -501,7 +470,7 @@ ch_exchange(scsi_changer *ch, u_int trans, u_int src,
cmd[8]  = (dest2 >> 8) & 0xff;
cmd[9]  =  dest2   & 0xff;
cmd[10] = (rotate1 ? 1 : 0) | (rotate2 ? 2 : 0);
-   
+
return ch_do_scsi(ch, cmd, NULL,0, DMA_NONE);
 }
 
@@ -539,14 +508,14 @@ ch_set_voltag(scsi_changer *ch, u_int elem,
elem, tag);
memset(cmd,0,sizeof(cmd));
   

[PATCH] ch: handle class_device_create failure properly

[FUJITA Tomonori]

When class_device_create fails, ch_probe needs to fail too.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/ch.c |   22 +++---
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index cead0f5..765f2fc 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -913,29 +913,37 @@ static long ch_ioctl_compat(struct file * file,
 static int ch_probe(struct device *dev)
 {
struct scsi_device *sd = to_scsi_device(dev);
+   struct class_device *class_dev;
scsi_changer *ch;
-   
+
if (sd->type != TYPE_MEDIUM_CHANGER)
return -ENODEV;
-
+
ch = kzalloc(sizeof(*ch), GFP_KERNEL);
if (NULL == ch)
return -ENOMEM;
 
ch->minor = ch_devcount;
sprintf(ch->name,"ch%d",ch->minor);
+
+   class_dev = class_device_create(ch_sysfs_class, NULL,
+   MKDEV(SCSI_CHANGER_MAJOR, ch->minor),
+   dev, "s%s", ch->name);
+   if (IS_ERR(class_dev)) {
+   printk(KERN_WARNING "ch%d: class_device_create failed\n",
+  ch->minor);
+   kfree(ch);
+   return PTR_ERR(class_dev);
+   }
+
mutex_init(&ch->lock);
ch->device = sd;
ch_readconfig(ch);
if (init)
ch_init_elem(ch);
 
-   class_device_create(ch_sysfs_class, NULL,
-   MKDEV(SCSI_CHANGER_MAJOR,ch->minor),
-   dev, "s%s", ch->name);
-
sdev_printk(KERN_INFO, sd, "Attached scsi changer %s\n", ch->name);
-   
+
spin_lock(&ch_devlist_lock);
list_add_tail(&ch->list,&ch_devlist);
ch_devcount++;
-- 
1.5.3.4

-
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 1/3] scsi_debug: add get_data_transfer_info helper function

[FUJITA Tomonori]

This adds get_data_transfer_info helper function that get lha and
sectors for READ_* and WRITE_* commands (and XDWRITEREAD_10 later).

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/scsi_debug.c |   83 
 1 files changed, 38 insertions(+), 45 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 82c06f0..31f7378 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -311,12 +311,47 @@ static void sdebug_max_tgts_luns(void);
 static struct device pseudo_primary;
 static struct bus_type pseudo_lld_bus;
 
+static void get_data_transfer_info(unsigned char *cmd,
+  unsigned long long *lba, unsigned int *num)
+{
+   int i;
+
+   switch (*cmd) {
+   case WRITE_16:
+   case READ_16:
+   for (*lba = 0, i = 0; i < 8; ++i) {
+   if (i > 0)
+   *lba <<= 8;
+   *lba += cmd[2 + i];
+   }
+   *num = cmd[13] + (cmd[12] << 8) +
+   (cmd[11] << 16) + (cmd[10] << 24);
+   break;
+   case WRITE_12:
+   case READ_12:
+   *lba = cmd[5] + (cmd[4] << 8) + (cmd[3] << 16) + (cmd[2] << 24);
+   *num = cmd[9] + (cmd[8] << 8) + (cmd[7] << 16) + (cmd[6] << 24);
+   break;
+   case WRITE_10:
+   case READ_10:
+   *lba = cmd[5] + (cmd[4] << 8) + (cmd[3] << 16) + (cmd[2] << 24);
+   *num = cmd[8] + (cmd[7] << 8);
+   break;
+   case WRITE_6:
+   case READ_6:
+   *lba = cmd[3] + (cmd[2] << 8) + ((cmd[1] & 0x1f) << 16);
+   *num = (0 == cmd[4]) ? 256 : cmd[4];
+   break;
+   default:
+   break;
+   }
+}
 
 static
 int scsi_debug_queuecommand(struct scsi_cmnd * SCpnt, done_funct_t done)
 {
unsigned char *cmd = (unsigned char *) SCpnt->cmnd;
-   int len, k, j;
+   int len, k;
unsigned int num;
unsigned long long lba;
int errsts = 0;
@@ -452,28 +487,7 @@ int scsi_debug_queuecommand(struct scsi_cmnd * SCpnt, 
done_funct_t done)
break;
if (scsi_debug_fake_rw)
break;
-   if ((*cmd) == READ_16) {
-   for (lba = 0, j = 0; j < 8; ++j) {
-   if (j > 0)
-   lba <<= 8;
-   lba += cmd[2 + j];
-   }
-   num = cmd[13] + (cmd[12] << 8) +
-   (cmd[11] << 16) + (cmd[10] << 24);
-   } else if ((*cmd) == READ_12) {
-   lba = cmd[5] + (cmd[4] << 8) +
-   (cmd[3] << 16) + (cmd[2] << 24);
-   num = cmd[9] + (cmd[8] << 8) +
-   (cmd[7] << 16) + (cmd[6] << 24);
-   } else if ((*cmd) == READ_10) {
-   lba = cmd[5] + (cmd[4] << 8) +
-   (cmd[3] << 16) + (cmd[2] << 24);
-   num = cmd[8] + (cmd[7] << 8);
-   } else {/* READ (6) */
-   lba = cmd[3] + (cmd[2] << 8) +
-   ((cmd[1] & 0x1f) << 16);
-   num = (0 == cmd[4]) ? 256 : cmd[4];
-   }
+   get_data_transfer_info(cmd, &lba, &num);
errsts = resp_read(SCpnt, lba, num, devip);
if (inj_recovered && (0 == errsts)) {
mk_sense_buffer(devip, RECOVERED_ERROR,
@@ -500,28 +514,7 @@ int scsi_debug_queuecommand(struct scsi_cmnd * SCpnt, 
done_funct_t done)
break;
if (scsi_debug_fake_rw)
break;
-   if ((*cmd) == WRITE_16) {
-   for (lba = 0, j = 0; j < 8; ++j) {
-   if (j > 0)
-   lba <<= 8;
-   lba += cmd[2 + j];
-   }
-   num = cmd[13] + (cmd[12] << 8) +
-   (cmd[11] << 16) + (cmd[10] << 24);
-   } else if ((*cmd) == WRITE_12) {
-   lba = cmd[5] + (cmd[4] << 8) +
-   (cmd[3] << 16) + (cmd[2] << 24);
-   num = cmd[9] + (cmd[8] << 8) +
-   (cmd[7] << 16) + (cmd[6] << 24);
-   } else if ((*cmd) == WRITE_10) {
-   lba = cmd[5] + (cmd[4] << 8) +
-  

[PATCH 3/3] scsi_debug: add XDWRITEREAD_10 support

[FUJITA Tomonori]

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/scsi_debug.c |   70 +
 include/scsi/scsi.h   |1 +
 2 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index d810aa7..1541c17 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -280,6 +280,8 @@ static int resp_write(struct scsi_cmnd * SCpnt, unsigned 
long long lba,
  unsigned int num, struct sdebug_dev_info * devip);
 static int resp_report_luns(struct scsi_cmnd * SCpnt,
struct sdebug_dev_info * devip);
+static int resp_xdwriteread(struct scsi_cmnd *scp, unsigned long long lba,
+   unsigned int num, struct sdebug_dev_info *devip);
 static int fill_from_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
 int arr_len);
 static int fetch_to_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
@@ -334,6 +336,7 @@ static void get_data_transfer_info(unsigned char *cmd,
break;
case WRITE_10:
case READ_10:
+   case XDWRITEREAD_10:
*lba = cmd[5] + (cmd[4] << 8) + (cmd[3] << 16) + (cmd[2] << 24);
*num = cmd[8] + (cmd[7] << 8);
break;
@@ -542,6 +545,28 @@ int scsi_debug_queuecommand(struct scsi_cmnd * SCpnt, 
done_funct_t done)
case WRITE_BUFFER:
errsts = check_readiness(SCpnt, 1, devip);
break;
+   case XDWRITEREAD_10:
+   if (!scsi_bidi_cmnd(SCpnt)) {
+   mk_sense_buffer(devip, ILLEGAL_REQUEST,
+   INVALID_FIELD_IN_CDB, 0);
+   errsts = check_condition_result;
+   break;
+   }
+
+   errsts = check_readiness(SCpnt, 0, devip);
+   if (errsts)
+   break;
+   if (scsi_debug_fake_rw)
+   break;
+   get_data_transfer_info(cmd, &lba, &num);
+   errsts = resp_read(SCpnt, lba, num, devip);
+   if (errsts)
+   break;
+   errsts = resp_write(SCpnt, lba, num, devip);
+   if (errsts)
+   break;
+   errsts = resp_xdwriteread(SCpnt, lba, num, devip);
+   break;
default:
if (SCSI_DEBUG_OPT_NOISE & scsi_debug_opts)
printk(KERN_INFO "scsi_debug: Opcode: 0x%x not "
@@ -1948,6 +1973,50 @@ static int resp_report_luns(struct scsi_cmnd * scp,
min((int)alloc_len, SDEBUG_RLUN_ARR_SZ));
 }
 
+static int resp_xdwriteread(struct scsi_cmnd *scp, unsigned long long lba,
+   unsigned int num, struct sdebug_dev_info *devip)
+{
+   int i, j, ret = -1;
+   unsigned char *kaddr, *buf;
+   unsigned int offset;
+   struct scatterlist *sg;
+   struct scsi_data_buffer *sdb = scsi_in(scp);
+
+   /* better not to use temporary buffer. */
+   buf = kmalloc(scsi_bufflen(scp), GFP_ATOMIC);
+   if (!buf)
+   return ret;
+
+   offset = 0;
+   scsi_for_each_sg(scp, sg, scsi_sg_count(scp), i) {
+   kaddr = (unsigned char *)kmap_atomic(sg_page(sg), KM_USER0);
+   if (!kaddr)
+   goto out;
+
+   memcpy(buf + offset, kaddr + sg->offset, sg->length);
+   offset += sg->length;
+   kunmap_atomic(kaddr, KM_USER0);
+   }
+
+   offset = 0;
+   for_each_sg(sdb->table.sgl, sg, sdb->table.nents, i) {
+   kaddr = (unsigned char *)kmap_atomic(sg_page(sg), KM_USER0);
+   if (!kaddr)
+   goto out;
+
+   for (j = 0; j < sg->length; j++)
+   *(kaddr + sg->offset + j) ^= *(buf + offset + j);
+
+   offset += sg->length;
+   kunmap_atomic(kaddr, KM_USER0);
+   }
+   ret = 0;
+out:
+   kfree(buf);
+
+   return ret;
+}
+
 /* When timer goes off this function is called. */
 static void timer_intr_handler(unsigned long indx)
 {
@@ -1981,6 +2050,7 @@ static int scsi_debug_slave_alloc(struct scsi_device * 
sdp)
if (SCSI_DEBUG_OPT_NOISE & scsi_debug_opts)
printk(KERN_INFO "scsi_debug: slave_alloc <%u %u %u %u>\n",
   sdp->host->host_no, sdp->channel, sdp->id, sdp->lun);
+   set_bit(QUEUE_FLAG_BIDI, &sdp->request_queue->queue_flags);
return 0;
 }
 
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 056c5af..19ca9e9 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -102,6 +102,7 @@ extern const unsigned char scsi_command_size[8];
 #define READ_TOC  0x43
 #defin

Re: [PATCH 0/3] update bidirectional series to sit on top of sg_table

[FUJITA Tomonori]

From: James Bottomley <[EMAIL PROTECTED]>
Subject: [PATCH 0/3] update bidirectional series to sit on top of sg_table
Date: Fri, 11 Jan 2008 21:09:00 -0600

> OK, I suppose in the scheme of things, it's my turn to bear some of the
> pain.  the SCSI bidirectional series rejects pretty badly with sg_table,
> and since sg_table has to go in, I rebased the series on top of it.
> 
> Additionally, I tidied up the patches to take advantages of some of the
> features of sg_table.  I killed both use_sg and sg_count in favour of
> using sg_table.nseg for the count.
> 
> Just so you can test all of this to make sure I got it right, you can
> pull the patch series from
> 
> master.kernel.org:/pub/scm/linux/kernel/git/jejb/scsi-bidi-2.6.git

I've just started to look at the bidi tree.

I suppose that only panasas guys have tested the bidi tree with their
OSD target devices. To test the bidi tree, I added XDWRITEREAD_10
support to scsi_debug and sgv4_xdwriteread tool to my makeshift bsg
tool collections:

git://git.kernel.org/pub/scm/linux/kernel/git/tomo/sgv4-tools.git

It just sends XDWRITEREAD_10 commands like this:

tulip:~# sgv4_xdwriteread --length 16384 /sys/class/bsg/1:0:0:0
driver:0, transport:0, device:0, din_resid: 0, dout_resid: 0

No errors.

I'll send the patchset (over the bidi tree) to add XDWRITEREAD_10
support to scsi_debug though I'm not sure whether it's worth adding it
to mainline.
-
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] scsi_debug: add bidi data transfer support

[FUJITA Tomonori]

This enables fill_from_dev_buffer and fetch_to_dev_buffer to handle
bidi commands.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/scsi_debug.c |   21 ++---
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 31f7378..d810aa7 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -594,18 +594,18 @@ static int fill_from_dev_buffer(struct scsi_cmnd * scp, 
unsigned char * arr,
int k, req_len, act_len, len, active;
void * kaddr;
void * kaddr_off;
-   struct scatterlist * sg;
+   struct scatterlist *sg;
+   struct scsi_data_buffer *sdb = scsi_in(scp);
 
-   if (0 == scsi_bufflen(scp))
+   if (!sdb->length)
return 0;
-   if (NULL == scsi_sglist(scp))
+   if (!sdb->table.sgl)
return (DID_ERROR << 16);
-   if (! ((scp->sc_data_direction == DMA_BIDIRECTIONAL) ||
- (scp->sc_data_direction == DMA_FROM_DEVICE)))
+   if (!(scsi_bidi_cmnd(scp) || scp->sc_data_direction == DMA_FROM_DEVICE))
return (DID_ERROR << 16);
active = 1;
req_len = act_len = 0;
-   scsi_for_each_sg(scp, sg, scsi_sg_count(scp), k) {
+   for_each_sg(sdb->table.sgl, sg, sdb->table.nents, k) {
if (active) {
kaddr = (unsigned char *)
kmap_atomic(sg_page(sg), KM_USER0);
@@ -623,10 +623,10 @@ static int fill_from_dev_buffer(struct scsi_cmnd * scp, 
unsigned char * arr,
}
req_len += sg->length;
}
-   if (scsi_get_resid(scp))
-   scsi_set_resid(scp, scsi_get_resid(scp) - act_len);
+   if (sdb->resid)
+   sdb->resid -= act_len;
else
-   scsi_set_resid(scp, req_len - act_len);
+   sdb->resid = req_len - act_len;
return 0;
 }
 
@@ -643,8 +643,7 @@ static int fetch_to_dev_buffer(struct scsi_cmnd * scp, 
unsigned char * arr,
return 0;
if (NULL == scsi_sglist(scp))
return -1;
-   if (! ((scp->sc_data_direction == DMA_BIDIRECTIONAL) ||
- (scp->sc_data_direction == DMA_TO_DEVICE)))
+   if (!(scsi_bidi_cmnd(scp) || scp->sc_data_direction == DMA_TO_DEVICE))
return -1;
req_len = fin = 0;
scsi_for_each_sg(scp, sg, scsi_sg_count(scp), k) {
-- 
1.5.3.4

-
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: Performance of SCST versus STGT

[FUJITA Tomonori]

On Tue, 22 Jan 2008 14:33:13 +0300
Vladislav Bolkhovitin <[EMAIL PROTECTED]> wrote:

> FUJITA Tomonori wrote:
> > The big problem of stgt iSER is disk I/Os (move data between disk and
> > page cache). We need a proper asynchronous I/O mechanism, however,
> > Linux doesn't provide such and we use a workaround, which incurs large
> > latency. I guess, we cannot solve this until syslets is merged into
> > mainline.
> 
> Hmm, SCST also doesn't have ability to use asynchronous I/O, but that 
> doesn't prevent it from showing good performance.

I don't know how SCST performs I/Os, but surely, in kernel space, you
can performs I/Os asynchronously. Or you use an event notification
mechanism with multiple kernel threads performing I/Os synchronously.

Xen blktap has the same problem as stgt. IIRC, Xen mainline uses a
kernel patch to add a proper event notification to AIO though redhat
uses the same workaround as stgt instead of applying the kernel patch.
-
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 v3] use dynamically allocated sense buffer

[FUJITA Tomonori]

On Sun, 20 Jan 2008 21:37:41 -0700
Matthew Wilcox <[EMAIL PROTECTED]> wrote:

> On Mon, Jan 21, 2008 at 01:08:58PM +0900, FUJITA Tomonori wrote:
> > On Sun, 20 Jan 2008 09:40:11 -0700
> > Matthew Wilcox <[EMAIL PROTECTED]> wrote:
> > > Longer-term, I want to allow low-level drivers to allocate the
> > > sense_buffer themselves so they can DMA directly into it (ie grown-up dma
> > > mapping, rather than this quaint x86 __GFP_DMA).  This patch doesn't get
> > 
> > Yeah, I think that the approach is one of candidates.
> > 
> > If we go with it, I think that the major issue is that LLDs don't know
> > when they can reclaim sense_buffer from scsi-ml; scsi-ml uses
> > sense_buffer after scmd->scsi_done.
> 
> The midlayer would call a function in the scsi_host_template to free the
> command.  The sense_buffer would be freed at the same time.

Yeah, it would work though I'm a bit concerned about adding another
phase to the scsi_cmnd interaction between the midlayer and LLDs.

Another possible option is that putting some sense_buffer information
to the host template and the midlayer allocates sense buffers for
LLDs. LLDs can ask for them when it's necessary. One disadvantage of
this is the many LLDs have sense buffer in their own data structure so
it's not fit well for these LLDs, I think.
-
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: Performance of SCST versus STGT

[FUJITA Tomonori]

On Sun, 20 Jan 2008 10:36:18 +0100
"Bart Van Assche" <[EMAIL PROTECTED]> wrote:

> On Jan 18, 2008 1:08 PM, Vladislav Bolkhovitin <[EMAIL PROTECTED]> wrote:
> >
> > [ ... ]
> > So, seems I understood your slides correctly: the more valuable data for
> > our SCST SRP vs STGT iSER comparison should be on page 26 for 1 command
> > read (~480MB/s, i.e. ~60% from Bart's result on the equivalent hardware).
> 
> At least in my tests SCST performed significantly better than STGT.
> These tests were performed with the currently available
> implementations of SCST and STGT. Which performance improvements are

First, I recommend you to examine iSER stuff more since it has some
parameters unlike SRP, which effects the performance, IIRC. At least,
you could get the iSER performances similar to Pete's.


> possible for these projects (e.g. zero-copying), and by how much is it
> expected that these performance improvements will increase throughput
> and will decrease latency ?

The major bottleneck about RDMA transfer is registering the buffer
before transfer. stgt's iSER driver has pre-registered buffers and
move data between page cache and thsse buffers, and then does RDMA
transfer.

The big problem of stgt iSER is disk I/Os (move data between disk and
page cache). We need a proper asynchronous I/O mechanism, however,
Linux doesn't provide such and we use a workaround, which incurs large
latency. I guess, we cannot solve this until syslets is merged into
mainline.

The above approach still needs one memory copy (between the
pre-registered buffers and page cahce). If we need more performance,
we have to implement a new caching mechanism using the pre-registered
buffers instead of just using page cache. AIO with O_DIRECT enables us
to implement such caching mechanism (we can use eventfd so we don't
need something like syslets, that is, we can implement such now).

I'm not sure someone will implement such RDMA caching mechanism for
stgt. Pete and his colleagues implemented stgt iSER driver (thanks!)
but they are not interested in block I/Os (they are OSD people).
-
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] remove use_sg_chaining

[FUJITA Tomonori]

On Sun, 20 Jan 2008 21:54:21 +0200
Boaz Harrosh <[EMAIL PROTECTED]> wrote:

> On Sun, Jan 20 2008 at 21:24 +0200, James Bottomley <[EMAIL PROTECTED]> wrote:
> > On Sun, 2008-01-20 at 21:18 +0200, Boaz Harrosh wrote:
> >> On Tue, Jan 15 2008 at 19:52 +0200, James Bottomley <[EMAIL PROTECTED]> 
> >> wrote:
> >>> this patch depends on the sg branch of the block tree
> >>>
> >>> James
> >>>
> >>> ---
> >>> From: James Bottomley <[EMAIL PROTECTED]>
> >>> Date: Tue, 15 Jan 2008 11:11:46 -0600
> >>> Subject: remove use_sg_chaining
> >>>
> >>> With the sg table code, every SCSI driver is now either chain capable
> >>> or broken, so there's no need to have a check in the host template.
> >>>
> >>> Also tidy up the code by moving the scatterlist size defines into the
> >>> SCSI includes and permit the last entry of the scatterlist pools not
> >>> to be a power of two.
> >>> ---
> >> I have a theoretical problem that BUGed me from the beginning.
> >>
> >> Could it happen that a memory critical IO, (that is needed to free
> >> memory), be collected into an sg-chained large IO, and the allocation 
> >> of the multiple sg-pool-allocations fail, thous dead locking on
> >> out-of-memory? Is there a mechanism in place that will split large IO's 
> >> into smaller chunks in the event of out-of-memory condition in prep_fn?
> >>
> >> Is it possible to call blk_rq_map_sg() with less then what is present
> >> at request to only map the starting portion?
> > 
> > Obviously, that's why I was worrying about mempool size and default
> > blocks a while ago.
> > 
> > However, the deadlock only occurs if the device is swap or backing a
> > filesystem with memory mapped files.  The use cases for this are really
> > tapes and other entities that need huge buffers.  That's why we're
> > keeping the system sector size at 1024 unless you alter it through sysfs
> > (here gun, there foot ...)
> > 
> > James
> > 
> 
> OK Thanks for confirming my concern, In modern life with devices like
> iSCSI that have ~0 as it's max_sector, swapping over that should be considered
> and configured carefully. Once with pNFS over blocks/objects it should be 
> addressed.
> Perhaps with a FAIL_FAST semantics for users like pNFS to split up the 
> requests if they
> fail with out-of-memory.

As James pointed out, with networked backed device, the things are
much more complicated (I have no idea when such configuration will be
possible).

http://kerneltrap.org/Linux/Swap_Over_NFS
-
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 v3] use dynamically allocated sense buffer

[FUJITA Tomonori]

On Sun, 20 Jan 2008 09:40:11 -0700
Matthew Wilcox <[EMAIL PROTECTED]> wrote:

> On Wed, Jan 16, 2008 at 01:32:17PM +0900, FUJITA Tomonori wrote:
> > This removes static array sense_buffer in scsi_cmnd and uses
> > dynamically allocated sense_buffer (with GFP_DMA).
> > 
> > The reason for doing this is that some architectures need cacheline
> > aligned buffer for DMA:
> > 
> > http://lkml.org/lkml/2007/11/19/2
> > 
> > The problems are that scsi_eh_prep_cmnd puts scsi_cmnd::sense_buffer
> > to sglist and some LLDs directly DMA to scsi_cmnd::sense_buffer. It's
> > necessary to DMA to scsi_cmnd::sense_buffer safely. This patch solves
> > these issues.
> > 
> > __scsi_get_command allocates sense_buffer via kmem_cache_alloc and
> > attaches it to a scsi_cmnd so everything just work as before.
> 
> I think this is fine for the moment.
> 
> Longer-term, I want to allow low-level drivers to allocate the
> sense_buffer themselves so they can DMA directly into it (ie grown-up dma
> mapping, rather than this quaint x86 __GFP_DMA).  This patch doesn't get

Yeah, I think that the approach is one of candidates.

If we go with it, I think that the major issue is that LLDs don't know
when they can reclaim sense_buffer from scsi-ml; scsi-ml uses
sense_buffer after scmd->scsi_done.


> us any closer to that, but it doesn't get us further away from it either.
-
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 v3] use dynamically allocated sense buffer

[FUJITA Tomonori]

On Sun, 20 Jan 2008 10:36:56 -0600
James Bottomley <[EMAIL PROTECTED]> wrote:

> 
> On Wed, 2008-01-16 at 13:32 +0900, FUJITA Tomonori wrote:
> > This is the third version of:
> > 
> > http://marc.info/?l=linux-scsi&m=120038907123706&w=2
> [...]
> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index 54ff611..0a4a5b8 100644
> > --- a/drivers/scsi/scsi.c
> > +++ b/drivers/scsi/scsi.c
> > @@ -186,6 +190,21 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host 
> > *shost, gfp_t gfp_mask)
> > list_del_init(&cmd->list);
> > }
> > spin_unlock_irqrestore(&shost->free_list_lock, flags);
> > +
> > +   if (cmd) {
> > +   buf = cmd->sense_buffer;
> > +   memset(cmd, 0, sizeof(*cmd));
> > +   cmd->sense_buffer = buf;
> > +   }
> > +   } else {
> > +   buf = kmem_cache_alloc(sense_buffer_slab, __GFP_DMA|gfp_mask);
> 
> This is going to cause the enterprise some angst because ZONE_DMA can be
> very small, and the enterprise requrements for commands in flight very
> large.  I also think its unnecessary if we know the host isn't requiring
> ISA DMA.

Yes, I should have done properly.


>  How about the below to fix this, it's based on the existing
> infrastructure for solving the very same problem.

Looks nice. Integrating sense_buffer_pool into struct
scsi_host_cmd_pool looks much better.


> James
> 
> ---
> 
> >From e7ffbd81684779f5eb41d66d5f499b82af40e12b Mon Sep 17 00:00:00 2001
> From: James Bottomley <[EMAIL PROTECTED]>
> Date: Sun, 20 Jan 2008 09:28:24 -0600
> Subject: [SCSI] don't use __GFP_DMA for sense buffers if not required
> 
> Only hosts which actually have ISA DMA requirements need sense buffers
> coming out of ZONE_DMA, so only use the __GFP_DMA flag for that case
> to avoid allocating this scarce resource if it's not necessary.
> 
> Signed-off-by: James Bottomley <[EMAIL PROTECTED]>
> ---
>  drivers/scsi/hosts.c |9 +
>  drivers/scsi/scsi.c  |  106 +++--
>  drivers/scsi/scsi_priv.h |2 -
>  3 files changed, 46 insertions(+), 71 deletions(-)
> 

(snip)

> @@ -310,7 +313,6 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
>  {
>   struct scsi_host_cmd_pool *pool;
>   struct scsi_cmnd *cmd;
> - unsigned char *sense_buffer;
>  
>   spin_lock_init(&shost->free_list_lock);
>   INIT_LIST_HEAD(&shost->free_list);
> @@ -322,10 +324,13 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
>   mutex_lock(&host_cmd_pool_mutex);
>   pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
>   if (!pool->users) {
> - pool->slab = kmem_cache_create(pool->name,
> - sizeof(struct scsi_cmnd), 0,
> - pool->slab_flags, NULL);
> - if (!pool->slab)
> + pool->cmd_slab = kmem_cache_create(pool->cmd_name,
> +sizeof(struct scsi_cmnd), 0,
> +pool->slab_flags, NULL);
> + pool->sense_slab = kmem_cache_create(pool->sense_name,
> +  SCSI_SENSE_BUFFERSIZE, 0,
> +  pool->slab_flags, NULL);
> + if (!pool->cmd_slab || !pool->sense_slab)
>   goto fail;


If one of the above allocations fails, the allocated slab leaks?


diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 045e69d..1a9fba6 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -327,11 +327,16 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
pool->cmd_slab = kmem_cache_create(pool->cmd_name,
   sizeof(struct scsi_cmnd), 0,
   pool->slab_flags, NULL);
+   if (!pool->cmd_slab)
+   goto fail;
+
pool->sense_slab = kmem_cache_create(pool->sense_name,
 SCSI_SENSE_BUFFERSIZE, 0,
 pool->slab_flags, NULL);
-   if (!pool->cmd_slab || !pool->sense_slab)
+   if (!pool->sense_slab) {
+   kmem_cache_destroy(pool->cmd_slab);
goto fail;
+   }
}
 
pool->users++;
-
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] firewire: fw-sbp2: prepare for s/g chaining

[FUJITA Tomonori]

On Tue, 15 Jan 2008 21:10:50 +0100 (CET)
Stefan Richter <[EMAIL PROTECTED]> wrote:

> Signed-off-by: Stefan Richter <[EMAIL PROTECTED]>
> ---
> 
> Replacement of patch "firewire: fw-sbp2: enable s/g chaining".
> 
> It's the same, minus '+ .use_sg_chaining = ENABLE_SG_CHAINING,' hunk
> to prevent conflicts when James is going to remove .use_sg_chaining.
> 
> 
>  drivers/firewire/fw-sbp2.c |7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Index: linux/drivers/firewire/fw-sbp2.c
> ===
> --- linux.orig/drivers/firewire/fw-sbp2.c
> +++ linux/drivers/firewire/fw-sbp2.c
> @@ -1107,9 +1107,9 @@ sbp2_map_scatterlist(struct sbp2_command
>* elements larger than 65535 bytes, some IOMMUs may merge sg elements
>* during DMA mapping, and Linux currently doesn't prevent this.
>*/

On a relate note, I fixed the IOMMU merge issue. The patches have been
-mm though I'm not sure whether they will go into v2.6.25. The patches
enable you to remove the following workaround if you configure the
maximum sg element length.

>From a quick look, fw-sbp2 uses scsi-ml in a different way so it would
be a bit trick to configure the maximum sg element length.

You call dma_map_sg with pci_dev::dev but don't call scsi_add_host
with pci_dev::dev.

If you set the maximum sg element length to pci_dev::dev, and then
call scsi_add_host with it, the block layer and the IOMMU send you
proper size sg elements.


> - for (i = 0, j = 0; i < count; i++) {
> - sg_len = sg_dma_len(sg + i);
> - sg_addr = sg_dma_address(sg + i);
> + for (i = 0, j = 0; i < count; i++, sg = sg_next(sg)) {
> + sg_len = sg_dma_len(sg);
> + sg_addr = sg_dma_address(sg);
>   while (sg_len) {
>   /* FIXME: This won't get us out of the pinch. */
>   if (unlikely(j >= ARRAY_SIZE(orb->page_table))) {
> 
> -- 
> Stefan Richter
> -=-==--- ---= -
> http://arcgraph.de/sr/
> 
> -
> 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: [PATCH v2] use dynamically allocated sense buffer

[FUJITA Tomonori]

On Thu, 17 Jan 2008 09:58:11 -0600
James Bottomley <[EMAIL PROTECTED]> wrote:

> 
> On Thu, 2008-01-17 at 18:13 +0900, FUJITA Tomonori wrote:
> > On Wed, 16 Jan 2008 14:35:50 +0200
> > Benny Halevy <[EMAIL PROTECTED]> wrote:
> > 
> > > On Jan. 15, 2008, 17:20 +0200, James Bottomley <[EMAIL PROTECTED]> wrote:
> > > > On Tue, 2008-01-15 at 18:23 +0900, FUJITA Tomonori wrote:
> > > >> This is the second version of
> > > >>
> > > >> http://marc.info/?l=linux-scsi&m=119933628210006&w=2
> > > >>
> > > >> I gave up once, but I found that the performance loss is negligible
> > > >> (within 1%) by using kmem_cache_alloc instead of mempool.
> > > >>
> > > >> I use scsi_debug with fake_rw=1 and disktest (DIO reads with 8
> > > >> threads) again:
> > > >>
> > > >> scsi-misc (slub) | 486.9 MB/s  IOPS 124652.9/s
> > > >> dynamic sense buf (slub) | 483.2 MB/s  IOPS 123704.1/s
> > > >>
> > > >> scsi-misc (slab) | 467.0 MB/s  IOPS 119544.3/s
> > > >> dynamic sense buf (slab) | 468.7 MB/s  IOPS 119986.0/s
> > > >>
> > > >> The results are the averages of three runs with a server using two
> > > >> dual-core 1.60 GHz Xeon processors with DDR2 memory.
> > > >>
> > > >>
> > > >> I doubt think that someone will complain about the performance
> > > >> regression due to this patch. In addition, unlike scsi_debug, the real
> > > >> LLDs allocate the own data structure per scsi_cmnd so the performance
> > > >> differences would be smaller (and with the real hard disk overheads).
> > > >>
> > > >> Here's the full results:
> > > >>
> > > >> http://www.kernel.org/pub/linux/kernel/people/tomo/sense/results.txt
> > > > 
> > > > Heh, that's one of those good news, bad news things.  Certainly good
> > > > news for you.  The bad news for the rest of us is that you just
> > > > implicated mempool in a performance problem  and since they're the core
> > > > of the SCSI scatterlist allocations and sit at the heart of the critical
> > > > path in SCSI, we have a potential performance issue in the whole of
> > > > SCSI.
> > > 
> > > Looking at mempool's code this is peculiar as what seems to be its
> > > critical path for alloc and free looks pretty harmless and lightweight.
> > > Maybe an extra memory barrier, spin_{,un}lock_* and two extra function 
> > > call
> > > (one of them can be eliminated BTW if the order of arguments to the
> > > mempool_{alloc,free}_t functions were the same as for 
> > > kmem_cache_{alloc,free}).
> > 
> > Yeah, so I wondered why the change made a big difference. After more
> > testing, it turned out that mempool is not so slow.
> > 
> > v1 patch reserves as many buffers as can_queue per shost. My test
> > server allocates 1519 sense buffers in total and then needs to
> > allocate more. Seems that it hurts the performance.
> 
> I would bet it does.  Mempools aren't a performance enhancer, they're a
> deadlock avoider.  So you don't prefill them with 1519 entries per host,
> you prefill them with at most two so that we can always guarantee
> getting a writeout command down in the event the system is totally out
> of GFP_ATOMIC memory and needs to free something.

Yes, I misunderstood how mempool works.

BTW, I preallocated as many buffers as can_queue an then the server
allocates 1519 buffers in total (with five scsi hosts).


> Plus, pool allocations of that size will get me hunted down and shot by
> the linux tiny (or other embedded) community.

Definitely.


> > I modified v3 patch to allocate unused 1519 sense buffers via
> > kmem_cache_alloc. It achieved 96.2% of the scsi-misc performance (note
> > that v1 patch achieved 94.6% of the scsi-misc).
> > 
> > I modified v3 patch to use mempool to allocate one buffer per host. It
> > achieved 98.3% of the scsi-misc (note that v3 patch achieved 99.3% of
> > the scsi-misc).
> 
> This is about the correct thing to do.

Will you merge the v3 patch or the modified v3 patch to use mempool to
allocate one buffer per host? Or always allocating sense buffer costs
too much?
-
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] atp870u: don't zero out sense_buffer in queuecommand

[FUJITA Tomonori]

LLDs don't need to zero out scsi_cmnd::sense_buffer in queuecommand
since scsi-ml does. This is a preparation of the future changes to
allocate the sense_buffer only when necessary.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/atp870u.c |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/atp870u.c b/drivers/scsi/atp870u.c
index db6de5e..fb27f2c 100644
--- a/drivers/scsi/atp870u.c
+++ b/drivers/scsi/atp870u.c
@@ -613,7 +613,6 @@ static int atp870u_queuecommand(struct scsi_cmnd * req_p,
struct Scsi_Host *host;
 
c = scmd_channel(req_p);
-   req_p->sense_buffer[0]=0;
scsi_set_resid(req_p, 0);
if (scmd_channel(req_p) > 1) {
req_p->result = 0x0004;
-- 
1.5.3.4

-
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: Performance of SCST versus STGT

[FUJITA Tomonori]

On Thu, 17 Jan 2008 12:48:28 +0300
Vladislav Bolkhovitin <[EMAIL PROTECTED]> wrote:

> FUJITA Tomonori wrote:
> > On Thu, 17 Jan 2008 10:27:08 +0100
> > "Bart Van Assche" <[EMAIL PROTECTED]> wrote:
> > 
> > 
> >>Hello,
> >>
> >>I have performed a test to compare the performance of SCST and STGT.
> >>Apparently the SCST target implementation performed far better than
> >>the STGT target implementation. This makes me wonder whether this is
> >>due to the design of SCST or whether STGT's performance can be
> >>improved to the level of SCST ?
> >>
> >>Test performed: read 2 GB of data in blocks of 1 MB from a target (hot
> >>cache -- no disk reads were performed, all reads were from the cache).
> >>Test command: time dd if=/dev/sde of=/dev/null bs=1M count=2000
> >>
> >>  STGT read SCST read
> >>   performance (MB/s)   performance (MB/s)
> >>Ethernet (1 Gb/s network)7789
> >>IPoIB (8 Gb/s network)   82   229
> >>SRP (8 Gb/s network)N/A   600
> >>iSER (8 Gb/s network)80   N/A
> >>
> >>These results show that SCST uses the InfiniBand network very well
> >>(effectivity of about 88% via SRP), but that the current STGT version
> >>is unable to transfer data faster than 82 MB/s. Does this mean that
> >>there is a severe bottleneck  present in the current STGT
> >>implementation ?
> > 
> > 
> > I don't know about the details but Pete said that he can achieve more
> > than 900MB/s read performance with tgt iSER target using ramdisk.
> > 
> > http://www.mail-archive.com/[EMAIL PROTECTED]/msg4.html
> 
> Please don't confuse multithreaded latency insensitive workload with 
> single threaded, hence latency sensitive one.

Seems that he can get good performance with single threaded workload:

http://www.osc.edu/~pw/papers/wyckoff-iser-snapi07-talk.pdf


But I don't know about the details so let's wait for Pete to comment
on this.

Perhaps Voltaire people could comment on the tgt iSER performances.
-
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: Performance of SCST versus STGT

[FUJITA Tomonori]

On Thu, 17 Jan 2008 10:27:08 +0100
"Bart Van Assche" <[EMAIL PROTECTED]> wrote:

> Hello,
> 
> I have performed a test to compare the performance of SCST and STGT.
> Apparently the SCST target implementation performed far better than
> the STGT target implementation. This makes me wonder whether this is
> due to the design of SCST or whether STGT's performance can be
> improved to the level of SCST ?
> 
> Test performed: read 2 GB of data in blocks of 1 MB from a target (hot
> cache -- no disk reads were performed, all reads were from the cache).
> Test command: time dd if=/dev/sde of=/dev/null bs=1M count=2000
> 
>   STGT read SCST read
>performance (MB/s)   performance (MB/s)
> Ethernet (1 Gb/s network)7789
> IPoIB (8 Gb/s network)   82   229
> SRP (8 Gb/s network)N/A   600
> iSER (8 Gb/s network)80   N/A
> 
> These results show that SCST uses the InfiniBand network very well
> (effectivity of about 88% via SRP), but that the current STGT version
> is unable to transfer data faster than 82 MB/s. Does this mean that
> there is a severe bottleneck  present in the current STGT
> implementation ?

I don't know about the details but Pete said that he can achieve more
than 900MB/s read performance with tgt iSER target using ramdisk.

http://www.mail-archive.com/[EMAIL PROTECTED]/msg4.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: [PATCH v2] use dynamically allocated sense buffer

[FUJITA Tomonori]

On Wed, 16 Jan 2008 14:35:50 +0200
Benny Halevy <[EMAIL PROTECTED]> wrote:

> On Jan. 15, 2008, 17:20 +0200, James Bottomley <[EMAIL PROTECTED]> wrote:
> > On Tue, 2008-01-15 at 18:23 +0900, FUJITA Tomonori wrote:
> >> This is the second version of
> >>
> >> http://marc.info/?l=linux-scsi&m=119933628210006&w=2
> >>
> >> I gave up once, but I found that the performance loss is negligible
> >> (within 1%) by using kmem_cache_alloc instead of mempool.
> >>
> >> I use scsi_debug with fake_rw=1 and disktest (DIO reads with 8
> >> threads) again:
> >>
> >> scsi-misc (slub) | 486.9 MB/s  IOPS 124652.9/s
> >> dynamic sense buf (slub) | 483.2 MB/s  IOPS 123704.1/s
> >>
> >> scsi-misc (slab) | 467.0 MB/s  IOPS 119544.3/s
> >> dynamic sense buf (slab) | 468.7 MB/s  IOPS 119986.0/s
> >>
> >> The results are the averages of three runs with a server using two
> >> dual-core 1.60 GHz Xeon processors with DDR2 memory.
> >>
> >>
> >> I doubt think that someone will complain about the performance
> >> regression due to this patch. In addition, unlike scsi_debug, the real
> >> LLDs allocate the own data structure per scsi_cmnd so the performance
> >> differences would be smaller (and with the real hard disk overheads).
> >>
> >> Here's the full results:
> >>
> >> http://www.kernel.org/pub/linux/kernel/people/tomo/sense/results.txt
> > 
> > Heh, that's one of those good news, bad news things.  Certainly good
> > news for you.  The bad news for the rest of us is that you just
> > implicated mempool in a performance problem  and since they're the core
> > of the SCSI scatterlist allocations and sit at the heart of the critical
> > path in SCSI, we have a potential performance issue in the whole of
> > SCSI.
> 
> Looking at mempool's code this is peculiar as what seems to be its
> critical path for alloc and free looks pretty harmless and lightweight.
> Maybe an extra memory barrier, spin_{,un}lock_* and two extra function call
> (one of them can be eliminated BTW if the order of arguments to the
> mempool_{alloc,free}_t functions were the same as for 
> kmem_cache_{alloc,free}).

Yeah, so I wondered why the change made a big difference. After more
testing, it turned out that mempool is not so slow.

v1 patch reserves as many buffers as can_queue per shost. My test
server allocates 1519 sense buffers in total and then needs to
allocate more. Seems that it hurts the performance.

I modified v3 patch to allocate unused 1519 sense buffers via
kmem_cache_alloc. It achieved 96.2% of the scsi-misc performance (note
that v1 patch achieved 94.6% of the scsi-misc).

I modified v3 patch to use mempool to allocate one buffer per host. It
achieved 98.3% of the scsi-misc (note that v3 patch achieved 99.3% of
the scsi-misc).

So I could say:

- mempool is only about 1% slower to using kmem_cache directly.

- reserving lots of slabs seems to hurt SLUB performance (I've not dug
into it yet).


The full results and two patches are:

http://www.kernel.org/pub/linux/kernel/people/tomo/sense/
-
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: INITIO scsi driver fails to work properly

[FUJITA Tomonori]

On Tue, 15 Jan 2008 09:16:06 -0600
James Bottomley <[EMAIL PROTECTED]> wrote:

> 
> On Sun, 2008-01-13 at 14:28 +0200, Filippos Papadopoulos wrote:
> > On 1/11/08, James Bottomley <[EMAIL PROTECTED]> wrote:
> > >
> > > On Fri, 2008-01-11 at 18:44 +0200, Filippos Papadopoulos wrote:
> > > > On Jan 11, 2008 5:44 PM, James Bottomley
> > > > <[EMAIL PROTECTED]> wrote:
> > > > > >
> > > > > > I havent reported "initio: I/O port range 0x0 is busy."
> > > > >
> > > > > Sorry ... we appear to have several reporters of different bugs in 
> > > > > this
> > > > > thread.  That message was copied by Chuck Ebbert from a Red Hat
> > > > > bugzilla ... I was assuming it was the same problem.
> > > > >
> > > > > > I applied the patch on 2.6.24-rc6-git9 but unfortunatelly same 
> > > > > > thing happens.
> > > > >
> > > > > First off, has this driver ever worked for you in 2.6?  Just booting
> > > > > SLES9 (2.6.5) or RHEL4 (2.6.9) ... or one of their open equivalents to
> > > > > check a really old kernel would be helpful.  If you can get it to 
> > > > > work,
> > > > > then we can proceed with a patch reversion regime based on the
> > > > > assumption that the problem is a recent commit.
> > > >
> > > > Yes it works under 2.6.16.13.  See the beginning of this thread, i
> > > > mention there some things about newer versions.
> > >
> > > Thanks, actually, I see this:
> > >
> > > > I tried to install OpenSUSE 10.3 (kernel 2.6.22.5) and the latest
> > > > OpenSUSE 11.0 Alpha 0  (kernel 2.6.24-rc4) but although the initio
> > > > drivergets loaded during the installation process, yast reports that no 
> > > > hard
> > > > disk is found.
> > >
> > > Could you try with a vanilla 2.6.22 kernel?  The reason for all of this
> > > is that 2.6.22 predates Alan's conversion of this driver (which was my
> > > 95% candidate for the source of the bug).  I want you to try the vanilla
> > > kernel just in case the opensuse one contains a backport.
> > 
> > 
> > Yes you are right. I compiled the vanilla 2.6.22 and initio driver works.
> > Tell me if you want to apply any patch to it.
> 
> 
> That's good news ... at least we know where the issue lies; now the
> problem comes: there are two candidate patches for this issue: Alan's
> driver update patch and Tomo's accessors patch.  Unfortunately, due to
> merge conflicts the two are pretty hopelessly intertwined.  I think I
> already spotted one bug in the accessor conversion, so I'll look at that
> again.  Alan's also going to acquire an inito board and retest his
> conversions.
> 
> I'm afraid it might be a while before we have anything for you to test.

Can you try this patch?

Thanks,

diff --git a/drivers/scsi/initio.c b/drivers/scsi/initio.c
index 01bf018..6891d2b 100644
--- a/drivers/scsi/initio.c
+++ b/drivers/scsi/initio.c
@@ -2609,6 +2609,7 @@ static void initio_build_scb(struct initio_host * host, 
struct scsi_ctrl_blk * c
cblk->bufptr = cpu_to_le32((u32)dma_addr);
cmnd->SCp.dma_handle = dma_addr;
 
+   cblk->sglen = nseg;
 
cblk->flags |= SCF_SG;  /* Turn on SG list flag   */
total_len = 0;
-
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 v3] use dynamically allocated sense buffer

[FUJITA Tomonori]

This is the third version of:

http://marc.info/?l=linux-scsi&m=120038907123706&w=2

The changes from the second version are:

- Fixed the memory leak bug that Boaz pointed out.

shots->backup_sense_buffer has gone. One sense buffer is allocated per
host and it's always attached to the scsi_cmnd linked with freelist
(backup command).

- Calling scsi_setup_command_sense_buffer in scsi_host_alloc instead
of scsi_add_host.

The first version tries to allocates as many buffers as
shost->can_queue so scsi_setup_command_sense_buffer is called in
scsi_add_host. But, it's not the case any more, so this calls
scsi_setup_command_sense_buffer in scsi_host_alloc like
scsi_setup_command_freelist.


I did the same tests against this patch (though I knew there were not
the performnace differences):

dynamic sense buf (slub) | 483.5 MB/s  IOPS 123772.7/s

For convenience, here are the previous results:

scsi-misc (slub) | 486.9 MB/s  IOPS 124652.9/s
dynamic sense buf (slub) | 483.2 MB/s  IOPS 123704.1/s


I put the results and the kernel configuration:

http://www.kernel.org/pub/linux/kernel/people/tomo/sense/


=
From: FUJITA Tomonori <[EMAIL PROTECTED]>
Subject: [PATCH] use dynamically allocated sense buffer

This removes static array sense_buffer in scsi_cmnd and uses
dynamically allocated sense_buffer (with GFP_DMA).

The reason for doing this is that some architectures need cacheline
aligned buffer for DMA:

http://lkml.org/lkml/2007/11/19/2

The problems are that scsi_eh_prep_cmnd puts scsi_cmnd::sense_buffer
to sglist and some LLDs directly DMA to scsi_cmnd::sense_buffer. It's
necessary to DMA to scsi_cmnd::sense_buffer safely. This patch solves
these issues.

__scsi_get_command allocates sense_buffer via kmem_cache_alloc and
attaches it to a scsi_cmnd so everything just work as before.


Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/hosts.c |9 ++-
 drivers/scsi/scsi.c  |   61 -
 drivers/scsi/scsi_priv.h |2 +
 include/scsi/scsi_cmnd.h |2 +-
 4 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 9a10b43..f5d3fbb 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -268,6 +268,7 @@ static void scsi_host_dev_release(struct device *dev)
}
 
scsi_destroy_command_freelist(shost);
+   scsi_destroy_command_sense_buffer(shost);
if (shost->bqt)
blk_free_tags(shost->bqt);
 
@@ -372,10 +373,14 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
else
shost->dma_boundary = 0x;
 
-   rval = scsi_setup_command_freelist(shost);
+   rval = scsi_setup_command_sense_buffer(shost);
if (rval)
goto fail_kfree;
 
+   rval = scsi_setup_command_freelist(shost);
+   if (rval)
+   goto fail_destroy_sense;
+
device_initialize(&shost->shost_gendev);
snprintf(shost->shost_gendev.bus_id, BUS_ID_SIZE, "host%d",
shost->host_no);
@@ -399,6 +404,8 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
 
  fail_destroy_freelist:
scsi_destroy_command_freelist(shost);
+ fail_destroy_sense:
+   scsi_destroy_command_sense_buffer(shost);
  fail_kfree:
kfree(shost);
return NULL;
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 54ff611..0a4a5b8 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -161,6 +161,9 @@ static struct scsi_host_cmd_pool scsi_cmd_dma_pool = {
 
 static DEFINE_MUTEX(host_cmd_pool_mutex);
 
+static struct kmem_cache *sense_buffer_slab;
+static int sense_buffer_slab_users;
+
 /**
  * __scsi_get_command - Allocate a struct scsi_cmnd
  * @shost: host to transmit command
@@ -172,6 +175,7 @@ static DEFINE_MUTEX(host_cmd_pool_mutex);
 struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
 {
struct scsi_cmnd *cmd;
+   unsigned char *buf;
 
cmd = kmem_cache_alloc(shost->cmd_pool->slab,
gfp_mask | shost->cmd_pool->gfp_mask);
@@ -186,6 +190,21 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host 
*shost, gfp_t gfp_mask)
list_del_init(&cmd->list);
}
spin_unlock_irqrestore(&shost->free_list_lock, flags);
+
+   if (cmd) {
+   buf = cmd->sense_buffer;
+   memset(cmd, 0, sizeof(*cmd));
+   cmd->sense_buffer = buf;
+   }
+   } else {
+   buf = kmem_cache_alloc(sense_buffer_slab, __GFP_DMA|gfp_mask);
+   if (likely(buf)) {
+   memset(cmd, 0, sizeof(*cmd));
+   cmd->sense_buffer = buf;
+   } el

Re: [PATCH v2] use dynamically allocated sense buffer

[FUJITA Tomonori]

On Tue, 15 Jan 2008 17:44:14 +0200
Boaz Harrosh <[EMAIL PROTECTED]> wrote:

> > If __scsi_put_command puts a command to shost->free_list, it doesn't
> > free scmd->sense_buffer since it's the sense_buffer for the backup
> > sense_buffer. If __scsi_put_command puts a command to
> > shost->cmd_pool->slab (if shost->free_list isn't empty), it alos puts
> > its sense_buffer to sense_buffer_slab.
> 
> Yes, but these are not necessarily the same commands. Think of this,

Ah, sorry, I need to update shost->backup_sense_buffer when
__scsi_put_command puts a command to the free_list.


> The run queues have commands in them, a request comes that demands
> a cmnd, out-of-memory condition causes the spare from free_list cmnd to
> be issued, and is put at tail of some run queue. Now comes the first
> done cmnd, it is immediately put to free_list, but it's sense_buffer
> was from sense_buffer_slab.
> 
> I think the solution is simple just immediately allocate the sense_buffer
> in scsi_setup_command_freelist() and put it on that first free_list command.
> Then make sure that also the sense_buffer is freed in 
> scsi_destroy_command_freelist().
> 
> This way sense_buffer is always allocated/freed together with cmnd and
> you don't need the shost->backup_sense_buffer pointer.

Yeah, it's more straightforward. I'll submit an update patch soon.

Thanks,
-
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 v2] use dynamically allocated sense buffer

[FUJITA Tomonori]

On Tue, 15 Jan 2008 15:56:56 +0200
Boaz Harrosh <[EMAIL PROTECTED]> wrote:

> On Tue, Jan 15 2008 at 11:23 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> > This is the second version of
> > 
> > http://marc.info/?l=linux-scsi&m=119933628210006&w=2
> > 
> > I gave up once, but I found that the performance loss is negligible
> > (within 1%) by using kmem_cache_alloc instead of mempool.
> > 
> > I use scsi_debug with fake_rw=1 and disktest (DIO reads with 8
> > threads) again:
> > 
> > scsi-misc (slub) | 486.9 MB/s  IOPS 124652.9/s
> > dynamic sense buf (slub) | 483.2 MB/s  IOPS 123704.1/s
> > 
> > scsi-misc (slab) | 467.0 MB/s  IOPS 119544.3/s
> > dynamic sense buf (slab) | 468.7 MB/s  IOPS 119986.0/s
> > 
> > The results are the averages of three runs with a server using two
> > dual-core 1.60 GHz Xeon processors with DDR2 memory.
> > 
> > 
> > I doubt think that someone will complain about the performance
> > regression due to this patch. In addition, unlike scsi_debug, the real
> > LLDs allocate the own data structure per scsi_cmnd so the performance
> > differences would be smaller (and with the real hard disk overheads).
> > 
> > Here's the full results:
> > 
> > http://www.kernel.org/pub/linux/kernel/people/tomo/sense/results.txt
> > 
> TOMO Hi.
> This is grate news. Thanks I like what you did here. and it's good
> to know. Why should a mempool be so slow ;)
> 
> I have a small concern of a leak, please see below, but otherwise
> this is grate.
> > 
> > =
> > From: FUJITA Tomonori <[EMAIL PROTECTED]>
> > Subject: [PATCH] use dynamically allocated sense buffer
> > 
> > This removes static array sense_buffer in scsi_cmnd and uses
> > dynamically allocated sense_buffer (with GFP_DMA).
> > 
> > The reason for doing this is that some architectures need cacheline
> > aligned buffer for DMA:
> > 
> > http://lkml.org/lkml/2007/11/19/2
> > 
> > The problems are that scsi_eh_prep_cmnd puts scsi_cmnd::sense_buffer
> > to sglist and some LLDs directly DMA to scsi_cmnd::sense_buffer. It's
> > necessary to DMA to scsi_cmnd::sense_buffer safely. This patch solves
> > these issues.
> > 
> > __scsi_get_command allocates sense_buffer via kmem_cache_alloc and
> > attaches it to a scsi_cmnd so everything just work as before.
> > 
> > A scsi_host reserves one sense buffer for the backup command
> > (shost->backup_sense_buffer).
> > 
> > 
> > Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
> > ---
> >  drivers/scsi/hosts.c |   10 ++-
> >  drivers/scsi/scsi.c  |   67 
> > -
> >  drivers/scsi/scsi_priv.h |2 +
> >  include/scsi/scsi_cmnd.h |2 +-
> >  include/scsi/scsi_host.h |3 ++
> >  5 files changed, 80 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> > index 9a10b43..35c5f0e 100644
> > --- a/drivers/scsi/hosts.c
> > +++ b/drivers/scsi/hosts.c
> > @@ -205,10 +205,14 @@ int scsi_add_host(struct Scsi_Host *shost, struct 
> > device *dev)
> > if (!shost->shost_gendev.parent)
> > shost->shost_gendev.parent = dev ? dev : &platform_bus;
> >  
> > -   error = device_add(&shost->shost_gendev);
> > +   error = scsi_setup_command_sense_buffer(shost);
> > if (error)
> > goto out;
> >  
> > +   error = device_add(&shost->shost_gendev);
> > +   if (error)
> > +   goto destroy_pool;
> > +
> > scsi_host_set_state(shost, SHOST_RUNNING);
> > get_device(shost->shost_gendev.parent);
> >  
> > @@ -248,6 +252,8 @@ int scsi_add_host(struct Scsi_Host *shost, struct 
> > device *dev)
> > class_device_del(&shost->shost_classdev);
> >   out_del_gendev:
> > device_del(&shost->shost_gendev);
> > + destroy_pool:
> > +   scsi_destroy_command_sense_buffer(shost);
> >   out:
> > return error;
> >  }
> > @@ -267,6 +273,8 @@ static void scsi_host_dev_release(struct device *dev)
> > scsi_free_queue(shost->uspace_req_q);
> > }
> >  
> > +   scsi_destroy_command_sense_buffer(shost);
> > +
> > scsi_destroy_command_freelist(shost);
> > if (shost->bqt)
> > blk_free_tags(shost->bqt);
> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index 54ff611..d153da3 100644
> &g

[PATCH v2] use dynamically allocated sense buffer

[FUJITA Tomonori]

This is the second version of

http://marc.info/?l=linux-scsi&m=119933628210006&w=2

I gave up once, but I found that the performance loss is negligible
(within 1%) by using kmem_cache_alloc instead of mempool.

I use scsi_debug with fake_rw=1 and disktest (DIO reads with 8
threads) again:

scsi-misc (slub) | 486.9 MB/s  IOPS 124652.9/s
dynamic sense buf (slub) | 483.2 MB/s  IOPS 123704.1/s

scsi-misc (slab) | 467.0 MB/s  IOPS 119544.3/s
dynamic sense buf (slab) | 468.7 MB/s  IOPS 119986.0/s

The results are the averages of three runs with a server using two
dual-core 1.60 GHz Xeon processors with DDR2 memory.


I doubt think that someone will complain about the performance
regression due to this patch. In addition, unlike scsi_debug, the real
LLDs allocate the own data structure per scsi_cmnd so the performance
differences would be smaller (and with the real hard disk overheads).

Here's the full results:

http://www.kernel.org/pub/linux/kernel/people/tomo/sense/results.txt


=
From: FUJITA Tomonori <[EMAIL PROTECTED]>
Subject: [PATCH] use dynamically allocated sense buffer

This removes static array sense_buffer in scsi_cmnd and uses
dynamically allocated sense_buffer (with GFP_DMA).

The reason for doing this is that some architectures need cacheline
aligned buffer for DMA:

http://lkml.org/lkml/2007/11/19/2

The problems are that scsi_eh_prep_cmnd puts scsi_cmnd::sense_buffer
to sglist and some LLDs directly DMA to scsi_cmnd::sense_buffer. It's
necessary to DMA to scsi_cmnd::sense_buffer safely. This patch solves
these issues.

__scsi_get_command allocates sense_buffer via kmem_cache_alloc and
attaches it to a scsi_cmnd so everything just work as before.

A scsi_host reserves one sense buffer for the backup command
(shost->backup_sense_buffer).


Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/hosts.c |   10 ++-
 drivers/scsi/scsi.c  |   67 -
 drivers/scsi/scsi_priv.h |2 +
 include/scsi/scsi_cmnd.h |2 +-
 include/scsi/scsi_host.h |3 ++
 5 files changed, 80 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 9a10b43..35c5f0e 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -205,10 +205,14 @@ int scsi_add_host(struct Scsi_Host *shost, struct device 
*dev)
if (!shost->shost_gendev.parent)
shost->shost_gendev.parent = dev ? dev : &platform_bus;
 
-   error = device_add(&shost->shost_gendev);
+   error = scsi_setup_command_sense_buffer(shost);
if (error)
goto out;
 
+   error = device_add(&shost->shost_gendev);
+   if (error)
+   goto destroy_pool;
+
scsi_host_set_state(shost, SHOST_RUNNING);
get_device(shost->shost_gendev.parent);
 
@@ -248,6 +252,8 @@ int scsi_add_host(struct Scsi_Host *shost, struct device 
*dev)
class_device_del(&shost->shost_classdev);
  out_del_gendev:
device_del(&shost->shost_gendev);
+ destroy_pool:
+   scsi_destroy_command_sense_buffer(shost);
  out:
return error;
 }
@@ -267,6 +273,8 @@ static void scsi_host_dev_release(struct device *dev)
scsi_free_queue(shost->uspace_req_q);
}
 
+   scsi_destroy_command_sense_buffer(shost);
+
scsi_destroy_command_freelist(shost);
if (shost->bqt)
blk_free_tags(shost->bqt);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 54ff611..d153da3 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -161,6 +161,9 @@ static struct scsi_host_cmd_pool scsi_cmd_dma_pool = {
 
 static DEFINE_MUTEX(host_cmd_pool_mutex);
 
+static struct kmem_cache *sense_buffer_slab;
+static int sense_buffer_slab_users;
+
 /**
  * __scsi_get_command - Allocate a struct scsi_cmnd
  * @shost: host to transmit command
@@ -186,6 +189,22 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host 
*shost, gfp_t gfp_mask)
list_del_init(&cmd->list);
}
spin_unlock_irqrestore(&shost->free_list_lock, flags);
+
+   if (cmd) {
+   memset(cmd, 0, sizeof(*cmd));
+   cmd->sense_buffer = shost->backup_sense_buffer;
+   }
+   } else {
+   unsigned char *buf;
+
+   buf = kmem_cache_alloc(sense_buffer_slab, __GFP_DMA|gfp_mask);
+   if (likely(buf)) {
+   memset(cmd, 0, sizeof(*cmd));
+   cmd->sense_buffer = buf;
+   } else {
+   kmem_cache_free(shost->cmd_pool->slab, cmd);
+   cmd = NULL;
+   }
}
 
return cmd;
@@ -212,7 +231,6 @@ struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, 
gfp_t gfp_mask)
if (likely(cmd != NULL)) {
  

Re: panic in sg_add when class_device_create() fails

[FUJITA Tomonori]

On Mon, 14 Jan 2008 14:11:36 -0800
Michael Reed <[EMAIL PROTECTED]> wrote:

> We're seeing an occasional panic in sg_add() when class_device_create()
> fails.  It's obvious in the code that it uses the pointer to sg_class_member
> even though it's invalid.  We do see the "class_device_create failed" message.
> 
> class_set_devdata(cl_dev, sdp);
> error = cdev_add(cdev, MKDEV(SCSI_GENERIC_MAJOR, sdp->index), 1);
> if (error)
> goto cdev_add_err;
> 
> if (sg_sysfs_valid) {
> struct class_device * sg_class_member;
> 
> sg_class_member = class_device_create(sg_sysfs_class, NULL,
> MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
> cl_dev->dev, "%s",
> disk->disk_name);
> if (IS_ERR(sg_class_member))
> printk(KERN_WARNING "sg_add: "
> "class_device_create failed\n");
> class_set_devdata(sg_class_member, sdp);
>   
> error = sysfs_create_link(&scsidp->sdev_gendev.kobj,
>   &sg_class_member->kobj, "generic");
> if (error)
> printk(KERN_ERR "sg_add: unable to make symlink "
> "'generic' back to sg%d\n", 
> sdp->index);
> } else
> printk(KERN_WARNING "sg_add: sg_sys Invalid\n");
> 
> I'm uncertain of the correct fix.  Perhaps it involves a call to 
> cdev_unmap()? 

The following patches work for me:

http://marc.info/?l=linux-scsi&m=120037070303939&w=2
http://marc.info/?l=linux-scsi&m=120037070303941&w=2


> I don't have a good way to test a fix as this problem is not easily
> reproduced.

I used the attached patch (though the fault injection feature can do
something better, I guess).


diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 527e2eb..4cdd213 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1424,12 +1424,16 @@ sg_add(struct class_device *cl_dev, struct 
class_interface *cl_intf)
 
sdp->cdev = cdev;
if (sg_sysfs_valid) {
+   static int count = 0;
struct class_device * sg_class_member;
 
-   sg_class_member = class_device_create(sg_sysfs_class, NULL,
-   MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
-   cl_dev->dev, "%s",
-   disk->disk_name);
+   if (++count % 2)
+   sg_class_member = class_device_create(sg_sysfs_class, 
NULL,
+ 
MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
+ cl_dev->dev, "%s",
+ disk->disk_name);
+   else
+   sg_class_member = ERR_PTR(-ENOMEM);
if (IS_ERR(sg_class_member)) {
printk(KERN_ERR "sg_add: "
   "class_device_create failed\n");


-
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/2] sg: handle class_device_create failure properly

[FUJITA Tomonori]

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/sg.c |   11 +++
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 92b4367..527e2eb 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1430,11 +1430,14 @@ sg_add(struct class_device *cl_dev, struct 
class_interface *cl_intf)
MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
cl_dev->dev, "%s",
disk->disk_name);
-   if (IS_ERR(sg_class_member))
-   printk(KERN_WARNING "sg_add: "
-   "class_device_create failed\n");
+   if (IS_ERR(sg_class_member)) {
+   printk(KERN_ERR "sg_add: "
+  "class_device_create failed\n");
+   error = PTR_ERR(sg_class_member);
+   goto cdev_add_err;
+   }
class_set_devdata(sg_class_member, sdp);
-   error = sysfs_create_link(&scsidp->sdev_gendev.kobj, 
+   error = sysfs_create_link(&scsidp->sdev_gendev.kobj,
  &sg_class_member->kobj, "generic");
if (error)
printk(KERN_ERR "sg_add: unable to make symlink "
-- 
1.5.3.4

-
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 1/2] sg: set class_data after success

[FUJITA Tomonori]

If cdev_add fails in sg_add, sg_remove crashes since class_data is
bogus.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/sg.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index f1871ea..92b4367 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1418,7 +1418,6 @@ sg_add(struct class_device *cl_dev, struct 
class_interface *cl_intf)
goto out;
}
 
-   class_set_devdata(cl_dev, sdp);
error = cdev_add(cdev, MKDEV(SCSI_GENERIC_MAJOR, sdp->index), 1);
if (error)
goto cdev_add_err;
@@ -1447,6 +1446,8 @@ sg_add(struct class_device *cl_dev, struct 
class_interface *cl_intf)
"Attached scsi generic sg%d type %d\n", sdp->index,
scsidp->type);
 
+   class_set_devdata(cl_dev, sdp);
+
return 0;
 
 cdev_add_err:
-- 
1.5.3.4

-
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/2] replace sizeof sense_buffer with SCSI_SENSE_BUFFERSIZE

[FUJITA Tomonori]

This is the second version of

http://marc.info/?l=linux-scsi&m=119933628210012&w=2

o I dropped fas216 since Boaz's patch in scsi-pending will be merged
before solving the sense_buffer dma issue.

o This is a 'grep and replace' style patch but cleans up dpt_i2o a bit
as by permission of Mark (I use min macro).

o The previous version overlooked some sizeof sense_buffer lines in
aacraid and qla4xxx.

o I overlooked the ncr53c8xx compile warning.

=
From: FUJITA Tomonori <[EMAIL PROTECTED]>
Subject: [PATCH 2/2] replace sizeof sense_buffer with SCSI_SENSE_BUFFERSIZE

This replaces sizeof sense_buffer with SCSI_SENSE_BUFFERSIZE in
several LLDs. It's a preparation for the future changes to remove
sense_buffer array in scsi_cmnd structure.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/ata/libata-scsi.c   |4 ++--
 drivers/message/fusion/mptscsih.c   |2 +-
 drivers/message/i2o/i2o_scsi.c  |2 +-
 drivers/scsi/53c700.c   |   11 ++-
 drivers/scsi/BusLogic.c |2 +-
 drivers/scsi/aacraid/aachba.c   |   32 
 drivers/scsi/advansys.c |   14 +++---
 drivers/scsi/aha1542.c  |4 ++--
 drivers/scsi/aha1740.c  |2 +-
 drivers/scsi/aic7xxx/aic79xx_osm.c  |6 +++---
 drivers/scsi/aic7xxx/aic7xxx_osm.c  |6 +++---
 drivers/scsi/aic7xxx_old.c  |   10 +-
 drivers/scsi/arcmsr/arcmsr_hba.c|6 +++---
 drivers/scsi/dc395x.c   |   16 +++-
 drivers/scsi/dpt_i2o.c  |5 ++---
 drivers/scsi/eata.c |4 ++--
 drivers/scsi/hptiop.c   |2 +-
 drivers/scsi/ips.c  |6 ++
 drivers/scsi/ncr53c8xx.c|3 ++-
 drivers/scsi/qla1280.c  |4 ++--
 drivers/scsi/qla2xxx/qla_isr.c  |   12 ++--
 drivers/scsi/qla4xxx/ql4_isr.c  |   11 ---
 drivers/scsi/qlogicpti.c|2 +-
 drivers/scsi/scsi_error.c   |6 +++---
 drivers/scsi/scsi_lib.c |2 +-
 drivers/scsi/sym53c8xx_2/sym_glue.c |5 ++---
 drivers/scsi/tmscsim.c  |6 +++---
 drivers/scsi/u14-34f.c  |4 ++--
 drivers/scsi/ultrastor.c|2 +-
 29 files changed, 92 insertions(+), 99 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4bb268b..b633341 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2334,7 +2334,7 @@ static void atapi_request_sense(struct ata_queued_cmd *qc)
DPRINTK("ATAPI request sense\n");
 
/* FIXME: is this needed? */
-   memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
+   memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
 
ap->ops->tf_read(ap, &qc->tf);
 
@@ -2344,7 +2344,7 @@ static void atapi_request_sense(struct ata_queued_cmd *qc)
 
ata_qc_reinit(qc);
 
-   ata_sg_init_one(qc, cmd->sense_buffer, sizeof(cmd->sense_buffer));
+   ata_sg_init_one(qc, cmd->sense_buffer, SCSI_SENSE_BUFFERSIZE);
qc->dma_dir = DMA_FROM_DEVICE;
 
memset(&qc->cdb, 0, qc->dev->cdb_len);
diff --git a/drivers/message/fusion/mptscsih.c 
b/drivers/message/fusion/mptscsih.c
index 626bb3c..5c614ec 100644
--- a/drivers/message/fusion/mptscsih.c
+++ b/drivers/message/fusion/mptscsih.c
@@ -111,7 +111,7 @@ int mptscsih_suspend(struct pci_dev *pdev, 
pm_message_t state);
 intmptscsih_resume(struct pci_dev *pdev);
 #endif
 
-#define SNS_LEN(scp)   sizeof((scp)->sense_buffer)
+#define SNS_LEN(scp)   SCSI_SENSE_BUFFERSIZE
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /**
diff --git a/drivers/message/i2o/i2o_scsi.c b/drivers/message/i2o/i2o_scsi.c
index aa6fb94..1bcdbbb 100644
--- a/drivers/message/i2o/i2o_scsi.c
+++ b/drivers/message/i2o/i2o_scsi.c
@@ -370,7 +370,7 @@ static int i2o_scsi_reply(struct i2o_controller *c, u32 m,
 */
if (cmd->result)
memcpy(cmd->sense_buffer, &msg->body[3],
-  min(sizeof(cmd->sense_buffer), (size_t) 40));
+  min(SCSI_SENSE_BUFFERSIZE, 40));
 
/* only output error code if AdapterStatus is not HBA_SUCCESS */
if ((error >> 8) & 0xff)
diff --git a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c
index 71ff3fb..f4c4fe9 100644
--- a/drivers/scsi/53c700.c
+++ b/drivers/scsi/53c700.c
@@ -608,7 +608,8 @@ NCR_700_scsi_done(struct NCR_700_Host_Parameters *hostdata,
scsi_print_sense("53c700", SCp);
 
 #endif
-   dma_unmap_single(hostdata->dev, slot->dma_handle, 
sizeof(SCp->sense_buffer), DMA_FROM_DEVICE);
+   dma_unmap_single(hostdata->dev, slot->dma_handle,
+ 

[PATCH 1/2] don't zero out sense_buffer in queuecommand

[FUJITA Tomonori]

LLDs don't need to zero out scsi_cmnd::sense_buffer in queuecommand
since scsi-ml does. This is a preparation of the future changes to
allocate the sense_buffer only when necessary.

Many LLDs zero out the sense_buffer before touching it on the error
case. This patch lets them alone for now because new APIs for them
would be added later on.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/aic7xxx_old.c  |1 -
 drivers/scsi/eata_pio.c |1 -
 drivers/scsi/ips.c  |3 ---
 drivers/scsi/libsas/sas_scsi_host.c |1 -
 4 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/aic7xxx_old.c b/drivers/scsi/aic7xxx_old.c
index 8f8db5f..2b402fa 100644
--- a/drivers/scsi/aic7xxx_old.c
+++ b/drivers/scsi/aic7xxx_old.c
@@ -10293,7 +10293,6 @@ static int aic7xxx_queue(struct scsi_cmnd *cmd, void 
(*fn)(struct scsi_cmnd *))
   aic7xxx_position(cmd) = scb->hscb->tag;
   cmd->scsi_done = fn;
   cmd->result = DID_OK;
-  memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
   aic7xxx_error(cmd) = DID_OK;
   aic7xxx_status(cmd) = 0;
   cmd->host_scribble = NULL;
diff --git a/drivers/scsi/eata_pio.c b/drivers/scsi/eata_pio.c
index 9579507..b5a6092 100644
--- a/drivers/scsi/eata_pio.c
+++ b/drivers/scsi/eata_pio.c
@@ -369,7 +369,6 @@ static int eata_pio_queue(struct scsi_cmnd *cmd,
cp = &hd->ccb[y];
 
memset(cp, 0, sizeof(struct eata_ccb));
-   memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
 
cp->status = USED;  /* claim free slot */
 
diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index e54d30c..b1b2295 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -2736,8 +2736,6 @@ ips_next(ips_ha_t * ha, int intr)
SC->result = DID_OK;
SC->host_scribble = NULL;
 
-   memset(SC->sense_buffer, 0, sizeof (SC->sense_buffer));
-
scb->target_id = SC->device->id;
scb->lun = SC->device->lun;
scb->bus = SC->device->channel;
@@ -3821,7 +3819,6 @@ ips_send_cmd(ips_ha_t * ha, ips_scb_t * scb)
/* attempted, a Check Condition occurred, and Sense   */
/* Data indicating an Invalid CDB OpCode is returned. */
sp = (char *) scb->scsi_cmd->sense_buffer;
-   memset(sp, 0, sizeof (scb->scsi_cmd->sense_buffer));
 
sp[0] = 0x70;   /* Error Code   */
sp[2] = ILLEGAL_REQUEST;/* Sense Key 5 Illegal 
Req. */
diff --git a/drivers/scsi/libsas/sas_scsi_host.c 
b/drivers/scsi/libsas/sas_scsi_host.c
index 828fed1..9c04225 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -148,7 +148,6 @@ static struct sas_task *sas_create_task(struct scsi_cmnd 
*cmd,
if (!task)
return NULL;
 
-   *(u32 *)cmd->sense_buffer = 0;
task->uldd_task = cmd;
ASSIGN_SAS_TASK(cmd, task);
 
-- 
1.5.3.4

-
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] libsas: fix sense_buffer overrun

[FUJITA Tomonori]

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/libsas/sas_scsi_host.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/libsas/sas_scsi_host.c 
b/drivers/scsi/libsas/sas_scsi_host.c
index b784089..828fed1 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -108,7 +108,7 @@ static void sas_scsi_task_done(struct sas_task *task)
break;
case SAM_CHECK_COND:
memcpy(sc->sense_buffer, ts->buf,
-  max(SCSI_SENSE_BUFFERSIZE, ts->buf_valid_size));
+  min(SCSI_SENSE_BUFFERSIZE, ts->buf_valid_size));
stat = SAM_CHECK_COND;
break;
default:
-- 
1.5.3.4

-
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 : Add support for io vectors in bsg

[FUJITA Tomonori]

On Thu, 10 Jan 2008 16:46:05 -0500
Pete Wyckoff <[EMAIL PROTECTED]> wrote:

> Is there another async I/O mechanism?  Userspace builds the CDBs,
> just needs some way to drop them in SCSI ML.  BSG is almost perfect
> for this, but doesn't do iovec, leading to lots of memcpy.

syslets?
-
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: Driver 'sd' needs updating

[FUJITA Tomonori]

CC'ed linux-scsi and James,

On Thu, 10 Jan 2008 08:51:50 +
Nick Warne <[EMAIL PROTECTED]> wrote:

> 
> Hi everybody - Happy New Year to you all!
> 
> OK, updated to git rc7 yesterday - I now see this in syslog:
> 
>"Driver 'sd' needs updating - please use bus_type methods"
> 
> The warning never appeared in RC6, and all google reveals are other
> peoples logs that are posted about other issues.
> 
> Do I need to fix up something here?

No, you don't. It's harmless, a side effect of:

commit 751bf4d7865e4ced406be93b04c7436d866d3684
Author: James Bottomley <[EMAIL PROTECTED]>
Date:   Wed Jan 2 11:14:30 2008 -0600

[SCSI] scsi_sysfs: restore prep_fn when ULD is removed


It would be better to silence this warning.

James, we need to reset prep_fn in each ULD? though it's not nice...
-
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 : Add support for io vectors in bsg

[FUJITA Tomonori]

On Tue, 8 Jan 2008 17:09:18 -0500
Pete Wyckoff <[EMAIL PROTECTED]> wrote:

> [EMAIL PROTECTED] wrote on Sat, 05 Jan 2008 14:01 +0900:
> > From: Deepak Colluru <[EMAIL PROTECTED]>
> > Subject: [PATCH] bsg : Add support for io vectors in bsg
> > Date: Fri, 4 Jan 2008 21:47:34 +0530 (IST)
> > 
> > > From: Deepak Colluru <[EMAIL PROTECTED]>
> > > 
> > > Add support for io vectors in bsg.
> > > 
> > > Signed-off-by: Deepak Colluru <[EMAIL PROTECTED]>
> > > ---
> > >   bsg.c |   52 +---
> > >   1 file changed, 49 insertions(+), 3 deletions(-)
> > 
> > Thanks, but I have to NACK this.
> > 
> > You can find the discussion about bsg io vector support and a similar
> > patch in linux-scsi archive. I have no plan to support it since it
> > needs the compat hack.
> 
> You may recall this is one of the patches I need to use bsg with OSD
> devices.  OSDs overload the SCSI buffer model to put mulitple fields
> in dataout and datain.  Some is user data, but some is more
> logically created by a library.  Memcpying in userspace to wedge all
> the segments into a single buffer is painful, and is required both
> on outgoing and incoming data buffers.
> 
> There are two approaches to add iovec to bsg.
> 
> 1.  Define a new sg_iovec_v4 that uses constant width types.  Both
> 32- and 64-bit userspace would hand arrays of this to the kernel.
> 
> struct sg_v4_iovec {
>   __u64 iov_base;
>   __u32 iov_len;
>   __u32 __pad1;
> };
> 
> Old patch here:  http://article.gmane.org/gmane.linux.scsi/30461/

As I said before, I don't think that inventing a new "iovec" is a good
idea. sgv3 use the common "iovec". In addition, sg_io_v4 can be used
by other OSes like sg_io_v3.


> 2.  Do as Deepak has done, using the existing sg_iovec, but then
> also work around the compat issue.  Old v3 sg_iovec is:
> 
> typedef struct sg_iovec /* same structure as used by readv() Linux system 
> */
> {   /* call. It defines one scatter-gather element. */
>   void __user *iov_base;  /* Starting address  */
>   size_t iov_len; /* Length in bytes  */
> } sg_iovec_t;
> 
> Old patch here:  http://article.gmane.org/gmane.linux.scsi/30460/
> 
> I took another look at the compat approach, to see if it is feasible
> to keep the compat handling somewhere else, without the use of #ifdef
> CONFIG_COMPAT and size-comparison code inside bsg.c.  I don't see how.
> The use of iovec is within a write operation on a char device.  It's
> not amenable to a compat_sys_ or a .compat_ioctl approach.
> 
> I'm partial to #1 because the use of architecture-independent fields
> matches the rest of struct sg_io_v4.  But if you don't want to have
> another iovec type in the kernel, could we do #2 but just return
> -EINVAL if the need for compat is detected?  I.e. change
> dout_iovec_count to dout_iovec_length and do the math?

If you are ok with removing the write/read interface and just have
ioctl, we could can handle comapt stuff like others do. But I think
that you (OSD people) really want to keep the write/read
interface. Sorry, I think that there is no workaround to support iovec
in bsg.
-
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: [GIT PATCH] SCSI bug fixes for 2.6.24-rc3

[FUJITA Tomonori]

CC'ed Jes,

On Tue, 8 Jan 2008 14:15:53 +0300
Evgeniy Dushistov <[EMAIL PROTECTED]> wrote:

> On Sun, Nov 25, 2007 at 01:24:25PM +0200, James Bottomley wrote:
> > This is a bit of a rash of bug fixes.  The qla1280 is actually a bug fix
> > (in spite of the title---it's actually correcting an existing problem
> > with the qla1280 implementation of accessors that broke the current
> > driver).
> > 
> 
> Recently I build last Linus's git tree, and got:
> req_cnt is used uninitialized in this function,
> and see bellow
> > The patch is available here:
> > 
> > master.kernel.org:/pub/scm/linux/kernel/git/jejb/scsi-rc-fixes-2.6.git
> > 
> > The short changelog is:
> ...
> > Jes Sorensen (1):
> >   qla1280: convert to use the data buffer accessors
> > 
> ...
> >  scsi/qla1280.c |  387 
> > +
> ...
> > /* Calculate number of entries and segments required. */
> > -   req_cnt = 1;
> > 
> 
> Initilization of req_cnt was removed, but in this function
> there are places like 
> req_cnt += (seg_cnt - 4) / 7;
> or
> req_cnt++;
> This is should be so?

req_cnt should not be removed. Jes tested the patch but this critical
bug appears only with BITS_PER_LONG != 64 && CONFIG_HIGHMEM=n case. So
he didn't see this, I think.

qla1280_32bit_start_scsi also gives the following warning:

drivers/scsi/qla1280.c: In function 'qla1280_32bit_start_scsi':
drivers/scsi/qla1280.c:3044: warning: unused variable 'dma_handle'


diff --git a/drivers/scsi/qla1280.c b/drivers/scsi/qla1280.c
index 146d540..2886407 100644
--- a/drivers/scsi/qla1280.c
+++ b/drivers/scsi/qla1280.c
@@ -3041,7 +3041,6 @@ qla1280_32bit_start_scsi(struct scsi_qla_host *ha, struct 
srb * sp)
int cnt;
int req_cnt;
int seg_cnt;
-   dma_addr_t dma_handle;
u8 dir;
 
ENTER("qla1280_32bit_start_scsi");
@@ -3050,6 +3049,7 @@ qla1280_32bit_start_scsi(struct scsi_qla_host *ha, struct 
srb * sp)
cmd->cmnd[0]);
 
/* Calculate number of entries and segments required. */
+   req_cnt = 1;
seg_cnt = scsi_dma_map(cmd);
if (seg_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: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd

[FUJITA Tomonori]

On Mon, 07 Jan 2008 15:25:36 +0200
Boaz Harrosh <[EMAIL PROTECTED]> wrote:

> On Mon, Jan 07 2008 at 8:53 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> > On Sun, 23 Dec 2007 13:09:05 +0200
> > Boaz Harrosh <[EMAIL PROTECTED]> wrote:
> > 
> >> On Fri, Dec 21 2007 at 4:30 +0200, Benjamin Herrenschmidt <[EMAIL 
> >> PROTECTED]> wrote:
> >>> The sense buffer ins scsi_cmnd can nowadays be DMA'ed into directly
> >>> by some low level drivers (that typically happens with USB mass
> >>> storage).
> >>>
> >>> This is a problem on non cache coherent architectures such as
> >>> embedded PowerPCs where the sense buffer can share cache lines with
> >>> other structure members, which leads to various forms of corruption.
> >>>
> >>> This uses the newly defined __dma_buffer annotation to enforce that
> >>> on such platforms, the sense_buffer is contained within its own
> >>> cache line. This has no effect on cache coherent architectures.
> >>>
> >>> Signed-off-by: Benjamin Herrenschmidt <[EMAIL PROTECTED]>
> >>> ---
> >>>
> >>>  include/scsi/scsi_cmnd.h |2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> --- linux-merge.orig/include/scsi/scsi_cmnd.h 2007-12-21 
> >>> 13:07:14.0 +1100
> >>> +++ linux-merge/include/scsi/scsi_cmnd.h  2007-12-21 13:07:29.0 
> >>> +1100
> >>> @@ -88,7 +88,7 @@ struct scsi_cmnd {
> >>>  working on */
> >>>  
> >>>  #define SCSI_SENSE_BUFFERSIZE96
> >>> - unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];
> >>> + unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE] __dma_buffer;
> >>>   /* obtained by REQUEST SENSE when
> >>>* CHECK CONDITION is received on original
> >>>* command (auto-sense) */
> >> This has the potential of leaving a big fat ugly hole in the middle of 
> >> scsi_cmnd. I would suggest of *just* moving the sense_buffer array to be
> >> the *first member* of struct scsi_cmnd. The command itself is already cache
> >> aligned, allocated by the proper flags to it's slab. And put a fat comment
> >> near it's definition.
> >>
> >> This is until a proper fix is sent. I have in my Q a proposition for a 
> >> more prominent solution, which I will send next month. Do to short comings
> >> in the sense handling and optimizations, but should definitely cover this
> >> problem.
> >>
> >> The code should have time to be discussed and tested, so it is only 2.6.26
> >> material. For the duration of the 2.6.25 kernel we can live with a reorder
> >> of scsi_cmnd members, if it solves such a grave bug for some ARCHs.
> >>
> >> Boaz
> >> 
> >> [RFD below]
> >> My proposed solution will be has follows:
> >>
> >>  1. Since the scsi protocol mandates an immediate REQUEST_SENSE after an 
> >> error
> >> in effect the Q is frozen until the REQUEST_SENSE command returns.
> >>
> >>  2. The scsi-ml needs the sense buffer for its normal operation, 
> >> independent 
> >> from the ULD's request of the sence_buffer or not at request->sense. 
> >> But
> >> in effect, 90% of all scsi-requests come with ULD's allocated buffer 
> >> for
> >> sense, that is copied to, on command completion.
> >>
> >>  3. 99% of all commands complete successfully, so if an optimization is 
> >> proposed for the successful case, sacrificing a few cycles for the 
> >> error
> >> case than thats a good thing.
> >>
> >>  My suggestion is to have a per Q, driver-overridable, sense buffer that 
> >> is 
> >>  DMAed/written to by drivers. At the end of the REQUEST_SENSE command one
> >>  of 2 things will be done. Either copy the sense to the ULD's supplied 
> >> buffer,
> >>  or if not available, allocate one from a dedicated mem_cache pool.
> >>  
> >>  So we are completely saving 92 bytes from scsi_cmnd by replacing the 
> >> buffer
> >>  with a pointer. We can always read the sense into a per Q buffer. And 10% 
> >> of
> >>  %1 of the times we will need to allocate a sense buffer from a dedicated 
> >> mem_cache
> >>  I

[PATCH] fix scsi_setup_command_freelist failure path race

[FUJITA Tomonori]

Looks like that host_cmd_pool_mutex are necessary here.

Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
 drivers/scsi/scsi.c |5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ebc0193..54ff611 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -319,17 +319,16 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
GFP_KERNEL | shost->cmd_pool->gfp_mask);
if (!cmd)
goto fail2;
-   list_add(&cmd->list, &shost->free_list);
+   list_add(&cmd->list, &shost->free_list);
return 0;
 
  fail2:
+   mutex_lock(&host_cmd_pool_mutex);
if (!--pool->users)
kmem_cache_destroy(pool->slab);
-   return -ENOMEM;
  fail:
mutex_unlock(&host_cmd_pool_mutex);
return -ENOMEM;
-
 }
 
 /**
-- 
1.5.3.4

-
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 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd

[FUJITA Tomonori]

On Sun, 23 Dec 2007 13:09:05 +0200
Boaz Harrosh <[EMAIL PROTECTED]> wrote:

> On Fri, Dec 21 2007 at 4:30 +0200, Benjamin Herrenschmidt <[EMAIL PROTECTED]> 
> wrote:
> > The sense buffer ins scsi_cmnd can nowadays be DMA'ed into directly
> > by some low level drivers (that typically happens with USB mass
> > storage).
> > 
> > This is a problem on non cache coherent architectures such as
> > embedded PowerPCs where the sense buffer can share cache lines with
> > other structure members, which leads to various forms of corruption.
> > 
> > This uses the newly defined __dma_buffer annotation to enforce that
> > on such platforms, the sense_buffer is contained within its own
> > cache line. This has no effect on cache coherent architectures.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <[EMAIL PROTECTED]>
> > ---
> > 
> >  include/scsi/scsi_cmnd.h |2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- linux-merge.orig/include/scsi/scsi_cmnd.h   2007-12-21 
> > 13:07:14.0 +1100
> > +++ linux-merge/include/scsi/scsi_cmnd.h2007-12-21 13:07:29.0 
> > +1100
> > @@ -88,7 +88,7 @@ struct scsi_cmnd {
> >working on */
> >  
> >  #define SCSI_SENSE_BUFFERSIZE  96
> > -   unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];
> > +   unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE] __dma_buffer;
> > /* obtained by REQUEST SENSE when
> >  * CHECK CONDITION is received on original
> >  * command (auto-sense) */
> 
> This has the potential of leaving a big fat ugly hole in the middle of 
> scsi_cmnd. I would suggest of *just* moving the sense_buffer array to be
> the *first member* of struct scsi_cmnd. The command itself is already cache
> aligned, allocated by the proper flags to it's slab. And put a fat comment
> near it's definition.
> 
> This is until a proper fix is sent. I have in my Q a proposition for a 
> more prominent solution, which I will send next month. Do to short comings
> in the sense handling and optimizations, but should definitely cover this
> problem.
> 
> The code should have time to be discussed and tested, so it is only 2.6.26
> material. For the duration of the 2.6.25 kernel we can live with a reorder
> of scsi_cmnd members, if it solves such a grave bug for some ARCHs.
> 
> Boaz
> 
> [RFD below]
> My proposed solution will be has follows:
> 
>  1. Since the scsi protocol mandates an immediate REQUEST_SENSE after an error
> in effect the Q is frozen until the REQUEST_SENSE command returns.
> 
>  2. The scsi-ml needs the sense buffer for its normal operation, independent 
> from the ULD's request of the sence_buffer or not at request->sense. But
> in effect, 90% of all scsi-requests come with ULD's allocated buffer for
> sense, that is copied to, on command completion.
> 
>  3. 99% of all commands complete successfully, so if an optimization is 
> proposed for the successful case, sacrificing a few cycles for the error
> case than thats a good thing.
> 
>  My suggestion is to have a per Q, driver-overridable, sense buffer that is 
>  DMAed/written to by drivers. At the end of the REQUEST_SENSE command one
>  of 2 things will be done. Either copy the sense to the ULD's supplied buffer,
>  or if not available, allocate one from a dedicated mem_cache pool.
>  
>  So we are completely saving 92 bytes from scsi_cmnd by replacing the buffer
>  with a pointer. We can always read the sense into a per Q buffer. And 10% of
>  %1 of the times we will need to allocate a sense buffer from a dedicated 
> mem_cache
>  I would say thats a nice optimization.
> 
>  The changes to scsi_error/scsi_cmnd and friends, is pretty strait forward. 
> But
>  it depends on a conversion of 4/5 drivers to the new scsi_eh API for 
>  REQUEST_SENSE. I have only converted these drivers that interfered with the 
> accessors
>  effort + 1 other places. But there are a few more places that are not 
> converted.
>  Once done. The implementation can easily change with no affect on drivers.

I think that removing the sense_buffer array from scsi_cmnd effects
lots of LLDs. As I wrote in other mail, many LLDs assume that
scsi_cmnd:sense_buffer is always available. Another big task is to
take care about auto sense.

Have you already had some patches? I've just started to work on this
and I'd like to push that fix into 2.6.25.
-
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 2/2] use dynamically allocated sense buffer

[FUJITA Tomonori]

On Thu, 3 Jan 2008 13:56:37 +0900
FUJITA Tomonori <[EMAIL PROTECTED]> wrote:

> This removes static array sense_buffer in scsi_cmnd and uses
> dynamically allocated sense_buffer (with GFP_DMA).
> 
> scsi_add_host allocates as many buffers as
> scsi_host->can_queue. __scsi_get_command attaches sense_buffer to a
> scsi_cmnd and __scsi_put_command detaches the sense_buffer from it.
> 
> There is a small possibility that a host need more sense buffers than
> can_queue. The failure of the buffer allocation works just like the
> failure of scsi_cmnd allocation. So everything should work as before.
> 
> Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
> ---
>  drivers/scsi/hosts.c |   10 ++-
>  drivers/scsi/scsi.c  |   67 
> +-
>  drivers/scsi/scsi_priv.h |2 +
>  include/scsi/scsi_cmnd.h |2 +-
>  include/scsi/scsi_host.h |3 ++
>  5 files changed, 81 insertions(+), 3 deletions(-)

On IRC, James told me that allocating the sense buffer in the critical
path could affet the performance. So I did some tests.

I use scsi_debug with fake_rw=1. So I didn't perform any real I/Os.


- Sequential reads

scsi-misc : 479.56 MB/s
scsi-misc + the patch : 453.523331 MB/s

- Sequential writes

scsi-misc : 441.643337 MB/s
scsi-misc + the patch : 420.303334 MB/s


I also tried an approach to use pre-allocated buffers instead of
mempool, but I got only around 460 MB/s with sequential reads.

Seems that short-term approaches don't work well though we might not
see the notable effects with real I/Os. I guess that it would be
better to work on a long-term approach now than adding a workaround.
-
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: Open-FCoE on linux-scsi

[FUJITA Tomonori]

On Fri, 4 Jan 2008 14:07:28 -0800
"Dev, Vasu" <[EMAIL PROTECTED]> wrote:

> 
> >>
> >> _If_ there will indeed be dedicated FCoE HBAs in the future, the
> >> following stack could exist in addition to the one above:
> >>
> >>   - SCSI core,
> >> scsi_transport_fc
> >>   - FCoE HBA driver(s)
> >
> >Agreed. My FCoE initiator design would be something like:
> >
> >scsi-ml
> >fcoe initiator driver
> >libfcoe
> >fc_transport_class (inclusing fcoe support)
> >
> >And FCoE HBA LLDs work like:
> >
> >scsi-ml
> >FCoE HBA LLDs (some of them might use libfcoe)
> >fc_transport_class (inclusing fcoe support)
> >
> >
> >That's the way that other transport classes do, I think. For me, the
> >current code tries to invent another fc class. For example, the code
> >newly defines:
> >
> >struct fc_remote_port {
> > struct list_head rp_list;   /* list under fc_virt_fab */
> > struct fc_virt_fab *rp_vf;  /* virtual fabric */
> > fc_wwn_trp_port_wwn;/* remote port world wide name
> */
> > fc_wwn_trp_node_wwn;/* remote node world wide name
> */
> > fc_fid_trp_fid; /* F_ID for remote_port if known
> */
> > atomic_trp_refcnt;  /* reference count */
> > u_int   rp_disc_ver;/* discovery instance */
> > u_int   rp_io_limit;/* limit on outstanding I/Os */
> > u_int   rp_io_count;/* count of outstanding I/Os */
> > u_int   rp_fcp_parm;/* remote FCP service parameters
> */
> > u_int   rp_local_fcp_parm; /* local FCP service
> parameters */
> > void*rp_client_priv; /* HBA driver private data */
> > void*rp_fcs_priv;   /* FCS driver private data */
> > struct sa_event_list *rp_events; /* event list */
> > struct sa_hash_link rp_fid_hash_link;
> > struct sa_hash_link rp_wwpn_hash_link;
> >
> > /*
> >  * For now, there's just one session per remote port.
> >  * Eventually, for multipathing, there will be more.
> >  */
> > u_char  rp_sess_ready;  /* session ready to be used */
> > struct fc_sess  *rp_sess;   /* session */
> > void*dns_lookup;/* private dns lookup */
> > int dns_lookup_count; /* number of attempted lookups
> */
> >};
> >
> >/*
> > * remote ports are created and looked up by WWPN.
> > */
> >struct fc_remote_port *fc_remote_port_create(struct fc_virt_fab *,
> fc_wwn_t);
> >struct fc_remote_port *fc_remote_port_lookup(struct fc_virt_fab *,
> >  fc_fid_t, fc_wwn_t wwpn);
> >struct fc_remote_port *fc_remote_port_lookup_create(struct fc_virt_fab
> *,
> > fc_fid_t,
> > fc_wwn_t wwpn,
> > fc_wwn_t wwnn);
> >
> >
> >The FCoE LLD needs to exploit the exsting struct fc_rport and APIs.
> 
> The openfc is software implementation of FC services such as FC login
> and target discovery and it is already using/exploiting  existing fc
> transport class including fc_rport struct. You can see openfc using
> fc_rport in openfc_queuecommand() and using fc transport API
> fc_port_remote_add() for fc_rport.

You just calls fc_remote_port_add. I don't think that reinventing the
whole rport management like reference counting doesn't mean exploiting
the exsting struct fc_rport and APIs.


> The fcoe module is just a first example of possible openfc transport but
> openfc can be used with other transports or HW HBAs also.
> 
> The openfc does provide generic transport interface using fcdev which is
> currently used by FCoE module.
> 
> One can certainly implement partly or fully  openfc and fcoe modules in
> FCoE HBA.

As pointed out in other mails, I believe that the similar job has done
in other transport classes using scsi transport class infrastructure
and the FCoE needs to follow the existing examples.
-
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