Re: [PATCH] scatterlist: enable sg chaining for all architectures
On 04/29/2015 05:15 AM, James Bottomley wrote: > > Perhaps the best thing to do is just fix target and call it quits? > Right! drivers write code for sg_chaining and on ARCHs that do not support it the code just works. Only the max_sg is smaller and the chaining code never kicks in and is dead code for these ARCHs. > James Cheers Boaz -- 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 v6 9/9] snic:Add Makefile, patch Kconfig, MAINTAINERS
On 05/27/2015 10:19 AM, Narsimhulu Musini wrote: > Kconfig for kbuild > Makefile to build snic module > > Updated MAINTAINERS file > > Signed-off-by: Narsimhulu Musini > Signed-off-by: Sesidhar Baddela > --- > * v3 > - Added additional config section (CONFIG_SNIC_DEBUG_FS) for enabling > debugging > functionality. > > * v2 > - Added compile time flags for debugfs dependent functionality. > > MAINTAINERS| 7 +++ > drivers/scsi/Kconfig | 17 + > drivers/scsi/Makefile | 1 + > drivers/scsi/snic/Makefile | 21 + > 4 files changed, 46 insertions(+) > create mode 100644 drivers/scsi/snic/Makefile > > diff --git a/MAINTAINERS b/MAINTAINERS > index 2a97e05..368fb76 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2536,6 +2536,13 @@ L: linux-scsi@vger.kernel.org > S: Supported > F: drivers/scsi/fnic/ > > +CISCO SCSI HBA DRIVER > +M: Narsimhulu Musini > +M: Sesidhar Baddela > +L: linux-scsi@vger.kernel.org > +S: Supported > +F: drivers/scsi/snic/ > + > CMPC ACPI DRIVER > M: Thadeu Lima de Souza Cascardo > M: Daniel Oliveira Nascimento > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig > index 9c92f41..8baab3f 100644 > --- a/drivers/scsi/Kconfig > +++ b/drivers/scsi/Kconfig > @@ -634,6 +634,23 @@ config FCOE_FNIC > . > The module will be called fnic. > > +config SCSI_SNIC > + tristate "Cisco SNIC Driver" > + depends on PCI && SCSI && X86_64 > + help > + This is support for the Cisco PCI-Express SCSI HBA. > + > + To compile this driver as a module, choose M here and read > + . > + The module will be called snic. > + > +config SCSI_SNIC_DEBUG_FS > + bool "Cisco SNIC Driver Debugfs Support" > + depends on SCSI_SNIC && DEBUG_FS > + help > + This enables to list debugging information from SNIC Driver > + available via debugfs file system > + > config SCSI_DMX3191D > tristate "DMX3191D SCSI support" > depends on PCI && SCSI > diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile > index 58158f1..f643942 100644 > --- a/drivers/scsi/Makefile > +++ b/drivers/scsi/Makefile > @@ -39,6 +39,7 @@ obj-$(CONFIG_LIBFC) += libfc/ > obj-$(CONFIG_LIBFCOE)+= fcoe/ > obj-$(CONFIG_FCOE) += fcoe/ > obj-$(CONFIG_FCOE_FNIC) += fnic/ > +obj-$(CONFIG_SCSI_SNIC) += snic/ > obj-$(CONFIG_SCSI_BNX2X_FCOE)+= libfc/ fcoe/ bnx2fc/ > obj-$(CONFIG_ISCSI_TCP) += libiscsi.o libiscsi_tcp.o iscsi_tcp.o > obj-$(CONFIG_INFINIBAND_ISER)+= libiscsi.o > diff --git a/drivers/scsi/snic/Makefile b/drivers/scsi/snic/Makefile > new file mode 100644 > index 000..572102a > --- /dev/null > +++ b/drivers/scsi/snic/Makefile > @@ -0,0 +1,21 @@ > +obj-$(CONFIG_SCSI_SNIC) += snic.o > + > +snic-y := \ > + snic_attrs.o \ > + snic_main.o \ > + snic_res.o \ > + snic_isr.o \ > + snic_ctl.o \ > + snic_io.o \ > + snic_scsi.o \ > + snic_disc.o \ > + vnic_cq.o \ > + vnic_intr.o \ > + vnic_dev.o \ > + vnic_wq.o > + > +ifeq ($(CONFIG_SCSI_SNIC_DEBUG_FS), y) > +ccflags-y += -DSNIC_DEBUG_FS Why do you need an extra define here just use CONFIG_SCSI_SNIC_DEBUG_FS in source code directly > +snic-y += snic_debugfs.o \ > + snic_trc.o > +endif > snic-$(CONFIG_SCSI_SNIC_DEBUG_FS) += snic_debugfs.o You do not the ifeq () thing at all Cheers Boaz -- 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 v6 9/9] snic:Add Makefile, patch Kconfig, MAINTAINERS
On 05/27/2015 01:02 PM, Narsimhulu Musini (nmusini) wrote: <> >>> +ifeq ($(CONFIG_SCSI_SNIC_DEBUG_FS), y) >>> +ccflags-y += -DSNIC_DEBUG_FS >> >> Why do you need an extra define here just use >> CONFIG_SCSI_SNIC_DEBUG_FS in source code directly > Agree, I just want to use a shorter macro in the source. Don't do this. It is a convention that if a programmer sees a CONFIG_XXX he knows that this is settable by a Kconfig. By making it shorter the programmer will think that this is dead code because it is not set anywhere. >> >>> +snic-y += snic_debugfs.o \ >>> + snic_trc.o >>> +endif >>> >> >> snic-$(CONFIG_SCSI_SNIC_DEBUG_FS) += snic_debugfs.o > If CONFIG_SCSI_SNIC_DEBUGFS is not set, then it leaves a build variable > ³snic-" in build system. ifeq() avoids such thing. That is fine. This is done all over the Kernel Makefile. There are two tons of these "do nothing make variables" It the way we do it in the Kernel. (I actually like it) >> >> You do not the ifeq () thing at all >> >> Cheers >> Boaz >> > Thanks > simha >> > -- 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-OSD: Delete an unnecessary check before the function call "put_disk"
On 06/24/2015 05:16 PM, SF Markus Elfring wrote:
> From: Markus Elfring
> Date: Wed, 24 Jun 2015 16:06:21 +0200
>
> The put_disk() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring
ACK-by: Boaz Harrosh
> ---
> drivers/scsi/osd/osd_uld.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> index 243eab3..e2075522 100644
> --- a/drivers/scsi/osd/osd_uld.c
> +++ b/drivers/scsi/osd/osd_uld.c
> @@ -407,9 +407,7 @@ static void __remove(struct device *dev)
>
> OSD_INFO("osd_remove %s\n",
>oud->disk ? oud->disk->disk_name : NULL);
> -
> - if (oud->disk)
> - put_disk(oud->disk);
> + put_disk(oud->disk);
> ida_remove(&osd_minor_ida, oud->minor);
>
> kfree(oud);
>
--
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: [Lsf-pc] [LSF/MM TOPIC] do we really need PG_error at all?
On 02/28/2017 03:11 AM, Jeff Layton wrote: <> > > I'll probably have questions about the read side as well, but for now it > looks like it's mostly used in an ad-hoc way to communicate errors > across subsystems (block to fs layer, for instance). If memory does not fail me it used to be checked long time ago in the read-ahead case. On the buffered read case, the first page is read synchronous and any error is returned to the caller, but then a read-ahead chunk is read async all the while the original thread returned to the application. So any errors are only recorded on the page-bit, since otherwise the uptodate is off and the IO will be retransmitted. Then the move to read_iter changed all that I think. But again this is like 5-6 years ago, and maybe I didn't even understand very well. > -- > Jeff Layton > I would like a Documentation of all this as well please. Where are the tests for this? Thanks Boaz
Re: [PATCH 2/2] block: support > 16 byte CDBs for SG_IO
On 07/20/2014 01:23 PM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig
Hi Christoph I've quickly reviewed your code and have a few questions
> ---
> block/scsi_ioctl.c | 24 +---
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index c4e6633..a804f3e 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -288,8 +288,6 @@ static int sg_io(struct request_queue *q, struct gendisk
> *bd_disk,
>
> if (hdr->interface_id != 'S')
> return -EINVAL;
> - if (hdr->cmd_len > BLK_MAX_CDB)
> - return -EINVAL;
>
> if (hdr->dxfer_len > (queue_max_hw_sectors(q) << 9))
> return -EIO;
> @@ -306,14 +304,21 @@ static int sg_io(struct request_queue *q, struct
> gendisk *bd_disk,
> break;
> }
>
> + ret = -ENOMEM;
> rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL);
> if (!rq)
> - return -ENOMEM;
> + goto out;
> blk_rq_set_block_pc(rq);
>
> + if (hdr->cmd_len > BLK_MAX_CDB) {
> + rq->cmd = kzalloc(hdr->cmd_len, GFP_KERNEL);
So two things here:
- hdr->cmd_len is char so can be MAX of 255. I understand that 4 bytes
alignment is a SCSI
thing so you found no point of checking any max size?
- Why the zero alloc, if you are going to paste over it the exact same length.
Now if like in scsi
you need 4 bytes alignment and zero padding yes, but here you do not do this
(and probably shouldn't).
Hence why zero-alloc?
- Looking at sg.h where hdr->cmd_len is defined it stills has a comment of <=
16 you might want to
remove the comment by now.
> + if (!rq->cmd)
> + goto out_put_request;
> + }
> +
> ret = -EFAULT;
> if (blk_fill_sghdr_rq(q, rq, hdr, mode))
Inside here: blk_fill_sghdr_rq() calls blk_verify_command() which does:
(Below cmd[] is the buffer copied from user)
/* Anybody who can open the device can do a read-safe command */
if (test_bit(cmd[0], filter->read_ok))
return 0;
/* Write-safe commands require a writable open */
if (test_bit(cmd[0], filter->write_ok) && has_write_perm)
return 0;
Now I am not sure what type "Commands" you guys intend for these large commands
but if they are say SCSI-VARLEN this test will not work. Also a user might fool
the system with pretending to be "read" a device modifying command.
I would pass len to this test function and only permit these above if command is
<= 16. Any "special" large command is root only.
Thanks
Boaz
> - goto out;
> + goto out_free_cdb;
>
> if (hdr->iovec_count) {
> size_t iov_data_len;
> @@ -323,7 +328,7 @@ static int sg_io(struct request_queue *q, struct gendisk
> *bd_disk,
> 0, NULL, &iov);
> if (ret < 0) {
> kfree(iov);
> - goto out;
> + goto out_free_cdb;
> }
>
> iov_data_len = ret;
> @@ -346,7 +351,7 @@ static int sg_io(struct request_queue *q, struct gendisk
> *bd_disk,
> GFP_KERNEL);
>
> if (ret)
> - goto out;
> + goto out_free_cdb;
>
> bio = rq->bio;
> memset(sense, 0, sizeof(sense));
> @@ -365,8 +370,13 @@ static int sg_io(struct request_queue *q, struct gendisk
> *bd_disk,
> hdr->duration = jiffies_to_msecs(jiffies - start_time);
>
> ret = blk_complete_sghdr_rq(rq, hdr, bio);
> -out:
> +
> +out_free_cdb:
> + if (rq->cmd != rq->__cmd)
> + kfree(rq->cmd);
> +out_put_request:
> blk_put_request(rq);
> +out:
> return ret;
> }
>
>
--
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] block: support > 16 byte CDBs for SG_IO
On 07/20/2014 04:27 PM, Christoph Hellwig wrote: > On Sun, Jul 20, 2014 at 02:47:49PM +0300, Boaz Harrosh wrote: >> >> So two things here: >> - hdr->cmd_len is char so can be MAX of 255. I understand that 4 bytes >> alignment is a SCSI >> thing so you found no point of checking any max size? > > I don't see any point to force the aligmnet - the devices need to reject > garbage commands, and if for some reason we'd see future commands > that are > 252 and < 255 we are prepared to handle them. > I agree >> - Why the zero alloc, if you are going to paste over it the exact same >> length. Now if like in scsi >> you need 4 bytes alignment and zero padding yes, but here you do not do >> this (and probably shouldn't). >> Hence why zero-alloc? > > No good reason except that's what sg and bsg do. > Ha sorry didn't look there. Looks redundant to me that's all <> >> Inside here: blk_fill_sghdr_rq() calls blk_verify_command() which does: >> (Below cmd[] is the buffer copied from user) >> >> /* Anybody who can open the device can do a read-safe command */ >> if (test_bit(cmd[0], filter->read_ok)) >> return 0; >> >> /* Write-safe commands require a writable open */ >> if (test_bit(cmd[0], filter->write_ok) && has_write_perm) >> return 0; >> >> Now I am not sure what type "Commands" you guys intend for these large >> commands >> but if they are say SCSI-VARLEN this test will not work. Also a user might >> fool >> the system with pretending to be "read" a device modifying command. >> >> I would pass len to this test function and only permit these above if >> command is >> <= 16. Any "special" large command is root only. > > Honestly that whole filter crap should never have been merged to start with, > you'll just need proper CAP_SYS_RAWIO for variable length commands. > > I agree. What I'm saying is - Are you sure that with current code as is we will not pass on large commands without the proper CAP_SYS_RAWIO. Should we not make sure and add: /* root can do any command. */ if (capable(CAP_SYS_RAWIO)) return 0; + + if (cmnd_len > BLK_MAX_CDB) + return -EPERM; Rrrr you are right. I finally get the filter code. Anything that is not "white-listed" is rejected. OK sorry for the noise. Reviewed-by: Boaz Harrosh Thanks Boaz -- 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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 06/25/2014 04:17 AM, Vladislav Bolkhovitin wrote: > Martin K. Petersen, on 06/23/2014 06:58 PM wrote: >>> "Mike" == Mike Christie writes: + unsigned int xfer_len = blk_rq_bytes(scmd->request); >> >> Mike> Can you do bidi and dif/dix? >> >> Nope. > > Correction: at the moment. > > There is a proposal of READ GATHERED command, which is bidirectional and > potentially > DIF/DIX. > That's all very good. But current infrastructure can not support BIDI+DIFF. Because you we'll need *two* diff vectors one for each side. Now in fact at the block layer this is actually supported. Since BIDI is two requests, and each can have its own DIFF bio. But on the scsi layer this is not implemented. So I'd say: *For now DIFF and BIDI are mutually exclusive.* (You'll need someone to love these new commands a lot in order to enable it) > Vlad > Cheers Boaz -- 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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
On 06/25/2014 01:32 PM, Sagi Grimberg wrote: > On 6/25/2014 11:48 AM, Sagi Grimberg wrote: > > >> >>> I made the patch below which should fix both bidi >>> support in iscsi and also WRITE_SAME (and similar commands) support. >> >> I'm a bit confused, for all commands (bidi or not) the iscsi header >> data_length >> should describe the scsi_data_buffer length, bidi information will lie >> in AHS header. >> (in case bidi will ever co-exist with PI, we might need another helper >> that will look >> in req->special + PI, something like scsi_bidi_transfer_length) >> >> So why not keep scsi_transfer_length to work on sdb length (take >> scsi_bufflen(scmnd) or >> scsi_out(scmnd)->length as MKP suggested) and that's it - don't touch >> libiscsi. >> >> Let me test that. >> > > So I tested a bidirectional command using: > $ sg_raw --infile=/root/filex --send=1024 --request=1024 > --outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02 00 > > And I see: > kernel: session1: iscsi_prep_scsi_cmd_pdu iscsi prep [bidirectional cid > 0 sc 880468ca1e00 cdb 0x53 itt 0x16 len 1024 bidi_len 1024 cmdsn 223 > win 64] > This is a very bad example and tested nothing, since len && bidi_len are the same. So even if you had a bug and took length from the wrong side it would come out the same. You must test with a bidi command that has two different lengths for each side If you have a tree that you want me to test I will be glad too. >From this thread I'm confused as to what patches you want me to test? please point me to a tree you need testing. You can bug me any time, any tree. I will be happy to test. Cheers Boaz > This confirms what I wrote above, so AFAICT no additional fix is > required for libiscsi wrt bidi commands support. > > Mike? > > Sagi. > -- > 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 > -- 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 v2 2/3] libiscsi, iser: Adjust data_length to include protection information
On 06/11/2014 12:09 PM, Sagi Grimberg wrote:
> In case protection information exists over the wire
> iscsi header data length is required to include it.
> Use protection information aware scsi helpers to set
> the correct transfer length.
>
> In order to avoid breakage, remove iser transfer length
> checks for each task as they are not always true and
> somewhat redundant anyway.
>
> Signed-off-by: Sagi Grimberg
> Reviewed-by: Mike Christie
> ---
> drivers/infiniband/ulp/iser/iser_initiator.c | 34 +++--
> drivers/scsi/libiscsi.c | 18 +++---
> 2 files changed, 19 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c
> b/drivers/infiniband/ulp/iser/iser_initiator.c
> index 2e2d903..8d44a40 100644
> --- a/drivers/infiniband/ulp/iser/iser_initiator.c
> +++ b/drivers/infiniband/ulp/iser/iser_initiator.c
> @@ -41,11 +41,11 @@
> #include "iscsi_iser.h"
>
> /* Register user buffer memory and initialize passive rdma
> - * dto descriptor. Total data size is stored in
> - * iser_task->data[ISER_DIR_IN].data_len
> + * dto descriptor. Data size is stored in
> + * task->data[ISER_DIR_IN].data_len, Protection size
> + * os stored in task->prot[ISER_DIR_IN].data_len
> */
> -static int iser_prepare_read_cmd(struct iscsi_task *task,
> - unsigned int edtl)
> +static int iser_prepare_read_cmd(struct iscsi_task *task)
>
> {
> struct iscsi_iser_task *iser_task = task->dd_data;
> @@ -73,14 +73,6 @@ static int iser_prepare_read_cmd(struct iscsi_task *task,
> return err;
> }
>
> - if (edtl > iser_task->data[ISER_DIR_IN].data_len) {
> - iser_err("Total data length: %ld, less than EDTL: "
> - "%d, in READ cmd BHS itt: %d, conn: 0x%p\n",
> - iser_task->data[ISER_DIR_IN].data_len, edtl,
> - task->itt, iser_task->ib_conn);
> - return -EINVAL;
> - }
> -
> err = device->iser_reg_rdma_mem(iser_task, ISER_DIR_IN);
> if (err) {
> iser_err("Failed to set up Data-IN RDMA\n");
> @@ -100,8 +92,9 @@ static int iser_prepare_read_cmd(struct iscsi_task *task,
> }
>
> /* Register user buffer memory and initialize passive rdma
> - * dto descriptor. Total data size is stored in
> - * task->data[ISER_DIR_OUT].data_len
> + * dto descriptor. Data size is stored in
> + * task->data[ISER_DIR_OUT].data_len, Protection size
> + * is stored at task->prot[ISER_DIR_OUT].data_len
> */
> static int
> iser_prepare_write_cmd(struct iscsi_task *task,
> @@ -135,14 +128,6 @@ iser_prepare_write_cmd(struct iscsi_task *task,
> return err;
> }
>
> - if (edtl > iser_task->data[ISER_DIR_OUT].data_len) {
> - iser_err("Total data length: %ld, less than EDTL: %d, "
> - "in WRITE cmd BHS itt: %d, conn: 0x%p\n",
> - iser_task->data[ISER_DIR_OUT].data_len,
> - edtl, task->itt, task->conn);
> - return -EINVAL;
> - }
> -
> err = device->iser_reg_rdma_mem(iser_task, ISER_DIR_OUT);
> if (err != 0) {
> iser_err("Failed to register write cmd RDMA mem\n");
> @@ -417,11 +402,12 @@ int iser_send_command(struct iscsi_conn *conn,
> if (scsi_prot_sg_count(sc)) {
> prot_buf->buf = scsi_prot_sglist(sc);
> prot_buf->size = scsi_prot_sg_count(sc);
> - prot_buf->data_len = sc->prot_sdb->length;
> + prot_buf->data_len = data_buf->data_len >>
> + ilog2(sc->device->sector_size) * 8;
> }
>
> if (hdr->flags & ISCSI_FLAG_CMD_READ) {
> - err = iser_prepare_read_cmd(task, edtl);
> + err = iser_prepare_read_cmd(task);
> if (err)
> goto send_command_error;
> }
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 26dc005..3f46234 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -338,7 +338,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task
> *task)
> struct iscsi_session *session = conn->session;
> struct scsi_cmnd *sc = task->sc;
> struct iscsi_scsi_req *hdr;
> - unsigned hdrlength, cmd_len;
> + unsigned hdrlength, cmd_len, transfer_length;
I hate that you introduced this new transfer_length variable. It does
not exist. In BIDI supporting driver there is out_len and in_len just
as original code.
> itt_t itt;
> int rc;
>
> @@ -391,11 +391,11 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task
> *task)
> if (scsi_get_prot_op(sc) != SCSI_PROT_NORMAL)
> task->protected = true;
>
> + transfer_length = scsi_transfer_length(sc);
> + hdr->data_length = cpu_to_be32(transfer_length);
> if (sc->sc_data_direction == DMA_TO_DEVICE) {
> - unsigned
[RFD] brd.ko Never supported partitions should we remove the dead code ?
Hi folks
I've been playing with yet unseen prd block device, and hit an issue with
partitioning
but since prd.c is a copy/paste of brd.c the same exact issue exists with brd.
It does not support partitions!
An attempt to fdisk say a couple of partitions, then mkfs && mount an individual
partition will result in the primary device being accessed and a corrupted disk
data.
If I enable max_part(=2) properly on modprobe, then fdisk a couple of
partitions all
goes well
But then if I pass the device to Kernel (Say via mount), after a call to
bdev = blkdev_get_by_path(dev_name, mode, fs_type);
I get the following results:
(size is extracted using: i_size_read(bdev->bd_inode);
part_size is extracted using: bdev->bd_part->nr_sects << 9;
)
dev_name == /dev/ram0
[Jul29 17:40] foo: [foo_mount:880] size=0x4000 bdev=88003dc24340
bd_inode=88003dc24430 bd_part=88003ca22848 part_size=0x4000
dev_name == /dev/ram0p1
[ +16.816065] foo: [foo_mount:880] size=0x4000 bdev=88003d2f6d80
bd_inode=88003d2f6e70 bd_part=88003ca22848 part_size=0x4000
dev_name == /dev/ram0p2
[Jul29 17:41] foo: [foo_mount:880] size=0x4000 bdev=88003dc24680
bd_inode=88003dc24770 bd_part=88003ca22848 part_size=0x4000
We can see that all 3 different bdev's point to the same bd_part, which is the
BUG. Funny is that bd_inode is different but contains
same wrong size, for exactly the same reason.
The fix for this is easy:
diff --git drivers/block/brd.c drivers/block/brd.c
index c7d138e..0d982d7 100644
--- drivers/block/brd.c
+++ drivers/block/brd.c
@@ -378,10 +378,12 @@ static int brd_direct_access(struct block_device *bdev,
sector_t sector,
if (!brd)
return -ENODEV;
+ sector += get_start_sect(bdev);
if (sector & (PAGE_SECTORS-1))
return -EINVAL;
+/* Check is wrong here we need to check against bdev->bd_part->nr_sects */
if (sector + PAGE_SECTORS > get_capacity(bdev->bd_disk))
return -ERANGE;
page = brd_insert_page(brd, sector);
if (!page)
return -ENOSPC;
@@ -532,11 +532,17 @@ static struct brd_device *brd_init_one(int i)
goto out;
}
+/* In the structure of this driver this will never happen and is wrong
+ * to do. We should just return NULL if not found.
+ * This is not the bug it is just DEAD CODE
+ *
brd = brd_alloc(i);
if (brd) {
add_disk(brd->brd_disk);
list_add_tail(&brd->brd_list, &brd_devices);
}
+*/
+ prd = NULL;
out:
return brd;
}
@@ -558,7 +564,8 @@ static struct kobject *brd_probe(dev_t dev, int *part, void
*data)
kobj = brd ? get_disk(brd->brd_disk) : NULL;
mutex_unlock(&brd_devices_mutex);
- *part = 0;
+// Fix the partition BUG *part comes in correctly must not need to touch it
+// *part = 0;
return kobj;
}
---
After this simple fix of commenting out *part = 0, running again I get
dev_name == /dev/ram0
[ +0.04] foo: [foo_mount:880] size=0x4000 bdev=88003d2f7740
inode=88003d2f7830 part=88001da8c048 part_size=0x4000
dev_name == /dev/ram0p1
[ +0.02] foo: [foo_mount:880] size=0x1e748200 bdev=88003dc25040
inode=88003dc25130 part=880036f6a000 part_size=0x1e748200
dev_name == /dev/ram0p2
[ +0.02] foo: [foo_mount:880] size=0x217b7e00 bdev=88003d2f7a80
inode=88003d2f7b70 part=880036f69000 part_size=0x217b7e00
But before we are running to fix this bug. Could we please do better and just
remove the support for partitions
all together.
Since it *never* worked anyway, so probably no one needs it! Surly no
one used it
Thanks
Boaz
--
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: [RFD] brd.ko Never supported partitions should we remove the dead code ?
On 07/29/2014 07:56 PM, Matthew Wilcox wrote: > On Tue, Jul 29, 2014 at 07:37:49PM +0300, Boaz Harrosh wrote: >> But before we are running to fix this bug. Could we please do better and >> just remove the support for partitions >> all together. >> Since it *never* worked anyway, so probably no one needs it! Surly no >> one used it > > I fixed this in patch 4/22 of the v8 series. The correct place to > handle partitioning is in the block layer, not the individual drivers > (nor the filesystems). > Sir Mathew hi > @@ -378,10 +378,12 @@ static int brd_direct_access(struct block_device *bdev, > sector_t sector, > > if (!brd) > return -ENODEV; > + sector += get_start_sect(bdev); > if (sector & (PAGE_SECTORS-1)) > return -EINVAL; > +/* Check is wrong here we need to check against bdev->bd_part->nr_sects */ > if (sector + PAGE_SECTORS > get_capacity(bdev->bd_disk)) > return -ERANGE; > page = brd_insert_page(brd, sector); > if (!page) > return -ENOSPC; you mean you fixed the brd_direct_access() hunk by pointing all users to a wrapper that does the proper offsetting, sure. But that is not the main bug I was talking about, the main BUG is that partitions are not supported at all because of the clubber of *part in brd_probe() > @@ -558,7 +564,8 @@ static struct kobject *brd_probe(dev_t dev, int *part, > void *data) > kobj = brd ? get_disk(brd->brd_disk) : NULL; > mutex_unlock(&brd_devices_mutex); > > - *part = 0; > +// Fix the partition BUG *part comes in correctly must not need to touch it > +// *part = 0; > return kobj; > } And my point is that: No one uses partitions with brd, why should we not remove it all together, why fix it and keep it? Cheers Boaz -- 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: [RFD] brd.ko Never supported partitions should we remove the dead code ?
On 07/29/2014 08:19 PM, Ross Zwisler wrote:
> On Tue, 2014-07-29 at 19:37 +0300, Boaz Harrosh wrote:
>> Hi folks
>>
>> I've been playing with yet unseen prd block device, and hit an issue with
>> partitioning
>> but since prd.c is a copy/paste of brd.c the same exact issue exists with
>> brd.
>>
>> It does not support partitions!
>>
>> An attempt to fdisk say a couple of partitions, then mkfs && mount an
>> individual
>> partition will result in the primary device being accessed and a corrupted
>> disk data.
>>
>> If I enable max_part(=2) properly on modprobe, then fdisk a couple of
>> partitions all
>> goes well
>>
>> But then if I pass the device to Kernel (Say via mount), after a call to
>> bdev = blkdev_get_by_path(dev_name, mode, fs_type);
>>
>> I get the following results:
>> (size is extracted using:i_size_read(bdev->bd_inode);
>> part_size is extracted using: bdev->bd_part->nr_sects << 9;
>> )
>>
>> dev_name == /dev/ram0
>> [Jul29 17:40] foo: [foo_mount:880] size=0x4000 bdev=88003dc24340
>> bd_inode=88003dc24430 bd_part=88003ca22848 part_size=0x4000
>>
>> dev_name == /dev/ram0p1
>> [ +16.816065] foo: [foo_mount:880] size=0x4000 bdev=88003d2f6d80
>> bd_inode=88003d2f6e70 bd_part=88003ca22848 part_size=0x4000
>>
>> dev_name == /dev/ram0p2
>> [Jul29 17:41] foo: [foo_mount:880] size=0x4000 bdev=88003dc24680
>> bd_inode=88003dc24770 bd_part=88003ca22848 part_size=0x4000
>>
>> We can see that all 3 different bdev's point to the same bd_part, which is
>> the BUG. Funny is that bd_inode is different but contains
>> same wrong size, for exactly the same reason.
>
> Yep, I've seen this as well. It turns out that partitions *do* work in brd if
> you don't specify the 'rd_nr' module parameter, but if you do specify the
> module parameter you get the partition overlapping behavior that you observed.
>
> AFAICT the difference between these two scenarios is that when you set rd_nr,
> the 'range' variable is set so something small, but when you don't set it
> 'range' is 1,048,576. That's done by this bit of code in brd_init:
>
> if (rd_nr) {
> nr = rd_nr;
> range = rd_nr << part_shift;
> } else {
> nr = CONFIG_BLK_DEV_RAM_COUNT;
> range = 1UL << MINORBITS;
> }
>
> The difference in behavior happens up the stack somewhere as a result of you
> passing 'range' into blk_register_region().
>
> Anyway, the quick and dirty workaround is to always have 'range' be something
> large. This is what I've done in the current master branch of the upstream
> PRD tree - see commit 0256739ccab03d1662221f595b03797370c2ac12.
>
> For what it's worth, I'm planning on updating prd (and probably brd) so that
> it dynamically allocates partition numbers. See commit
> 469071a37afc8a627b6b2ddf29db0a097d864845 for how this was done in the nvme
> driver.
>
Hi Ross
I've looked at 469071a37afc8a627b6b2ddf29db0a097d864845, but surly as you said
it is not the fix. I have posted the fix, in the code you snipped out.
So I get it that you guys are not convinced that we need to remove support for
Partitions for brd? For me a partition for /dev/ramX is totally bogus. What's
the point at all? Since we have the same effect if we just do rd_nr and
partition memory this way, and save 1M of memory. And it is not as if you can
loose power and want the partitioning persistent for the next boot. But sigh I
will not fight you about it, I made my point of this being pointless and that is
that.
I will post a proper patch for brd on top of Jens's tree unless you want it
rebased over some other tree, I guess it can just be queued for 3.17.
> - Ross
Thanks
Boaz
--
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
[PATCH 1/2] brd: Fix the partitions BUG
With current code after a call to:
bdev = blkdev_get_by_path(dev_name, mode, fs_type);
size = i_size_read(bdev->bd_inode);
part_size = bdev->bd_part->nr_sects << 9;
I get the following bad results:
dev_name == /dev/ram0
foo: [foo_mount:880] size=0x4000 bdev=88003dc24340 \
bd_inode=88003dc24430 bd_part=88003ca22848 part_size=0x4000
dev_name == /dev/ram0p1
foo: [foo_mount:880] size=0x4000 bdev=88003d2f6d80 \
bd_inode=88003d2f6e70 bd_part=88003ca22848 part_size=0x4000
dev_name == /dev/ram0p2
foo: [foo_mount:880] size=0x4000 bdev=88003dc24680 \
bd_inode=88003dc24770 bd_part=88003ca22848 part_size=0x4000
Note how all three bdev(s) point to the same bd_part.
This is do to a single bad clubber in brd_probe() which is
removed in this patch:
- *part = 0;
because of this all 3 bdev(s) above get to point to the same bd_part[0]
While at it fix/rename brd_init_one() since all devices are created on
load of driver, brd_probe() will never be called with a new un-created
device.
brd_init_one() is now renamed to brd_find() which is what it does.
TODO: There is one more partitions BUG regarding
brd_direct_access() which is fixed on the next patch.
Signed-off-by: Boaz Harrosh
---
drivers/block/brd.c | 19 ---
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index c7d138e..92334f6 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -523,22 +523,20 @@ static void brd_free(struct brd_device *brd)
kfree(brd);
}
-static struct brd_device *brd_init_one(int i)
+static struct brd_device *brd_find(int i)
{
struct brd_device *brd;
list_for_each_entry(brd, &brd_devices, brd_list) {
if (brd->brd_number == i)
- goto out;
+ return brd;
}
- brd = brd_alloc(i);
- if (brd) {
- add_disk(brd->brd_disk);
- list_add_tail(&brd->brd_list, &brd_devices);
- }
-out:
- return brd;
+ /* brd always allocates all its devices at load time, therefor
+* brd_probe will never be called with a new brd_number
+*/
+ printk(KERN_EROR "brd: brd_find unexpected device %d\n", i);
+ return NULL;
}
static void brd_del_one(struct brd_device *brd)
@@ -554,11 +552,10 @@ static struct kobject *brd_probe(dev_t dev, int *part,
void *data)
struct kobject *kobj;
mutex_lock(&brd_devices_mutex);
- brd = brd_init_one(MINOR(dev) >> part_shift);
+ brd = brd_find(MINOR(dev) >> part_shift);
kobj = brd ? get_disk(brd->brd_disk) : NULL;
mutex_unlock(&brd_devices_mutex);
- *part = 0;
return kobj;
}
--
1.9.3
--
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
[PATCH 2/2] brd: Fix brd_direct_access with partitions
When brd_direct_access() is called on a partition-bdev it would access the wrong sector. And caller would then corrupt the device's data. This is a preliminary fix, Matthew Wilcox has a patch in his DAX patchset which will define a global wrapper to bdev->bd_disk->fops->direct_access that will do the proper checks and translations before calling a driver global member. (The way it is done at the rest of the block stack) CC: Matthew Wilcox Signed-off-by: Boaz Harrosh --- drivers/block/brd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/block/brd.c b/drivers/block/brd.c index 92334f6..7506864 100644 --- a/drivers/block/brd.c +++ b/drivers/block/brd.c @@ -378,9 +378,10 @@ static int brd_direct_access(struct block_device *bdev, sector_t sector, if (!brd) return -ENODEV; + sector += get_start_sect(bdev); if (sector & (PAGE_SECTORS-1)) return -EINVAL; - if (sector + PAGE_SECTORS > get_capacity(bdev->bd_disk)) + if (unlikely(sector + PAGE_SECTORS > part_nr_sects_read(bdev->bd_part))) return -ERANGE; page = brd_insert_page(brd, sector); if (!page) -- 1.9.3 -- 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] brd: Fix brd_direct_access with partitions
On 07/30/2014 06:34 PM, Matthew Wilcox wrote: > On Wed, Jul 30, 2014 at 05:18:47PM +0300, Boaz Harrosh wrote: >> When brd_direct_access() is called on a partition-bdev >> it would access the wrong sector. And caller would then >> corrupt the device's data. >> >> This is a preliminary fix, Matthew Wilcox has a patch >> in his DAX patchset which will define a global wrapper >> to bdev->bd_disk->fops->direct_access that will do the >> proper checks and translations before calling a driver >> global member. (The way it is done at the rest of the >> block stack) > > Uh, no, let's just focus on getting the DAX code in instead of putting > in interim patches that will conflict. Patch 4/22 is uncontroversial, > fixes this problem, has no dependencies, and is key to the rest of the DAX > patchset. If this problem wants to be fixed, then put 4/22 in instead. > OK, I agree, could you please push 4/22 as reply to here for Jens to take for 3.17 ? (together with my 1/2) Thanks Boaz -- 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 1/2] brd: Fix the partitions BUG
On 07/30/2014 07:50 PM, Ross Zwisler wrote: <> >> + */ >> +printk(KERN_EROR "brd: brd_find unexpected device %d\n", i); > > s/KERN_EROR/KERN_ERR/ > Yes thanks, sigh, code should compile driver error. I used pr_err but last inspection I saw that printk is used everywhere and, crapped ... >> +return NULL; >> } >> >> static void brd_del_one(struct brd_device *brd) >> @@ -554,11 +552,10 @@ static struct kobject *brd_probe(dev_t dev, int *part, >> void *data) >> struct kobject *kobj; >> >> mutex_lock(&brd_devices_mutex); >> -brd = brd_init_one(MINOR(dev) >> part_shift); >> +brd = brd_find(MINOR(dev) >> part_shift); >> kobj = brd ? get_disk(brd->brd_disk) : NULL; >> mutex_unlock(&brd_devices_mutex); >> >> -*part = 0; >> return kobj; >> } > > It is possible to create new block devices with BRD at runtime: > > # mknod /dev/new_brd b 1 4 > # fdisk -l /dev/new_brd > > This causes a new BRD disk to be created, and hits your error case: > > Jul 30 10:40:57 alara kernel: brd: brd_find unexpected device 4 > Ha OK, So this is the mystery key. I never knew that trick, OK I guess I need to leave this in > I guess in general I'm not saying that BRD needs to have partitions - indeed > it may not give you much in the way of added functionality. As the code > currently stands partitions aren't surfaced anyway > (GENHD_FL_SUPPRESS_PARTITION_INFO is set). For PRD, however, I *do* want to > enable partitions correctly because eventually I'd like to enhance PRD so that > it *does* actually handle NVDIMMs correctly, and for that partitions do make > sense. So lets talk about that for a bit. Why would you want legacy partitions for NVDIMMs. For one fdisk will waste 1M of memory on this. And with NVDIMMs you actually need a different manager all together, more like lvm stuff. I have patches here that changes prd a lot. - Global memory parameters are gone each device remaps and operates on it's own memory range. - the prd_params are gone instead you have one memmap= parameter of the form memmap=nnn$ooo,[nnn$ooo]... where: nnn - is size in bytes of the form number[K/M/G] ooo - is offset in bytes of the form number[K/M/G] Very much the same as the memmap= Kernel parameters. So just copy/paste what you did there and it will work. Now each memory range has one prd_device created for it. Note that if specified ranges are just contiguous then it is like your prd_count thing where you took a contiguous range and sliced it up, only here they can be of different size. Off course it has an added fixture of dis-contiguous ranges like we have in our multy-nodes NUMA system in the lab that host DDR3 NvDIMMs in two banks. - A sysfs interface is added to add/remove memory range on the fly like osdblk - If no parameters are specified at all, the Kernel command-line is parsed and all memmap= sections are attempted and used if are not already claimed. An interface like mknod above is not supported since it is actually pointless. My current code still supports partitions but I still think it is silly. [ This is all speculative for DDR3-NVDIMMs, we have seen that the memory controller actually tags these DIMMs with type 12 but it looks like this is all vendor specific right now and I understand that DDR4 standardize all that. So I was hoping you guys are working on all that with the NvDIMM stuff. Now lets say that I have established auto-detection for each DIMM and have extracted its SN (Our DDR3 DIMMs each have an SN that can be displayed by the vendor supplied tool) An NvDIMM manager will need to establish an NvDIMM table and set order. The motivation of a partition table is that a disk moved to another machine and/or booted into another OS will have persistent and STD way to not clobber over established DATA. But here we have like a disk cluster/raid, an ordered set of drives. Unique to NvDIMMs is the interleaving. If pairs are then inserted in wrong order or are re-mixed. This should be detected and refused to be mounted (unless a re-initialize is forced) So data is not silently lost on operator errors. All this is then persisted on some DIMM, first or last. If code is able to re-arrange order, like when physical address have changed it will, but in the wrong interleaving case it will refuse to mount. ] > And if I have to implement and debug partitions for PRD, it's easy to > stick them in BRD in case anyone wants to use them. > I was finally playing with BRD, and it is missing your getgeo patch, so it is currently completely alien to fdisk and partitions. So why, why keep a fixture that never existed, I still hope to convince you that this is crap. Specially with brd that can have devices added on the fly like you showed above. Who needs them at all? Why waist 1M of memory on each device for no extra gain? Specially in light of my new prd that does away of any needs of
Re: [PATCH V5] Save command pool address of Scsi_Host
On 08/04/2014 02:30 PM, jgr...@suse.com wrote:
> From: Juergen Gross
>
> If a scsi host driver specifies .cmd_len in it's scsi_host_template, a
> driver's
> private command pool is needed. scsi_find_host_cmd_pool() will locate it, but
> scsi_alloc_host_cmd_pool() isn't saving the pool address in the host template.
>
> This will result in an access error when the host is removed.
>
> Avoid the problem by saving the address of a new allocated command pool where
> it is expected.
>
> Signed-off-by: Juergen Gross
> ---
> drivers/scsi/scsi.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 88d46fe..b0cef5b 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -380,6 +380,10 @@ scsi_alloc_host_cmd_pool(struct Scsi_Host *shost)
> pool->slab_flags |= SLAB_CACHE_DMA;
> pool->gfp_mask = __GFP_DMA;
> }
> +
> + if (hostt->cmd_size)
> + hostt->cmd_pool = pool;
> +
> return pool;
> }
>
> @@ -424,8 +428,10 @@ out:
> out_free_slab:
> kmem_cache_destroy(pool->cmd_slab);
> out_free_pool:
> - if (hostt->cmd_size)
> + if (hostt->cmd_size) {
> scsi_free_host_cmd_pool(pool);
I disagree I liked the V4 version better. It is much nicer on the review
that we NULL the same one we pass in with no need for context that's outside
of this scope
Just my $0.017
Boaz
> + hostt->cmd_pool = NULL;
> + }
> goto out;
> }
>
> @@ -447,8 +453,10 @@ static void scsi_put_host_cmd_pool(struct Scsi_Host
> *shost)
> if (!--pool->users) {
> kmem_cache_destroy(pool->cmd_slab);
> kmem_cache_destroy(pool->sense_slab);
> - if (hostt->cmd_size)
> + if (hostt->cmd_size) {
> scsi_free_host_cmd_pool(pool);
> + hostt->cmd_pool = NULL;
> + }
> }
> mutex_unlock(&host_cmd_pool_mutex);
> }
>
--
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 v2 2/3] libiscsi, iser: Adjust data_length to include protection information
On 08/06/2014 03:43 PM, Sagi Grimberg wrote: > Hi Boaz, > <> >> >> I hate that you introduced this new transfer_length variable. It does >> not exist. In BIDI supporting driver there is out_len and in_len just >> as original code. > > Effectively, out_len and in_len are the same except for the bidi case. > Moreover, the hdr->data_length is exactly the command scsi data buffer > length, the bidi length is taken from scsi_in when we build the AHS for > bidi (rlen_ahdr->read_length). > > So to me it is correct and makes sense. But I'm don't feel so strong > about it... > > Mike what's your take on this? > I have a patch to clean all that, will send tomorrow. What I mean is that this is on the out-path only the in path is different. See the code this variable is only used in the if (== DMA_TO_DEVICE) case and should be local to that scope. This is my clean up <> >> this particular driver puts them together in the same payload. There can be >> other DIFF supporting drivers that put the DIFF payload on another stream / >> another >> SG list, like the sata one, right? > > I think that DIF specification says that on the wire the data and > protection are interleaved i.e. Block1, DIF1, Block2, DIF2... > No it does not. This is a per transport, and actually per device host driver. Yes in iSCSI_tcp they are inline in HW cards they might come as two different SGs (Like the Linux model). Even with iscsi-offload they might want to be two SG-lists. > So I do think that the transfer length should always include > data_length + protection_length. > This is at the iscsi level. But the scsi_transfer_length() is on the scsi level which keeps them separate. So I think the proper API should be scsi_proto_length() And for LLDs that want them together they can do scsi_bufflen() + scsi_proto_length() and for other drivers they can do it separately. Don't infect iscsi level assumptions on the generic layer API. Again my patch fixes this. >> And this >> >> This print is correct as it covers all cases. If you want to print the >> adjusted >> length then OK, you'd need to store this I guess, but store it as a different >> variable like proto_length and print it as an additional variable. > > But it is the transfer length that is sent in iSCSI header. Why do you > want to print it as additional info? I want to see what was the length the app/FS sent, then as added info how much was added for DIFF, your way there is lost information. > for transactions that include DIF > the length is the data + protection. > > It is still one-to-one isn't it? > No! the original submitted length is lost from the print > Sagi. > Shalom Boaz -- 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 v2 2/3] libiscsi, iser: Adjust data_length to include protection information
On 08/13/2014 04:09 PM, Sagi Grimberg wrote: > On 8/6/2014 4:25 PM, Boaz Harrosh wrote: > > Hey Boaz, > > So in the current flow, I still don't think it is wrong/buggy, the > transfer byte length related to scsi buffer length (In iscsi for sure > but I think that for all transports it is the same). > > But, > Since you have such a strong objection on this I'm also OK with changing > stuff... We can pass a buffer to scsi_transfer_length (although it has > no meaning effectively) and we can keep in/out_len local variables and > print them along with total transfer. > > Do you want me to send out a patch here (since I have some hardware to > test it...) or are you still planning to send out something? > I'll do it. As you said there is no bugs, just ugly code. I will send a cleanup Thanks Boaz > Cheers, > Sagi. > -- 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 v4] block: handle pointer error from blk_get_request
On 23/05/13 23:09, Joe Lawrence wrote: > Hi Jens, > > Subject: [PATCH v4] block: handle pointer error from blk_get_request > > The blk_get_request function may fail in low-memory conditions or during > device removal (even if __GFP_WAIT is set). To distinguish between these > errors, modify the blk_get_request call stack to return the appropriate > error pointer. Verify that all callers check the return status and > consider IS_ERR instead of a simple NULL pointer check. > > Signed-off-by: Joe Lawrence > Cc: Jens Axboe > Cc: "James E.J. Bottomley" > Cc: Bart Van Assche > Cc: linux-scsi@vger.kernel.org ACK-by: Boaz Harrosh > --- <> > drivers/scsi/osd/osd_initiator.c| 4 ++-- <> > diff --git a/drivers/scsi/osd/osd_initiator.c > b/drivers/scsi/osd/osd_initiator.c > index d8293f2..b4cd56b 100644 > --- a/drivers/scsi/osd/osd_initiator.c > +++ b/drivers/scsi/osd/osd_initiator.c > @@ -1567,8 +1567,8 @@ static struct request *_make_request(struct > request_queue *q, bool has_write, > struct request *req; > > req = blk_get_request(q, has_write ? WRITE : READ, flags); > - if (unlikely(!req)) > - return ERR_PTR(-ENOMEM); > + if (unlikely(IS_ERR(req))) > + return req; > > return req; > } -- 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: Bypass block layer and Fill SCSI lower layer driver queue
On 09/18/2013 05:07 PM, Douglas Gilbert wrote: > On 13-09-18 03:58 AM, Jack Wang wrote: >> On 09/18/2013 08:41 AM, Alireza Haghdoost wrote: >>> Hi >>> >>> I am working on a high throughput and low latency application which >>> does not tolerate block layer overhead to send IO request directly to >>> fiber channel lower layer SCSI driver. I used to work with libaio but >>> currently I am looking for a way to by pass the block layer and send >>> SCSI commands from the application layer directly to the SCSI driver >>> using /dev/sgX device and ioctl() system call. >>> >>> I have noticed that sending IO request through sg device even with >>> nonblocking and direct IO flags is quite slow and does not fill up >>> lower layer SCSI driver TCQ queue. i.e IO depth or >>> /sys/block/sdX/in_flight is always ZERO. Therefore the application >>> throughput is even lower that sending IO request through block layer >>> with libaio and io_submit() system call. In both cases I used only one >>> IO context (or fd) and single threaded. >>> >> Hi Alireza, >> >> I think what you want is in_flight command scsi dispatch to low level >> device. >> I submit a simple patch to export device_busy >> >> http://www.spinics.net/lists/linux-scsi/msg68697.html >> >> I also notice fio sg engine will not fill queue properly, but haven't >> look into deeper. >> >> Cheers >> Jack >> >>> I have noticed that some well known benchmarking tools like fio does >>> not support IO depth for sg devices as well. Therefore, I was >>> wondering if it is feasible to bypass block layer and achieve higher >>> throughput and lower latency (for sending IO request only). >>> >>> >>> Any comment on my issue is highly appreciated. > > I'm not sure if this is relevant to your problem but by > default both the bsg and sg drivers "queue at head" > when they inject SCSI commands into the block layer. > > The bsg driver has a BSG_FLAG_Q_AT_TAIL flag to change > that queueing to what may be preferable for your purposes. > The sg driver could, but does not, support that flag. > Yes! The current best bet for keeping the queues full are with libaio and direct + asynchronous IO. It should not be significantly slower than bsg. (Believe me, with direct IO Block-Device cache is bypassed and the only difference is in who prepares the struct requests for submission) As Doug said sg can not do it. Also with bsg and above BSG_FLAG_Q_AT_TAIL You will need to use the write() option and not the ioctl() because the later is synchronous and you want an asynchronous submit of commands and background completion of them. (Which is what libaio does with async IO) With bsg you achieve that with using write() in combination of read() to receive the completions. > Doug Gilbert > Cheers Boaz -- 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 0/17] Clear up bidi command confusion
On 01/23/2015 03:12 PM, Christoph Hellwig wrote: > On Fri, Jan 23, 2015 at 01:05:30PM +0100, Bart Van Assche wrote: >> There is some confusion in the SCSI core and in SCSI LLDs around the >> meaning of sc_data_direction and whether or not this member can have the >> value DMA_BIDIRECTIONAL. Clear up this confusion. The patches in this >> series are: > > I wonder if we should change the code to set DMA_BIDIRECTIONAL for > bidi commands. That seems a lot more logical than the current > version. > You cannot do this. Because a Bidi Command is actually two commands one for the WRITE side (DMA_TO_DEVICE) which is this one, and another command for the READ side (DMA_FROM_DEVICE). The DMA_XXX_ enum must not to be confused with the t10-scsi Bidi commands. In DMA world the enum means: What are the allowed access on the memory buffer, is Device allowed to read-from-memory-only or write-to-memory-only or both simultaneously "on the same buffer". It is a flag to the IO-mmu on the direction of its mappings. A t10-scsi bidi command is "two buffers". The command caries two distinct buffers one is only-written-to 2nd only-read-from. But these are two distinct buffers each his proper direction. If you ask me you need to remove sc_data_direction all together and just go directly to the READ/WRITE flag at request level. SCSI has no business duplicating the same information in another member another way. In any way t10-scsi has no use in the entire of its STD of the DMA_BIDIRECTIONAL sense. ie. same buffer, two directional access. So DMA_BIDIRECTIONAL is just dead code for sc_data_direction. It could be imagined but t10-scsi does not have a use for it. Again Not to be confused with one-command two buffers, which is exactly the opposite. > Also I don't think all the debug checks for bidi commands that you > change should stay at all - driver need to set the QUEUE_FLAG_BIDI to > ever see a bidi command. > Exactly just remove the all checks. > It would also nice to add a host template flag for bidi support instead > of having to poke into the block layer request_queue while we're at it. Cheers Boaz -- 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 0/17] Clear up bidi command confusion
On 01/26/2015 11:58 AM, Bart Van Assche wrote: > Hello Christoph, > > This makes sense to me. I will rework this patch series as you proposed. > Do you have a bidi setup to test against? Sending xor command to scsi_dbg is only half the test for me because, yes there are two buffers, but they are of same size so bugs might be masked. (From experience) Thanks Boaz > Bart. -- 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 0/17] Clear up bidi command confusion
On 01/29/2015 03:20 PM, Bart Van Assche wrote: > On 01/29/15 14:07, Boaz Harrosh wrote: >> On 01/26/2015 11:58 AM, Bart Van Assche wrote: >> >>> Hello Christoph, >>> >>> This makes sense to me. I will rework this patch series as you proposed. >> >> Do you have a bidi setup to test against? >> >> Sending xor command to scsi_dbg is only half the test for me because, yes >> there are two buffers, but they are of same size so bugs might be masked. >> (From experience) > > Hello Boaz, > > If anyone would like to submit a scsi_debug patch that adds support to > that kernel driver for a bidi command for which the input and output > buffers have different sizes I think that would be helpful. > Hi Bart. scsi_dbg is a scsi-blk-device type device (forgot the exact name). I do not think there is such a command defined by the STD for this command set. its only for other device types like osd, printer and so on. But sending XOR commands to scsi_dbg is a good start. > Bart. > Please Note: I do not agree for the use of the constant DMA_BIDIRECTIONAL to denote scsi_bidi_cmnd(cmd). This is wrong and takes us back not forward. If anything at all is done, cmd->sc_data_direction should be just removed all together. It is not needed and denotes nothing more than what is already available at the request level. Please do not add any more new code on top of cmd->sc_data_direction. (At minimum just remove the all DMA_BIDIRECTIONAL checks and you are done) Cheers Boaz -- 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 0/17] Clear up bidi command confusion
On 01/29/2015 04:41 PM, James Bottomley wrote: > On Thu, 2015-01-29 at 15:00 +0200, Boaz Harrosh wrote: >> On 01/23/2015 03:12 PM, Christoph Hellwig wrote: >>> On Fri, Jan 23, 2015 at 01:05:30PM +0100, Bart Van Assche wrote: >>>> There is some confusion in the SCSI core and in SCSI LLDs around the >>>> meaning of sc_data_direction and whether or not this member can have the >>>> value DMA_BIDIRECTIONAL. Clear up this confusion. The patches in this >>>> series are: >>> >>> I wonder if we should change the code to set DMA_BIDIRECTIONAL for >>> bidi commands. That seems a lot more logical than the current >>> version. >>> >> >> You cannot do this. Because a Bidi Command is actually two commands >> one for the WRITE side (DMA_TO_DEVICE) which is this one, and another >> command for the READ side (DMA_FROM_DEVICE). > > You're not thinking about this the correct way. DMA_BIDIRECTIONAL is a > DMA engine flag. We use it in SCSI for historical reasons (mainly to > prevent a translation from the SCSI version). Since it was never used, > most SCSI drivers have code to check and reject commands with it set. > For actual bidirectional commands, you have to check scsi_bidi_command() > and programme the DMA engine separately for both phases, so the flag is > useless in this case. > This is what I meant how is above different from what I said? > The proposal is to make it the signal for bidirectional commands This I disagree, and would be a very bad idea and will take us back, to hacking time. As you noted, please let us leave DMA_BIDIRECTIONAL to the DMA engines. Let us just stir away from the DMA_XXX flags altogether and move back to block layer flags. (in which case most HBAs would make it do the right thing; i.e. reject) This is not necessary, (the check and rejection), commands will not be issued to LLDs that did not mark support for BiDi. So they will never receive such commands and it is added dead code. > but > it still wouldn't be how you'd program the DMA enigne; that still would > have to be done in two phases as it is today. > Exactly my point. Only one small correction, these are not two phases, as in phases-1 then phases-2. These are "two" memory buffers independently programmed for DMA transfer. The actual transfer occurs simultaneously, on both buffers. > James > > Thanks Boaz -- 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
[GIT PULL] exofs: 3 changes to exofs & osd
Hi Linus. Please pull the following changes since commit [ddffeb8c] Linux 3.7-rc1 They are available in the git repository at: git://git.open-osd.org/linux-open-osd.git for-linus for you to fetch changes up to [861d6660] exofs: don't leak io_state and pages on read error (2012-12-14 12:17:32 +0200) These are just 3 patches, the last two are bug fixes on the error paths in exofs. The important patch is the one to osd_uld which adds sysfs info to osd devices for use by user-mode clustering discovery software. I'm already sitting on this patch since before February this year, It is important for some of the big installation cluster systems, who's been compiling their own kernel just for that patch. Thanks in advance Boaz -------- Boaz Harrosh (1): exofs: don't leak io_state and pages on read error Idan Kedar (1): exofs: clean up the correct page collection on write error Sachin Bhamare (1): osduld: Add osdname & systemid sysfs at scsi_osd class drivers/scsi/osd/osd_uld.c | 28 fs/exofs/inode.c | 16 +--- 2 files changed, 37 insertions(+), 7 deletions(-) -- 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: [GIT PULL] exofs: 3 changes to exofs & osd
On 12/18/2012 12:58 PM, James Bottomley wrote: > On Mon, 2012-12-17 at 18:53 +0200, Boaz Harrosh wrote: >> Hi Linus. >> >> Please pull the following changes since commit [ddffeb8c] Linux 3.7-rc1 >> They are available in the git repository at: >> >> git://git.open-osd.org/linux-open-osd.git for-linus >> >> for you to fetch changes up to >> [861d6660] exofs: don't leak io_state and pages on read error >> (2012-12-14 12:17:32 +0200) >> >> These are just 3 patches, the last two are bug fixes on the error paths >> in exofs. >> >> The important patch is the one to osd_uld which adds sysfs info to osd >> devices for use by user-mode clustering discovery software. I'm already >> sitting on this patch since before February this year, It is important for >> some of the big installation cluster systems, who's been compiling their >> own kernel just for that patch. > > I'm a bit perplexed by this. You got notice when it was added to the > SCSI tree and now it's already upstream: > > commit 51976a8c85cec0c62e410bc38b8a11dbc690764d > Author: Boaz Harrosh > Date: Wed Oct 24 14:51:41 2012 -0700 > > [SCSI] osd_uld: Add osdname & systemid sysfs at scsi_osd class > > But the authorship info differs ... it looks like you forgot to include > the From: tag in your original patch send. > I'm so sorry, I completely goofed on this one. It's what happens when you are swamped with other work and are doing things without thinking. I totally forgot that I need to remove this patch. Both these patches where in linux-next for a long time. So I believe the merge will go just fine. Lets leave it like this, or I can rebase and remove it? > James > > Thanks Boaz -- 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: [GIT PULL] exofs: 3 changes to exofs & osd
On 12/18/2012 01:28 PM, James Bottomley wrote: <> >> Both these patches where in linux-next for a long time. So I believe >> the merge will go just fine. Lets leave it like this, or I can rebase >> and remove it? > > If it merges OK, I'd just leave it as is. It wouldn't be anywhere close > to the first time we've had the same patch via different trees. > Thanks James, it will not happen again. I just had a look on linux-next of Dec 17 they both where merged in and I did not see any complains. > James > Boaz -- 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] libosd: check for kzalloc() failure
On 01/30/2013 04:34 PM, walter harms wrote:
>
<>
> I start to see the complexity of the situation. Would you mind to add
> the comment "it can be anything. UTF-8 is more likely but not guaranteed
> either" ?
> For now using a pascal-string seems the best solution but it should be warned
> that get_attrs[a].val_ptr is NOT a c-string and therefore can not be used with
> the printf-family (i guess the situation will become more clear in future)
>
OK, So... The long story
The STD says that osdname can be *anything* binary or otherwise, and of *any*
length. And is used to uniquely identify the OSD-device/partition in a cluster.
We decided that if so we would stick an 40 char UUID in it, generated by a
uuidgen
and all the external utilities and surrounding code forces it, and treat it as
a c-string. But this code here in the core cannot make that assumption and still
support the STD.
On the other hand we did want the osdname to be printable in traces and messages
because it is such an important identifier. So I have decided to sacrifice an
extra char in-memory to carry a \NUL and safely stick it into printk(s). Those
(none existent) OSD devices that will put unprintable characters in here will
still function fine, but will look real scary in printk(s).
Please note that the one that sets policy is the osd-target vendor. (And they
all currently use my code base)
So save the kzalloc check this code (tested) is safe and will show strings when
present, but will gracefully show ugly things but still work when not.
> I have no clue why you need that, using c-strings makes life more easy in the
> last minute a sprintf(buf,"%u") will save the day ;)
>
Actually it is very funny, because just recently (last week) I have discovered
something that eliminates all those funny business.
printf("%*s", odi->osdname, odi->osdname_len);
The "*" will instruct c to expect an extra variable following %s which is the
max_length of %s. This is exactly for pascal strings and the such like above.
So I added a TODO to clean that a bit by always printing with "%*s"
>>
> It look clever to add the NULL test here.
> Noone reviewing the code will understand that.
> (Rule of least surprise)
>
Thanks for looking, I agree it needs a fat comment, I'll do that when I'll
convert to above system. Thanks for looking, That code is really old and never
had any extra eyes on it.
> just my 2 cents,
> re,
> wh
>
Cheers
Boaz
--
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] libosd: check for kzalloc() failure
On 01/30/2013 09:06 AM, Dan Carpenter wrote:
> There wasn't any error handling for this kzalloc().
>
ACK-by: Boaz Harrosh
James please queue for inclusion
> Signed-off-by: Dan Carpenter
Thanks Dan
>
> diff --git a/drivers/scsi/osd/osd_initiator.c
> b/drivers/scsi/osd/osd_initiator.c
> index c06b8e5..d8293f2 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od,
> odi->osdname_len = get_attrs[a].len;
> /* Avoid NULL for memcmp optimization 0-length is good enough */
> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
> + if (!odi->osdname) {
> + ret = -ENOMEM;
> + goto out;
> + }
> if (odi->osdname_len)
> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
> OSD_INFO("OSD_NAME [%s]\n", odi->osdname);
>
--
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: [LSF/MM TOPIC][ATTEND] protection information and userspace
On 02/07/2013 01:27 PM, Hannes Reinecke wrote:
> On 02/07/2013 11:01 AM, Darrick J. Wong wrote:
>> On Thu, Feb 07, 2013 at 01:40:14AM -0800, Joel Becker wrote:
>>> On Wed, Feb 06, 2013 at 03:34:49PM -0500, Chuck Lever wrote:
On Feb 6, 2013, at 3:24 PM, "Darrick J. Wong"
wrote:
> On Wed, Feb 06, 2013 at 01:51:22PM -0600, Ben Myers wrote:
>> Hi,
>>
>> I'm interested in discussing how to pass protection information to and
>> from
>> userspace. Maybe Martin could be enlisted for the discussion.
>>
>> I read that some work has already been done in this area but have not
>> been able
>> to locate it. It looks like the bio-integrity code already makes it
>> possible
>> to generate the t10-dif crc in the filesystem. It would be good to be
>> able to
>> get the guard and application tags back out to backup applications such
>> as
>> xfsdump. Enabling other applications to generate their own tags in
>> userspace
>> is also interesting.
>
> This one's been on my list for a couple of years (and companies) too. A
> few
> years ago Joel Becker had support for it in his sys_dio proposal (that
> hasn't
> gone anywhere), and more recently I've theorized that we could add a magic
> fcntl/ioctl to make the kernel recognize, say, the first iovec of a
> O_DIRECT
> *{read,write}v call as the PI buffer, which I think is similar to how DIX
> gets
> PI data to a disk. But it's not like I have any code to show for it.
>
> I /think/ it's fairly straightforward to change the directio submit code
> to
> find the userspace PI buffer and amend the block integrity code to attach
> our
> own PI buffer. You'd still have to let the block layer set the sector #
> field,
> but afaik that won't affect the crc or the app tag.
>
> I hear that the NFS guys want to propose some sort of protocol for
> transmitting
> PI data (across NFS), but I haven't seen anything concrete yet.
I'm writing a requirements document for the NFS protocol which I can
discuss at LSF. The use cases for NFS for now would be virtual disk
devices (hypervisors) or direct NFS access to storage from user space.
Like everyone else we are waiting for a magical VFS and user space API to
appear that can pass PI to and from storage.
>>>
>>> I'm happy to chat about it. Unfortunately, like Darrick says, sys_dio()
>>> coding hasn't happened. I do think we're better off with some kind of
>>> explicit API than some magic state on the file. I mean, even something
>>> like:
>>>
>>> ssize_t write_with_pi(int fd, const void *buf, size_t count,
>>> const void *pi, size_t pi_count);
>>>
>>> It's not as nice as a non-historical API (eg sys_dio), but it also
>>> probably plays nicer with buffered I/O.
>>
>> I also pondered simply adding a new io_prep_* function + IO_CMD_ code to
>> libaio
>> and all the other plumbing necessary to make that happen...
>>
>> void io_prep_preadv_pi(struct iocb *iocb, int fd, const struct iovec *iov,
>> int iovcnt, long long offset, const void *pi,
>> size_t pi_count);
>>
> This is also what I've envisioned.
> Updating io_prep / async I/O is reasonably easy as its been using a
> separate structure for passing in the I/O details.
>
> Normal read/write calls don't really map as you simply don't have
> enough parameter to feed PI information into the kernel.
> So for that you'd need to invent a new interface / syscall.
>
> For aio we just need to add additional fields to an existing structure.
>
> So yeah, I'd be interested in that discussion as well.
>
Me too, in multiple fronts. It's part of my general concern about
"things we would like for user-mode servers"
I think that the current aio and libaio Interface is broken for a long
time, for multitude of reasons. For instance the nested structure definitions
are COMPAT broken, and lots of missing pieces. (For example search in archives
for why bsg does not support sg-lists.)
And there are all these additions that everyone wants on top, that call for
a new interface anyway.
So I would like to see a deep fixup of this interface, with an aio version2
that can take into considerations, all of future needs including these
above. Kernel code will be very happy to be implemented with the new, interface
and a COMPAT layer could be put in place for the old interface.
All interested parties should bring to the table what is the extension/changes
they need. And we can try and union all of them together.
(My addition is for support of sg_lists to bsg, in a way that makes Tomo happy
I know that qemu was wanting this for a while as well as the multitude of
user-mode servers)
Thanks
Boaz
> Cheers,
>
> Hannes
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of
Re: [LSF/MM TOPIC][ATTEND] protection information and userspace
On 02/07/2013 02:08 PM, Boaz Harrosh wrote:
> On 02/07/2013 01:27 PM, Hannes Reinecke wrote:
>> On 02/07/2013 11:01 AM, Darrick J. Wong wrote:
>>> On Thu, Feb 07, 2013 at 01:40:14AM -0800, Joel Becker wrote:
>>>> On Wed, Feb 06, 2013 at 03:34:49PM -0500, Chuck Lever wrote:
>>>>>
>>>>> On Feb 6, 2013, at 3:24 PM, "Darrick J. Wong"
>>>>> wrote:
>>>>>
>>>>>> On Wed, Feb 06, 2013 at 01:51:22PM -0600, Ben Myers wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I'm interested in discussing how to pass protection information to and
>>>>>>> from
>>>>>>> userspace. Maybe Martin could be enlisted for the discussion.
>>>>>>>
>>>>>>> I read that some work has already been done in this area but have not
>>>>>>> been able
>>>>>>> to locate it. It looks like the bio-integrity code already makes it
>>>>>>> possible
>>>>>>> to generate the t10-dif crc in the filesystem. It would be good to be
>>>>>>> able to
>>>>>>> get the guard and application tags back out to backup applications such
>>>>>>> as
>>>>>>> xfsdump. Enabling other applications to generate their own tags in
>>>>>>> userspace
>>>>>>> is also interesting.
>>>>>>
>>>>>> This one's been on my list for a couple of years (and companies) too. A
>>>>>> few
>>>>>> years ago Joel Becker had support for it in his sys_dio proposal (that
>>>>>> hasn't
>>>>>> gone anywhere), and more recently I've theorized that we could add a
>>>>>> magic
>>>>>> fcntl/ioctl to make the kernel recognize, say, the first iovec of a
>>>>>> O_DIRECT
>>>>>> *{read,write}v call as the PI buffer, which I think is similar to how
>>>>>> DIX gets
>>>>>> PI data to a disk. But it's not like I have any code to show for it.
>>>>>>
>>>>>> I /think/ it's fairly straightforward to change the directio submit code
>>>>>> to
>>>>>> find the userspace PI buffer and amend the block integrity code to
>>>>>> attach our
>>>>>> own PI buffer. You'd still have to let the block layer set the sector #
>>>>>> field,
>>>>>> but afaik that won't affect the crc or the app tag.
>>>>>>
>>>>>> I hear that the NFS guys want to propose some sort of protocol for
>>>>>> transmitting
>>>>>> PI data (across NFS), but I haven't seen anything concrete yet.
>>>>>
>>>>> I'm writing a requirements document for the NFS protocol which I can
>>>>> discuss at LSF. The use cases for NFS for now would be virtual disk
>>>>> devices (hypervisors) or direct NFS access to storage from user space.
>>>>>
>>>>> Like everyone else we are waiting for a magical VFS and user space API to
>>>>> appear that can pass PI to and from storage.
>>>>
>>>> I'm happy to chat about it. Unfortunately, like Darrick says, sys_dio()
>>>> coding hasn't happened. I do think we're better off with some kind of
>>>> explicit API than some magic state on the file. I mean, even something
>>>> like:
>>>>
>>>>ssize_t write_with_pi(int fd, const void *buf, size_t count,
>>>> const void *pi, size_t pi_count);
>>>>
>>>> It's not as nice as a non-historical API (eg sys_dio), but it also
>>>> probably plays nicer with buffered I/O.
>>>
>>> I also pondered simply adding a new io_prep_* function + IO_CMD_ code to
>>> libaio
>>> and all the other plumbing necessary to make that happen...
>>>
>>> void io_prep_preadv_pi(struct iocb *iocb, int fd, const struct iovec *iov,
>>>int iovcnt, long long offset, const void *pi,
>>>size_t pi_count);
>>>
>> This is also what I've envisioned.
>> Updating io_prep / async I/O is reasonably easy as its been using a
>> separate structure for passing in the I/O details.
>>
>> Normal read/write calls don't really map as you simply don't have
>>
Re: [LSF/MM TOPIC][ATTEND] protection information and userspace
On 02/07/2013 02:29 PM, Bart Van Assche wrote: > On 02/07/13 13:08, Boaz Harrosh wrote: >> (My addition is for support of sg_lists to bsg, in a way that makes Tomo >> happy >> I know that qemu was wanting this for a while as well as the multitude of >> user-mode servers) > > Do you think it would help / make sense if sg_alloc_table() would be > modified such that it allocates the entire scatterlist table via one > vmalloc() call instead of chaining several page-sized scatterlist tables > ? Note: such a change is not possible without modifying > scsi_alloc_sgtable(). > I don't think so, no. sg_alloc_table() is used not only for direct IO also for buffered, Now vmalloc() is terribly slow and would be a bottleneck in today's SSD performance. I love it that the Linux Kernel never uses vmalloc internally, and only ever chains everything to upto PAGE_SIZE sized objects. Coming from all these other OSs that don't, believe me, it is great great performance pain. (TLBs are a bitch) > Bart. > Thanks Boaz -- 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: [LSF/MM TOPIC][ATTEND] protection information and userspace
On 02/07/2013 02:33 PM, Hannes Reinecke wrote: > On 02/07/2013 01:16 PM, Boaz Harrosh wrote: >> (Again libaio should be changed in concert with Kernel's new API, and we >> can sacrifice old user-mode performance, with a COMPAT layer. Distro >> maintainers should consider replacing libaio, together with the new >> Kernel, so it is only those that do their own mix-and-match, who can >> fix that mismatch too) >> > And while we're at it, I still would _love_ to connect aio_cancel() > and blk_abort_request(). > > That way we could sensibly abort an I/O and get out of the darn 'D' > state. > Yes!! Thanks. It is very interesting how the socket side of the world had it correct for ages, and the same "fd" object on disks is second grade citizen in UNIX land. (Anybody voting for epoll on async disk IO? ) Thanks Hannes yes that too. And wait_interuptable() too, at couple of places, will need some serious error handling audit for that. > Cheers, > > Hannes > Boaz -- 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: [LSF/MM TOPIC] Thin provisioning SOFT_THRESHOLD error handling
On 01/29/2013 10:14 AM, Hannes Reinecke wrote: > Hi all, > > Thin-provisioned devices have the ability to set a 'soft threshold', > which is triggered if the real free space for this device is beyond > this mark. > > The intention behind this is to allow the system to induce some > garbage collection with possibly freeing up unused space. > > Initially it would be possible to execute garbage collection on > filesystems (eg for btrfs). > > However, as this concept applies to other areas within the kernel > (like dm-thinp or even btrfs itself) it might be an idea to have > a general mechanism / error handling etc in place. > > I would like to discuss at LSF the possible implementations > and handling mechanism for this kind of failure scenarios. > Hannes hi! This is received as an "unit attentions", right? Will it not be worth while to solve the general "unit attentions" under udev events, once and for all. Than such a btrfs GC above can just be a simple oneline udev rule. (I think that the event-storm problem you had at the time can be solved with some Kernel side "unit attentions" queue, and greatly reduce the chance for missed events) Thanks Boaz > Cheers, > > Hannes > -- 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] osd: fix signed char versus %02x issue
On 12/08/2015 04:25 PM, Rasmus Villemoes wrote:
> If char is signed and one of these bytes happen to have a value
> outside the ascii range, the corresponding output will consist of
> "ff" followed by the two hex chars that were actually
> intended. One way to fix it would be to change the casts to (u8*) aka
> (unsigned char*), but it is much simpler (and generates smaller code)
> to use the %ph extension which was created for such short hexdumps.
>
Ha real cool, thanks I hated that crap
ACK-by: Boaz Harrosh
> Signed-off-by: Rasmus Villemoes
> ---
> drivers/scsi/osd/osd_initiator.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/osd/osd_initiator.c
> b/drivers/scsi/osd/osd_initiator.c
> index 0cccd6033feb..d8a2b5185f56 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -170,10 +170,7 @@ static int _osd_get_print_system_info(struct osd_dev *od,
>
> /* FIXME: Where are the time utilities */
> pFirst = get_attrs[a++].val_ptr;
> - OSD_INFO("CLOCK [0x%02x%02x%02x%02x%02x%02x]\n",
> - ((char *)pFirst)[0], ((char *)pFirst)[1],
> - ((char *)pFirst)[2], ((char *)pFirst)[3],
> - ((char *)pFirst)[4], ((char *)pFirst)[5]);
> + OSD_INFO("CLOCK [0x%6phN]\n", pFirst);
>
> if (a < nelem) { /* IBM-OSD-SIM bug, Might not have it */
> unsigned len = get_attrs[a].len;
>
--
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 v2] osd: remove deadcode
On 02/24/2016 01:21 PM, Sudip Mukherjee wrote:
> The variable is_ver1 is always true and so OSD_CAP_LEN can never be
> used.
> Reported by Coverity.
>
> Signed-off-by: Sudip Mukherjee
ACK-by: Boaz harrosh
Thanks
> ---
>
> v2: Joe Perches asked to mention the tool used in the commit log.
>
> drivers/scsi/osd/osd_initiator.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/osd/osd_initiator.c
> b/drivers/scsi/osd/osd_initiator.c
> index d8a2b51..3b11aad 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -2006,9 +2006,8 @@ EXPORT_SYMBOL(osd_sec_init_nosec_doall_caps);
> */
> void osd_set_caps(struct osd_cdb *cdb, const void *caps)
> {
> - bool is_ver1 = true;
> /* NOTE: They start at same address */
> - memcpy(&cdb->v1.caps, caps, is_ver1 ? OSDv1_CAP_LEN : OSD_CAP_LEN);
> + memcpy(&cdb->v1.caps, caps, OSDv1_CAP_LEN);
> }
>
> bool osd_is_sec_alldata(struct osd_security_parameters *sec_parms __unused)
>
--
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: [REGRESSION v4.3] scsi_dh: use-after-free when removing scsi device
On 09/30/2015 12:28 PM, Hannes Reinecke wrote:
<>
> Pushing things into the background is typically not the best of
> ideas; actually I've been running into issues with udev not being
> complete by the time the next round is started. So more often than
> not I would be greeted with messages:
>
> 'write: no such file or directory'
>
> when executing this line. Removing the '&' at the end made this
> warning go away.
>
> And actually I'm not sure if the above script is a valid testcase;
So are you saying it is allowed to crash the Kernel with a crappy
script?
> from what I've seen there is no locking / reference counting when
> accessing sysfs attributes. So as soon as you _can_ access the sysfs
> attribute it is implicitly assumed to be valid.
> In your case you will be _removing_ the sysfs attribute even though
> it is still accessed, which of course will crash.
>
Is that allowed? for usermode script to race and crash the Kernel?
>From the original email it sounds like this used to be fine and it
now crashes (with the &)
Thanks
Boaz
> Can you still reproduce this problem after removing the '&' in that
> line?
>
>> echo "-- delete $dev --" > /dev/kmsg
>> echo 1 > /sys/class/scsi_device/${dev}/device/delete
>>
>> n=$((n + 1))
>> done
>> --- cut here --
>
> Having said that I've retried your test script with my ALUA handler
> update, and didn't find any issues there.
> It happily completed about 500 rounds at which point I got bored.
> Of course, this is after removing the '&' in the said line.
>
> Cheers,
>
> Hannes
>
--
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 4/4] aic7xxx: Enable 16-bit CDBs
On Sun, Oct 21 2007 at 18:19 +0200, James Bottomley <[EMAIL PROTECTED]> wrote:
> On Fri, 2007-10-19 at 10:32 +0200, Hannes Reinecke wrote:
>> The patch enables support for 16-bit CDBs in aic7xxx and aic79xx.
>> aic7xxx can actually support up to 32-bit CDBs, should they ever see
>> the light of day.
>>
>> Signed-off-by: Hannes Reinecke <[EMAIL PROTECTED]>
>> ---
>> drivers/scsi/aic7xxx/aic79xx.h |2 ++
>> drivers/scsi/aic7xxx/aic79xx_osm.c |1 +
>> drivers/scsi/aic7xxx/aic7xxx_osm.c |1 +
>> 3 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/scsi/aic7xxx/aic79xx.h b/drivers/scsi/aic7xxx/aic79xx.h
>> index ce638aa..883f26a 100644
>> --- a/drivers/scsi/aic7xxx/aic79xx.h
>> +++ b/drivers/scsi/aic7xxx/aic79xx.h
>> @@ -413,6 +413,8 @@ struct target_status {
>> * the sense buffer address in the SCB. This allows
>> * us to retrieve sense information without interrupting
>> * the host in packetized mode.
>> + * The current firmware relies on a CDB len of 16, so
>> + * don't change it unless you know what you're doing.
>> */
>> typedef uint32_t sense_addr_t;
>> #define MAX_CDB_LEN 16
>> diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c
>> b/drivers/scsi/aic7xxx/aic79xx_osm.c
>> index 2d02040..f4e12e1 100644
>> --- a/drivers/scsi/aic7xxx/aic79xx_osm.c
>> +++ b/drivers/scsi/aic7xxx/aic79xx_osm.c
>> @@ -1090,6 +1090,7 @@ ahd_linux_register_host(struct ahd_softc *ahd, struct
>> scsi_host_template *templa
>> host->max_id = (ahd->features & AHD_WIDE) ? 16 : 8;
>> host->max_lun = AHD_NUM_LUNS;
>> host->max_channel = 0;
>> +host->max_cmd_len = 16;
>
> Actually, the aic79xx doc we now have through the LF NDA programme says
> that the 79xx can also go up to 32 byte CDBs.
>
>> host->sg_tablesize = AHD_NSEG;
>> ahd_lock(ahd, &s);
>> ahd_set_unit(ahd, ahd_linux_unit++);
>> diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm.c
>> b/drivers/scsi/aic7xxx/aic7xxx_osm.c
>> index 390b0fc..d488764 100644
>> --- a/drivers/scsi/aic7xxx/aic7xxx_osm.c
>> +++ b/drivers/scsi/aic7xxx/aic7xxx_osm.c
>> @@ -1048,6 +1048,7 @@ ahc_linux_register_host(struct ahc_softc *ahc, struct
>> scsi_host_template *templa
>> host->max_id = (ahc->features & AHC_WIDE) ? 16 : 8;
>> host->max_lun = AHC_NUM_LUNS;
>> host->max_channel = (ahc->features & AHC_TWIN) ? 1 : 0;
>> +host->max_cmd_len = 32;
>> host->sg_tablesize = AHC_NSEG;
>> ahc_lock(ahc, &s);
>> ahc_set_unit(ahc, ahc_linux_unit++);
>
> But, I suppose the main point is: is this really necessary? The
> default, when unspecified, is 16 bytes and the mid-layer can't deliver
> anything larger. I would anticipate the large CDB infrastructure will
> have some separate enabling, so why not simply do nothing until that
> gets merged?
>
> James
I'm about to finish an RFC patchset for the extended commands.
I have implemented a more aggressive approach than the one
I've been sending for the last year.
(Matthew I have an extra 8-bytes save to scsi_cmnd on
64bit and 12 bytes for 32bit. Guess how? ;))
>From what I have now, (And also the old patch sets) host->max_cmd_len serves
>the same
function, Limit the command size sent by the mid-layer. Even more so with the
support
of extended commands.
So I recommend this patch.
If a driver does not have internal fixed-sized buffers to store the CDB than
setting of host->max_cmd_len, and use of cmnd->cmd_len will be enough to support
extended CDB's.
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/2] scsi core: alloc_cmnd
On Fri, Oct 19 2007 at 20:33 +0200, Matthew Wilcox <[EMAIL PROTECTED]> wrote:
> This fairly naive patch introduces alloc_cmnd and destroy_cmnd. I'm not
> exactly happy about passing down the gfp_mask -- I'd prefer to be able
> to allocate scsi_cmnds before we grab the queue_lock (and hence get
> rid of the possibility we might need to GFP_ATOMIC), but I don't see
> anywhere to do that. Maybe there's a simple change we can make to the
> block layer to allow it.
>
> Merely-an-RFC-by: Matthew Wilcox <[EMAIL PROTECTED]>
You know Matthew when you first talked about this, I envisioned
something else.
My idea was to have a new variable in scsi_host_template that states
the host_cmnd_extra_bytes. The mid-layer allocates a mempool with
sizeof(struct scsi_cmnd) + host_cmnd_extra_bytes.
A utility function will return that space for drivers given
a cmnd like:
void *host_cmnd_priv(struct scsi_cmnd *cmd)
{
return cmd + 1;
}
This serves the drivers well because all they need to do is set
host_cmnd_extra_bytes = size_of(my_cmnd_stuff) and start using
it. much more simple than setting up cache pools. It also keeps
the Q per host vs Q per driver.
This also solves your problem with locks and allocation flags
since nothing changes, and it is all private to the mid-layer.
And it serves me, because when bidi comes I just do:
sizeof(struct scsi_cmnd) + host_cmnd_extra_bytes + sizeof(bidi_stuff)
for hosts that support bidi. And the rest of the work was done by you.
Do you need that I scribble some tryout patch for above + example driver
Thanks for doing this
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/2] scsi core: alloc_cmnd
On Tue, Oct 23 2007 at 19:48 +0200, Matthew Wilcox <[EMAIL PROTECTED]> wrote:
> On Tue, Oct 23, 2007 at 06:49:17PM +0200, Boaz Harrosh wrote:
>> You know Matthew when you first talked about this, I envisioned
>> something else.
>
> Right, so did I. Christoph felt that alloc_cmnd/destroy_cmnd a la the
> VFS layer's alloc_inode/destroy_inode was a better route. I didn't have
> a strong feeling, so I implemented that.
>
>> This serves the drivers well because all they need to do is set
>> host_cmnd_extra_bytes = size_of(my_cmnd_stuff) and start using
>> it. much more simple than setting up cache pools. It also keeps
>> the Q per host vs Q per driver.
>
> The allocation pool is currently for the whole subsystem, not per host.
> I assumed you meant 'pool' rather than 'queue', since I didn't touch the
> queues.
>
rrr Your right, I got confused here, the pools are global, I forgot.
That kind of kills my plan (I don't even dare a maybe)
> Hosts already have allocation pools (er, I think they all do anyway
> ... and any that don't, I'm quite happy to write a generic_alloc_cmnd
> and stick it in scsi_lib).
>
Yes that would be nice. Make it as easy as possible for drivers.
Also for us when we sweep through them. (I'm here to help).
Most of them don't. Most of them either overload SCp or have
static arrays the size of their .can_queue .
>> This also solves your problem with locks and allocation flags
>> since nothing changes, and it is all private to the mid-layer.
>
> It doesn't solve that problem -- we already have that problem; it's just
> that it's kept in the midlayer.
>
OK I see...
>> And it serves me, because when bidi comes I just do:
>> sizeof(struct scsi_cmnd) + host_cmnd_extra_bytes + sizeof(bidi_stuff)
>> for hosts that support bidi. And the rest of the work was done by you.
>
> I have to admit to not having a clear picture of how bidi is supposed to
> work. Could we do it something like this?
>
> struct bidi_cmnd {
> struct scsi_cmnd;
> ...
> };
>
Yes nice
> struct qla_cmnd {
> struct bidi_cmnd bd_cmnd;
> ...
> };
>
yes OK
> We could also add an alloc_bidi_cmnd/destroy_bidi_cmnd to the shost
> template. Presumably most commands won't be bidi for any given host,
> so it'd be a waste of space to allocate them for all commands.
>
Well no one really knows. The OSD scsi devices I use are bidi only commands
(OK not only, 99%). The rest are not yet defined. (Like raid arrays that do
write-return-XOR)
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/2] scsi core: alloc_cmnd
On Tue, Oct 23 2007 at 21:15 +0200, Matthew Wilcox <[EMAIL PROTECTED]> wrote:
> On Tue, Oct 23, 2007 at 08:46:52PM +0200, Boaz Harrosh wrote:
>>> We could also add an alloc_bidi_cmnd/destroy_bidi_cmnd to the shost
>>> template. Presumably most commands won't be bidi for any given host,
>>> so it'd be a waste of space to allocate them for all commands.
>> Well no one really knows. The OSD scsi devices I use are bidi only commands
>> (OK not only, 99%). The rest are not yet defined. (Like raid arrays that do
>> write-return-XOR)
>
> What's the usage scenario though? Do we envisage one scsi_host being
> dedicated to OSD, or do we envisage OSDs being one component on a FC
> network? I suspect the latter, which leads me to want to do something
> like ...
>
Yes, like I have OSD devices on the other side of an iscsi transport.
So there can be other none OSD devices on the iscsi initiator. It's
a per device thing.
> struct qla_cmnd {
> char *sp;
> unsigned int compl_status;
> unsigned int resid_len;
> unsigned int scsi_status;
> unsigned int actual_snslen;
> unsigned int entry_status;
> }
>
> struct qla_bidi_cmnd {
> struct bidi_cmnd;
> struct qla_cmnd;
> }
>
> struct qla_cmnd {
> struct scsi_cmnd;
> struct qla_cmnd;
> }
>
> But then this requires us to have a bidi_queue_command. That might not
> be such a bad idea anyway ...
>
The current solution is pretty simple, we hang an extra scsi_data_buffer on
cmd->request->next_rq->special and allocate it when needed from a new
cache_mem_pool
of bidi scsi_data_buffer's. Since for an OSD device all Cmnds are bidi, I
thought
of saving that extra allocation. But it looks like the current model is not
working
for my case, so I'll keep it as it is now.
You can see an example of this here:
http://www.bhalevy.com/open-osd/download/bidi_2.6.23/0004-SCSI-bidi-support.patch
Thanks
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi/sym53c8xx: Fix a warning in sym_eh_handler
On Wed, Oct 24 2007 at 11:59 +0200, Jean Delvare <[EMAIL PROTECTED]> wrote:
> In 2.6.24-rc1 I see the following warning:
> drivers/scsi/sym53c8xx_2/sym_glue.c: In function "sym_eh_handler":
> drivers/scsi/sym53c8xx_2/sym_glue.c:612: warning: "io_reset" may be used
> uninitialized in this function
>
> Although gcc is wrong and the code is actually correct, it can easily
> be made more obvious to keep the compiler quiet.
>
> Signed-off-by: Jean Delvare <[EMAIL PROTECTED]>
> ---
> drivers/scsi/sym53c8xx_2/sym_glue.c | 13 ++---
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> --- linux-2.6.24-rc1.orig/drivers/scsi/sym53c8xx_2/sym_glue.c 2007-10-24
> 09:59:46.0 +0200
> +++ linux-2.6.24-rc1/drivers/scsi/sym53c8xx_2/sym_glue.c 2007-10-24
> 11:53:14.0 +0200
> @@ -609,8 +609,8 @@ static int sym_eh_handler(int op, char *
>*/
> #define WAIT_FOR_PCI_RECOVERY35
> if (pci_channel_offline(pdev)) {
> - struct completion *io_reset;
> - int finished_reset = 0;
> + struct completion *io_reset = NULL;
> + int finished_reset;
> init_completion(&eh_done);
> spin_lock_irq(shost->host_lock);
> /* Make sure we didn't race */
> @@ -618,15 +618,14 @@ static int sym_eh_handler(int op, char *
> if (!sym_data->io_reset)
> sym_data->io_reset = &eh_done;
> io_reset = sym_data->io_reset;
> - } else {
> - finished_reset = 1;
> }
> spin_unlock_irq(shost->host_lock);
> - if (!finished_reset)
> + if (io_reset) {
> finished_reset = wait_for_completion_timeout(io_reset,
> WAIT_FOR_PCI_RECOVERY*HZ);
> - if (!finished_reset)
> - return SCSI_FAILED;
> + if (!finished_reset)
> + return SCSI_FAILED;
> + }
> }
>
> spin_lock_irq(shost->host_lock);
>
>
I think you should use uninitialized_var and not brute = NULL;
This is because, than uninitialized_var can be tested for in
the future and removed if no longer relevant. And also for
the user of the code it is easier to understand.
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Short INQUIRY return
On Thu, Oct 25 2007 at 15:03 +0200, Alexander Sabourenkov <[EMAIL PROTECTED]> wrote: > Hello. > > > Background: > Promise TX4 SATA300 with sata_promise driver generates lots of errors, > both read and write. > > I'm now trying to port Promise-written driver, which works ok with > 2.6.11, to 2.6.22, > so that I can hopefully discover the error-inducing difference between > them, and gain > some understanding of kernel internals in the process. > > I have somewhat cleaned up the horrible mess the Promise driver code > is, so that it compiles, > loads and attaches to the controller. > > > Problem: > > Its internal port scan prints out correct information about hard > drive(s) discovered, and correctly > (i think) fills out response to the INQUIRY command, but when scsi > subsystem receives it back, > it contains some garbage instead. > > The same code works ok on 2.6.11, and I think it would work ok up to > 2.6.16 at least, but I have not checked yet. > > Question: > Can anyone please point out some drastic change in SCSI subsystem > (and what it would take to fix the driver up) > that would result in such behavior, so that I can save some time > digging it up myself? > > Just at the top of my head without looking at the code at all. I would say the bigest change would be the use of use_sg != 0 for all commands in Later kernels. (k>=2.6.17) Look for and around the scsi_cmnd->request_buffer usage. It used to be that for commands like INQUIRY it points to a linear char pointer. Today it will always point to a scatterlist array and use_sg is the sg count of that array. (Usually ==1 in the INQUIRY case) Just a shot in the dark (Where is the code for that driver?) Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
On Thu, Oct 25 2007 at 7:09 +0200, Jeff Garzik <[EMAIL PROTECTED]> wrote:
> Andrew Morton wrote:
>> On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik <[EMAIL PROTECTED]>
>> wrote:
>>
>>> drivers/scsi/ips.c | 178
>>>
>> this driver seems a bit of a basket case :(
>>
>>
>> What's going on here?
>>
>> scb->dcdb.cmd_attribute =
>> ips_command_direction[scb->scsi_cmd->cmnd[0]];
>>
>> /* Allow a WRITE BUFFER Command to Have no Data */
>> /* This is Used by Tape Flash Utilites */
>> if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len ==
>> 0))
>> scb->dcdb.cmd_attribute = 0;
>>
>> if (!(scb->dcdb.cmd_attribute & 0x3))
>> scb->dcdb.transfer_length = 0;
>>
>> if (scb->data_len >= IPS_MAX_XFER) {
>>
>> I hope that's just busted indentation and not a missing {} block.
>
> The driver is one of the grotty drivers people are afraid to touch,
> therefore it bitrots even faster, in a vicious cycle. You don't have to
> look hard at all to find checkpatch pukeage, very real bugs, and
> Pythonesque silliness.
>
> Not having hardware I went only for changes that are provable and
> obvious, really...
>
> Jeff
>
>From the experience with gdth I would say that a first patch of any
serious cleanup, should be the whitespace and formating cleanup.
And now that we can all read what it says, start changing.
In the gdth case I know of 3 bugs that happen just because of that mess.
And I promise to send that patch for gdth sn.
I found that lint, even with the command line options recommended by
kernel, is to aggressive, and leaves lots of work to be fixed by hand.
(e.g it will touch the comments)
I found that a small util called astyle with the right settings for the
tab==8/use-tabs will give you a clean tab indent, but will not touch the
longer than 80, broken out lines, which keeps things better formated.
Morton checkpatch is very good in whining about bad files, but did anyone
attempt a script for linux-style Indentation, formating, and whitespace cleanup,
that give you something like 95% success. As it is lint gives you 50-70%
and astyle 60-80%
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
Matthew Wilcox wrote: > On Thu, Oct 25, 2007 at 05:04:38PM +0200, Boaz Harrosh wrote: >> I found that lint, even with the command line options recommended by > > Do you mean Lindent / indent? > Yes "indent". I found that there are better switches to indent than what's in Lindent. But both are crap as for instance it does not understand the linux style multi-line-comments and mess them up, or it tabafy's broken-out-lines. Which is not what we usually want. And lots of other stuff. astyle does a much better job just by being less aggressive. >> kernel, is to aggressive, and leaves lots of work to be fixed by hand. >> (e.g it will touch the comments) > > It's not perfect, but code beautification is an art, not a science ;-) > > A lot of ugly code can't be made beautiful by a simple parser like > indent because what it really needs is refactoring. But you can't > refactor until you've made it at least partially readable, so Lindent is > the first step. > I think I disagree 89% and agree 11%. - remove trailing spaces - Indent, - Convert "Indents" to tabs - split long lines to multi lines, indent up to the last indent level, than fill with spaces, right align. These can all be done by a machine, the Linux way. Than - Adjust broken out lines, like if and while statements to be more readable - Remove/add blank lines for readability or after end of locals. That can be adjusted by a person. But I think with a given code it can be made 89% acceptable by a machine and 11% adjusted by a man. That is before I start a single char of coding. Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI/gdth: kill unneeded 'irq' argument
On Fri, Oct 26 2007 at 11:40 +0200, Jeff Garzik <[EMAIL PROTECTED]> wrote:
> Neither gdth_get_status() nor __gdth_interrupt() need their 'irq'
> argument, so remove it.
>
> Signed-off-by: Jeff Garzik <[EMAIL PROTECTED]>
> ---
> drivers/scsi/gdth.c | 21 +
> 1 files changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index b253b8c..65c21b1 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -141,7 +141,7 @@
> static void gdth_delay(int milliseconds);
> static void gdth_eval_mapping(ulong32 size, ulong32 *cyls, int *heads, int
> *secs);
> static irqreturn_t gdth_interrupt(int irq, void *dev_id);
> -static irqreturn_t __gdth_interrupt(gdth_ha_str *ha, int irq,
> +static irqreturn_t __gdth_interrupt(gdth_ha_str *ha,
> int gdth_from_wait, int* pIndex);
> static int gdth_sync_event(gdth_ha_str *ha, int service, unchar index,
> Scsi_Cmnd
> *scp);
> @@ -165,7 +165,6 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha,
> Scsi_Cmnd *scp);
> static int gdth_fill_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, ushort
> hdrive);
>
> static void gdth_enable_int(gdth_ha_str *ha);
> -static unchar gdth_get_status(gdth_ha_str *ha, int irq);
> static int gdth_test_busy(gdth_ha_str *ha);
> static int gdth_get_cmd_index(gdth_ha_str *ha);
> static void gdth_release_event(gdth_ha_str *ha);
> @@ -1334,14 +1333,12 @@ static void __init gdth_enable_int(gdth_ha_str *ha)
> }
>
> /* return IStatus if interrupt was from this card else 0 */
> -static unchar gdth_get_status(gdth_ha_str *ha, int irq)
> +static unchar gdth_get_status(gdth_ha_str *ha)
> {
> unchar IStatus = 0;
>
> -TRACE(("gdth_get_status() irq %d ctr_count %d\n", irq, gdth_ctr_count));
> +TRACE(("gdth_get_status() irq %d ctr_count %d\n", ha->irq,
> gdth_ctr_count));
>
> -if (ha->irq != (unchar)irq) /* check IRQ */
> -return false;
> if (ha->type == GDT_EISA)
> IStatus = inb((ushort)ha->bmic + EDOORREG);
> else if (ha->type == GDT_ISA)
> @@ -1523,7 +1520,7 @@ static int gdth_wait(gdth_ha_str *ha, int index,
> ulong32 time)
> return 1; /* no wait required */
>
> do {
> -__gdth_interrupt(ha, (int)ha->irq, true, &wait_index);
> +__gdth_interrupt(ha, true, &wait_index);
> if (wait_index == index) {
> answer_found = TRUE;
> break;
> @@ -3036,7 +3033,7 @@ static void gdth_clear_events(void)
>
> /* SCSI interface functions */
>
> -static irqreturn_t __gdth_interrupt(gdth_ha_str *ha, int irq,
> +static irqreturn_t __gdth_interrupt(gdth_ha_str *ha,
> int gdth_from_wait, int* pIndex)
> {
> gdt6m_dpram_str __iomem *dp6m_ptr = NULL;
> @@ -3054,7 +3051,7 @@ static irqreturn_t __gdth_interrupt(gdth_ha_str *ha,
> int irq,
> int act_int_coal = 0;
> #endif
>
> -TRACE(("gdth_interrupt() IRQ %d\n",irq));
> +TRACE(("gdth_interrupt() IRQ %d\n", ha->irq));
>
> /* if polling and not from gdth_wait() -> return */
> if (gdth_polling) {
> @@ -3067,7 +3064,7 @@ static irqreturn_t __gdth_interrupt(gdth_ha_str *ha,
> int irq,
> spin_lock_irqsave(&ha->smp_lock, flags);
>
> /* search controller */
> -if (0 == (IStatus = gdth_get_status(ha, irq))) {
> +if (0 == (IStatus = gdth_get_status(ha))) {
> /* spurious interrupt */
> if (!gdth_polling)
> spin_unlock_irqrestore(&ha->smp_lock, flags);
> @@ -3294,9 +3291,9 @@ static irqreturn_t __gdth_interrupt(gdth_ha_str *ha,
> int irq,
>
> static irqreturn_t gdth_interrupt(int irq, void *dev_id)
> {
> - gdth_ha_str *ha = (gdth_ha_str *)dev_id;
> + gdth_ha_str *ha = dev_id;
>
> - return __gdth_interrupt(ha, irq, false, NULL);
> + return __gdth_interrupt(ha, false, NULL);
> }
>
> static int gdth_sync_event(gdth_ha_str *ha, int service, unchar index,
ACK
I've done the last heavy lifting of gdth_interrupt. And I second this patch.
the irq was just redundant information to the ha pointer. Good riddance.
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [2.6 patch] scsi/seagate.c: remove dead code
On Sun, Oct 28 2007 at 17:51 +0200, Adrian Bunk <[EMAIL PROTECTED]> wrote: > This patch removes obviously dead code. > > Signed-off-by: Adrian Bunk <[EMAIL PROTECTED]> > > --- > > drivers/scsi/seagate.c |2 -- > 1 file changed, 2 deletions(-) > Actually I have sent a patch to completely remove this driver. http://www.spinics.net/lists/linux-scsi/msg20308.html Does any one objects? Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Drivers accessors latest version
Hi lots of drivers changed yet again do to latest SG API fixes. There is no use me sending all of them again. They are available on a public git tree. you can pull them from: git-pull git://bhalevy.com/open-osd accessors They are based on todays scsi-misc tree. If you want that I preform any changes before you pull please just ask. shortlog below b6f2026... isd200.c: use one-element sg list in issuing commands 6b3727d... usb: transport - convert to accessors and !use_sg code path removal 3439a91... usb: protocol.c - convert to accessors and !use_sg code path removal 8739148... usb: shuttle_usbat.c - convert to accessors and !use_sg code path removal a5207db... usb: freecom.c & sddr09.c - convert to accessors and !use_sg cleanup 1390243... NCR5380 familly convert to accessors & !use_sg cleanup cdd77de... arm: scsi convert to accessors and !use_sg cleanup 9bc7373... nsp_cs.c convert to data accessors and !use_sg cleanup b82e0e9... eata_pio.c: convert to accessors and !use_sg cleanup 2761373... a2091.c: convert to accessors and !use_sg cleanup 7f8e3b0... a3000.c: convert to accessors and !use_sg cleanup 36f4896... aha1542.c: convert to accessors and !use_sg cleanup 707fe55... atp870u.c: convert to accessors and !use_sg cleanup 2fb56c0... fd_mcs.c: convert to accessors and !use_sg cleanup 80736b1... imm.c: convert to accessors and !use_sg cleanup 25a55f7... ppa.c: convert to accessors and !use_sg cleanup f9fb2ad... wd33c93.c: convert to accessors and !use_sg cleanup ac0b49c... qlogicpti.c: convert to accessors and !use_sg cleanup 2f5cb3d... in2000.c: convert to accessors and !use_sg cleanup cb23b86... scsi_debug: convert to use the data buffer accessors f7e0b4d... wd7000.c - proper fix for boards without sg support 9cd3d65... Remove psi240i driver from kernel 4dcf751... Remove of seagate.c driver there is also a scsi_data_buffer branch on the same tree. The last version I sent had a stupid but fatal bug which is now fixed. Thanks Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Drivers accessors latest version
On Tue, Oct 30 2007 at 19:49 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote: > Hi > > lots of drivers changed yet again do to latest SG API fixes. > There is no use me sending all of them again. They are available > on a public git tree. > > you can pull them from: > git-pull git://bhalevy.com/open-osd accessors > > They are based on todays scsi-misc tree. If you want that I preform > any changes before you pull please just ask. > There is also gitweb at: http://www.bhalevy.com/git Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] aha152x: Use scsi_eh API for REQUEST_SENSE invocation
- Use new scsi_eh_prep/restor_cmnd() for synchronous
REQUEST_SENSE invocation.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
drivers/scsi/aha152x.c | 38 --
1 files changed, 8 insertions(+), 30 deletions(-)
diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index ea8c699..6ccdc96 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -260,6 +260,7 @@
#include
#include
#include
+#include
#include "aha152x.h"
static LIST_HEAD(aha152x_host_list);
@@ -558,9 +559,7 @@ struct aha152x_hostdata {
struct aha152x_scdata {
Scsi_Cmnd *next;/* next sc in queue */
struct completion *done;/* semaphore to block on */
- unsigned char aha_orig_cmd_len;
- unsigned char aha_orig_cmnd[MAX_COMMAND_SIZE];
- int aha_orig_resid;
+ struct scsi_eh_save ses;
};
/* access macros for hostdata */
@@ -1017,16 +1016,10 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt,
struct completion *complete,
SCp.buffers_residual : left buffers in list
SCp.phase: current state of the command */
- if ((phase & (check_condition|resetting)) || !scsi_sglist(SCpnt)) {
- if (phase & check_condition) {
- SCpnt->SCp.ptr = SCpnt->sense_buffer;
- SCpnt->SCp.this_residual = sizeof(SCpnt->sense_buffer);
- scsi_set_resid(SCpnt, sizeof(SCpnt->sense_buffer));
- } else {
- SCpnt->SCp.ptr = NULL;
- SCpnt->SCp.this_residual = 0;
- scsi_set_resid(SCpnt, 0);
- }
+ if ((phase & resetting) || !scsi_sglist(SCpnt)) {
+ SCpnt->SCp.ptr = NULL;
+ SCpnt->SCp.this_residual = 0;
+ scsi_set_resid(SCpnt, 0);
SCpnt->SCp.buffer = NULL;
SCpnt->SCp.buffers_residual = 0;
} else {
@@ -1561,10 +1554,7 @@ static void busfree_run(struct Scsi_Host *shpnt)
}
#endif
- /* restore old command */
- memcpy(cmd->cmnd, sc->aha_orig_cmnd, sizeof(cmd->cmnd));
- cmd->cmd_len = sc->aha_orig_cmd_len;
- scsi_set_resid(cmd, sc->aha_orig_resid);
+ scsi_eh_restore_cmnd(cmd, &sc->ses);
cmd->SCp.Status = SAM_STAT_CHECK_CONDITION;
@@ -1587,22 +1577,10 @@ static void busfree_run(struct Scsi_Host *shpnt)
DPRINTK(debug_eh, ERR_LEAD "requesting
sense\n", CMDINFO(ptr));
#endif
- /* save old command */
sc = SCDATA(ptr);
/* It was allocated in aha152x_internal_queue?
*/
BUG_ON(!sc);
- memcpy(sc->aha_orig_cmnd, ptr->cmnd,
- sizeof(ptr->cmnd));
- sc->aha_orig_cmd_len = ptr->cmd_len;
- sc->aha_orig_resid = scsi_get_resid(ptr);
-
- ptr->cmnd[0] = REQUEST_SENSE;
- ptr->cmnd[1] = 0;
- ptr->cmnd[2] = 0;
- ptr->cmnd[3] = 0;
- ptr->cmnd[4] =
sizeof(ptr->sense_buffer);
- ptr->cmnd[5] = 0;
- ptr->cmd_len = 6;
+ scsi_eh_prep_cmnd(ptr, &sc->ses, NULL, 0, ~0);
DO_UNLOCK(flags);
aha152x_internal_queue(ptr, NULL,
check_condition, ptr->scsi_done);
--
1.5.3.1
-
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
[RFC 0/4] varlen extended and vendor-specific cdbs
This is a proposal for adding support for variable-length, extended, and vendor specific CDBs. It should now cover the entire range of the SCSI standard. This patchset extends my original submission (over a year old) in that it starts with cleaning up scsi_cmnd, hence the first patch. The other three are similar to the ones submitted before. This effort is orthogonal to the bidi and scsi_data_buffer effort and can be accepted now. The patchset, however, is presented in this RFC on top of the scsi_data_buffer patches, as they sit in my tree. They can easily be rebased to current scsi-misc. The iscsi patch is on top of the iscsi branch of the iscsi-git-tree. (Matthew, these patches will conflict with your scsi_cmnd cleanup patches I promise to rebase them before submission) A complete git-tree based on scsi-misc which includes a complete bidi and varlen work can be fetched from: - git://git.bhalevy.com/open-osd bidi branch (varlen branch is this work) - http://www.bhalevy.com/git - has the gitweb GUI [1/4] Let scsi_cmnd->cmnd use request->cmd buffer Here I let scsi_cmnd->cmnd point to the space allocated by request->cmd, instead of copying the data. The scsi_cmnd->cmd_len is guaranteed to contain the right length of the command. I have tried to go over every single place in the kernel that uses scsi_cmnd->cmnd and make sure it looks sane. Surprisingly to me, that was not at all bad. I hope I did not miss anything. I've tested on an x86_64 machine booting from a sata disk and ran the iscsi regression tests as well as my bidi and varlen tests on top of the complete patchset and all tests passed. [2/4] block layer varlen-cdb Unlike the scsi approach (see below), I did not want to unify cmd[]/cmd_len and the *varlen_cdb and varlen_cdb_len members of struct request. This is because unlike scsi, block devices do not have a .max_cmd_len parameter to protect them from unexpected large commands. In the case varlen_cdb and varlen_cdb_len are used the cmd[] buffer is ignored and the cmd_len will be set to zero. [3/4] scsi: varlen extended and vendor-specific cdbs Adds support for variable length, extended, and vendor-specific cdbs at the scsi mid-layer. [4/4] iscsi: extended cdb support This is on top of the iscsi branch. In the URL above there are the three missing patches for iscsi, that add support for AHSs. Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] Let scsi_cmnd->cmnd use request->cmd[] buffer
- struct scsi_cmnd had a 16 bytes command buffer of its own.
This is an unnecessary duplication and copy of request's
cmd. It is probably left overs from the time that scsi_cmnd
could function without a request attached. So clean that up.
- Once above is done, few places, apart from scsi-ml, needed
adjustments due to changing the data type of scsi_cmnd->cmnd.
- Lots of drivers still use MAX_COMMAND_SIZE. So I have left
that #define but equate it to BLK_MAX_CDB. The way I see it
and is reflected in the patch below is.
MAX_COMMAND_SIZE - means: The longest fixed-length (*) SCSI CDB
as per the SCSI standard and is not related
to the implementation.
BLK_MAX_CDB. - The allocated space at the request level
(*)fixed-length here means commands that their size can be determined
by their opcode and the CDB does not carry a length specifier, like
the VARIABLE_LENGTH_CMD(0x7f) command. This is actually not exactly
true and the SCSI standard also defines extended commands and
vendor specific commands that can be bigger than 16 bytes. The kernel
will support these using the same infrastructure used for VARLEN CDB's.
So in effect MAX_COMMAND_SIZE means the maximum size command
scsi-ml supports without specifying a cmd_len by ULD's
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/firewire/fw-sbp2.c |2 +-
drivers/s390/scsi/zfcp_dbf.c |2 +-
drivers/scsi/53c700.c|6 +++---
drivers/scsi/ibmvscsi/ibmvscsi.c |2 +-
drivers/scsi/scsi_error.c| 14 --
drivers/scsi/scsi_lib.c |5 +++--
drivers/scsi/scsi_tgt_lib.c |2 ++
drivers/usb/storage/isd200.c |2 ++
include/scsi/scsi_cmnd.h | 10 +++---
include/scsi/scsi_eh.h |6 --
10 files changed, 32 insertions(+), 19 deletions(-)
diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c
index 5596df6..151d02c 100644
--- a/drivers/firewire/fw-sbp2.c
+++ b/drivers/firewire/fw-sbp2.c
@@ -1212,7 +1212,7 @@ static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd,
scsi_done_fn_t done)
memset(orb->request.command_block,
0, sizeof(orb->request.command_block));
- memcpy(orb->request.command_block, cmd->cmnd, COMMAND_SIZE(*cmd->cmnd));
+ memcpy(orb->request.command_block, cmd->cmnd, cmd->cmd_len);
orb->base.callback = complete_command_orb;
orb->base.request_bus =
diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index ffa3bf7..2d43c10 100644
--- a/drivers/s390/scsi/zfcp_dbf.c
+++ b/drivers/s390/scsi/zfcp_dbf.c
@@ -730,7 +730,7 @@ _zfcp_scsi_dbf_event_common(const char *tag, const char
*tag2, int level,
rec->scsi_result = scsi_cmnd->result;
rec->scsi_cmnd = (unsigned long)scsi_cmnd;
rec->scsi_serial = scsi_cmnd->serial_number;
- memcpy(rec->scsi_opcode, &scsi_cmnd->cmnd,
+ memcpy(rec->scsi_opcode, scsi_cmnd->cmnd,
min((int)scsi_cmnd->cmd_len,
ZFCP_DBF_SCSI_OPCODE));
rec->scsi_retries = scsi_cmnd->retries;
diff --git a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c
index 71ff3fb..85460f8 100644
--- a/drivers/scsi/53c700.c
+++ b/drivers/scsi/53c700.c
@@ -599,7 +599,7 @@ NCR_700_scsi_done(struct NCR_700_Host_Parameters *hostdata,
(struct NCR_700_command_slot *)SCp->host_scribble;
dma_unmap_single(hostdata->dev, slot->pCmd,
-sizeof(SCp->cmnd), DMA_TO_DEVICE);
+MAX_COMMAND_SIZE, DMA_TO_DEVICE);
if (slot->flags == NCR_700_FLAG_AUTOSENSE) {
char *cmnd = NCR_700_get_sense_cmnd(SCp->device);
#ifdef NCR_700_DEBUG
@@ -1003,7 +1003,7 @@ process_script_interrupt(__u32 dsps, __u32 dsp, struct
scsi_cmnd *SCp,
* here */
NCR_700_unmap(hostdata, SCp, slot);
dma_unmap_single(hostdata->dev, slot->pCmd,
-sizeof(SCp->cmnd),
+MAX_COMMAND_SIZE,
DMA_TO_DEVICE);
cmnd[0] = REQUEST_SENSE;
@@ -1900,7 +1900,7 @@ NCR_700_queuecommand(struct scsi_cmnd *SCp, void
(*done)(struct scsi_cmnd *))
}
slot->resume_offset = 0;
slot->pCmd = dma_map_single(hostdata->dev, SCp->cmnd,
- sizeof(SCp->cmnd), DMA_TO_DEVI
[PATCH 2/4] block layer varlen-cdb
- add varlen_cdb and varlen_cdb_len to hold a large user cdb
if needed. They start as empty. Allocation of buffer must
be done by user and held until request execution is done.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
block/ll_rw_blk.c |2 ++
include/linux/blkdev.h |7 ++-
2 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index b01dee3..df478f2 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -261,6 +261,8 @@ static void rq_init(struct request_queue *q, struct request
*rq)
rq->end_io_data = NULL;
rq->completion_data = NULL;
rq->next_rq = NULL;
+ rq->varlen_cdb_len = 0;
+ rq->varlen_cdb = NULL;
}
/**
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index bbf906a..990643d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -287,8 +287,13 @@ struct request {
/*
* when request is used as a packet command carrier
*/
- unsigned int cmd_len;
+ unsigned short cmd_len;
+ unsigned short varlen_cdb_len; /* length of varlen_cdb buffer */
unsigned char cmd[BLK_MAX_CDB];
+ unsigned char *varlen_cdb; /* an optional variable-length cdb.
+* points to a user buffer that must be
+* valid until end of request
+*/
unsigned int data_len;
unsigned int sense_len;
--
1.5.3.1
-
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 3/4] scsi: varlen extended and vendor-specific cdbs
Add support for variable-length, extended, and vendor specific
CDBs to scsi-ml. It is now possible for initiators and ULD's
to issue these types of commands. LLDs need not change much.
All they need is to raise the .max_cmd_len to the longest command
they support (see iscsi patches).
- clean-up some code paths that did not expect commands to be
larger than 16, and change cmd_len members' type to short as
char is not enough.
- Add support for varlen_cdb in scsi_execute.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
Signed-off-by: Benny Halevy <[EMAIL PROTECTED]>
---
block/scsi_ioctl.c |4 ++--
drivers/scsi/constants.c | 10 +++---
drivers/scsi/scsi.c | 22 +++---
drivers/scsi/scsi_lib.c | 25 +
include/scsi/scsi.h | 40 +---
include/scsi/scsi_cmnd.h |2 +-
include/scsi/scsi_host.h |8 +++-
7 files changed, 74 insertions(+), 37 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 91c7322..f08e196 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -33,13 +33,13 @@
#include
/* Command group 3 is reserved and should never be used. */
-const unsigned char scsi_command_size[8] =
+const unsigned char scsi_command_size_tbl[8] =
{
6, 10, 10, 12,
16, 12, 10, 10
};
-EXPORT_SYMBOL(scsi_command_size);
+EXPORT_SYMBOL(scsi_command_size_tbl);
#include
diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 024553f..5edfe0f 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -28,7 +28,6 @@
#define SERVICE_ACTION_OUT_12 0xa9
#define SERVICE_ACTION_IN_16 0x9e
#define SERVICE_ACTION_OUT_16 0x9f
-#define VARIABLE_LENGTH_CMD 0x7f
@@ -210,7 +209,7 @@ static void print_opcode_name(unsigned char * cdbp, int
cdb_len)
cdb0 = cdbp[0];
switch(cdb0) {
case VARIABLE_LENGTH_CMD:
- len = cdbp[7] + 8;
+ len = scsi_varlen_cdb_length(cdbp);
if (len < 10) {
printk("short variable length command, "
"len=%d ext_len=%d", len, cdb_len);
@@ -300,7 +299,7 @@ static void print_opcode_name(unsigned char * cdbp, int
cdb_len)
cdb0 = cdbp[0];
switch(cdb0) {
case VARIABLE_LENGTH_CMD:
- len = cdbp[7] + 8;
+ len = scsi_varlen_cdb_length(cdbp);
if (len < 10) {
printk("short opcode=0x%x command, len=%d "
"ext_len=%d", cdb0, len, cdb_len);
@@ -335,10 +334,7 @@ void __scsi_print_command(unsigned char *cdb)
int k, len;
print_opcode_name(cdb, 0);
- if (VARIABLE_LENGTH_CMD == cdb[0])
- len = cdb[7] + 8;
- else
- len = COMMAND_SIZE(cdb[0]);
+ len = scsi_command_size(cdb);
/* print out all bytes in cdb */
for (k = 0; k < len; ++k)
printk(" %02x", cdb[k]);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index e7dd171..6bbc053 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -79,15 +79,6 @@ static void scsi_done(struct scsi_cmnd *cmd);
#define MIN_RESET_PERIOD (15*HZ)
/*
- * Macro to determine the size of SCSI command. This macro takes vendor
- * unique commands into account. SCSI commands in groups 6 and 7 are
- * vendor unique and we will depend upon the command length being
- * supplied correctly in cmd_len.
- */
-#define CDB_SIZE(cmd) (cmd)->cmnd[0] >> 5) & 7) < 6) ? \
- COMMAND_SIZE((cmd)->cmnd[0]) : (cmd)->cmd_len)
-
-/*
* Note - the initial logging level can be set here to log events at boot time.
* After the system is up, you may enable logging via the /proc interface.
*/
@@ -467,6 +458,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
unsigned long flags = 0;
unsigned long timeout;
int rtn = 0;
+ unsigned cmd_len;
/* check if the device is still usable */
if (unlikely(cmd->device->sdev_state == SDEV_DEL)) {
@@ -548,9 +540,17 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
* Before we queue this command, check if the command
* length exceeds what the host adapter can handle.
*/
- if (CDB_SIZE(cmd) > cmd->device->host->max_cmd_len) {
+ cmd_len = cmd->cmd_len;
+ if (!cmd_len) {
+ BUG_ON(cmd->cmnd[0] == VARIABLE_LENGTH_CMD);
+ cmd_len = COMMAND_SIZE((cmd)->cmnd[0]);
+ }
+
+ if (cmd_len > cmd->device->host->max_cmd_len) {
SCSI_LOG_MLQUEUE(3,
- printk("queuecommand : command too long.\n"));
+ printk("queuecommand : command too long. "
+
[PATCH 4/4] iscsi: extended cdb support
Once varlen cdbs are supported by the block and scsi-ml layers
we can apply this patch to support extended CDBs in iscsi.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/iscsi_tcp.h |3 +-
drivers/scsi/libiscsi.c| 56
include/scsi/iscsi_proto.h |6 +++-
3 files changed, 57 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/iscsi_tcp.h b/drivers/scsi/iscsi_tcp.h
index ec4e5cf..30077fa 100644
--- a/drivers/scsi/iscsi_tcp.h
+++ b/drivers/scsi/iscsi_tcp.h
@@ -23,6 +23,7 @@
#define ISCSI_TCP_H
#include
+#include
/* Socket's Receive state machine */
#define IN_PROGRESS_WAIT_HEADER0x0
@@ -49,7 +50,7 @@
#define XMSTATE_SOL_HDR_INIT 0x2000
#define ISCSI_SG_TABLESIZE SG_ALL
-#define ISCSI_TCP_MAX_CMD_LEN 16
+#define ISCSI_TCP_MAX_CMD_LEN SCSI_MAX_VARLEN_CDB_SIZE
struct crypto_hash;
struct socket;
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 6a747cc..e57963c 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -139,6 +139,45 @@ static int iscsi_add_hdr(struct iscsi_cmd_task *ctask,
unsigned len)
return 0;
}
+/*
+ * make an extended cdb AHS
+ */
+static int iscsi_prep_ecdb_ahs(struct iscsi_cmd_task *ctask)
+{
+ struct scsi_cmnd *cmd = ctask->sc;
+ unsigned rlen, pad_len;
+ unsigned short ahslength;
+ struct iscsi_ecdb_ahdr *ecdb_ahdr;
+ int rc;
+
+ ecdb_ahdr = iscsi_next_hdr(ctask);
+ rlen = cmd->cmd_len - ISCSI_CDB_SIZE;
+
+ BUG_ON(rlen > sizeof(ecdb_ahdr->ecdb));
+ ahslength = rlen + sizeof(ecdb_ahdr->reserved);
+
+ pad_len = iscsi_padding(rlen);
+
+ rc = iscsi_add_hdr(ctask, sizeof(ecdb_ahdr->ahslength) +
+ sizeof(ecdb_ahdr->ahstype) + ahslength + pad_len);
+ if (rc)
+ return rc;
+
+ if (pad_len)
+ memset(&ecdb_ahdr->ecdb[rlen], 0, pad_len);
+
+ ecdb_ahdr->ahslength = cpu_to_be16(ahslength);
+ ecdb_ahdr->ahstype = ISCSI_AHSTYPE_CDB;
+ ecdb_ahdr->reserved = 0;
+ memcpy(ecdb_ahdr->ecdb, cmd->cmnd + ISCSI_CDB_SIZE, rlen);
+
+ debug_scsi("iscsi_prep_ecdb_ahs: varlen_cdb_len %d "
+ "rlen %d pad_len %d ahs_length %d iscsi_headers_size %u\n",
+ cmd->cmd_len, rlen, pad_len, ahslength, ctask->hdr_len);
+
+ return 0;
+}
+
/**
* iscsi_prep_scsi_cmd_pdu - prep iscsi scsi cmd pdu
* @ctask: iscsi cmd task
@@ -152,7 +191,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task
*ctask)
struct iscsi_session *session = conn->session;
struct iscsi_cmd *hdr = ctask->hdr;
struct scsi_cmnd *sc = ctask->sc;
- unsigned hdrlength;
+ unsigned hdrlength, cmd_len;
int rc;
ctask->hdr_len = 0;
@@ -167,10 +206,17 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task
*ctask)
hdr->cmdsn = cpu_to_be32(session->cmdsn);
session->cmdsn++;
hdr->exp_statsn = cpu_to_be32(conn->exp_statsn);
-memcpy(hdr->cdb, sc->cmnd, sc->cmd_len);
- if (sc->cmd_len < MAX_COMMAND_SIZE)
- memset(&hdr->cdb[sc->cmd_len], 0,
- MAX_COMMAND_SIZE - sc->cmd_len);
+ cmd_len = sc->cmd_len;
+ if (cmd_len < ISCSI_CDB_SIZE)
+ memset(&hdr->cdb[cmd_len], 0,
+ ISCSI_CDB_SIZE - cmd_len);
+ else if (cmd_len > ISCSI_CDB_SIZE) {
+ rc = iscsi_prep_ecdb_ahs(ctask);
+ if (rc)
+ return rc;
+ cmd_len = ISCSI_CDB_SIZE;
+ }
+ memcpy(hdr->cdb, sc->cmnd, cmd_len);
ctask->data_count = 0;
ctask->imm_count = 0;
diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h
index 4b2dce7..9dff9cc 100644
--- a/include/scsi/iscsi_proto.h
+++ b/include/scsi/iscsi_proto.h
@@ -110,6 +110,7 @@ struct iscsi_ahs_hdr {
#define ISCSI_AHSTYPE_CDB 1
#define ISCSI_AHSTYPE_RLENGTH 2
+#define ISCSI_CDB_SIZE 16
/* iSCSI PDU Header */
struct iscsi_cmd {
@@ -123,7 +124,7 @@ struct iscsi_cmd {
__be32 data_length;
__be32 cmdsn;
__be32 exp_statsn;
- uint8_t cdb[16];/* SCSI Command Block */
+ uint8_t cdb[ISCSI_CDB_SIZE];/* SCSI Command Block */
/* Additional Data (Command Dependent) */
};
@@ -152,7 +153,8 @@ struct iscsi_ecdb_ahdr {
__be16 ahslength; /* CDB length - 15, including reserved byte */
uint8_t ahstype;
uint8_t reserved;
- uint8_t ecdb[260 - 16]; /* 4-byte aligned extended CDB spillover */
+ /* 4-byte aligned extended CDB spillover */
+ uint8_t ecdb[260 - ISCSI_CDB_SIZE];
};
/* SCSI Response Header
Re: Drivers accessors latest version
On Sat, Nov 03 2007 at 21:41 +0200, James Bottomley <[EMAIL PROTECTED]> wrote: > > This one has a clear merge bug: > > index 2597209..bc63349 100644 > --- a/drivers/scsi/NCR5380.c > +++ b/drivers/scsi/NCR5380.c > > So could you fix it up correctly, please (and just email the patch, I > think it's the only problem commit). > > Thanks, > > James > Sorry about that. I saw it and fixed it in git tree, (rebase), anticipating a branch pull. But since you had to cherry-pick them by number you fetched the older patches. There is also a fix needed for wd7000.c, the one you have will not compile. I will send both patches as reply to this mail. Again sorry Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] block layer varlen-cdb
On Fri, Nov 02 2007 at 13:17 +0200, Matthew Wilcox <[EMAIL PROTECTED]> wrote: > On Fri, Nov 02, 2007 at 08:32:12AM +0200, Benny Halevy wrote: >> I agree this is probably the cleanest implementation but when Boaz and I >> initially discussed this approach he convinced me that LL block devices >> assume >> that req->cmd_len <= BLK_MAX_CDB and it is unsafe at the moment to expose >> them >> potentially larger commands. > > We'll never submit a command to a low level driver that is longer than > the max_cmd_len in the Scsi_Host. So if they've set it higher than they > really can deal with, that's an easy bug to fix. > This is true for scsi devices, and is what I did in patches 1/4 + 3/4, but for none-scsi, block devices, there is not such a ".max_cmd_len". There are no clients of large commands that are not scsi, so there is no use fixing any of that. The pointer at request is for the scsi case only. (Or can be used by new code for additional private command info) Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] wd7000.c - proper fix for boards without sg support
- code used to set sg_tablesize to zero for board revision
less than 6. This is no longer supported, therefore I
use sg_tablesize=1 and open code the sg handling for that case.
- Get rid of use of SG_NONE which will be removed soon.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/wd7000.c | 12 ++--
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/wd7000.c b/drivers/scsi/wd7000.c
index 03cd44f..b4304ae 100644
--- a/drivers/scsi/wd7000.c
+++ b/drivers/scsi/wd7000.c
@@ -1108,13 +1108,10 @@ static int wd7000_queuecommand(struct scsi_cmnd *SCpnt,
scb->host = host;
nseg = scsi_sg_count(SCpnt);
- if (nseg) {
+ if (nseg > 1) {
struct scatterlist *sg;
unsigned i;
- if (SCpnt->device->host->sg_tablesize == SG_NONE) {
- panic("wd7000_queuecommand: scatter/gather not
supported.\n");
- }
dprintk("Using scatter/gather with %d elements.\n", nseg);
sgb = scb->sgb;
@@ -1128,7 +1125,10 @@ static int wd7000_queuecommand(struct scsi_cmnd *SCpnt,
}
} else {
scb->op = 0;
- any2scsi(scb->dataptr, isa_virt_to_bus(scsi_sglist(SCpnt)));
+ if (nseg) {
+ struct scatterlist *sg = scsi_sglist(SCpnt);
+ any2scsi(scb->dataptr, isa_page_to_bus(sg_page(sg)) +
sg->offset);
+ }
any2scsi(scb->maxlen, scsi_bufflen(SCpnt));
}
@@ -1524,7 +1524,7 @@ static __init int wd7000_detect(struct scsi_host_template
*tpnt)
* For boards before rev 6.0, scatter/gather
isn't supported.
*/
if (host->rev1 < 6)
- sh->sg_tablesize = SG_NONE;
+ sh->sg_tablesize = 1;
present++; /* count it */
--
1.5.3.1
-
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] NCR5380 familly convert to accessors & !use_sg cleanup
- This patch depends on:
NCR5380: Use scsi_eh API for REQUEST_SENSE invocation
- convert to accessors and !use_sg cleanup
- FIXME: Not sg-chain ready look for ++cmd->SCp.buffer
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/NCR5380.c | 14 +++---
drivers/scsi/atari_NCR5380.c | 22 +++---
drivers/scsi/sun3_NCR5380.c | 22 +++---
3 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 2597209..1e9f828 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -295,16 +295,16 @@ static __inline__ void initialize_SCp(Scsi_Cmnd * cmd)
* various queues are valid.
*/
- if (cmd->use_sg) {
- cmd->SCp.buffer = (struct scatterlist *) cmd->request_buffer;
- cmd->SCp.buffers_residual = cmd->use_sg - 1;
+ if (scsi_bufflen(cmd)) {
+ cmd->SCp.buffer = scsi_sglist(cmd);
+ cmd->SCp.buffers_residual = scsi_sg_count(cmd) - 1;
cmd->SCp.ptr = sg_virt(cmd->SCp.buffer);
cmd->SCp.this_residual = cmd->SCp.buffer->length;
} else {
cmd->SCp.buffer = NULL;
cmd->SCp.buffers_residual = 0;
- cmd->SCp.ptr = (char *) cmd->request_buffer;
- cmd->SCp.this_residual = cmd->request_bufflen;
+ cmd->SCp.ptr = NULL;
+ cmd->SCp.this_residual = 0;
}
}
@@ -975,14 +975,14 @@ static int NCR5380_queue_command(Scsi_Cmnd * cmd, void
(*done) (Scsi_Cmnd *))
case WRITE_6:
case WRITE_10:
hostdata->time_write[cmd->device->id] -= (jiffies -
hostdata->timebase);
- hostdata->bytes_write[cmd->device->id] +=
cmd->request_bufflen;
+ hostdata->bytes_write[cmd->device->id] +=
scsi_bufflen(cmd);
hostdata->pendingw++;
break;
case READ:
case READ_6:
case READ_10:
hostdata->time_read[cmd->device->id] -= (jiffies -
hostdata->timebase);
- hostdata->bytes_read[cmd->device->id] +=
cmd->request_bufflen;
+ hostdata->bytes_read[cmd->device->id] +=
scsi_bufflen(cmd);
hostdata->pendingr++;
break;
}
diff --git a/drivers/scsi/atari_NCR5380.c b/drivers/scsi/atari_NCR5380.c
index a9680b5..d2ca3fa 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -511,9 +511,9 @@ static inline void initialize_SCp(Scsi_Cmnd *cmd)
* various queues are valid.
*/
- if (cmd->use_sg) {
- cmd->SCp.buffer = (struct scatterlist *)cmd->request_buffer;
- cmd->SCp.buffers_residual = cmd->use_sg - 1;
+ if (scsi_bufflen(cmd)) {
+ cmd->SCp.buffer = scsi_sglist(cmd);
+ cmd->SCp.buffers_residual = scsi_sg_count(cmd) - 1;
cmd->SCp.ptr = sg_virt(cmd->SCp.buffer);
cmd->SCp.this_residual = cmd->SCp.buffer->length;
/* ++roman: Try to merge some scatter-buffers if they are at
@@ -523,8 +523,8 @@ static inline void initialize_SCp(Scsi_Cmnd *cmd)
} else {
cmd->SCp.buffer = NULL;
cmd->SCp.buffers_residual = 0;
- cmd->SCp.ptr = (char *)cmd->request_buffer;
- cmd->SCp.this_residual = cmd->request_bufflen;
+ cmd->SCp.ptr = NULL;
+ cmd->SCp.this_residual = 0;
}
}
@@ -936,21 +936,21 @@ static int NCR5380_queue_command(Scsi_Cmnd *cmd, void
(*done)(Scsi_Cmnd *))
}
# endif
# ifdef NCR5380_STAT_LIMIT
- if (cmd->request_bufflen > NCR5380_STAT_LIMIT)
+ if (scsi_bufflen(cmd) > NCR5380_STAT_LIMIT)
# endif
switch (cmd->cmnd[0]) {
case WRITE:
case WRITE_6:
case WRITE_10:
hostdata->time_write[cmd->device->id] -= (jiffies -
hostdata->timebase);
- hostdata->bytes_write[cmd->device->id] +=
cmd->request_bufflen;
+ hostdata->bytes_write[cmd->device->id] +=
scsi_bufflen(cmd);
hostdata->pendingw++;
break;
case READ:
case READ_6:
case READ_10:
hostdata->time_read[cmd->device->id] -= (jiffies -
hostdata->timebase);
- hostdata->bytes_read[cmd->device->id] +=
cmd->request_bufflen;
+ hostdata->
Re: [PATCH] wd7000.c - proper fix for boards without sg support
On Mon, Nov 05 2007 at 11:21 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote:
> - code used to set sg_tablesize to zero for board revision
> less than 6. This is no longer supported, therefore I
> use sg_tablesize=1 and open code the sg handling for that case.
> - Get rid of use of SG_NONE which will be removed soon.
>
> Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
> ---
> drivers/scsi/wd7000.c | 12 ++--
> 1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/wd7000.c b/drivers/scsi/wd7000.c
> index 03cd44f..b4304ae 100644
> --- a/drivers/scsi/wd7000.c
> +++ b/drivers/scsi/wd7000.c
> @@ -1108,13 +1108,10 @@ static int wd7000_queuecommand(struct scsi_cmnd
> *SCpnt,
I see it is already applied so you might want to use this one instead
---
>From f7df6bc147256b015636a2ba8b1c8677bc4e Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <[EMAIL PROTECTED]>
Date: Mon, 5 Nov 2007 18:31:51 +0200
Subject: [PATCH] wd7000.c sg breakage do to last commit
Last merge of this driver reverted an SG fix.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/wd7000.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/wd7000.c b/drivers/scsi/wd7000.c
index 4ae2e5c..77f5847 100644
--- a/drivers/scsi/wd7000.c
+++ b/drivers/scsi/wd7000.c
@@ -1127,7 +1127,8 @@ static int wd7000_queuecommand(struct scsi_cmnd *SCpnt,
scb->op = 0;
if (nseg) {
struct scatterlist *sg = scsi_sglist(SCpnt);
- any2scsi(scb->dataptr, isa_page_to_bus(sg->page) +
sg->offset);
+ any2scsi(scb->dataptr,
+isa_page_to_bus(sg_page(sg)) + sg->offset);
}
any2scsi(scb->maxlen, scsi_bufflen(SCpnt));
}
--
1.5.3.1
-
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] gdth: scp timeout clean up
On Tue, Nov 06 2007 at 8:54 +0200, [EMAIL PROTECTED] wrote:
> gdth driver is modified NOT to use scp->eh_timeout. Now, it has
> eh_timed_out (gdth_timed_out) to handle command timeouts for locked
> I/O's. Have not tested as I don't have needed hardware! Patch is
> against 2.6.23-mm1.
>
> Thank you Matthew Wilcox for your input on the IRC channel.
>
> Signed-off-by: Malahal Naineni <[EMAIL PROTECTED]>
>
> diff -r dbb45a1edd2a drivers/scsi/gdth.c
> --- a/drivers/scsi/gdth.c Mon Nov 05 21:32:26 2007 -0800
> +++ b/drivers/scsi/gdth.c Mon Nov 05 21:54:27 2007 -0800
> @@ -2056,22 +2056,12 @@ static void gdth_putq(gdth_ha_str *ha, S
> register Scsi_Cmnd *pscp;
> register Scsi_Cmnd *nscp;
> ulong flags;
> -unchar b, t;
>
> TRACE(("gdth_putq() priority %d\n",priority));
> spin_lock_irqsave(&ha->smp_lock, flags);
>
> if (!cmndinfo->internal_command) {
> cmndinfo->priority = priority;
> -b = scp->device->channel;
> -t = scp->device->id;
> -if (priority >= DEFAULT_PRI) {
> -if ((b != ha->virt_bus && ha->raw[BUS_L2P(ha,b)].lock) ||
> -(b==ha->virt_bus && thdr[t].lock)) {
> -TRACE2(("gdth_putq(): locked IO ->update_timeout()\n"));
> -cmndinfo->timeout = gdth_update_timeout(scp, 0);
> -}
> -}
> }
>
> if (ha->req_first==NULL) {
> @@ -2359,7 +2349,7 @@ static void gdth_copy_internal_data(gdth
> {
> ushort cpcount,i, max_sg = gdth_sg_count(scp);
> ushort cpsum,cpnow;
> -struct scatterlist *sl, *sg;
> +struct scatterlist *sl;
> char *address;
>
> cpcount = min_t(ushort, count, gdth_bufflen(scp));
> @@ -3938,6 +3928,38 @@ static const char *gdth_info(struct Scsi
> return ((const char *)ha->binfo.type_string);
> }
>
> +static enum scsi_eh_timer_return gdth_timed_out(struct scsi_cmnd *scp)
> +{
> + gdth_ha_str *ha = shost_priv(scp->device->host);
> + struct gdth_cmndinfo *cmndinfo = gdth_get_cmndinfo(ha);
a gdth_get_cmndinfo(ha) is for allocating a new cmndinfo out of free
cmndinfo list, and must be paired by a call to gdth_put_cmndinfo(ha).
And usually you want to put it on scp->host_scribble, other wise it will
be lost.
To get the cmndinfo associated with a scsi_cmnd use gdth_cmnd_priv(scp)
> + unchar b, t;
> + ulong flags;
> + enum scsi_eh_timer_return retval = EH_NOT_HANDLED;
> +
> + TRACE(("%s() cmd 0x%x\n", scp->cmnd[0], __FUNCTION__));
> + b = scp->device->channel;
> + t = scp->device->id;
> +
> + /*
> + * We don't really honor the command timeout, but we try to
> + * honor 6 times of the actual command timeout! So reset the
> + * timer if this is less than 6th timeout on this command!
> + */
> + if (++cmndinfo->timeout_count < 6)
> + retval = EH_RESET_TIMER;
> +
> + /* Reset the timeout if it is locked IO */
> + spin_lock_irqsave(&ha->smp_lock, flags);
> + if ((b != ha->virt_bus && ha->raw[BUS_L2P(ha, b)].lock) ||
> + (b == ha->virt_bus && t < MAX_HDRIVES && ha->hdr[t].lock)) {
> + TRACE2(("%s(): locked IO, reset timeout\n", __FUNCTION__));
> + retval = EH_RESET_TIMER;
> + }
> + spin_unlock_irqrestore(&ha->smp_lock, flags);
> +
> + return retval;
> +}
> +
> static int gdth_eh_bus_reset(Scsi_Cmnd *scp)
> {
> gdth_ha_str *ha = shost_priv(scp->device->host);
> @@ -4031,7 +4053,7 @@ static int gdth_queuecommand(struct scsi
> BUG_ON(!cmndinfo);
>
> scp->scsi_done = done;
> -gdth_update_timeout(scp, scp->timeout_per_command * 6);
> +cmndinfo->timeout_count = 0;
> cmndinfo->priority = DEFAULT_PRI;
>
> gdth_set_bufflen(scp, scsi_bufflen(scp));
> @@ -4137,12 +4159,10 @@ static int ioc_lockdrv(void __user *arg)
> ha->hdr[j].lock = 1;
> spin_unlock_irqrestore(&ha->smp_lock, flags);
> gdth_wait_completion(ha, ha->bus_cnt, j);
> -gdth_stop_timeout(ha, ha->bus_cnt, j);
> } else {
> spin_lock_irqsave(&ha->smp_lock, flags);
> ha->hdr[j].lock = 0;
> spin_unlock_irqrestore(&ha->smp_lock, flags);
> -gdth_start_timeout(ha, ha->bus_cnt, j);
> gdth_next(ha);
> }
> }
> @@ -4582,14 +4602,12 @@ static int gdth_ioctl(struct inode *inod
> spin_unlock_irqrestore(&ha->smp_lock, flags);
> for (j = 0; j < ha->tid_cnt; ++j) {
> gdth_wait_completion(ha, i, j);
> -gdth_stop_timeout(ha, i, j);
> }
> } else {
> spin_lock_irqsave(&ha->smp_lock, flags);
> ha->raw[i].lock = 0;
> spin_unlock_irqrestore(&ha->smp_lock, flags);
> for (j = 0; j < ha->tid_cnt; ++j) {
> -gdth_start_timeout(ha, i, j);
> gdth_next(ha);
> }
>
[0/3] Last 3 patches for bidi support
[1] I propose a small change to scsi_tgt_lib.c that will make tgt completely neutral to the scsi_data_buffer patch. And will make it all that more ready for bidi, too. TOMO is this OK? (Can you do without the GFP_KERNEL allocation flag? It could make the code a bit more simple) [2] scsi_data_buffer patch. As requested by TOMO the all patch is squashed into one, 600 and some lines. TOMO if it makes you happy, OK, here it is. There is one hunk from libsrp.c that I really hate. and from what I understand of the code, only the "request_bufflen =" is really used, All users of "request_buffer = addr" pass NULL, as far as I can see. If you would like to make me happy, and it is at all possible, please clean it. [3] Once scsi_data_buffer is in, then scsi bidi support can be applied. (Block bidi was already merged 3 kernels ago). It makes no changes to scsi_cmnd and only adds some, not-at-all-dangerous, code to scsi_lib. So I don't see any reason to wait. please all review. If ACKed by all parties and applied for inclusion for 2.6.25, then they will have a long time to sit in MM and collect compilation breakage reports from ARCHs we never compiled. I wish these make everybody happy this time Boaz Harrosh - 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_tgt_lib: Use scsi_init_io instead of scsi_alloc_sgtable
- If we export scsi_init_io()/scsi_release_buffers() instead of
scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
much more insulated from scsi_lib changes. As a bonus it will
also gain bidi capability when it comes.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/scsi_lib.c | 21 ++---
drivers/scsi/scsi_tgt_lib.c | 34 +-
include/scsi/scsi_cmnd.h|4 ++--
3 files changed, 17 insertions(+), 42 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0e81e4c..a8bf7cb 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -737,7 +737,8 @@ static inline unsigned int scsi_sgtable_index(unsigned
short nents)
return index;
}
-struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
+ gfp_t gfp_mask)
{
struct scsi_host_sg_pool *sgp;
struct scatterlist *sgl, *prev, *ret;
@@ -823,9 +824,7 @@ enomem:
return NULL;
}
-EXPORT_SYMBOL(scsi_alloc_sgtable);
-
-void scsi_free_sgtable(struct scsi_cmnd *cmd)
+static void scsi_free_sgtable(struct scsi_cmnd *cmd)
{
struct scatterlist *sgl = cmd->request_buffer;
struct scsi_host_sg_pool *sgp;
@@ -871,8 +870,6 @@ void scsi_free_sgtable(struct scsi_cmnd *cmd)
mempool_free(sgl, sgp->pool);
}
-EXPORT_SYMBOL(scsi_free_sgtable);
-
/*
* Function:scsi_release_buffers()
*
@@ -890,7 +887,7 @@ EXPORT_SYMBOL(scsi_free_sgtable);
* the scatter-gather table, and potentially any bounce
* buffers.
*/
-static void scsi_release_buffers(struct scsi_cmnd *cmd)
+void scsi_release_buffers(struct scsi_cmnd *cmd)
{
if (cmd->use_sg)
scsi_free_sgtable(cmd);
@@ -902,6 +899,7 @@ static void scsi_release_buffers(struct scsi_cmnd *cmd)
cmd->request_buffer = NULL;
cmd->request_bufflen = 0;
}
+EXPORT_SYMBOL(scsi_release_buffers);
/*
* Function:scsi_io_completion()
@@ -1104,7 +1102,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
* BLKPREP_DEFER if the failure is retryable
* BLKPREP_KILL if the failure is fatal
*/
-static int scsi_init_io(struct scsi_cmnd *cmd)
+int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
{
struct request *req = cmd->request;
intcount;
@@ -1119,7 +1117,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
/*
* If sg table allocation fails, requeue request later.
*/
- cmd->request_buffer = scsi_alloc_sgtable(cmd, GFP_ATOMIC);
+ cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
if (unlikely(!cmd->request_buffer)) {
scsi_unprep_request(req);
return BLKPREP_DEFER;
@@ -1148,6 +1146,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
return BLKPREP_KILL;
}
+EXPORT_SYMBOL(scsi_init_io);
static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
struct request *req)
@@ -1193,7 +1192,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev,
struct request *req)
BUG_ON(!req->nr_phys_segments);
- ret = scsi_init_io(cmd);
+ ret = scsi_init_io(cmd, GFP_ATOMIC);
if (unlikely(ret))
return ret;
} else {
@@ -1244,7 +1243,7 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct
request *req)
if (unlikely(!cmd))
return BLKPREP_DEFER;
- return scsi_init_io(cmd);
+ return scsi_init_io(cmd, GFP_ATOMIC);
}
EXPORT_SYMBOL(scsi_setup_fs_cmnd);
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index deea3cd..5fd5fca 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -331,8 +331,7 @@ static void scsi_tgt_cmd_done(struct scsi_cmnd *cmd)
scsi_tgt_uspace_send_status(cmd, tcmd->itn_id, tcmd->tag);
- if (scsi_sglist(cmd))
- scsi_free_sgtable(cmd);
+ scsi_release_buffers(cmd);
queue_work(scsi_tgtd, &tcmd->work);
}
@@ -353,31 +352,6 @@ static int scsi_tgt_transfer_response(struct scsi_cmnd
*cmd)
return 0;
}
-static int scsi_tgt_init_cmd(struct scsi_cmnd *cmd, gfp_t gfp_mask)
-{
- struct request *rq = cmd->request;
- int count;
-
- cmd->use_sg = rq->nr_phys_segments;
- cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
- if (!cmd->request_buffer)
- return -ENOMEM;
-
- cmd->request_bufflen = rq->data_len;
-
- dprintk("cmd %p cnt %d %lu\n", cmd, scsi_sg_count(cmd),
- rq_data_dir(rq));
- count = blk_rq_map_sg(rq->q, rq, scsi_sglist(cmd));
-
[PATCH 2/3] scsi_data_buffer
In preparation for bidi we abstract all IO members of scsi_cmnd,
that will need to duplicate, into a substructure.
- Group all IO members of scsi_cmnd into a scsi_data_buffer
structure.
- Adjust accessors to new members.
- scsi_{alloc,free}_sgtable receive a scsi_data_buffer instead of
scsi_cmnd. And work on it.
- Adjust scsi_init_io() and scsi_release_buffers() for above
change.
- Fix other parts of scsi_lib/scsi.c to members migration. Use
accessors where appropriate.
- fix Documentation about scsi_cmnd in scsi_host.h
- scsi_error.c
* Changed needed members of struct scsi_eh_save.
* Careful considerations in scsi_eh_prep/restore_cmnd.
- sd.c and sr.c
* sd and sr would adjust IO size to align on device's block
size so code needs to change once we move to scsi_data_buff
implementation.
* Convert code to use scsi_for_each_sg
* Use data accessors where appropriate.
* Remove dead code (req_data_dir() != READ && != WRITE)
- tgt: convert libsrp to use scsi_data_buffer
- isd200: This driver still bangs on scsi_cmnd IO members,
so need changing
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
drivers/scsi/libsrp.c|4 +-
drivers/scsi/scsi.c |2 +-
drivers/scsi/scsi_error.c| 28 +-
drivers/scsi/scsi_lib.c | 79 --
drivers/scsi/sd.c|7 +---
drivers/scsi/sr.c| 28 +++
drivers/usb/storage/isd200.c |8 ++--
include/scsi/scsi_cmnd.h | 39 +---
include/scsi/scsi_eh.h |8 ++---
include/scsi/scsi_host.h |4 +-
10 files changed, 92 insertions(+), 115 deletions(-)
diff --git a/drivers/scsi/libsrp.c b/drivers/scsi/libsrp.c
index 5cff020..8a8562a 100644
--- a/drivers/scsi/libsrp.c
+++ b/drivers/scsi/libsrp.c
@@ -426,8 +426,8 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd
*cmd, void *info,
sc->SCp.ptr = info;
memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
- sc->request_bufflen = len;
- sc->request_buffer = (void *) (unsigned long) addr;
+ sc->sdb.length = len;
+ sc->sdb.sglist = (void *) (unsigned long) addr;
sc->tag = tag;
err = scsi_tgt_queue_command(sc, itn_id, (struct scsi_lun *)&cmd->lun,
cmd->tag);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1929488..73d2216 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -698,7 +698,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
"Notifying upper driver of completion "
"(result %x)\n", cmd->result));
- good_bytes = cmd->request_bufflen;
+ good_bytes = scsi_bufflen(cmd);
if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
drv = scsi_cmd_to_driver(cmd);
if (drv->done)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ebaca4c..e7b87ea 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -617,29 +617,25 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct
scsi_eh_save *ses,
ses->cmd_len = scmd->cmd_len;
memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
ses->data_direction = scmd->sc_data_direction;
- ses->bufflen = scmd->request_bufflen;
- ses->buffer = scmd->request_buffer;
- ses->use_sg = scmd->use_sg;
- ses->resid = scmd->resid;
+ ses->sdb = scmd->sdb;
ses->result = scmd->result;
+ memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+
if (sense_bytes) {
- scmd->request_bufflen = min_t(unsigned,
+ scmd->sdb.length = min_t(unsigned,
sizeof(scmd->sense_buffer), sense_bytes);
sg_init_one(&ses->sense_sgl, scmd->sense_buffer,
- scmd->request_bufflen);
- scmd->request_buffer = &ses->sense_sgl;
+ scmd->sdb.length);
+ scmd->sdb.sglist = &ses->sense_sgl;
scmd->sc_data_direction = DMA_FROM_DEVICE;
- scmd->use_sg = 1;
+ scmd->sdb.sg_count = 1;
memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
scmd->cmnd[0] = REQUEST_SENSE;
- scmd->cmnd[4] = scmd->request_bufflen;
+ scmd->cmnd[4] = scmd->sdb.length;
scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
} else {
- scmd->request_buffer = NULL;
- scmd->request_buff
[PATCH 3/3] SCSI: bidi support
At the block level bidi request uses req->next_rq pointer for a second
bidi_read request.
At Scsi-midlayer a second scsi_data_buffer structure is used for the
bidi_read part. This bidi scsi_data_buffer is put on
request->next_rq->special. Struct scsi_cmnd is not changed.
- Define scsi_bidi_cmnd() to return true if it is a bidi request and a
second sgtable was allocated.
- Define scsi_in()/scsi_out() to return the in or out scsi_data_buffer
from this command This API is to isolate users from the mechanics of
bidi.
- Define scsi_end_bidi_request() to do what scsi_end_request() does but
for a bidi request. This is necessary because bidi commands are a bit
tricky here. (See comments in body)
- scsi_release_buffers() will also release the bidi_read scsi_data_buffer
- scsi_io_completion() on bidi commands will now call
scsi_end_bidi_request() and return.
- The previous work done in scsi_init_io() is now done in a new
scsi_init_sgtable() (which is 99% identical to old scsi_init_io())
The new scsi_init_io() will call the above twice if needed also for
the bidi_read command. Only at this point is a command bidi.
- In scsi_error.c at scsi_eh_prep/restore_cmnd() make sure bidi-lld is not
confused by a get-sense command that looks like bidi. This is done
by puting NULL at request->next_rq, and restoring.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/scsi_error.c |3 +
drivers/scsi/scsi_lib.c | 154 -
include/scsi/scsi_cmnd.h | 23 +++-
include/scsi/scsi_eh.h|1 +
4 files changed, 149 insertions(+), 32 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index e7b87ea..51e2a2a 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -618,9 +618,11 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct
scsi_eh_save *ses,
memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
ses->data_direction = scmd->sc_data_direction;
ses->sdb = scmd->sdb;
+ ses->next_rq = scmd->request->next_rq;
ses->result = scmd->result;
memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+ scmd->request->next_rq = NULL;
if (sense_bytes) {
scmd->sdb.length = min_t(unsigned,
@@ -673,6 +675,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct
scsi_eh_save *ses)
memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
scmd->sc_data_direction = ses->data_direction;
scmd->sdb = ses->sdb;
+ scmd->request->next_rq = ses->next_rq;
scmd->result = ses->result;
}
EXPORT_SYMBOL(scsi_eh_restore_cmnd);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f904692..85eda10 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -64,6 +64,8 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
};
#undef SP
+static struct kmem_cache *scsi_bidi_sdb_cache;
+
static void scsi_run_queue(struct request_queue *q);
/*
@@ -625,6 +627,28 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
scsi_run_queue(sdev->request_queue);
}
+static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
+{
+ struct request_queue *q = cmd->device->request_queue;
+ struct request *req = cmd->request;
+ unsigned long flags;
+
+ add_disk_randomness(req->rq_disk);
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ if (blk_rq_tagged(req))
+ blk_queue_end_tag(q, req);
+
+ end_that_request_last(req, uptodate);
+ spin_unlock_irqrestore(q->queue_lock, flags);
+
+ /*
+* This will goose the queue request function at the end, so we don't
+* need to worry about launching another command.
+*/
+ scsi_next_command(cmd);
+}
+
/*
* Function:scsi_end_request()
*
@@ -652,7 +676,6 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd
*cmd, int uptodate,
{
struct request_queue *q = cmd->device->request_queue;
struct request *req = cmd->request;
- unsigned long flags;
/*
* If there are blocks left over at the end, set up the command
@@ -681,19 +704,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd
*cmd, int uptodate,
}
}
- add_disk_randomness(req->rq_disk);
-
- spin_lock_irqsave(q->queue_lock, flags);
- if (blk_rq_tagged(req))
- blk_queue_end_tag(q, req);
- end_that_request_last(req, uptodate);
- spin_unlock_irqrestore(q->queue_lock, flags);
-
- /*
-* This will goose the queue request function at the end, so we don't
-* need to worry about launching another command.
-*/
- scsi_ne
Re: [0/3] Last 3 patches for bidi support
On Tue, Nov 06 2007 at 20:25 +0200, Mike Christie <[EMAIL PROTECTED]> wrote: > Boaz Harrosh wrote: >> [1] >> I propose a small change to scsi_tgt_lib.c that will make >> tgt completely neutral to the scsi_data_buffer patch. And will >> make it all that more ready for bidi, too. TOMO is this OK? >> >> (Can you do without the GFP_KERNEL allocation flag? It could >> make the code a bit more simple) >> > > GFP_KERNEL is nice for the target layer because it can sleep in that > path you changed and it and does not have the "cannot write out pages > because it may come back to the same device issues" like an initiator does. > > If we ever changed to a softirq instead of the work queue then we would > not need the flag since it would have to GFP_ATOMIC, but I am not sure > if we have plans to do that anytime soon. Yes I understand that, hence the GFP_KERNEL was kept intact in my patch. But I was thinking perhaps it was possible to sleep outside, if the return value was BLKPREP_DEFER, the way the block layer sleeps, just not in allocation stage. Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [SCSI] iscsi: return data transfer residual for data-out commands
On Wed, Nov 07 2007 at 20:06 +0200, Tony Battersby <[EMAIL PROTECTED]> wrote:
> Currently, the iSCSI driver returns the data transfer residual for
> data-in commands (e.g. read) but not data-out commands (e.g. write).
> This patch makes it return the data transfer residual for both types of
> commands.
All types of commands, also good for BIDI ;)
>
> Signed-off-by: Tony Battersby <[EMAIL PROTECTED]>
> ---
> --- linux-2.6.24-rc2/drivers/scsi/libiscsi.c.orig 2007-11-07
> 12:52:20.0 -0500
> +++ linux-2.6.24-rc2/drivers/scsi/libiscsi.c 2007-11-07 12:52:27.0
> -0500
> @@ -291,9 +291,6 @@ invalid_datalen:
> min_t(uint16_t, senselen, SCSI_SENSE_BUFFERSIZE));
> }
>
> - if (sc->sc_data_direction == DMA_TO_DEVICE)
> - goto out;
> -
> if (rhdr->flags & ISCSI_FLAG_CMD_UNDERFLOW) {
> int res_count = be32_to_cpu(rhdr->residual_count);
>
>
>
Thanks, this looks right to me. (And good catch)
I have went through the code and it looks like the right thing to do.
"svn blame" annotates this code to the patch (r527) that libiscsi was
cut out of iscsi_tcp so perhaps then it made sense, but does not anymore.
It is also needed for the bidi patches, as currently bidi commands
have a sc_data_direction == DMA_TO_DEVICE.
Pleas accept this patch
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] scsi_data_buffer
On Thu, Nov 08 2007 at 5:14 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> On Tue, 06 Nov 2007 20:19:32 +0200
> Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>
>
> Hmm, checkpatch.pl complains reasonably:
>
> ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28815
> ERROR: use tabs not spaces
> #177: FILE: drivers/scsi/scsi_error.c:629:
> +^I^I scmd->sdb.length);$
>
> ERROR: use tabs not spaces
> #237: FILE: drivers/scsi/scsi_lib.c:741:
> + unsigned short sg_count, gfp_t
> gfp_mask)$
>
> WARNING: no space between function name and open parenthesis '('
> #487: FILE: drivers/scsi/sr.c:377:
> + scsi_for_each_sg (SCpnt, sg, sg_count, i)
>
> ERROR: "foo* bar" should be "foo *bar"
> #563: FILE: include/scsi/scsi_cmnd.h:20:
> + struct scatterlist* sglist;
>
> total: 3 errors, 1 warnings, 482 lines checked
I think that checkpatch is wrong in two accounts.
1. Tabs are used for "Indents" not "align-to-the-right".
All above have 0 indent and are right aligned for
readability.
2. check patch should be fixed that if a macro is followed
by a "{" it means a control block and not a function-call/
macro-expansion, and a space is recommended.
(Like: for, if, while ...)
But I don't mind. I'll change all in a fixing patch.
>
>
>> @@ -821,24 +820,24 @@ enomem:
>>
>> mempool_free(prev, sgp->pool);
>> }
>> -return NULL;
>> +return -1;
>
> I think that -ENOMEM is better. The other functions in scsi_lib.c
> (even static functions) use proper error values.
>
>
will fix, thanks.
>> -/*
>> - * If sg table allocation fails, requeue request later.
>> - */
>> -cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
>> -if (unlikely(!cmd->request_buffer)) {
>> +if (scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask)) {
>
> IIRC, preferable style is:
>
> ret = scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask);
> if (ret) {
>
>
It is more readable I agree, but I did not want to allocate one more
stack variable just for the if(), since I am returning a BLKPREP_DEFER
any way. I am not using ret for anything else.
>> scsi_unprep_request(req);
>> return BLKPREP_DEFER;
>> }
>>
>> req->buffer = NULL;
>> if (blk_pc_request(req))
>> -cmd->request_bufflen = req->data_len;
>> +sdb->length = req->data_len;
>> else
>> -cmd->request_bufflen = req->nr_sectors << 9;
>> +sdb->length = req->nr_sectors << 9;
>>
>> /*
>> * Next, walk the list, and fill in the addresses and sizes of
>> * each segment.
>> */
>> -count = blk_rq_map_sg(req->q, req, cmd->request_buffer);
>> -if (likely(count <= cmd->use_sg)) {
>> -cmd->use_sg = count;
>> +count = blk_rq_map_sg(req->q, req, sdb->sglist);
>> +if (likely(count <= sdb->sg_count)) {
>> +sdb->sg_count = count;
>> return BLKPREP_OK;
>> }
>>
>> printk(KERN_ERR "Incorrect number of segments after building list\n");
>> -printk(KERN_ERR "counted %d, received %d\n", count, cmd->use_sg);
>> +printk(KERN_ERR "counted %d, received %d\n", count, sdb->sg_count);
>> printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
>> req->current_nr_sectors);
>>
>> @@ -1199,9 +1182,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev,
>> struct request *req)
>> BUG_ON(req->data_len);
>> BUG_ON(req->data);
>>
>> -cmd->request_bufflen = 0;
>> -cmd->request_buffer = NULL;
>> -cmd->use_sg = 0;
>> +memset(&cmd->sdb, 0, sizeof(cmd->sdb));
>> req->buffer = NULL;
>> }
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 18343a6..28cf6fe 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct
>> request *rq)
>> } else if (rq_data_dir(rq) == READ) {
>> SCpnt->cmnd[0] = READ_6;
>> SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>> -} else {
>&
Re: [PATCH 2/3] scsi_data_buffer
James, Jens please note the question below
It is something that bothers me about sr.c
On Tue, Nov 06 2007 at 20:19 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote:
> In preparation for bidi we abstract all IO members of scsi_cmnd,
> that will need to duplicate, into a substructure.
>
>
> - sd.c and sr.c
> * sd and sr would adjust IO size to align on device's block
> size so code needs to change once we move to scsi_data_buff
> implementation.
> * Convert code to use scsi_for_each_sg
> * Use data accessors where appropriate.
> * Remove dead code (req_data_dir() != READ && != WRITE)
>
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 7702681..6d3bf41 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -226,7 +226,7 @@ out:
> static int sr_done(struct scsi_cmnd *SCpnt)
> {
> int result = SCpnt->result;
> - int this_count = SCpnt->request_bufflen;
> + int this_count = scsi_bufflen(SCpnt);
> int good_bytes = (result == 0 ? this_count : 0);
> int block_sectors = 0;
> long error_sector;
> @@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct
> request *rq)
> } else if (rq_data_dir(rq) == READ) {
> SCpnt->cmnd[0] = READ_10;
> SCpnt->sc_data_direction = DMA_FROM_DEVICE;
> - } else {
> - blk_dump_rq_flags(rq, "Unknown sr command");
> - goto out;
> }
>
> {
> - struct scatterlist *sg = SCpnt->request_buffer;
> - int i, size = 0;
> - for (i = 0; i < SCpnt->use_sg; i++)
> - size += sg[i].length;
> + struct scatterlist *sg;
> + int i, size = 0, sg_count = scsi_sg_count(SCpnt);
> +
> + scsi_for_each_sg (SCpnt, sg, sg_count, i)
> + size += sg->length;
>
> - if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
> + if (size != scsi_bufflen(SCpnt)) {
> scmd_printk(KERN_ERR, SCpnt,
> "mismatch count %d, bytes %d\n",
> - size, SCpnt->request_bufflen);
> - if (SCpnt->request_bufflen > size)
> - SCpnt->request_bufflen = size;
> + size, scsi_bufflen(SCpnt));
> + if (scsi_bufflen(SCpnt) > size)
> + SCpnt->sdb.length = size;
> }
> }
>
> @@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct
> request *rq)
>* request doesn't start on hw block boundary, add scatter pads
>*/
> if (((unsigned int)rq->sector % (s_size >> 9)) ||
> - (SCpnt->request_bufflen % s_size)) {
> + (scsi_bufflen(SCpnt) % s_size)) {
> scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
> goto out;
> }
Here we check I/O is "large-block" aligned. Both start and size
>
> - this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
> + this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
>
Number of "large-blocks"
>
> SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
> @@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct
> request *rq)
>
> if (this_count > 0x) {
> this_count = 0x;
> - SCpnt->request_bufflen = this_count * s_size;
> + SCpnt->sdb.length = this_count * s_size;
> }
>
Here is my problem:
In the case that the transfer is bigger than 0x * s_size (512/1024/2048)
we modify ->request_bufflen. Now this has two bad effects, the way I understand
it,
please fix me in my misunderstanding.
1. Later in sr_done doing return good_bytes=cmd->request_bufflen will only
complete
the cut-out bytes. Meaning possible BIO leak, since the original
request_bufflen
was lost. (not all bytes are completed)
2. What mechanics will re-send, or even knows, that not the complete request
was
transfered? The way I understand it, a cmnd->resid must be set, in this case.
Now the normal cmnd->resid is not enough because it will be written by
drivers, sr needs to stash a resid somewhere and add it to cmnd->resid in
sr_done(). But ...
I have a better solution for this. At attachment time. sr will modify the
request_queue's max_hw_sectors to not max over 0x * s_size, this way
the block layer will split it's I/O, and no extra resid han
Re: [PATCH 2/3] scsi_data_buffer
On Thu, Nov 08 2007 at 15:03 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> On Thu, 08 Nov 2007 11:24:36 +0200
> Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>
>>>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>>>> index 18343a6..28cf6fe 100644
>>>> --- a/drivers/scsi/sd.c
>>>> +++ b/drivers/scsi/sd.c
>>>> @@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct
>>>> request *rq)
>>>>} else if (rq_data_dir(rq) == READ) {
>>>>SCpnt->cmnd[0] = READ_6;
>>>>SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>>>> - } else {
>>>> - scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n",
>>>> rq->cmd_flags);
>>>> - goto out;
>>> This should go to the bidi patch?
>>>
>>>>}
>>>>
>> This is just a dead code cleanup. It is got nothing to do with bidi or
>> scsi_data_buffer
>> for that matter. It could be in it's own patch, but surly it will not go
>> into the bidi
>> patch. I will submit a new patch just for that code. Independent of these.
>> (I was hoping to save the extra effort)
>
> Hm, is it dead code? I think it's kinda BUG_ON, that is, we should not
> hit that code. sd only accetps READ and WRITE requests. It prevents
> funcy requests like BIDI from accidentally comming.
It is dead code. The rq_data_dir(rq) does a (->flags & 0x1) inline
the compiler will remove the extra code.
Also with bidi rq_data_dir(rq) is decided to return WRITE, a bidi
request is blk_bidi_rq(rq).
(I have separated this to a patch of it's own.)
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] scsi_tgt_lib: Use scsi_init_io instead of scsi_alloc_sgtable
On Thu, Nov 08 2007 at 15:04 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
> On Thu, 08 Nov 2007 10:32:56 +0200
> Benny Halevy <[EMAIL PROTECTED]> wrote:
>
>> On Nov. 08, 2007, 5:13 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote:
>>> On Tue, 06 Nov 2007 20:16:19 +0200
>>> Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>>>
>>>> - If we export scsi_init_io()/scsi_release_buffers() instead of
>>>> scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
>>>> much more insulated from scsi_lib changes. As a bonus it will
>>>> also gain bidi capability when it comes.
>>>>
>>>> Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
>>> Looks good for me except for this:
>>>
>>> ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
>>> ERROR: use tabs not spaces
>>> #101: FILE: drivers/scsi/scsi_lib.c:741:
>>> + gfp_t gfp_mask)$
>> Come on Tomo, tabs should be used for nesting, not for decoration.
>> This way no matter what's your tab expansion setup is the
>> code will look correct and will make sense. The number of space
>
> I've never heard about that rule. I use tabs and minimum spaces for
> decoration.
>
> But it's just about the style. The patch is fine by me if you like to
> use only spaces there.
>
> Thanks,
Thanks Tomo, I'm resending with the way you like it. You are
the maintainer and you should be comfortable with the code.
I do need your Signed-off-by on this. Since you are the maintainer.
Do you want that we push this through James, or through your tree?
Thanks again, will send revision soon.
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] scsi_data_buffer
On Thu, Nov 08 2007 at 15:54 +0200, Jens Axboe <[EMAIL PROTECTED]> wrote:
> On Thu, Nov 08 2007, Boaz Harrosh wrote:
>> James, Jens please note the question below
>> It is something that bothers me about sr.c
>>
>> On Tue, Nov 06 2007 at 20:19 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>>> In preparation for bidi we abstract all IO members of scsi_cmnd,
>>> that will need to duplicate, into a substructure.
>>>
>>
>>> - sd.c and sr.c
>>> * sd and sr would adjust IO size to align on device's block
>>> size so code needs to change once we move to scsi_data_buff
>>> implementation.
>>> * Convert code to use scsi_for_each_sg
>>> * Use data accessors where appropriate.
>>> * Remove dead code (req_data_dir() != READ && != WRITE)
>>>
>>
>>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
>>> index 7702681..6d3bf41 100644
>>> --- a/drivers/scsi/sr.c
>>> +++ b/drivers/scsi/sr.c
>>> @@ -226,7 +226,7 @@ out:
>>> static int sr_done(struct scsi_cmnd *SCpnt)
>>> {
>>> int result = SCpnt->result;
>>> - int this_count = SCpnt->request_bufflen;
>>> + int this_count = scsi_bufflen(SCpnt);
>>> int good_bytes = (result == 0 ? this_count : 0);
>>> int block_sectors = 0;
>>> long error_sector;
>>> @@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct
>>> request *rq)
>>> } else if (rq_data_dir(rq) == READ) {
>>> SCpnt->cmnd[0] = READ_10;
>>> SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>>> - } else {
>>> - blk_dump_rq_flags(rq, "Unknown sr command");
>>> - goto out;
>>> }
>>>
>>> {
>>> - struct scatterlist *sg = SCpnt->request_buffer;
>>> - int i, size = 0;
>>> - for (i = 0; i < SCpnt->use_sg; i++)
>>> - size += sg[i].length;
>>> + struct scatterlist *sg;
>>> + int i, size = 0, sg_count = scsi_sg_count(SCpnt);
>>> +
>>> + scsi_for_each_sg (SCpnt, sg, sg_count, i)
>>> + size += sg->length;
>>>
>>> - if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
>>> + if (size != scsi_bufflen(SCpnt)) {
>>> scmd_printk(KERN_ERR, SCpnt,
>>> "mismatch count %d, bytes %d\n",
>>> - size, SCpnt->request_bufflen);
>>> - if (SCpnt->request_bufflen > size)
>>> - SCpnt->request_bufflen = size;
>>> + size, scsi_bufflen(SCpnt));
>>> + if (scsi_bufflen(SCpnt) > size)
>>> + SCpnt->sdb.length = size;
>>> }
>>> }
>>>
>>> @@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct
>>> request *rq)
>>> * request doesn't start on hw block boundary, add scatter pads
>>> */
>>> if (((unsigned int)rq->sector % (s_size >> 9)) ||
>>> - (SCpnt->request_bufflen % s_size)) {
>>> + (scsi_bufflen(SCpnt) % s_size)) {
>>> scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
>>> goto out;
>>> }
>> Here we check I/O is "large-block" aligned. Both start and size
>>
>>>
>>> - this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
>>> + this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
>>>
>> Number of "large-blocks"
>>
>>>
>>> SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
>>> @@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct
>>> request *rq)
>>>
>>> if (this_count > 0x) {
>>> this_count = 0x;
>>> - SCpnt->request_bufflen = this_count * s_size;
>>> + SCpnt->sdb.length = this_count * s_size;
>>> }
>>>
>> Here is my problem:
>> In the case that the transfer is bigger than 0x * s_size
>> (512/1024/2048) we modify ->request_bufflen. Now this has two bad
>> effects, the way I understand it, please fix me in
[PATCH 4/4] SCSI: bidi support
At the block level bidi request uses req->next_rq pointer for a second
bidi_read request.
At Scsi-midlayer a second scsi_data_buffer structure is used for the
bidi_read part. This bidi scsi_data_buffer is put on
request->next_rq->special. Struct scsi_cmnd is not changed.
- Define scsi_bidi_cmnd() to return true if it is a bidi request and a
second sgtable was allocated.
- Define scsi_in()/scsi_out() to return the in or out scsi_data_buffer
from this command This API is to isolate users from the mechanics of
bidi.
- Define scsi_end_bidi_request() to do what scsi_end_request() does but
for a bidi request. This is necessary because bidi commands are a bit
tricky here. (See comments in body)
- scsi_release_buffers() will also release the bidi_read scsi_data_buffer
- scsi_io_completion() on bidi commands will now call
scsi_end_bidi_request() and return.
- The previous work done in scsi_init_io() is now done in a new
scsi_init_sgtable() (which is 99% identical to old scsi_init_io())
The new scsi_init_io() will call the above twice if needed also for
the bidi_read command. Only at this point is a command bidi.
- In scsi_error.c at scsi_eh_prep/restore_cmnd() make sure bidi-lld is not
confused by a get-sense command that looks like bidi. This is done
by puting NULL at request->next_rq, and restoring.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/scsi_error.c |3 +
drivers/scsi/scsi_lib.c | 154 -
include/scsi/scsi_cmnd.h | 23 +++-
include/scsi/scsi_eh.h|1 +
4 files changed, 149 insertions(+), 32 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 9daa983..13b6802 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -618,9 +618,11 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct
scsi_eh_save *ses,
memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
ses->data_direction = scmd->sc_data_direction;
ses->sdb = scmd->sdb;
+ ses->next_rq = scmd->request->next_rq;
ses->result = scmd->result;
memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+ scmd->request->next_rq = NULL;
if (sense_bytes) {
scmd->sdb.length = min_t(unsigned,
@@ -673,6 +675,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct
scsi_eh_save *ses)
memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
scmd->sc_data_direction = ses->data_direction;
scmd->sdb = ses->sdb;
+ scmd->request->next_rq = ses->next_rq;
scmd->result = ses->result;
}
EXPORT_SYMBOL(scsi_eh_restore_cmnd);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index edfd8d8..1ed6c6b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -64,6 +64,8 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
};
#undef SP
+static struct kmem_cache *scsi_bidi_sdb_cache;
+
static void scsi_run_queue(struct request_queue *q);
/*
@@ -625,6 +627,28 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
scsi_run_queue(sdev->request_queue);
}
+static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
+{
+ struct request_queue *q = cmd->device->request_queue;
+ struct request *req = cmd->request;
+ unsigned long flags;
+
+ add_disk_randomness(req->rq_disk);
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ if (blk_rq_tagged(req))
+ blk_queue_end_tag(q, req);
+
+ end_that_request_last(req, uptodate);
+ spin_unlock_irqrestore(q->queue_lock, flags);
+
+ /*
+* This will goose the queue request function at the end, so we don't
+* need to worry about launching another command.
+*/
+ scsi_next_command(cmd);
+}
+
/*
* Function:scsi_end_request()
*
@@ -652,7 +676,6 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd
*cmd, int uptodate,
{
struct request_queue *q = cmd->device->request_queue;
struct request *req = cmd->request;
- unsigned long flags;
/*
* If there are blocks left over at the end, set up the command
@@ -681,19 +704,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd
*cmd, int uptodate,
}
}
- add_disk_randomness(req->rq_disk);
-
- spin_lock_irqsave(q->queue_lock, flags);
- if (blk_rq_tagged(req))
- blk_queue_end_tag(q, req);
- end_that_request_last(req, uptodate);
- spin_unlock_irqrestore(q->queue_lock, flags);
-
- /*
-* This will goose the queue request function at the end, so we don't
-* need to worry about launching another command.
-*/
- scsi_ne
Re: [0/4 ver2] Last 3 patches for bidi support
On Tue, Nov 06 2007 at 20:04 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote: > [1] > I propose a small change to scsi_tgt_lib.c that will make > tgt completely neutral to the scsi_data_buffer patch. And will > make it all that more ready for bidi, too. TOMO is this OK? > > (Can you do without the GFP_KERNEL allocation flag? It could > make the code a bit more simple) > > [2] > scsi_data_buffer patch. > As requested by TOMO the all patch is squashed into one, 600 > and some lines. TOMO if it makes you happy, OK, here it is. > > There is one hunk from libsrp.c that I really hate. and from what > I understand of the code, only the "request_bufflen =" is really used, > All users of "request_buffer = addr" pass NULL, as far as I can see. > If you would like to make me happy, and it is at all possible, please > clean it. > > [3] > Once scsi_data_buffer is in, then scsi bidi support can be applied. > (Block bidi was already merged 3 kernels ago). It makes no changes > to scsi_cmnd and only adds some, not-at-all-dangerous, code to scsi_lib. > So I don't see any reason to wait. please all review. > > If ACKed by all parties and applied for inclusion for 2.6.25, > then they will have a long time to sit in MM and collect > compilation breakage reports from ARCHs we never compiled. > > I wish these make everybody happy this time > > Boaz Harrosh > Version 2, fixed as of Tomo's comments. Thanks Tomo. [0] Remove dead code from sd.c & sr.c. It was conceived by programmers in the passed that rq_data_dir() could perhaps be expanded to 2 bits and return READ/WRITE/BIDI. But we have decided long ago that a bidi request is when blk_bidi_rq() == true, and in that case rq_data_dir() == WRITE. So now we can be sure of what the compiler knew for a long time. (The future is already here) [1] [2] [3] See above I must insist again they should have time to sit in -mm. For posible compilation breakage of other ARCHs. Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] tgt: Use scsi_init_io instead of scsi_alloc_sgtable
- If we export scsi_init_io()/scsi_release_buffers() instead of
scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
much more insulated from scsi_lib changes. As a bonus it will
also gain bidi capability when it comes.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
Acked-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
drivers/scsi/scsi_lib.c | 21 ++---
drivers/scsi/scsi_tgt_lib.c | 34 +-
include/scsi/scsi_cmnd.h|4 ++--
3 files changed, 17 insertions(+), 42 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0e81e4c..6d8ea69 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -737,7 +737,8 @@ static inline unsigned int scsi_sgtable_index(unsigned
short nents)
return index;
}
-struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
+ gfp_t gfp_mask)
{
struct scsi_host_sg_pool *sgp;
struct scatterlist *sgl, *prev, *ret;
@@ -823,9 +824,7 @@ enomem:
return NULL;
}
-EXPORT_SYMBOL(scsi_alloc_sgtable);
-
-void scsi_free_sgtable(struct scsi_cmnd *cmd)
+static void scsi_free_sgtable(struct scsi_cmnd *cmd)
{
struct scatterlist *sgl = cmd->request_buffer;
struct scsi_host_sg_pool *sgp;
@@ -871,8 +870,6 @@ void scsi_free_sgtable(struct scsi_cmnd *cmd)
mempool_free(sgl, sgp->pool);
}
-EXPORT_SYMBOL(scsi_free_sgtable);
-
/*
* Function:scsi_release_buffers()
*
@@ -890,7 +887,7 @@ EXPORT_SYMBOL(scsi_free_sgtable);
* the scatter-gather table, and potentially any bounce
* buffers.
*/
-static void scsi_release_buffers(struct scsi_cmnd *cmd)
+void scsi_release_buffers(struct scsi_cmnd *cmd)
{
if (cmd->use_sg)
scsi_free_sgtable(cmd);
@@ -902,6 +899,7 @@ static void scsi_release_buffers(struct scsi_cmnd *cmd)
cmd->request_buffer = NULL;
cmd->request_bufflen = 0;
}
+EXPORT_SYMBOL(scsi_release_buffers);
/*
* Function:scsi_io_completion()
@@ -1104,7 +1102,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
* BLKPREP_DEFER if the failure is retryable
* BLKPREP_KILL if the failure is fatal
*/
-static int scsi_init_io(struct scsi_cmnd *cmd)
+int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
{
struct request *req = cmd->request;
intcount;
@@ -1119,7 +1117,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
/*
* If sg table allocation fails, requeue request later.
*/
- cmd->request_buffer = scsi_alloc_sgtable(cmd, GFP_ATOMIC);
+ cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
if (unlikely(!cmd->request_buffer)) {
scsi_unprep_request(req);
return BLKPREP_DEFER;
@@ -1148,6 +1146,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
return BLKPREP_KILL;
}
+EXPORT_SYMBOL(scsi_init_io);
static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
struct request *req)
@@ -1193,7 +1192,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev,
struct request *req)
BUG_ON(!req->nr_phys_segments);
- ret = scsi_init_io(cmd);
+ ret = scsi_init_io(cmd, GFP_ATOMIC);
if (unlikely(ret))
return ret;
} else {
@@ -1244,7 +1243,7 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct
request *req)
if (unlikely(!cmd))
return BLKPREP_DEFER;
- return scsi_init_io(cmd);
+ return scsi_init_io(cmd, GFP_ATOMIC);
}
EXPORT_SYMBOL(scsi_setup_fs_cmnd);
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index deea3cd..5fd5fca 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -331,8 +331,7 @@ static void scsi_tgt_cmd_done(struct scsi_cmnd *cmd)
scsi_tgt_uspace_send_status(cmd, tcmd->itn_id, tcmd->tag);
- if (scsi_sglist(cmd))
- scsi_free_sgtable(cmd);
+ scsi_release_buffers(cmd);
queue_work(scsi_tgtd, &tcmd->work);
}
@@ -353,31 +352,6 @@ static int scsi_tgt_transfer_response(struct scsi_cmnd
*cmd)
return 0;
}
-static int scsi_tgt_init_cmd(struct scsi_cmnd *cmd, gfp_t gfp_mask)
-{
- struct request *rq = cmd->request;
- int count;
-
- cmd->use_sg = rq->nr_phys_segments;
- cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
- if (!cmd->request_buffer)
- return -ENOMEM;
-
- cmd->request_bufflen = rq->data_len;
-
- dprintk("cmd %p cnt %d %lu\n", cmd, scsi_sg_count(cmd),
- rq_data_dir(rq))
[PATCH 1/4] sr/sd: Remove dead code
if (rq_data_dir() == WRITE) else if() else chain had an extra
"else" since the if() is on a value of 1 bit.
Also with a bidi request, rq_data_dir() == WRITE
and blk_bidi_rq() == true.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/sd.c |5 +
drivers/scsi/sr.c |5 +
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 18343a6..f1ec2bd 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -445,12 +445,9 @@ static int sd_prep_fn(struct request_queue *q, struct
request *rq)
}
SCpnt->cmnd[0] = WRITE_6;
SCpnt->sc_data_direction = DMA_TO_DEVICE;
- } else if (rq_data_dir(rq) == READ) {
+ } else {
SCpnt->cmnd[0] = READ_6;
SCpnt->sc_data_direction = DMA_FROM_DEVICE;
- } else {
- scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n",
rq->cmd_flags);
- goto out;
}
SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 7702681..a72ae85 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -365,12 +365,9 @@ static int sr_prep_fn(struct request_queue *q, struct
request *rq)
SCpnt->cmnd[0] = WRITE_10;
SCpnt->sc_data_direction = DMA_TO_DEVICE;
cd->cdi.media_written = 1;
- } else if (rq_data_dir(rq) == READ) {
+ } else {
SCpnt->cmnd[0] = READ_10;
SCpnt->sc_data_direction = DMA_FROM_DEVICE;
- } else {
- blk_dump_rq_flags(rq, "Unknown sr command");
- goto out;
}
{
--
1.5.3.1
-
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 3/4] scsi_data_buffer
In preparation for bidi we abstract all IO members of scsi_cmnd,
that will need to duplicate, into a substructure.
- Group all IO members of scsi_cmnd into a scsi_data_buffer
structure.
- Adjust accessors to new members.
- scsi_{alloc,free}_sgtable receive a scsi_data_buffer instead of
scsi_cmnd. And work on it.
- Adjust scsi_init_io() and scsi_release_buffers() for above
change.
- Fix other parts of scsi_lib/scsi.c to members migration. Use
accessors where appropriate.
- fix Documentation about scsi_cmnd in scsi_host.h
- scsi_error.c
* Changed needed members of struct scsi_eh_save.
* Careful considerations in scsi_eh_prep/restore_cmnd.
- sd.c and sr.c
* sd and sr would adjust IO size to align on device's block
size so code needs to change once we move to scsi_data_buff
implementation.
* Convert code to use scsi_for_each_sg
* Use data accessors where appropriate.
- tgt: convert libsrp to use scsi_data_buffer
- isd200: This driver still bangs on scsi_cmnd IO members,
so need changing
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
drivers/scsi/libsrp.c|4 +-
drivers/scsi/scsi.c |2 +-
drivers/scsi/scsi_error.c| 28 +-
drivers/scsi/scsi_lib.c | 79 --
drivers/scsi/sd.c|4 +-
drivers/scsi/sr.c| 25 +++--
drivers/usb/storage/isd200.c |8 ++--
include/scsi/scsi_cmnd.h | 39 +---
include/scsi/scsi_eh.h |8 ++---
include/scsi/scsi_host.h |4 +-
10 files changed, 92 insertions(+), 109 deletions(-)
diff --git a/drivers/scsi/libsrp.c b/drivers/scsi/libsrp.c
index 5cff020..8a8562a 100644
--- a/drivers/scsi/libsrp.c
+++ b/drivers/scsi/libsrp.c
@@ -426,8 +426,8 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd
*cmd, void *info,
sc->SCp.ptr = info;
memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
- sc->request_bufflen = len;
- sc->request_buffer = (void *) (unsigned long) addr;
+ sc->sdb.length = len;
+ sc->sdb.sglist = (void *) (unsigned long) addr;
sc->tag = tag;
err = scsi_tgt_queue_command(sc, itn_id, (struct scsi_lun *)&cmd->lun,
cmd->tag);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1929488..73d2216 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -698,7 +698,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
"Notifying upper driver of completion "
"(result %x)\n", cmd->result));
- good_bytes = cmd->request_bufflen;
+ good_bytes = scsi_bufflen(cmd);
if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
drv = scsi_cmd_to_driver(cmd);
if (drv->done)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ebaca4c..9daa983 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -617,29 +617,25 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct
scsi_eh_save *ses,
ses->cmd_len = scmd->cmd_len;
memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
ses->data_direction = scmd->sc_data_direction;
- ses->bufflen = scmd->request_bufflen;
- ses->buffer = scmd->request_buffer;
- ses->use_sg = scmd->use_sg;
- ses->resid = scmd->resid;
+ ses->sdb = scmd->sdb;
ses->result = scmd->result;
+ memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+
if (sense_bytes) {
- scmd->request_bufflen = min_t(unsigned,
+ scmd->sdb.length = min_t(unsigned,
sizeof(scmd->sense_buffer), sense_bytes);
sg_init_one(&ses->sense_sgl, scmd->sense_buffer,
- scmd->request_bufflen);
- scmd->request_buffer = &ses->sense_sgl;
+ scmd->sdb.length);
+ scmd->sdb.sglist = &ses->sense_sgl;
scmd->sc_data_direction = DMA_FROM_DEVICE;
- scmd->use_sg = 1;
+ scmd->sdb.sg_count = 1;
memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
scmd->cmnd[0] = REQUEST_SENSE;
- scmd->cmnd[4] = scmd->request_bufflen;
+ scmd->cmnd[4] = scmd->sdb.length;
scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
} else {
- scmd->request_buffer = NULL;
- scmd->request_bufflen = 0;
scmd->sc_data_direction = DMA_NONE;
Re: [PATCH 3/4] scsi_data_buffer
On Tue, Nov 13 2007 at 8:40 +0200, FUJITA Tomonori <[EMAIL PROTECTED]> wrote: > On Mon, 12 Nov 2007 22:06:52 -0800 > Andrew Morton <[EMAIL PROTECTED]> wrote: > >> On Thu, 08 Nov 2007 18:59:30 +0200 Boaz Harrosh <[EMAIL PROTECTED]> wrote: >> >>> In preparation for bidi we abstract all IO members of scsi_cmnd, >>> that will need to duplicate, into a substructure. >>> >>> - Group all IO members of scsi_cmnd into a scsi_data_buffer >>> structure. >> drivers/scsi/qla1280.c: In function 'qla1280_done': >> drivers/scsi/qla1280.c:1313: error: 'struct scsi_cmnd' has no member named >> 'use_sg' >> drivers/scsi/qla1280.c:1314: error: 'struct scsi_cmnd' has no member named >> 'request_buffer' >> drivers/scsi/qla1280.c:1315: error: 'struct scsi_cmnd' has no member named >> 'use_sg' >> drivers/scsi/qla1280.c:1316: error: 'struct scsi_cmnd' has no member named >> 'request_bufflen' >> drivers/scsi/qla1280.c:1318: error: 'struct scsi_cmnd' has no member named >> 'request_bufflen' >> drivers/scsi/qla1280.c: In function 'qla1280_return_status': >> drivers/scsi/qla1280.c:1409: error: 'struct scsi_cmnd' has no member named >> 'request_bufflen' >> drivers/scsi/qla1280.c:1416: error: 'struct scsi_cmnd' has no member named >> 'resid' >> drivers/scsi/qla1280.c: In function 'qla1280_64bit_start_scsi': >> drivers/scsi/qla1280.c:2791: error: 'struct scsi_cmnd' has no member named >> 'use_sg' >> drivers/scsi/qla1280.c:2792: error: 'struct scsi_cmnd' has no member named >> 'request_buffer' >> drivers/scsi/qla1280.c:2793: error: 'struct scsi_cmnd' has no member named >> 'use_sg' >> drivers/scsi/qla1280.c:2801: error: 'struct scsi_cmnd' has no member named >> 'request_bufflen' >> drivers/scsi/qla1280.c:2896: error: 'struct scsi_cmnd' has no member named >> 'use_sg' >> drivers/scsi/qla1280.c:2991: error: 'struct scsi_cmnd' has no member named >> 'request_buffer' >> drivers/scsi/qla1280.c:2992: error: 'struct scsi_cmnd' has no member named >> 'request_bufflen' >> drivers/scsi/qla1280.c:3004: error: 'struct scsi_cmnd' has no member named >> 'request_bufflen' >> make[2]: *** [drivers/scsi/qla1280.o] Error 1 >> >> It mystfies me how a patch like this can have been floating about in N >> submissions across M months and nobody has done an allmodconfig build or >> even a grep to find out what broke. >> >> ho hum. I shall mark qla1280 BROKEN and shall plod onwards. > > A patch to fix this is in James' scsi-pending tree. Jes tested and > fixed it (thanks !) so it will go to -mm via scsi-misc soon. > > Boaz, it's better to send major scsi patches to -mm via scsi-misc to > avoid problems like this. > > > By the way, Andrew, can you add the following patchset to -mm? > > http://lkml.org/lkml/2007/10/24/138 > > It fixes the IOMMUs' problem to merge scatter/gather segments without > considering LLDs' restrictions. > > > Thanks, I was surprised that the patches reached -mm. My bad, I CCed Morton because I thought that they should sit in mm early for collecting complains, but I meant them to go through scsi-misc. scsi-pending is patches that need approval from maintainers, but that James thought they are ready. I was sure it is included in -mm, I should have checked. The scsi_data_buffer patch must wait behind the patches in scsi-pending and behind the patch I sent for removal of the NCR53C9x/esp family of drivers. http://www.spinics.net/lists/linux-scsi/msg20914.html James please Submit this patch. At least to scsi-pending I have sent it to all the registered maintainers and mailing lists. I have got no response ether way. >From what Christoph said the only way we can get their comments, if any, is by removing the drivers. The only 2 active people (CCed) with interest in this HW, are working on the new driver set, and it should be easy to convert any esp HW to the new driver set. Sorry for the mess Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH rebased] tgt: fix build when dprintk is defined
From: Randy Dunlap <[EMAIL PROTECTED]>
Fix scsi_tgt_lib build when dprintk is defined:
drivers/scsi/scsi_tgt_lib.c: In function 'scsi_tgt_cmd_destroy':
drivers/scsi/scsi_tgt_lib.c:183: warning: format '%lu' expects type 'long
unsigned int', but argument 6 has type 'unsigned int'
drivers/scsi/scsi_tgt_lib.c: In function 'scsi_tgt_cmd_done':
drivers/scsi/scsi_tgt_lib.c:330: warning: format '%lu' expects type 'long
unsigned int', but argument 5 has type 'unsigned int'
drivers/scsi/scsi_tgt_lib.c: In function 'scsi_tgt_transfer_response':
drivers/scsi/scsi_tgt_lib.c:345: warning: format '%lu' expects type 'long
unsigned int', but argument 5 has type 'unsigned int'
drivers/scsi/scsi_tgt_lib.c: In function 'scsi_tgt_init_cmd':
drivers/scsi/scsi_tgt_lib.c:368: warning: format '%lu' expects type 'long
unsigned int', but argument 6 has type 'unsigned int'
drivers/scsi/scsi_tgt_lib.c: In function 'scsi_tgt_kspace_exec':
drivers/scsi/scsi_tgt_lib.c:499: warning: format '%lu' expects type 'long
unsigned int', but argument 9 has type 'unsigned int'
drivers/scsi/scsi_tgt_lib.c: In function 'scsi_tgt_kspace_it_nexus_rsp':
drivers/scsi/scsi_tgt_lib.c:620: error: 'mid' undeclared (first use in this
function)
drivers/scsi/scsi_tgt_lib.c:620: error: (Each undeclared identifier is reported
only once
drivers/scsi/scsi_tgt_lib.c:620: error: for each function it appears in.)
make[2]: *** [drivers/scsi/scsi_tgt_lib.o] Error 1
Signed-off-by: Randy Dunlap <[EMAIL PROTECTED]>
---
drivers/scsi/scsi_tgt_lib.c | 10 +-
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index 5fd5fca..fad03a4 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -180,7 +180,7 @@ static void scsi_tgt_cmd_destroy(struct work_struct *work)
container_of(work, struct scsi_tgt_cmd, work);
struct scsi_cmnd *cmd = tcmd->rq->special;
- dprintk("cmd %p %d %lu\n", cmd, cmd->sc_data_direction,
+ dprintk("cmd %p %d %u\n", cmd, cmd->sc_data_direction,
rq_data_dir(cmd->request));
scsi_unmap_user_pages(tcmd);
scsi_host_put_command(scsi_tgt_cmd_to_host(cmd), cmd);
@@ -327,7 +327,7 @@ static void scsi_tgt_cmd_done(struct scsi_cmnd *cmd)
{
struct scsi_tgt_cmd *tcmd = cmd->request->end_io_data;
- dprintk("cmd %p %lu\n", cmd, rq_data_dir(cmd->request));
+ dprintk("cmd %p %u\n", cmd, rq_data_dir(cmd->request));
scsi_tgt_uspace_send_status(cmd, tcmd->itn_id, tcmd->tag);
@@ -341,7 +341,7 @@ static int scsi_tgt_transfer_response(struct scsi_cmnd *cmd)
struct Scsi_Host *shost = scsi_tgt_cmd_to_host(cmd);
int err;
- dprintk("cmd %p %lu\n", cmd, rq_data_dir(cmd->request));
+ dprintk("cmd %p %u\n", cmd, rq_data_dir(cmd->request));
err = shost->hostt->transfer_response(cmd, scsi_tgt_cmd_done);
switch (err) {
@@ -473,7 +473,7 @@ int scsi_tgt_kspace_exec(int host_no, u64 itn_id, int
result, u64 tag,
}
cmd = rq->special;
- dprintk("cmd %p scb %x result %d len %d bufflen %u %lu %x\n",
+ dprintk("cmd %p scb %x result %d len %d bufflen %u %u %x\n",
cmd, cmd->cmnd[0], result, len, cmd->request_bufflen,
rq_data_dir(rq), cmd->cmnd[0]);
@@ -594,7 +594,7 @@ int scsi_tgt_kspace_it_nexus_rsp(int host_no, u64 itn_id,
int result)
struct Scsi_Host *shost;
int err = -EINVAL;
- dprintk("%d %d %llx\n", host_no, result, (unsigned long long) mid);
+ dprintk("%d %d\n", host_no, result);
shost = scsi_host_lookup(host_no);
if (IS_ERR(shost)) {
--
1.5.3.1
-
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 rebased ver2] tgt: fix build when dprintk is defined
From: Randy Dunlap <[EMAIL PROTECTED]>
Fix scsi_tgt_lib build when dprintk is defined:
Also fix accessors problem when dprintk is defined
drivers/scsi/scsi_tgt_lib.c: In function 'scsi_tgt_cmd_destroy':
drivers/scsi/scsi_tgt_lib.c:183: warning: format '%lu' expects type 'long
unsigned int', but argument 6 has type 'unsigned int'
drivers/scsi/scsi_tgt_lib.c: In function 'scsi_tgt_cmd_done':
drivers/scsi/scsi_tgt_lib.c:330: warning: format '%lu' expects type 'long
unsigned int', but argument 5 has type 'unsigned int'
drivers/scsi/scsi_tgt_lib.c: In function 'scsi_tgt_transfer_response':
drivers/scsi/scsi_tgt_lib.c:345: warning: format '%lu' expects type 'long
unsigned int', but argument 5 has type 'unsigned int'
drivers/scsi/scsi_tgt_lib.c: In function 'scsi_tgt_init_cmd':
drivers/scsi/scsi_tgt_lib.c:368: warning: format '%lu' expects type 'long
unsigned int', but argument 6 has type 'unsigned int'
drivers/scsi/scsi_tgt_lib.c: In function 'scsi_tgt_kspace_exec':
drivers/scsi/scsi_tgt_lib.c:499: warning: format '%lu' expects type 'long
unsigned int', but argument 9 has type 'unsigned int'
drivers/scsi/scsi_tgt_lib.c: In function 'scsi_tgt_kspace_it_nexus_rsp':
drivers/scsi/scsi_tgt_lib.c:620: error: 'mid' undeclared (first use in this
function)
drivers/scsi/scsi_tgt_lib.c:620: error: (Each undeclared identifier is reported
only once
drivers/scsi/scsi_tgt_lib.c:620: error: for each function it appears in.)
make[2]: *** [drivers/scsi/scsi_tgt_lib.o] Error 1
Signed-off-by: Randy Dunlap <[EMAIL PROTECTED]>
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/scsi_tgt_lib.c | 12 ++--
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index 5fd5fca..746f1a1 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -180,7 +180,7 @@ static void scsi_tgt_cmd_destroy(struct work_struct *work)
container_of(work, struct scsi_tgt_cmd, work);
struct scsi_cmnd *cmd = tcmd->rq->special;
- dprintk("cmd %p %d %lu\n", cmd, cmd->sc_data_direction,
+ dprintk("cmd %p %d %u\n", cmd, cmd->sc_data_direction,
rq_data_dir(cmd->request));
scsi_unmap_user_pages(tcmd);
scsi_host_put_command(scsi_tgt_cmd_to_host(cmd), cmd);
@@ -327,7 +327,7 @@ static void scsi_tgt_cmd_done(struct scsi_cmnd *cmd)
{
struct scsi_tgt_cmd *tcmd = cmd->request->end_io_data;
- dprintk("cmd %p %lu\n", cmd, rq_data_dir(cmd->request));
+ dprintk("cmd %p %u\n", cmd, rq_data_dir(cmd->request));
scsi_tgt_uspace_send_status(cmd, tcmd->itn_id, tcmd->tag);
@@ -341,7 +341,7 @@ static int scsi_tgt_transfer_response(struct scsi_cmnd *cmd)
struct Scsi_Host *shost = scsi_tgt_cmd_to_host(cmd);
int err;
- dprintk("cmd %p %lu\n", cmd, rq_data_dir(cmd->request));
+ dprintk("cmd %p %u\n", cmd, rq_data_dir(cmd->request));
err = shost->hostt->transfer_response(cmd, scsi_tgt_cmd_done);
switch (err) {
@@ -473,8 +473,8 @@ int scsi_tgt_kspace_exec(int host_no, u64 itn_id, int
result, u64 tag,
}
cmd = rq->special;
- dprintk("cmd %p scb %x result %d len %d bufflen %u %lu %x\n",
- cmd, cmd->cmnd[0], result, len, cmd->request_bufflen,
+ dprintk("cmd %p scb %x result %d len %d bufflen %u %u %x\n",
+ cmd, cmd->cmnd[0], result, len, scsi_bufflen(cmd),
rq_data_dir(rq), cmd->cmnd[0]);
if (result == TASK_ABORTED) {
@@ -594,7 +594,7 @@ int scsi_tgt_kspace_it_nexus_rsp(int host_no, u64 itn_id,
int result)
struct Scsi_Host *shost;
int err = -EINVAL;
- dprintk("%d %d %llx\n", host_no, result, (unsigned long long) mid);
+ dprintk("%d %d\n", host_no, result);
shost = scsi_host_lookup(host_no);
if (IS_ERR(shost)) {
--
1.5.3.1
-
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: [Linux-usb-users] Read errors on Flash Drive Transcend TS1GJF2A
On Mon, Nov 26 2007 at 17:35 +0200, Alan Stern <[EMAIL PROTECTED]> wrote: > Not all devices correctly report the error-causing LBA in the > Information field of their sense data -- even when they set the Valid > bit. This patch (as1019) makes sd much more cautious about accepting > the reported LBA. If the value isn't within the range of blocks > accessed during the I/O operation it is rejected, and instead the > driver will try looking at the residue field (which currently it > ignores completely). > > This fixes a data-corruption bug reported here: > > http://marc.info/?t=11874576475&r=1&w=2 > > Signed-off-by: Alan Stern <[EMAIL PROTECTED]> > CC: Luben Tuikov <[EMAIL PROTECTED]> > > --- > > This patch should be considered for inclusion in 2.6.24. The bug in > question has always existed, as far as I know, but before 2.6.18 it was > masked by a different bug. > > This doesn't use the new SCSI accessors. In the development trees > I've seen, those accessors haven't yet been imported into sd.c. If > the patch needs to be rebased, please let me know where to find the > current sd source. > It's currently only in mm patchset has: bidi-support-sr-sd-remove-dead-code.patch && bidi-support-scsi_data_buffer.patch > Presumably sr should use the same algorithm. That's grist for another > patch. > > > Index: usb-2.6/drivers/scsi/sd.c > === > --- usb-2.6.orig/drivers/scsi/sd.c > +++ usb-2.6/drivers/scsi/sd.c > @@ -968,7 +968,17 @@ static int sd_done(struct scsi_cmnd *SCp If this is a bugfix for 2.6.24 than I will be the one to rebase, as scsi_data_buffer is only for 2.6.25, I hope ;) Andrew once above goes into scsi-rc-fixes or scsi-misc I will send a rebase, if I've fallen asleep please bang me on the head, thanks. Alen thanks for doing this It was on my must-investigate list. (Though I'm usually not using sd) Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 2.6.24-rc3-mm2: Result: hostbyte=0x01 driverbyte=0x00\nend_request: I/O error
On Thu, Nov 29 2007 at 1:36 +0200, Andrew Morton <[EMAIL PROTECTED]> wrote:
> On Wed, 28 Nov 2007 16:14:21 -0700
> Matthew Wilcox <[EMAIL PROTECTED]> wrote:
>
>> On Wed, Nov 28, 2007 at 01:40:36PM -0800, Andrew Morton wrote:
>>> On Wed, 28 Nov 2007 23:01:31 +0300
>>> Alexey Dobriyan <[EMAIL PROTECTED]> wrote:
>>>
Reliably spams dmesg with end_request() horrors. This happens when git
starts checking out linux tree to fresh ext2 partition. Disk is several
month old and there were no prolems with, say, 2.6.24-rc3:
>> Could you try reverting 6f5391c283d7fdcf24bf40786ea79061919d1e1d and see
>> if the problem still exists?
>>
>
> That's not completely trivial..
>
> I did a hand-made revert against 2.6.24-rc3-mm2 (below) but some other patch
> in there causes:
>
> drivers/scsi/scsi_lib.c: In function 'scsi_blk_pc_done':
> drivers/scsi/scsi_lib.c:1251: error: 'struct scsi_cmnd' has no member named
> 'request_bufflen'
>
That would be the bidi patches. You need to use scsi_bufflen(cmd) instead of
cmd->request_bufflen. (See below)
Do you need that I send a patch?
>
> --- a/drivers/scsi/scsi.c~revert-6f5391c283d7fdcf24bf40786ea79061919d1e1d
> +++ a/drivers/scsi/scsi.c
> @@ -59,7 +59,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -379,8 +378,9 @@ void scsi_log_send(struct scsi_cmnd *cmd
> scsi_print_command(cmd);
> if (level > 3) {
> printk(KERN_INFO "buffer = 0x%p, bufflen = %d,"
> -" queuecommand 0x%p\n",
> +" done = 0x%p, queuecommand 0x%p\n",
> scsi_sglist(cmd), scsi_bufflen(cmd),
> + cmd->done,
> cmd->device->host->hostt->queuecommand);
>
> }
> @@ -667,12 +667,6 @@ void __scsi_done(struct scsi_cmnd *cmd)
> blk_complete_request(rq);
> }
>
> -/* Move this to a header if it becomes more generally useful */
> -static struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
> -{
> - return *(struct scsi_driver **)cmd->request->rq_disk->private_data;
> -}
> -
> /**
> * scsi_finish_command - cleanup and pass command back to upper layer
> * @cmd: the command
> @@ -685,8 +679,6 @@ void scsi_finish_command(struct scsi_cmn
> {
> struct scsi_device *sdev = cmd->device;
> struct Scsi_Host *shost = sdev->host;
> - struct scsi_driver *drv;
> - unsigned int good_bytes;
>
> scsi_device_unbusy(sdev);
>
> @@ -712,13 +704,7 @@ void scsi_finish_command(struct scsi_cmn
> "Notifying upper driver of completion "
> "(result %x)\n", cmd->result));
>
> - good_bytes = scsi_bufflen(cmd);
> -if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
> - drv = scsi_cmd_to_driver(cmd);
> - if (drv->done)
> - good_bytes = drv->done(cmd);
> - }
> - scsi_io_completion(cmd, good_bytes);
> + cmd->done(cmd);
> }
> EXPORT_SYMBOL(scsi_finish_command);
>
> diff -puN
> drivers/scsi/scsi_error.c~revert-6f5391c283d7fdcf24bf40786ea79061919d1e1d
> drivers/scsi/scsi_error.c
> ---
> a/drivers/scsi/scsi_error.c~revert-6f5391c283d7fdcf24bf40786ea79061919d1e1d
> +++ a/drivers/scsi/scsi_error.c
> @@ -1697,6 +1697,7 @@ scsi_reset_provider(struct scsi_device *
>
> scmd->scsi_done = scsi_reset_provider_done_command;
> memset(&scmd->sdb, 0, sizeof(scmd->sdb));
> + scmd->done = NULL;
>
> scmd->cmd_len = 0;
>
> diff -puN
> drivers/scsi/scsi_lib.c~revert-6f5391c283d7fdcf24bf40786ea79061919d1e1d
> drivers/scsi/scsi_lib.c
> --- a/drivers/scsi/scsi_lib.c~revert-6f5391c283d7fdcf24bf40786ea79061919d1e1d
> +++ a/drivers/scsi/scsi_lib.c
> @@ -944,6 +944,7 @@ void scsi_end_bidi_request(struct scsi_c
>
> scsi_finalize_request(cmd, 1);
> }
> +EXPORT_SYMBOL(scsi_io_completion);
>
> /*
> * Function:scsi_io_completion()
> @@ -1238,6 +1239,18 @@ static struct scsi_cmnd *scsi_get_cmd_fr
> return cmd;
> }
>
> +static void scsi_blk_pc_done(struct scsi_cmnd *cmd)
> +{
> + BUG_ON(!blk_pc_request(cmd->request));
> + /*
> + * This will complete the whole command with uptodate=1 so
> + * as far as the block layer is concerned the command completed
> + * successfully. Since this is a REQ_BLOCK_PC command the
> + * caller should check the request's errors value
> + */
> + scsi_io_completion(cmd, cmd->request_bufflen);
+ scsi_io_completion(cmd, scsi_bufflen(cmd));
> +}
> +
> int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
> {
> struct scsi_cmnd *cmd;
> @@ -1285,6 +1298,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_d
> cmd->transfersize = req->data_len;
> cmd->allowed = req
Re: [PATCH 01/28] blk_end_request: add new request completion interface (take 3)
On Sat, Dec 01 2007 at 1:24 +0200, Kiyoshi Ueda <[EMAIL PROTECTED]> wrote:
> This patch adds 2 new interfaces for request completion:
> o blk_end_request() : called without queue lock
> o __blk_end_request() : called with queue lock held
>
> Some device drivers call some generic functions below between
> end_that_request_{first/chunk} and end_that_request_last().
> o add_disk_randomness()
> o blk_queue_end_tag()
> o blkdev_dequeue_request()
> These are called in the blk_end_request() as a part of generic
> request completion.
> So all device drivers become to call above functions.
>
> "Normal" drivers can be converted to use blk_end_request()
> in a standard way shown below.
>
> a) end_that_request_{chunk/first}
> spin_lock_irqsave()
> (add_disk_randomness(), blk_queue_end_tag(), blkdev_dequeue_request())
> end_that_request_last()
> spin_unlock_irqrestore()
> => blk_end_request()
>
> b) spin_lock_irqsave()
> end_that_request_{chunk/first}
> (add_disk_randomness(), blk_queue_end_tag(), blkdev_dequeue_request())
> end_that_request_last()
> spin_unlock_irqrestore()
> => spin_lock_irqsave()
>__blk_end_request()
>spin_unlock_irqsave()
>
> c) end_that_request_last()
> => __blk_end_request()
>
> Signed-off-by: Kiyoshi Ueda <[EMAIL PROTECTED]>
> Signed-off-by: Jun'ichi Nomura <[EMAIL PROTECTED]>
comments in body
> ---
> block/ll_rw_blk.c | 67
> +
> include/linux/blkdev.h |2 +
> 2 files changed, 69 insertions(+)
>
> Index: 2.6.24-rc3-mm2/block/ll_rw_blk.c
> ===
> --- 2.6.24-rc3-mm2.orig/block/ll_rw_blk.c
> +++ 2.6.24-rc3-mm2/block/ll_rw_blk.c
> @@ -3769,6 +3769,73 @@ void end_request(struct request *req, in
> }
> EXPORT_SYMBOL(end_request);
>
> +static void complete_request(struct request *rq, int uptodate)
> +{
> + if (blk_rq_tagged(rq))
> + blk_queue_end_tag(rq->q, rq);
> +
> + /* rq->queuelist of dequeued request should be list_empty() */
> + if (!list_empty(&rq->queuelist))
> + blkdev_dequeue_request(rq);
> +
> + end_that_request_last(rq, uptodate);
> +}
> +
> +/**
> + * blk_end_request - Helper function for drivers to complete the request.
> + * @rq: the request being processed
> + * @uptodate: 1 for success, 0 for I/O error, < 0 for specific error
> + * @nr_bytes: number of bytes to complete
> + *
> + * Description:
> + * Ends I/O on a number of bytes attached to @rq.
> + * If @rq has leftover, sets it up for the next range of segments.
> + *
> + * Return:
> + * 0 - we are done with this request
> + * 1 - still buffers pending for this request
> + **/
> +int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
I always hated that uptodate boolean with possible negative error value.
I guess it was done for backward compatibility of then users of
end_that_request_first(). But since you are introducing a new API then
this is not the case. Just have regular status int where 0 means ALL_OK
and negative value means error code.
Just my $0.02.
> +{
> + struct request_queue *q = rq->q;
> + unsigned long flags = 0UL;
> +
> + if (blk_fs_request(rq) || blk_pc_request(rq)) {
> + if (__end_that_request_first(rq, uptodate, nr_bytes))
> + return 1;
> + }
> +
> + add_disk_randomness(rq->rq_disk);
> +
> + spin_lock_irqsave(q->queue_lock, flags);
> + complete_request(rq, uptodate);
> + spin_unlock_irqrestore(q->queue_lock, flags);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(blk_end_request);
> +
> +/**
> + * __blk_end_request - Helper function for drivers to complete the request.
> + *
> + * Description:
> + * Must be called with queue lock held unlike blk_end_request().
> + **/
> +int __blk_end_request(struct request *rq, int uptodate, int nr_bytes)
> +{
> + if (blk_fs_request(rq) || blk_pc_request(rq)) {
> + if (__end_that_request_first(rq, uptodate, nr_bytes))
> + return 1;
> + }
> +
> + add_disk_randomness(rq->rq_disk);
> +
> + complete_request(rq, uptodate);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(__blk_end_request);
> +
> static void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
> struct bio *bio)
> {
> Index: 2.6.24-rc3-mm2/include/linux/blkdev.h
> ===
> --- 2.6.24-rc3-mm2.orig/include/linux/blkdev.h
> +++ 2.6.24-rc3-mm2/include/linux/blkdev.h
> @@ -725,6 +725,8 @@ static inline void blk_run_address_space
> * for parts of the original function. This prevents
> * code duplication in drivers.
> */
> +extern int blk_end_request(struct request *rq, int uptodate, int nr_bytes);
> +extern int __blk_end_request(struct request *rq, int uptodate, int nr_bytes);
> extern int end_that_request_first(struct re
Re: [PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)
On Sat, Dec 01 2007 at 1:35 +0200, Kiyoshi Ueda <[EMAIL PROTECTED]> wrote:
> This patch converts bidi of scsi mid-layer to use blk_end_request().
>
> rq->next_rq represents a pair of bidi requests.
> (There are no other use of 'next_rq' of struct request.)
> For both requests in the pair, end_that_request_chunk() should be
> called before end_that_request_last() is called for one of them.
> Since the calls to end_that_request_first()/chunk() and
> end_that_request_last() are packaged into blk_end_request(),
> the handling of next_rq completion has to be moved into
> blk_end_request(), too.
>
> Bidi sets its specific value to rq->data_len before the request is
> completed so that upper-layer can read it.
> This setting must be between end_that_request_chunk() and
> end_that_request_last(), because rq->data_len may be used
> in end_that_request_chunk() by blk_trace and so on.
> To satisfy the requirement, use blk_end_request_callback() which
> is added in PATCH 25 only for the tricky drivers.
>
> If bidi didn't reuse rq->data_len and added new members to request
> for the specific value, it could set before end_that_request_chunk()
> and use the standard blk_end_request() like below.
>
> void scsi_end_bidi_request(struct scsi_cmnd *cmd)
> {
> struct request *req = cmd->request;
>
> rq->resid = scsi_out(cmd)->resid;
> rq->next_rq->resid = scsi_in(cmd)->resid;
>
> if (blk_end_request(req, 1, req->data_len))
> BUG();
>
> scsi_release_buffers(cmd);
> scsi_next_command(cmd);
> }
>
> Signed-off-by: Kiyoshi Ueda <[EMAIL PROTECTED]>
> Signed-off-by: Jun'ichi Nomura <[EMAIL PROTECTED]>
> ---
> block/ll_rw_blk.c | 18 +
> drivers/scsi/scsi_lib.c | 66
>
> 2 files changed, 52 insertions(+), 32 deletions(-)
>
> Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> ===
> --- 2.6.24-rc3-mm2.orig/drivers/scsi/scsi_lib.c
> +++ 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> @@ -629,28 +629,6 @@ void scsi_run_host_queues(struct Scsi_Ho
> scsi_run_queue(sdev->request_queue);
> }
>
> -static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
> -{
> - struct request_queue *q = cmd->device->request_queue;
> - struct request *req = cmd->request;
> - unsigned long flags;
> -
> - add_disk_randomness(req->rq_disk);
> -
> - spin_lock_irqsave(q->queue_lock, flags);
> - if (blk_rq_tagged(req))
> - blk_queue_end_tag(q, req);
> -
> - end_that_request_last(req, uptodate);
> - spin_unlock_irqrestore(q->queue_lock, flags);
> -
> - /*
> - * This will goose the queue request function at the end, so we don't
> - * need to worry about launching another command.
> - */
> - scsi_next_command(cmd);
> -}
> -
> /*
> * Function:scsi_end_request()
> *
> @@ -921,6 +899,20 @@ void scsi_release_buffers(struct scsi_cm
> EXPORT_SYMBOL(scsi_release_buffers);
>
> /*
> + * Called from blk_end_request_callback() after all DATA in rq and its
> next_rq
> + * are completed before rq is completed/freed.
> + */
> +static int scsi_end_bidi_request_cb(struct request *rq)
> +{
> + struct scsi_cmnd *cmd = rq->special;
> +
> + rq->data_len = scsi_out(cmd)->resid;
> + rq->next_rq->data_len = scsi_in(cmd)->resid;
> +
> + return 0;
> +}
> +
> +/*
> * Bidi commands Must be complete as a whole, both sides at once.
> * If part of the bytes were written and lld returned
> * scsi_in()->resid and/or scsi_out()->resid this information will be left
> @@ -931,22 +923,32 @@ void scsi_end_bidi_request(struct scsi_c
> {
> struct request *req = cmd->request;
>
> - end_that_request_chunk(req, 1, req->data_len);
> - req->data_len = scsi_out(cmd)->resid;
> -
> - end_that_request_chunk(req->next_rq, 1, req->next_rq->data_len);
> - req->next_rq->data_len = scsi_in(cmd)->resid;
> -
> - scsi_release_buffers(cmd);
> -
> /*
>*FIXME: If ll_rw_blk.c is changed to also put_request(req->next_rq)
> - * in end_that_request_last() then this WARN_ON must be removed.
> + * in blk_end_request() then this WARN_ON must be removed.
>* for now, upper-driver must have registered an end_io.
>*/
> WARN_ON(!req->end_io);
>
> - scsi_finalize_request(cmd, 1);
> + /*
> + * blk_end_request() family take care of data completion of next_rq.
> + *
> + * req->data_len and req->next_rq->data_len must be set after
> + * all data are completed, since they may be referenced during
> + * the data completion process.
> + * So use the callback feature of blk_end_request() here.
> + *
> + * NOTE: If bidi doesn't reuse the data_len field for upper-layer's
> + * reference (e.g. adds new members for it to struct request),
> + * we can use the standard blk_end
Re: [PATCH 00/28] blk_end_request: full I/O completion handler (take 3)
On Tue, Dec 04 2007 at 14:16 +0200, Jens Axboe <[EMAIL PROTECTED]> wrote: > On Fri, Nov 30 2007, Kiyoshi Ueda wrote: >> Hello Jens, >> >> The following is the updated patch-set for blk_end_request(). >> Changes since the last version are only minor updates to catch up >> with the base kernel changes. >> Do you agree the implementation of blk_end_request()? >> If there's no problem, could you merge it to your tree? >> Or does it have to be merged to -mm tree first? >> >> >> Boaz, >> Could you review the newly added PATCH 27 which converts the bidi part, >> and give me your comments? >> It uses blk_end_request_callback() in PATCH 25, which was only for >> the tricky ide-cd driver. >> If bidi added a 'resid' member to struct request instead of reusing >> 'data_len' for the other purpose, it could use the standard >> blk_end_request() instead. >> >> -- Changes from the previous post - >> Changes between take2 and take3: >> o Rebased on top of 2.6.24-rc3-mm2 > > OK, so this means that I can't apply it unfortunately. It depends on > other patches in -mm (bidi). > > SCSI sits on block, so the best approach imho is to base this patchset > on mainline so I can include the block bits. > > I was wishing that the bidi work can go into 2.6.25, I guess that's James to say. If so than it is not important what order they go in. Or the patchset can be submitted in two parts, with scsi and remove-of- old-API in a later stage. Or *rant* Boaz can just rebase his work again *rant*. Kiyoshi, It's OK, if you have a maintainer that is willing to accept your work then go head, My code can wait, no problem. Just resolve the resid issue in scsi_io_completion() (See my other mail) Thanks for doing this Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linux-usb-users] Read errors on Flash Drive Transcend TS1GJF2A
On Mon, Nov 26 2007 at 17:35 +0200, Alan Stern <[EMAIL PROTECTED]> wrote:
> Not all devices correctly report the error-causing LBA in the
> Information field of their sense data -- even when they set the Valid
> bit. This patch (as1019) makes sd much more cautious about accepting
> the reported LBA. If the value isn't within the range of blocks
> accessed during the I/O operation it is rejected, and instead the
> driver will try looking at the residue field (which currently it
> ignores completely).
>
> This fixes a data-corruption bug reported here:
>
> http://marc.info/?t=11874576475&r=1&w=2
>
> Signed-off-by: Alan Stern <[EMAIL PROTECTED]>
> CC: Luben Tuikov <[EMAIL PROTECTED]>
>
> ---
>
> This patch should be considered for inclusion in 2.6.24. The bug in
> question has always existed, as far as I know, but before 2.6.18 it was
> masked by a different bug.
>
> This doesn't use the new SCSI accessors. In the development trees
> I've seen, those accessors haven't yet been imported into sd.c. If
> the patch needs to be rebased, please let me know where to find the
> current sd source.
>
> Presumably sr should use the same algorithm. That's grist for another
> patch.
>
>
> Index: usb-2.6/drivers/scsi/sd.c
> ===
> --- usb-2.6.orig/drivers/scsi/sd.c
> +++ usb-2.6/drivers/scsi/sd.c
> @@ -968,7 +968,17 @@ static int sd_done(struct scsi_cmnd *SCp
> /* This computation should always be done in terms of
>* the resolution of the device's medium.
>*/
> - good_bytes = (bad_lba - start_lba)*SCpnt->device->sector_size;
> + if (start_lba <= bad_lba && bad_lba < start_lba +
> + (xfer_size / SCpnt->device->sector_size))
> + good_bytes = SCpnt->device->sector_size *
> + (unsigned int) (bad_lba - start_lba);
> +
> + /* If the bad_lba value is no good, maybe the residue value
> + * is better.
> + */
> + else if (SCpnt->resid > 0 && SCpnt->resid < xfer_size)
> + good_bytes = (xfer_size - SCpnt->resid) &
> + (- SCpnt->device->sector_size);
> break;
> case RECOVERED_ERROR:
> case NO_SENSE:
>
> -
perhaps below hunk should be added to your patch.
Was it decided when this data corruption bugfix is
merged.
Also in sr.c. It does the range check but it might
enjoy the resid handling as well.
-
drivers/scsi/scsi.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 796c0bd..1a576bc 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -714,6 +714,8 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
good_bytes = cmd->request_bufflen;
if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+ if ( cmd->resid > 0 && cmd->resid < good_bytes)
+ good_bytes -= cmd->resid;
drv = scsi_cmd_to_driver(cmd);
if (drv->done)
good_bytes = drv->done(cmd);
-
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: [Linux-usb-users] Read errors on Flash Drive Transcend TS1GJF2A
On Tue, Dec 04 2007 at 20:03 +0200, Alan Stern <[EMAIL PROTECTED]> wrote:
> On Tue, 4 Dec 2007, Boaz Harrosh wrote:
>
>> perhaps below hunk should be added to your patch.
>
> It looks like a good idea.
>
>> Was it decided when this data corruption bugfix is
>> merged.
>
> I don't know -- I haven't heard anything back from James.
>
>> Also in sr.c. It does the range check but it might
>> enjoy the resid handling as well.
>
> I think the range checking in sr.c is completely wrong. The code
> doesn't check carefully to see that the error sector lies within the
> range of sectors being accessed; there's a possibility of overflow if
> the capacity is larger than 2**31 bytes. Also this line in particular
> makes no sense:
>
> error_sector &= ~(block_sectors - 1);
>
> Like you said, using the residue value too wouldn't hurt.
>
> Furthermore the check for the Valid bit is done wrongly:
>
> if (!(SCpnt->sense_buffer[0] & 0x90))
>
> This will never be true because of the earlier test:
>
> if (driver_byte(result) != 0 && /* An error occurred */
> (SCpnt->sense_buffer[0] & 0x7f) == 0x70) { /* Sense current */
>
> It probably should test against 0x80 instead of 0x90.
>
> Alan Stern
>
Hi Alen
Yes, I have not inspected sr.c very carefully, you are absolutely right.
Could you submit a unified patch for sd, sr and scsi.c I have hit this
bug 2 in my error injection tests. I was doing sg_chaining tests and now
with the possibly very large requests the problem gets worse. At the time,
I could not identify the exact problem, thanks
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)
On Thu, Dec 06 2007 at 2:26 +0200, Kiyoshi Ueda <[EMAIL PROTECTED]> wrote:
> Hi Boaz,
>
> On Tue, 04 Dec 2007 15:39:12 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>> On Sat, Dec 01 2007 at 1:35 +0200, Kiyoshi Ueda <[EMAIL PROTECTED]> wrote:
>>> This patch converts bidi of scsi mid-layer to use blk_end_request().
>>>
>>> rq->next_rq represents a pair of bidi requests.
>>> (There are no other use of 'next_rq' of struct request.)
>>> For both requests in the pair, end_that_request_chunk() should be
>>> called before end_that_request_last() is called for one of them.
>>> Since the calls to end_that_request_first()/chunk() and
>>> end_that_request_last() are packaged into blk_end_request(),
>>> the handling of next_rq completion has to be moved into
>>> blk_end_request(), too.
>>>
>>> Bidi sets its specific value to rq->data_len before the request is
>>> completed so that upper-layer can read it.
>>> This setting must be between end_that_request_chunk() and
>>> end_that_request_last(), because rq->data_len may be used
>>> in end_that_request_chunk() by blk_trace and so on.
>>> To satisfy the requirement, use blk_end_request_callback() which
>>> is added in PATCH 25 only for the tricky drivers.
>>>
>>> If bidi didn't reuse rq->data_len and added new members to request
>>> for the specific value, it could set before end_that_request_chunk()
>>> and use the standard blk_end_request() like below.
>>>
>>> void scsi_end_bidi_request(struct scsi_cmnd *cmd)
>>> {
>>> struct request *req = cmd->request;
>>>
>>> rq->resid = scsi_out(cmd)->resid;
>>> rq->next_rq->resid = scsi_in(cmd)->resid;
>>>
>>> if (blk_end_request(req, 1, req->data_len))
>>> BUG();
>>>
>>> scsi_release_buffers(cmd);
>>> scsi_next_command(cmd);
>>> }
> ...
> snip
> ...
>> rq->data_len = scsi_out(cmd)->resid is Not Just a problem of bidi
>> it is a General problem of scsi residual handling, and user code.
>>
>> Even today before any bidi. at scsi_lib.c at scsi_io_completion()
>> we do req->data_len = scsi_get_resid(cmd);
>> ( or: req->data_len = cmd->resid; depends which version you look)
>> And then call scsi_end_request() which calls __end_that_request_first/last
>> So it is assumed even today that req->data_len is not touched by
>> __end_that_request_first/last unless __end_that_request_first returned
>> that there is more work to do and the command is resubmitted in which
>> case the resid information is discarded.
>>
>> So if the regular resid handling is acceptable - Set req->data_len
>> before the call to __end_that_request_first/last, or blk_end_request()
>> in your case, then here goes your second client of the _callback and
>> it can be removed.
>> But if it is found that req->data_len is touched and the resid information
>> gets lost, than it should be fixed for the common uni-io case, by - for
>> example
>> - pass resid to the blk_end_request() function.
>> (So in any way the _callback can go)
>
> Thank you for the explanation of scsi's rq->data_len usage.
> I see that scsi usually uses rq->data_len for cmd->resid.
>
> I have investigated the possibility of setting data_len before
> the call to blk_end_request.
> But no matter whether data_len is touched or not, we need a callback
> for bidi. So I would like to go with the current patch.
>
> I explained the reason and some details below.
>
>
> As far as I can see, rq->data_len is just referenced
> by blk_add_trace_rq() in __end_that_request_first(), not modified.
> And I don't change any logic around there in the block-layer.
> So there shouldn't be any critical problem for scsi residual handing.
> (although I'm not sure that scsi expectes cmd->resid to be traced
> by blk_trace.)
>
> Anyway, I see that it is no critical problem for bidi to set cmd->resid
> to rq->data_len before blk_end_request() call.
> But if I do that, blk_end_request() can't get the next_rq's size
> to complete in its code below.
>
>> +/* Bidi request must be completed as a whole */
>> +if (blk_bidi_rq(rq) &&
>> +__end_that_request_first(rq->next_rq, uptodate,
>> + blk_rq_bytes(rq->next_rq)))
>> +return 1;
>
> So I will have to move next_rq compl
Re: [PATCH 27/28] blk_end_request: changing scsi mid-layer for bidi (take 3)
On Thu, Dec 06 2007 at 21:44 +0200, Kiyoshi Ueda <[EMAIL PROTECTED]> wrote:
> Hi Boaz, Jens,
>
> On Thu, 06 Dec 2007 11:24:44 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>>> Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
>> No I don't like it. The only client left for blk_end_request_callback()
>> is bidi,
>
> ide-cd (cdrom_newpc_intr) is another client.
> So I can't drop blk_end_request_callback() even if bidi doesn't use it.
>
It looks like all is needed for the ide-cd case is a flag that says
"don't complete the request". And the call-back is not actually used.
(Inspecting the last: [PATCH 26/28] blk_end_request: changing ide-cd (take 3))
The same exact flag could also help the bidi case. Perhaps have an API
for that?
>
> Index: 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> ===
> --- 2.6.24-rc3-mm2.orig/drivers/scsi/scsi_lib.c
> +++ 2.6.24-rc3-mm2/drivers/scsi/scsi_lib.c
> @@ -629,28 +629,6 @@ void scsi_run_host_queues(struct Scsi_Ho
> scsi_run_queue(sdev->request_queue);
> }
>
> -static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
> -{
> - struct request_queue *q = cmd->device->request_queue;
> - struct request *req = cmd->request;
> - unsigned long flags;
> -
> - add_disk_randomness(req->rq_disk);
> -
> - spin_lock_irqsave(q->queue_lock, flags);
> - if (blk_rq_tagged(req))
> - blk_queue_end_tag(q, req);
> -
> - end_that_request_last(req, uptodate);
> - spin_unlock_irqrestore(q->queue_lock, flags);
> -
> - /*
> - * This will goose the queue request function at the end, so we don't
> - * need to worry about launching another command.
> - */
> - scsi_next_command(cmd);
> -}
> -
> /*
> * Function:scsi_end_request()
> *
> @@ -930,23 +908,25 @@ EXPORT_SYMBOL(scsi_release_buffers);
> void scsi_end_bidi_request(struct scsi_cmnd *cmd)
> {
> struct request *req = cmd->request;
> + unsigned int dlen = req->data_len;
> + unsigned int next_dlen = req->next_rq->data_len;
>
> - end_that_request_chunk(req, 1, req->data_len);
> req->data_len = scsi_out(cmd)->resid;
> -
> - end_that_request_chunk(req->next_rq, 1, req->next_rq->data_len);
> req->next_rq->data_len = scsi_in(cmd)->resid;
>
> - scsi_release_buffers(cmd);
> -
> /*
>*FIXME: If ll_rw_blk.c is changed to also put_request(req->next_rq)
> - * in end_that_request_last() then this WARN_ON must be removed.
> + * in blk_end_bidi_request() then this WARN_ON must be removed.
>* for now, upper-driver must have registered an end_io.
>*/
> WARN_ON(!req->end_io);
>
> - scsi_finalize_request(cmd, 1);
> + if (blk_end_bidi_request(req, 1, dlen, next_dlen))
> + /* req has not been completed */
> + BUG();
> +
> + scsi_release_buffers(cmd);
> + scsi_next_command(cmd);
> }
>
> /*
> Index: 2.6.24-rc3-mm2/block/ll_rw_blk.c
> ===
> --- 2.6.24-rc3-mm2.orig/block/ll_rw_blk.c
> +++ 2.6.24-rc3-mm2/block/ll_rw_blk.c
> @@ -3792,24 +3792,17 @@ static void complete_request(struct requ
> if (!list_empty(&rq->queuelist))
> blkdev_dequeue_request(rq);
>
> + if (blk_bidi_rq(rq) && !rq->end_io)
> + __blk_put_request(rq->next_rq->q, rq->next_rq);
> +
> end_that_request_last(rq, uptodate);
> }
>
> -/**
> - * blk_end_request - Helper function for drivers to complete the request.
> - * @rq: the request being processed
> - * @uptodate: 1 for success, 0 for I/O error, < 0 for specific error
> - * @nr_bytes: number of bytes to complete
> - *
> - * Description:
> - * Ends I/O on a number of bytes attached to @rq.
> - * If @rq has leftover, sets it up for the next range of segments.
> - *
> - * Return:
> - * 0 - we are done with this request
> - * 1 - still buffers pending for this request
> - **/
> -int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
> +/*
> + * Internal function
> + */
> +static int blk_end_io(struct request *rq, int uptodate, int nr_bytes,
> + int bidi_bytes, int (drv_callback)(struct request *))
> {
> struct request_queue *q = rq->q;
> unsigned long flags = 0UL;
> @@ -3817,8 +3810,17 @@ int blk_end_request(struct request *rq,
> if (blk_fs_request(rq) || blk_pc_request(rq))
Re: [PATCH 20/30] blk_end_request: changing xsysace (take 4)
Kiyoshi Ueda wrote:
> This patch converts xsysace to use blk_end_request interfaces.
> Related 'uptodate' arguments are converted to 'error'.
>
> xsysace is a little bit different from "normal" drivers.
> xsysace driver has a state machine in it.
> It calls end_that_request_first() and end_that_request_last()
> from different states. (ACE_FSM_STATE_REQ_TRANSFER and
> ACE_FSM_STATE_REQ_COMPLETE, respectively.)
>
> However, those states are consecutive and without any interruption
> inbetween.
> So we can just follow the standard conversion rule (b) mentioned in
> the patch subject "[PATCH 01/30] blk_end_request: add new request
> completion interface".
>
> Cc: Grant Likely <[EMAIL PROTECTED]>
> Signed-off-by: Kiyoshi Ueda <[EMAIL PROTECTED]>
> Signed-off-by: Jun'ichi Nomura <[EMAIL PROTECTED]>
> ---
> drivers/block/xsysace.c |5 +
> 1 files changed, 1 insertion(+), 4 deletions(-)
>
> Index: 2.6.24-rc4/drivers/block/xsysace.c
> ===
> --- 2.6.24-rc4.orig/drivers/block/xsysace.c
> +++ 2.6.24-rc4/drivers/block/xsysace.c
> @@ -703,7 +703,7 @@ static void ace_fsm_dostate(struct ace_d
>
> /* bio finished; is there another one? */
> i = ace->req->current_nr_sectors;
> - if (end_that_request_first(ace->req, 1, i)) {
> + if (__blk_end_request(ace->req, 0, i)) {
end_that_request_first() took sectors __blk_end_request() now takes
bytes
> /* dev_dbg(ace->dev, "next block; h=%li c=%i\n",
>* ace->req->hard_nr_sectors,
>* ace->req->current_nr_sectors);
> @@ -718,9 +718,6 @@ static void ace_fsm_dostate(struct ace_d
> break;
>
> case ACE_FSM_STATE_REQ_COMPLETE:
> - /* Complete the block request */
> - blkdev_dequeue_request(ace->req);
> - end_that_request_last(ace->req, 1);
> ace->req = NULL;
>
> /* Finished request; go to idle state */
> -
> 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
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 2.6.24-rc3-mm1
On Tue, Dec 11 2007 at 18:33 +0200, James Bottomley <[EMAIL PROTECTED]> wrote:
> On Mon, 2007-11-26 at 22:15 -0800, Andrew Morton wrote:
>> OK, thanks. I'll assume that James and Hannes have this in hand (or will
>> have, by mid-week) and I won't do anything here.
>
> Just to confirm what I think I'm going to be doing: rebasing the
> scsi-misc tree to remove this commit:
>
> commit 8655a546c83fc43f0a73416bbd126d02de7ad6c0
> Author: Hannes Reinecke <[EMAIL PROTECTED]>
> Date: Tue Nov 6 09:23:40 2007 +0100
>
> [SCSI] Do not requeue requests if REQ_FAILFAST is set
>
> And its allied fix ups:
>
> commit 983289045faa96fba8841d3c51b98bb8623d9504
> Author: James Bottomley <[EMAIL PROTECTED]>
> Date: Sat Nov 24 19:47:25 2007 +0200
>
> [SCSI] fix up REQ_FASTFAIL not to fail when state is QUIESCE
>
> commit 9dd15a13b332e9f5c8ee752b1ccd9b84cb5bdf17
> Author: James Bottomley <[EMAIL PROTECTED]>
> Date: Sat Nov 24 19:55:53 2007 +0200
>
> [SCSI] fix domain validation to work again
>
> James
>
The problems caused by this patch where nagging me at the back of my head
from the begging. Why should we fail on a check of FAIL_FAST in all kind
of weird places like boots, when the only place that should ever set the
flag should be one of the multi-path drivers. finally it struck me:
It might be a bug in ll_rw_blk at blk_rq_bio_prep() there is this:
static void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
struct bio *bio)
{
/* first two bits are identical in rq->cmd_flags and bio->bi_rw */
rq->cmd_flags |= (bio->bi_rw & 3);
...
Now this is no longer true and is a bug.
Second bit of bio->bi_rw defined in bio.h is:
#define BIO_RW_AHEAD1
but
Second bit of rq->cmd_flags is __REQ_FAILFAST
so maybe we are getting FAILFAST in the wrong places?
(I will look for an old patch I sent a year ago that fixes
this bug)
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] REQ-flags to/from BIO-flags bugfix
- BIO flags bio->bi_rw and REQ flags req->cmd_flags no longer match.
Remove comments and do a proper translation between the 2 systems.
(Please look in ll_rw_blk.c/blk_rq_bio_prep() below if we need more flags)
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
block/ll_rw_blk.c| 23 +--
include/linux/blktrace_api.h |8 +++-
2 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index 8b91994..c6a84bb 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -1990,10 +1990,6 @@ blk_alloc_request(struct request_queue *q, int rw, int
priv, gfp_t gfp_mask)
if (!rq)
return NULL;
- /*
-* first three bits are identical in rq->cmd_flags and bio->bi_rw,
-* see bio.h and blkdev.h
-*/
rq->cmd_flags = rw | REQ_ALLOCED;
if (priv) {
@@ -3772,8 +3768,23 @@ EXPORT_SYMBOL(end_request);
static void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
struct bio *bio)
{
- /* first two bits are identical in rq->cmd_flags and bio->bi_rw */
- rq->cmd_flags |= (bio->bi_rw & 3);
+ if (bio_data_dir(bio))
+ rq->cmd_flags |= REQ_RW;
+ else
+ rq->cmd_flags &= ~REQ_RW;
+
+ if (bio->bi_rw & (1<cmd_flags |= REQ_RW_SYNC;
+ else
+ rq->cmd_flags &= ~REQ_RW_SYNC;
+ /* FIXME: what about other flags, should we sync these too? */
+ /*
+ BIO_RW_AHEAD==> ??
+ BIO_RW_BARRIER ==> REQ_SOFTBARRIER/REQ_HARDBARRIER
+ BIO_RW_FAILFAST ==> REQ_FAILFAST
+ BIO_RW_SYNC ==> REQ_RW_SYNC
+ BIO_RW_META ==> REQ_RW_META
+ */
rq->nr_phys_segments = bio_phys_segments(q, bio);
rq->nr_hw_segments = bio_hw_segments(q, bio);
diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index 7e11d23..9e7ce65 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -165,7 +165,13 @@ static inline void blk_add_trace_rq(struct request_queue
*q, struct request *rq,
u32 what)
{
struct blk_trace *bt = q->blk_trace;
- int rw = rq->cmd_flags & 0x03;
+ /* blktrace.c prints them according to bio flags */
+ int rw = (((rq_rw_dir(rq) == WRITE) << BIO_RW) |
+ (((rq->cmd_flags & (REQ_SOFTBARRIER|REQ_HARDBARRIER)) != 0) <<
+ BIO_RW_BARRIER) |
+ (((rq->cmd_flags & REQ_FAILFAST) != 0) << BIO_RW_FAILFAST) |
+ (((rq->cmd_flags & REQ_RW_SYNC) != 0) << BIO_RW_SYNC) |
+ (((rq->cmd_flags & REQ_RW_META) != 0) << BIO_RW_META));
if (likely(!bt))
return;
--
1.5.3.3
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] REQ-flags to/from BIO-flags bugfix
On Wed, Dec 12 2007 at 17:18 +0200, Matthew Wilcox <[EMAIL PROTECTED]> wrote:
> On Wed, Dec 12, 2007 at 01:03:10PM +0200, Boaz Harrosh wrote:
>> - BIO flags bio->bi_rw and REQ flags req->cmd_flags no longer match.
>>Remove comments and do a proper translation between the 2 systems.
>
> I'd rather see them resynchronised ... in a way that makes it obvious
> that they should be desynced again:
>
> I don't know whether BIO_RW_BARRIER is __REQ_SOFTBARRIER or
> __REQ_HARDBARRIER, so I didn't include that in this patch. There also
> doesn't seem to be a __REQ equivalent to BIO_RW_AHEAD, but we can do
> the other four bits (and leave gaps for those two).
>
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index d18ee67..6aef34b 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -167,11 +167,13 @@ enum {
> };
>
> /*
> - * request type modified bits. first three bits match BIO_RW* bits, important
> + * request type modified bits. Don't change without looking at bi_rw flags
> */
> enum rq_flag_bits {
> - __REQ_RW, /* not set, read. set, write */
> - __REQ_FAILFAST, /* no low level driver retries */
> + __REQ_RW = BIO_RW, /* not set, read. set, write */
> + __REQ_FAILFAST = BIO_RW_FAILFAST, /* no low level driver retries */
> + __REQ_RW_SYNC = BIO_RW_SYNC,/* request is sync (O_DIRECT) */
> + __REQ_RW_META = BIO_RW_META,/* metadata io request */
> __REQ_SORTED, /* elevator knows about this request */
> __REQ_SOFTBARRIER, /* may not be passed by ioscheduler */
> __REQ_HARDBARRIER, /* may not be passed by drive either */
> @@ -185,9 +187,7 @@ enum rq_flag_bits {
> __REQ_QUIET,/* don't worry about errors */
> __REQ_PREEMPT, /* set for "ide_preempt" requests */
> __REQ_ORDERED_COLOR,/* is before or after barrier */
> - __REQ_RW_SYNC, /* request is sync (O_DIRECT) */
> __REQ_ALLOCED, /* request came from our alloc pool */
> - __REQ_RW_META, /* metadata io request */
> __REQ_NR_BITS, /* stops here */
> };
>
>
Thats not enough You still need to fix code in ll_rw_blk(), I would
define a rq_flags_bio_match_mask = 0xf for that.
(and also add what Jens called "needed" with the
BIO_RW_AHEAD selects REQ_FAILFAST.)
And I still don't understand why, for example, "Domain Validation" fails
with the original patch. What sets BIO_RW_FAILFAST and than panics
on Errors?
(All I see is this flag set in dm/multipath.c & dm-mpath.c)
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 20/30] blk_end_request: changing xsysace (take 4)
On Wed, Dec 12 2007 at 19:06 +0200, Kiyoshi Ueda <[EMAIL PROTECTED]> wrote:
> Hi,
>
> On Wed, 12 Dec 2007 11:09:12 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote:
>>> Index: 2.6.24-rc4/drivers/block/xsysace.c
>>> ===
>>> --- 2.6.24-rc4.orig/drivers/block/xsysace.c
>>> +++ 2.6.24-rc4/drivers/block/xsysace.c
>>> @@ -703,7 +703,7 @@ static void ace_fsm_dostate(struct ace_d
>>>
>>> /* bio finished; is there another one? */
>>> i = ace->req->current_nr_sectors;
>>> - if (end_that_request_first(ace->req, 1, i)) {
>>> + if (__blk_end_request(ace->req, 0, i)) {
>> end_that_request_first() took sectors __blk_end_request() now takes
>> bytes
>
> Thank you for pointing it out! And I'm very sorry for the bug.
> I have checked all conversions between sectors and bytes through
> all patches again, and I found no other miss conversions.
>
> Below is the revised patch for xsysace.
>
> Thanks,
> Kiyoshi Ueda
>
>
NP, I know how it is, after you rebased your work for the
"who's counting" times, you become blind. You need fresh
eyes to look at it. Thanks for doing all this.
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[0/3 ver2] Last 3 patches for bidi support
James hi. Bidi patches just broke again, by a patch that fixes some to-be-dead code. (scsi: BUG_ON() impossible condition) Could it not just be accepted into the tree now. It sat in -mm tree with no reports of breakage or complains. What are we waiting for? the way I see it there is nothing holding it back, it's not even dangerous anymore. You need Arm's accessors patch from scsi-pending Russell King <[EMAIL PROTECTED]> Please send an Acked-by for this patch and the patch that removes the old esp drivers (http://www.spinics.net/lists/linux-scsi/msg20914.html) Christoph Hellwig <[EMAIL PROTECTED]> David S. Miller <[EMAIL PROTECTED]> Maciej W. Rozycki <[EMAIL PROTECTED]> Please send an Ack-by or Recommended-by to the removal of these old esp drivers. And the 3 patches (based on scsi-misc) [1] tgt: Use scsi_init_io instead of scsi_alloc_sgtable Was Ack-by the maintainer of tgt. Please accept independent of the other 2. [2] scsi: scsi_data_buffer The move to scsi_data_buffer. From here on any unconverted driver will not compile. [3] scsi: bidi support Actual very simple really. All parties involved, send your reservations if any NOW. Else James please put it in. Andrew could they be included back into -mm tree? Boaz - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] tgt: Use scsi_init_io instead of scsi_alloc_sgtable
- If we export scsi_init_io()/scsi_release_buffers() instead of
scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
much more insulated from scsi_lib changes. As a bonus it will
also gain bidi capability when it comes.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
Acked-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
drivers/scsi/scsi_lib.c | 21 ++---
drivers/scsi/scsi_tgt_lib.c | 29 +
include/scsi/scsi_cmnd.h|4 ++--
3 files changed, 17 insertions(+), 37 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e273e4b..d1a4671 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -739,7 +739,8 @@ static inline unsigned int scsi_sgtable_index(unsigned
short nents)
return index;
}
-struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
+ gfp_t gfp_mask)
{
struct scsi_host_sg_pool *sgp;
struct scatterlist *sgl, *prev, *ret;
@@ -825,9 +826,7 @@ enomem:
return NULL;
}
-EXPORT_SYMBOL(scsi_alloc_sgtable);
-
-void scsi_free_sgtable(struct scsi_cmnd *cmd)
+static void scsi_free_sgtable(struct scsi_cmnd *cmd)
{
struct scatterlist *sgl = cmd->request_buffer;
struct scsi_host_sg_pool *sgp;
@@ -873,8 +872,6 @@ void scsi_free_sgtable(struct scsi_cmnd *cmd)
mempool_free(sgl, sgp->pool);
}
-EXPORT_SYMBOL(scsi_free_sgtable);
-
/*
* Function:scsi_release_buffers()
*
@@ -892,7 +889,7 @@ EXPORT_SYMBOL(scsi_free_sgtable);
* the scatter-gather table, and potentially any bounce
* buffers.
*/
-static void scsi_release_buffers(struct scsi_cmnd *cmd)
+void scsi_release_buffers(struct scsi_cmnd *cmd)
{
if (cmd->use_sg)
scsi_free_sgtable(cmd);
@@ -904,6 +901,7 @@ static void scsi_release_buffers(struct scsi_cmnd *cmd)
cmd->request_buffer = NULL;
cmd->request_bufflen = 0;
}
+EXPORT_SYMBOL(scsi_release_buffers);
/*
* Function:scsi_io_completion()
@@ -1105,7 +1103,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
* Returns: 0 on success
* BLKPREP_DEFER if the failure is retryable
*/
-static int scsi_init_io(struct scsi_cmnd *cmd)
+int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
{
struct request *req = cmd->request;
intcount;
@@ -1120,7 +1118,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
/*
* If sg table allocation fails, requeue request later.
*/
- cmd->request_buffer = scsi_alloc_sgtable(cmd, GFP_ATOMIC);
+ cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
if (unlikely(!cmd->request_buffer)) {
scsi_unprep_request(req);
return BLKPREP_DEFER;
@@ -1141,6 +1139,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
cmd->use_sg = count;
return BLKPREP_OK;
}
+EXPORT_SYMBOL(scsi_init_io);
static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
struct request *req)
@@ -1186,7 +1185,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev,
struct request *req)
BUG_ON(!req->nr_phys_segments);
- ret = scsi_init_io(cmd);
+ ret = scsi_init_io(cmd, GFP_ATOMIC);
if (unlikely(ret))
return ret;
} else {
@@ -1237,7 +1236,7 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct
request *req)
if (unlikely(!cmd))
return BLKPREP_DEFER;
- return scsi_init_io(cmd);
+ return scsi_init_io(cmd, GFP_ATOMIC);
}
EXPORT_SYMBOL(scsi_setup_fs_cmnd);
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index 93ece8f..91630ba 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -331,8 +331,7 @@ static void scsi_tgt_cmd_done(struct scsi_cmnd *cmd)
scsi_tgt_uspace_send_status(cmd, tcmd->itn_id, tcmd->tag);
- if (scsi_sglist(cmd))
- scsi_free_sgtable(cmd);
+ scsi_release_buffers(cmd);
queue_work(scsi_tgtd, &tcmd->work);
}
@@ -353,26 +352,6 @@ static int scsi_tgt_transfer_response(struct scsi_cmnd
*cmd)
return 0;
}
-static int scsi_tgt_init_cmd(struct scsi_cmnd *cmd, gfp_t gfp_mask)
-{
- struct request *rq = cmd->request;
- int count;
-
- cmd->use_sg = rq->nr_phys_segments;
- cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
- if (!cmd->request_buffer)
- return -ENOMEM;
-
- cmd->request_bufflen = rq->data_len;
-
- dprintk("cmd %p cnt %d %lu\n", cmd, scsi_sg_count(cmd),
- rq_data_dir(rq));
-
[PATCH] scsi: scsi_data_buffer
In preparation for bidi we abstract all IO members of scsi_cmnd,
that will need to duplicate, into a substructure.
- Group all IO members of scsi_cmnd into a scsi_data_buffer
structure.
- Adjust accessors to new members.
- scsi_{alloc,free}_sgtable receive a scsi_data_buffer instead of
scsi_cmnd. And work on it.
- Adjust scsi_init_io() and scsi_release_buffers() for above
change.
- Fix other parts of scsi_lib/scsi.c to members migration. Use
accessors where appropriate.
- fix Documentation about scsi_cmnd in scsi_host.h
- scsi_error.c
* Changed needed members of struct scsi_eh_save.
* Careful considerations in scsi_eh_prep/restore_cmnd.
- sd.c and sr.c
* sd and sr would adjust IO size to align on device's block
size so code needs to change once we move to scsi_data_buff
implementation.
* Convert code to use scsi_for_each_sg
* Use data accessors where appropriate.
- tgt: convert libsrp to use scsi_data_buffer
- isd200: This driver still bangs on scsi_cmnd IO members,
so need changing
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
---
drivers/scsi/libsrp.c|4 +-
drivers/scsi/scsi.c |2 +-
drivers/scsi/scsi_error.c| 28 +--
drivers/scsi/scsi_lib.c | 77 --
drivers/scsi/sd.c|4 +-
drivers/scsi/sr.c| 25 +++--
drivers/usb/storage/isd200.c |8 ++--
include/scsi/scsi_cmnd.h | 39 +
include/scsi/scsi_eh.h |8 ++---
include/scsi/scsi_host.h |4 +-
10 files changed, 91 insertions(+), 108 deletions(-)
diff --git a/drivers/scsi/libsrp.c b/drivers/scsi/libsrp.c
index 5cff020..8a8562a 100644
--- a/drivers/scsi/libsrp.c
+++ b/drivers/scsi/libsrp.c
@@ -426,8 +426,8 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd
*cmd, void *info,
sc->SCp.ptr = info;
memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
- sc->request_bufflen = len;
- sc->request_buffer = (void *) (unsigned long) addr;
+ sc->sdb.length = len;
+ sc->sdb.sglist = (void *) (unsigned long) addr;
sc->tag = tag;
err = scsi_tgt_queue_command(sc, itn_id, (struct scsi_lun *)&cmd->lun,
cmd->tag);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ebc0193..a0fd785 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -712,7 +712,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
"Notifying upper driver of completion "
"(result %x)\n", cmd->result));
- good_bytes = cmd->request_bufflen;
+ good_bytes = scsi_bufflen(cmd);
if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
drv = scsi_cmd_to_driver(cmd);
if (drv->done)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 169bc59..241ab48 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -617,29 +617,25 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct
scsi_eh_save *ses,
ses->cmd_len = scmd->cmd_len;
memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
ses->data_direction = scmd->sc_data_direction;
- ses->bufflen = scmd->request_bufflen;
- ses->buffer = scmd->request_buffer;
- ses->use_sg = scmd->use_sg;
- ses->resid = scmd->resid;
+ ses->sdb = scmd->sdb;
ses->result = scmd->result;
+ memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+
if (sense_bytes) {
- scmd->request_bufflen = min_t(unsigned,
+ scmd->sdb.length = min_t(unsigned,
sizeof(scmd->sense_buffer), sense_bytes);
sg_init_one(&ses->sense_sgl, scmd->sense_buffer,
- scmd->request_bufflen);
- scmd->request_buffer = &ses->sense_sgl;
+ scmd->sdb.length);
+ scmd->sdb.sglist = &ses->sense_sgl;
scmd->sc_data_direction = DMA_FROM_DEVICE;
- scmd->use_sg = 1;
+ scmd->sdb.sg_count = 1;
memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
scmd->cmnd[0] = REQUEST_SENSE;
- scmd->cmnd[4] = scmd->request_bufflen;
+ scmd->cmnd[4] = scmd->sdb.length;
scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
} else {
- scmd->request_buffer = NULL;
- scmd->request_bufflen = 0;
scmd->sc_data_direction = DMA_NONE;
[PATCH] scsi: bidi support
At the block level bidi request uses req->next_rq pointer for a second
bidi_read request.
At Scsi-midlayer a second scsi_data_buffer structure is used for the
bidi_read part. This bidi scsi_data_buffer is put on
request->next_rq->special. Struct scsi_cmnd is not changed.
- Define scsi_bidi_cmnd() to return true if it is a bidi request and a
second sgtable was allocated.
- Define scsi_in()/scsi_out() to return the in or out scsi_data_buffer
from this command This API is to isolate users from the mechanics of
bidi.
- Define scsi_end_bidi_request() to do what scsi_end_request() does but
for a bidi request. This is necessary because bidi commands are a bit
tricky here. (See comments in body)
- scsi_release_buffers() will also release the bidi_read scsi_data_buffer
- scsi_io_completion() on bidi commands will now call
scsi_end_bidi_request() and return.
- The previous work done in scsi_init_io() is now done in a new
scsi_init_sgtable() (which is 99% identical to old scsi_init_io())
The new scsi_init_io() will call the above twice if needed also for
the bidi_read command. Only at this point is a command bidi.
- In scsi_error.c at scsi_eh_prep/restore_cmnd() make sure bidi-lld is not
confused by a get-sense command that looks like bidi. This is done
by puting NULL at request->next_rq, and restoring.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/scsi_error.c |3 +
drivers/scsi/scsi_lib.c | 144 -
include/scsi/scsi_cmnd.h | 23 +++-
include/scsi/scsi_eh.h|1 +
4 files changed, 141 insertions(+), 30 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 241ab48..5c8ba6a 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -618,9 +618,11 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct
scsi_eh_save *ses,
memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
ses->data_direction = scmd->sc_data_direction;
ses->sdb = scmd->sdb;
+ ses->next_rq = scmd->request->next_rq;
ses->result = scmd->result;
memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+ scmd->request->next_rq = NULL;
if (sense_bytes) {
scmd->sdb.length = min_t(unsigned,
@@ -673,6 +675,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct
scsi_eh_save *ses)
memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
scmd->sc_data_direction = ses->data_direction;
scmd->sdb = ses->sdb;
+ scmd->request->next_rq = ses->next_rq;
scmd->result = ses->result;
}
EXPORT_SYMBOL(scsi_eh_restore_cmnd);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7ac36fe..a6aae56 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -64,6 +64,8 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
};
#undef SP
+static struct kmem_cache *scsi_bidi_sdb_cache;
+
static void scsi_run_queue(struct request_queue *q);
/*
@@ -627,6 +629,28 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
scsi_run_queue(sdev->request_queue);
}
+static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
+{
+ struct request_queue *q = cmd->device->request_queue;
+ struct request *req = cmd->request;
+ unsigned long flags;
+
+ add_disk_randomness(req->rq_disk);
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ if (blk_rq_tagged(req))
+ blk_queue_end_tag(q, req);
+
+ end_that_request_last(req, uptodate);
+ spin_unlock_irqrestore(q->queue_lock, flags);
+
+ /*
+* This will goose the queue request function at the end, so we don't
+* need to worry about launching another command.
+*/
+ scsi_next_command(cmd);
+}
+
/*
* Function:scsi_end_request()
*
@@ -654,7 +678,6 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd
*cmd, int uptodate,
{
struct request_queue *q = cmd->device->request_queue;
struct request *req = cmd->request;
- unsigned long flags;
/*
* If there are blocks left over at the end, set up the command
@@ -683,19 +706,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd
*cmd, int uptodate,
}
}
- add_disk_randomness(req->rq_disk);
-
- spin_lock_irqsave(q->queue_lock, flags);
- if (blk_rq_tagged(req))
- blk_queue_end_tag(q, req);
- end_that_request_last(req, uptodate);
- spin_unlock_irqrestore(q->queue_lock, flags);
-
- /*
-* This will goose the queue request function at the end, so we don't
-* need to worry about launching another command.
-*/
- scsi_ne
