Re: [PATCH 1/3] [SCSI] Fix abort state memory problem
On 03/28/2014 06:49 PM, James Bottomley wrote:
> The abort state is being stored persistently across commands, meaning if the
> command times out a second time, the error handler thinks an abort is still
> pending. Fix this by properly resetting the abort flag after the abort is
> finished.
>
> Signed-off-by: James Bottomley
> ---
> drivers/scsi/scsi_error.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 771c16b..b9f3b07 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -869,10 +869,13 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd
> *scmd)
>
> static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt, struct
> scsi_cmnd *scmd)
> {
> - if (!hostt->eh_abort_handler)
> - return FAILED;
> + int retval = FAILED;
> +
> + if (hostt->eh_abort_handler)
> + retval = hostt->eh_abort_handler(scmd);
>
> - return hostt->eh_abort_handler(scmd);
> + scmd->eh_eflags &= ~SCSI_EH_ABORT_SCHEDULED;
> + return retval;
> }
>
> static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
>
Hmm. No, I don't think this is correct.
SCSI_EH_ABORT_SCHEDULED signifies whether scmd_eh_abort_handler()
needs to run. As such it needs to be reset when the workqueue item
is triggered.
Can you try with this instead?
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 771c16b..9694803 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -120,6 +120,8 @@ scmd_eh_abort_handler(struct work_struct *work)
struct scsi_device *sdev = scmd->device;
int rtn;
+ scmd->eh_eflags &= ~SCSI_EH_ABORT_SCHEDULED;
+
if (scsi_host_eh_past_deadline(sdev->host)) {
SCSI_LOG_ERROR_RECOVERY(3,
scmd_printk(KERN_INFO, scmd,
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 3/4] scsi: reintroduce scsi_driver.init_command
On 03/27/2014 11:14 AM, Christoph Hellwig wrote: > @@ -1663,6 +1652,8 @@ static int sd_done(struct scsi_cmnd *SCpnt) > unsigned char op = SCpnt->cmnd[0]; > unsigned char unmap = SCpnt->cmnd[1] & 8; > > + sd_uninit_command(SCpnt); > + The above call would free the cmnd->cmnd and set it to null. If then scsi_io_completion was going to do some error processing it looks like it could try to access the scsi_cmnd->cmnd field. With the current code that would not be a problem because the blk unprep callback is not called until the block layer does its request cleanup in blk_finish_request which as you know is after scsi_io_completion/scsi_end_request is done with the cmnd. -- 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 3/4] scsi: reintroduce scsi_driver.init_command
On Mon, 2014-03-31 at 08:08 +0200, Christoph Hellwig wrote:
> On Sun, Mar 30, 2014 at 10:45:04PM -0700, Nicholas A. Bellinger wrote:
> > > int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
> > > {
> > > - struct scsi_cmnd *cmd;
> > > - int ret = scsi_prep_state_check(sdev, req);
> > > -
> > > - if (ret != BLKPREP_OK)
> > > - return ret;
> > > -
> > > - cmd = scsi_get_cmd_from_req(sdev, req);
> > > - if (unlikely(!cmd))
> > > - return BLKPREP_DEFER;
> > > + struct scsi_cmnd *cmd = req->special;
> > >
> >
> > Mmm, I thought that req->special was only holding a pointer to a
> > pre-allocated scsi_cmnd during mq operation, no..?
>
> For the mq case the block layer sets up req->special. For the old case
> scsi_get_cmd_from_req sets up req->special the first it is called, and
> leaves it in there until the command is done.
Er, yes of course.
Reviewed-by: Nicholas Bellinger
--
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: misc scsi midlayer updates
Hi Boaz, > Hi Christoph > > Many years ago I have sent these exact patches but no-one cared Including > me I guess. I didn't remember your older patches, sorry. > I think my: > scsi_lib: Remove that __scsi_release_buffers contraption > Is more elegant, is layered better and is smaller code. (please > consider my version) I very much disagree - the bidi code uses a separate request for it's payload, uses separate functions to set it up at the low-level so mirroring it with a separate teardown makes sense. This also avoids having to do any bidi check at all in the fast path. > Also the first patch is some more cleanup you'd like. Doesn't look bad, but not that importan either. > The main patch of collapsing scsi_end_request is basically the same. I like the goto version better beause it avoids additional duplication from inside the switch and the bidi path, but it should be functionally equivalent. > Please note the 4th patch which is a real BUG, titled: > scsi_lib: Can't RETRY scsi_cmnd if some bytes were completed That fix seems very hard to read due to the arithmetic comparism on the enum value. The way I try to understand it is that you never want to retry if ((an error happened) && (bytes were completed)) but the explanation should be expanded. > [Your patches have been tested within my MQ testing right?] Yes. -- 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/4] scsi: explicitly release bidi buffers
On Mon, Mar 31, 2014 at 12:58:01AM -0500, Mike Christie wrote:
>
> Hey,
>
> I just wanted to double check that the only time we do bidi requests is
> when they come through as REQ_TYPE_BLOCK_PC requests. I saw some of the
> code comments on the init side that indicated that, but there was that
> scsi_end_request() -> scsi_release_buffers() -> __scsi_release_buffers()
> call which then did not make sense because we were passing in 1 for the
> bidi check argument.
We only support BIDI for REQ_TYPE_BLOCK_PC requests, and scsi_io_completion
enforces this:
if (req->cmd_type == REQ_TYPE_BLOCK_PC) { /* SG_IO ioctl from block
level */
...
if (scsi_bidi_cmnd(cmd)) {
...
< end_io and return >
}
}
/* no bidi support for !REQ_TYPE_BLOCK_PC yet */
BUG_ON(blk_bidi_rq(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: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command
On Sun, Mar 30, 2014 at 10:45:04PM -0700, Nicholas A. Bellinger wrote:
> > int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
> > {
> > - struct scsi_cmnd *cmd;
> > - int ret = scsi_prep_state_check(sdev, req);
> > -
> > - if (ret != BLKPREP_OK)
> > - return ret;
> > -
> > - cmd = scsi_get_cmd_from_req(sdev, req);
> > - if (unlikely(!cmd))
> > - return BLKPREP_DEFER;
> > + struct scsi_cmnd *cmd = req->special;
> >
>
> Mmm, I thought that req->special was only holding a pointer to a
> pre-allocated scsi_cmnd during mq operation, no..?
For the mq case the block layer sets up req->special. For the old case
scsi_get_cmd_from_req sets up req->special the first it is called, and
leaves it in there until the command is done.
> > + ret = scsi_prep_state_check(sdev, req);
> > + if (ret != BLKPREP_OK)
> > + goto out;
> > +
> > + cmd = scsi_get_cmd_from_req(sdev, req);
> > + if (unlikely(!cmd)) {
> > + ret = BLKPREP_DEFER;
> > + goto out;
> > + }
> > +
>From here the req->special pointer may or may not be set depending on mq
operation, no..?
a) there is no blk-mq support yet in James tree (+ these patches)
b) even in my scsi-mq tree this code path won't be called for the mq case
c) after scsi_get_cmd_from_req returns req->special will always be set up
to point to the scsi_cmnd:
static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
struct request *req)
{
struct scsi_cmnd *cmd;
if (!req->special) {
...
req->special = cmd;
} else {
cmd = req->special;
}
/* pull a tag out of the request if we have one */
cmd->tag = req->tag;
cmd->request = req;
cmd->cmnd = req->cmd;
cmd->prot_op = SCSI_PROT_NORMAL;
return cmd;
}
> > }
> > ret = scsi_setup_fs_cmnd(sdp, rq);
> > if (ret != BLKPREP_OK)
>
> And this currently assumes req->special is always set when calling
> scsi_setup_fs_cmnd()
That is the case both before and after this patch.
--
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/4] scsi: explicitly release bidi buffers
On 03/27/2014 11:14 AM, Christoph Hellwig wrote:
> +static void scsi_release_bidi_buffers(struct scsi_cmnd *cmd)
> +{
> + struct scsi_data_buffer *bidi_sdb = cmd->request->next_rq->special;
> +
> + scsi_free_sgtable(bidi_sdb);
> + kmem_cache_free(scsi_sdb_cache, bidi_sdb);
> + cmd->request->next_rq->special = NULL;
> +}
> +
> /**
> * __scsi_error_from_host_byte - translate SCSI error code into errno
> * @cmd: SCSI command (unused)
> @@ -800,6 +792,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
> int good_bytes)
> req->next_rq->resid_len = scsi_in(cmd)->resid;
>
> scsi_release_buffers(cmd);
> + scsi_release_bidi_buffers(cmd);
> blk_end_request_all(req, 0);
>
> scsi_next_command(cmd);
>
Hey,
I just wanted to double check that the only time we do bidi requests is
when they come through as REQ_TYPE_BLOCK_PC requests. I saw some of the
code comments on the init side that indicated that, but there was that
scsi_end_request() -> scsi_release_buffers() -> __scsi_release_buffers()
call which then did not make sense because we were passing in 1 for the
bidi check argument.
--
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] aic79xx: fix misuse of static variables
On 03/30/2014 03:30 PM, Mathias Krause wrote:
> The format strings for various printk()s make use of a temporary
> variable that is declared 'static'. This is probably not intended,
> so fix those.
>
> Found in the PaX patch, written by the PaX Team.
>
> Cc: PaX Team
> Cc: Hannes Reinecke
> Cc: "James E.J. Bottomley"
> Signed-off-by: Mathias Krause
> ---
>
> Remark: Compile tested only! I've no such hardware.
>
> drivers/scsi/aic7xxx/aic79xx_pci.c | 18 +-
> 1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/scsi/aic7xxx/aic79xx_pci.c
> b/drivers/scsi/aic7xxx/aic79xx_pci.c
> index 14b5f8d0e7..cc9bd26f5d 100644
> --- a/drivers/scsi/aic7xxx/aic79xx_pci.c
> +++ b/drivers/scsi/aic7xxx/aic79xx_pci.c
> @@ -827,7 +827,7 @@ ahd_pci_intr(struct ahd_softc *ahd)
> for (bit = 0; bit < 8; bit++) {
>
> if ((pci_status[i] & (0x1 << bit)) != 0) {
> - static const char *s;
> + const char *s;
>
> s = pci_status_strings[bit];
> if (i == 7/*TARG*/ && bit == 3)
> @@ -887,23 +887,15 @@ ahd_pci_split_intr(struct ahd_softc *ahd, u_int intstat)
>
> for (bit = 0; bit < 8; bit++) {
>
> - if ((split_status[i] & (0x1 << bit)) != 0) {
> - static const char *s;
> -
> - s = split_status_strings[bit];
> - printk(s, ahd_name(ahd),
> + if ((split_status[i] & (0x1 << bit)) != 0)
> + printk(split_status_strings[bit], ahd_name(ahd),
> split_status_source[i]);
> - }
>
> if (i > 1)
> continue;
>
> - if ((sg_split_status[i] & (0x1 << bit)) != 0) {
> - static const char *s;
> -
> - s = split_status_strings[bit];
> - printk(s, ahd_name(ahd), "SG");
> - }
> + if ((sg_split_status[i] & (0x1 << bit)) != 0)
> + printk(split_status_strings[bit],
> ahd_name(ahd), "SG");
> }
> }
> /*
>
Looks good.
Acked-by: Hannes Reinecke
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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/7] Performance improvements for LSI SCSI cards
Hi Matthew, On Thu, 2014-03-27 at 16:40 -0400, Matthew Wilcox wrote: > The host lock is a serious scalability problem on 2-socket and larger > systems which are doing a lot of I/O. Before removing the temporary > usgae of DEF_SCSI_QCMD, we need to remove all uses of serial_number. > > An unrelated performance issue is that reusing the most recent > driver-specific data structure to track the I/O instead of the least > recently used keeps the cache-hot lines in use, which is a nice > performance improvement. It's already present in the mpt3sas driver, > it just didn't make it into the fusion or mpt2sas drivers yet. > > Matthew Wilcox (7): > mpt3sas: Remove uses of serial_number > mpt3sas: Remove use of DEF_SCSI_QCMD > mpt2sas: Remove uses of serial_number > mpt2sas: Remove use of DEF_SCSI_QCMD > mpt2sas: Add free smids to the head, not tail of list > fusion: Add free msg frames to the head, not tail of list > fusion: Remove use of DEF_SCSI_QCMD > > +1 to this long overdue series to enable host_lock-less mode with mpt*sas + fusion. (CC'ing LSI folks + jejb) Reviewed-by: Nicholas Bellinger -- 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 3/4] scsi: reintroduce scsi_driver.init_command
On Thu, 2014-03-27 at 17:14 +0100, Christoph Hellwig wrote:
> Move control of the prep_fn back from the ULDs into the scsi layer. Besides
> cleaning up the code and removing the only use of the unprep_fn
> requeuest_queue method this also prepares for usinng blk-mq, which doesn't
> have equivalent functionality to the prep_fn method and requires the driver
> to provide just a single I/O submission method.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/scsi/scsi_lib.c| 66
> ++--
> drivers/scsi/sd.c | 46 +++---
> drivers/scsi/sr.c | 19 -
> include/scsi/scsi_driver.h |8 ++
> 4 files changed, 63 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index a73751b..48c5c77 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1071,15 +1083,7 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct
> scsi_device *sdev,
>
> int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
> {
> - struct scsi_cmnd *cmd;
> - int ret = scsi_prep_state_check(sdev, req);
> -
> - if (ret != BLKPREP_OK)
> - return ret;
> -
> - cmd = scsi_get_cmd_from_req(sdev, req);
> - if (unlikely(!cmd))
> - return BLKPREP_DEFER;
> + struct scsi_cmnd *cmd = req->special;
>
Mmm, I thought that req->special was only holding a pointer to a
pre-allocated scsi_cmnd during mq operation, no..?
> /*
>* BLOCK_PC requests may transfer data, in which case they must
> @@ -1123,15 +1127,11 @@ EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd);
> */
> int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
> {
> - struct scsi_cmnd *cmd;
> - int ret = scsi_prep_state_check(sdev, req);
> -
> - if (ret != BLKPREP_OK)
> - return ret;
> + struct scsi_cmnd *cmd = req->special;
>
Ditto here for REQ_TYPE_FS_CMND
> if (unlikely(sdev->scsi_dh_data && sdev->scsi_dh_data->scsi_dh
>&& sdev->scsi_dh_data->scsi_dh->prep_fn)) {
> - ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req);
> + int ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req);
> if (ret != BLKPREP_OK)
> return ret;
> }
> @@ -1141,16 +1141,13 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev,
> struct request *req)
>*/
> BUG_ON(!req->nr_phys_segments);
>
> - cmd = scsi_get_cmd_from_req(sdev, req);
> - if (unlikely(!cmd))
> - return BLKPREP_DEFER;
> -
> memset(cmd->cmnd, 0, BLK_MAX_CDB);
> return scsi_init_io(cmd, GFP_ATOMIC);
> }
> EXPORT_SYMBOL(scsi_setup_fs_cmnd);
>
> -int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
> +static int
> +scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
> {
> int ret = BLKPREP_OK;
>
> @@ -1202,9 +1199,9 @@ int scsi_prep_state_check(struct scsi_device *sdev,
> struct request *req)
> }
> return ret;
> }
> -EXPORT_SYMBOL(scsi_prep_state_check);
>
> -int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
> +static int
> +scsi_prep_return(struct request_queue *q, struct request *req, int ret)
> {
> struct scsi_device *sdev = q->queuedata;
>
> @@ -1235,18 +1232,33 @@ int scsi_prep_return(struct request_queue *q, struct
> request *req, int ret)
>
> return ret;
> }
> -EXPORT_SYMBOL(scsi_prep_return);
>
> -int scsi_prep_fn(struct request_queue *q, struct request *req)
> +static int scsi_prep_fn(struct request_queue *q, struct request *req)
> {
> struct scsi_device *sdev = q->queuedata;
> - int ret = BLKPREP_KILL;
> + struct scsi_cmnd *cmd;
> + int ret;
>
> - if (req->cmd_type == REQ_TYPE_BLOCK_PC)
> + ret = scsi_prep_state_check(sdev, req);
> + if (ret != BLKPREP_OK)
> + goto out;
> +
> + cmd = scsi_get_cmd_from_req(sdev, req);
> + if (unlikely(!cmd)) {
> + ret = BLKPREP_DEFER;
> + goto out;
> + }
> +
>From here the req->special pointer may or may not be set depending on mq
operation, no..?
> + if (req->cmd_type == REQ_TYPE_FS)
> + ret = scsi_cmd_to_driver(cmd)->init_command(cmd);
> + else if (req->cmd_type == REQ_TYPE_BLOCK_PC)
> ret = scsi_setup_blk_pc_cmnd(sdev, req);
> + else
> + ret = BLKPREP_KILL;
> +
> +out:
> return scsi_prep_return(q, req, ret);
> }
> -EXPORT_SYMBOL(scsi_prep_fn);
>
> /*
> * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 89e6c04..d95c4fd 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -886,12 +882,6 @@ static int sd_prep_fn(struct request_queue *q, struct
> request *rq)
> } else if (rq->cmd_flags & REQ_FLUSH) {
> ret =
Re: [PATCH 4/4] scsi: handle command allocation failure in scsi_reset_provider
On Thu, 2014-03-27 at 17:14 +0100, Christoph Hellwig wrote: > Signed-off-by: Christoph Hellwig > --- > drivers/scsi/scsi_error.c |6 ++ > 1 file changed, 6 insertions(+) > Reviewed-by: Nicholas Bellinger -- 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 09/10] qla4xxx: Fix memory leak in func qla4_84xx_config_acb()
On 02/24/2014 09:07 PM, vikas.chaudh...@qlogic.com wrote: > From: Vikas Chaudhary > > Use correct goto statement to free dma memory in case of > failure in function qla4_84xx_config_acb() > > Signed-off-by: Vikas Chaudhary > --- > drivers/scsi/qla4xxx/ql4_mbx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/qla4xxx/ql4_mbx.c b/drivers/scsi/qla4xxx/ql4_mbx.c > index 0a6b782..1345c0e 100644 > --- a/drivers/scsi/qla4xxx/ql4_mbx.c > +++ b/drivers/scsi/qla4xxx/ql4_mbx.c > @@ -2381,7 +2381,7 @@ int qla4_84xx_config_acb(struct scsi_qla_host *ha, int > acb_config) > ql4_printk(KERN_ERR, ha, "%s: Unable to alloc acb\n", > __func__); > rval = QLA_ERROR; > - goto exit_config_acb; > + goto exit_free_acb; > } > memcpy(ha->saved_acb, acb, acb_len); > break; > This patch looks ok, but is it possible to still leak the ha->saved_acb still? If you were going to do ACB_CONFIG_SET but the dma_alloc_coherent failed then it would not be freed. -- 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/4] scsi: remove scsi_end_request
Hi hch & jejb, On Thu, 2014-03-27 at 17:14 +0100, Christoph Hellwig wrote: > Simply the I/O completion logic by folding scsi_end_request into its only > caller. There is a single site to either requeue the command or move on > to the next one instead of of the previous convoluted logic. > > Signed-off-by: Christoph Hellwig > --- > drivers/scsi/scsi_lib.c | 114 > +-- > 1 file changed, 32 insertions(+), 82 deletions(-) > Looks like a reasonable simplification. Reviewed-by: Nicholas Bellinger -- 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/6] be2iscsi: relinquishing control after processing 512 CQE
On 03/27/2014 11:39 AM, Jayamohan Kallickal wrote:
> @@ -2323,14 +2319,33 @@ void beiscsi_process_all_cqs(struct work_struct *work)
>
> static int be_iopoll(struct blk_iopoll *iop, int budget)
> {
> - unsigned int ret;
> + unsigned int ret, num_eq_processed;
> struct beiscsi_hba *phba;
> struct be_eq_obj *pbe_eq;
> + struct be_eq_entry *eqe = NULL;
> + struct be_queue_info *eq;
>
> + num_eq_processed = 0;
> pbe_eq = container_of(iop, struct be_eq_obj, iopoll);
> + phba = pbe_eq->phba;
> + eq = &pbe_eq->q;
> + eqe = queue_tail_node(eq);
> +
> + hwi_ring_eq_db(phba, eq->id, 1, num_eq_processed, 0, 1);
Is this right? num_eq_processed will be 0 above. Should this be moved
down below to after num_eq_processed has been incremented?
> +
> + while (eqe->dw[offsetof(struct amap_eq_entry, valid) / 32]
> + & EQE_VALID_MASK) {
> +
> + AMAP_SET_BITS(struct amap_eq_entry, valid, eqe, 0);
> + queue_tail_inc(eq);
> + eqe = queue_tail_node(eq);
> + num_eq_processed++;
> + }
--
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 5/6] be2iscsi: Fix TCP parameters while connection offloading.
On 03/27/2014 11:39 AM, Jayamohan Kallickal wrote:
> +int mgmt_open_connection_v1(struct beiscsi_hba *phba,
> + struct sockaddr *dst_addr,
> + struct beiscsi_endpoint *beiscsi_ep,
> + struct be_dma_mem *nonemb_cmd)
> +{
> + struct hwi_controller *phwi_ctrlr;
> + struct hwi_context_memory *phwi_context;
> + struct sockaddr_in *daddr_in = (struct sockaddr_in *)dst_addr;
> + struct sockaddr_in6 *daddr_in6 = (struct sockaddr_in6 *)dst_addr;
> + struct be_ctrl_info *ctrl = &phba->ctrl;
> + struct be_mcc_wrb *wrb;
> + struct tcp_connect_and_offload_in_v1 *req;
> + unsigned short def_hdr_id;
> + unsigned short def_data_id;
> + struct phys_addr template_address = { 0, 0 };
> + struct phys_addr *ptemplate_address;
> + unsigned int tag = 0;
> + unsigned int i, ulp_num;
> + unsigned short cid = beiscsi_ep->ep_cid;
> + struct be_sge *sge;
> +
> + phwi_ctrlr = phba->phwi_ctrlr;
> + phwi_context = phwi_ctrlr->phwi_ctxt;
> +
> + ulp_num = phwi_ctrlr->wrb_context[BE_GET_CRI_FROM_CID(cid)].ulp_num;
> +
> + def_hdr_id = (unsigned short)HWI_GET_DEF_HDRQ_ID(phba, ulp_num);
> + def_data_id = (unsigned short)HWI_GET_DEF_BUFQ_ID(phba, ulp_num);
> +
> + ptemplate_address = &template_address;
> + ISCSI_GET_PDU_TEMPLATE_ADDRESS(phba, ptemplate_address);
> + spin_lock(&ctrl->mbox_lock);
> + tag = alloc_mcc_tag(phba);
> + if (!tag) {
> + spin_unlock(&ctrl->mbox_lock);
> + return tag;
> + }
> + wrb = wrb_from_mccq(phba);
> + sge = nonembedded_sgl(wrb);
> +
> + req = nonemb_cmd->va;
> + memset(req, 0, sizeof(*req));
> + wrb->tag0 |= tag;
> +
> + be_wrb_hdr_prepare(wrb, sizeof(*req), false, 1);
> + be_cmd_hdr_prepare(&req->hdr, CMD_SUBSYSTEM_ISCSI,
> +OPCODE_COMMON_ISCSI_TCP_CONNECT_AND_OFFLOAD,
> +sizeof(*req));
> + if (dst_addr->sa_family == PF_INET) {
> + __be32 s_addr = daddr_in->sin_addr.s_addr;
> + req->ip_address.ip_type = BE2_IPV4;
> + req->ip_address.addr[0] = s_addr & 0x00ff;
> + req->ip_address.addr[1] = (s_addr & 0xff00) >> 8;
> + req->ip_address.addr[2] = (s_addr & 0x00ff) >> 16;
> + req->ip_address.addr[3] = (s_addr & 0xff00) >> 24;
> + req->tcp_port = ntohs(daddr_in->sin_port);
> + beiscsi_ep->dst_addr = daddr_in->sin_addr.s_addr;
> + beiscsi_ep->dst_tcpport = ntohs(daddr_in->sin_port);
> + beiscsi_ep->ip_type = BE2_IPV4;
> + } else if (dst_addr->sa_family == PF_INET6) {
> + req->ip_address.ip_type = BE2_IPV6;
> + memcpy(&req->ip_address.addr,
> +&daddr_in6->sin6_addr.in6_u.u6_addr8, 16);
> + req->tcp_port = ntohs(daddr_in6->sin6_port);
> + beiscsi_ep->dst_tcpport = ntohs(daddr_in6->sin6_port);
> + memcpy(&beiscsi_ep->dst6_addr,
> +&daddr_in6->sin6_addr.in6_u.u6_addr8, 16);
> + beiscsi_ep->ip_type = BE2_IPV6;
> + } else{
> + beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_CONFIG,
> + "BG_%d : unknown addr family %d\n",
> + dst_addr->sa_family);
> + spin_unlock(&ctrl->mbox_lock);
> + free_mcc_tag(&phba->ctrl, tag);
> + return -EINVAL;
> +
> + }
> + req->cid = cid;
> + i = phba->nxt_cqid++;
> + if (phba->nxt_cqid == phba->num_cpus)
> + phba->nxt_cqid = 0;
> + req->cq_id = phwi_context->be_cq[i].id;
> + beiscsi_log(phba, KERN_INFO, BEISCSI_LOG_CONFIG,
> + "BG_%d : i=%d cq_id=%d\n", i, req->cq_id);
> + req->defq_id = def_hdr_id;
> + req->hdr_ring_id = def_hdr_id;
> + req->data_ring_id = def_data_id;
> + req->do_offload = 1;
> + req->dataout_template_pa.lo = ptemplate_address->lo;
> + req->dataout_template_pa.hi = ptemplate_address->hi;
> + sge->pa_hi = cpu_to_le32(upper_32_bits(nonemb_cmd->dma));
> + sge->pa_lo = cpu_to_le32(nonemb_cmd->dma & 0x);
> + sge->len = cpu_to_le32(nonemb_cmd->size);
> +
> + req->hdr.version = 1;
> + req->tcp_window_size = 0;
> + req->tcp_window_scale_count = 2;
Is this approx 100 lines exactly the same as mgmt_open_connection except
these 3 lines? Maybe a lib/helper function that does the common 100
lines then some v1 function over that which does the extra 3 lines.
--
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: misc scsi midlayer updates
On 03/27/2014 06:14 PM, Christoph Hellwig wrote:
> Various patches from the scsi multiqueue development that make sense on their
> own.
>
> --
> 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
>
Hi Christoph
Many years ago I have sent these exact patches but no-one cared Including
me I guess.
(one instance is here, I sent it several times)
http://comments.gmane.org/gmane.linux.scsi/63347
Please note the 4th patch which is a real BUG, titled:
scsi_lib: Can't RETRY scsi_cmnd if some bytes were completed
I think my:
scsi_lib: Remove that __scsi_release_buffers contraption
Is more elegant, is layered better and is smaller code. (please
consider my version)
Also the first patch is some more cleanup you'd like.
The main patch of collapsing scsi_end_request is basically the same.
Funny that I've just rebased an exactly 2 years ago version and it
rebased cleanly. So here they are for your reference.
[PATCH 1/4] scsi_lib: request_queue is only needed inside
[PATCH 2/4] scsi_lib: Remove that __scsi_release_buffers contraption
[PATCH 3/4] scsi_lib: Collapse scsi_end_request into only user
[PATCH 4/4] scsi_lib: BUG: Can't RETRY scsi_cmnd if some bytes were
[Your patches have been tested within my MQ testing right?]
Thanks for pushing on this
Boaz
>From 9809b82bbb9813370738ac00400c01d61b0c51b4 Mon Sep 17 00:00:00 2001
From: Boaz Harrosh
Date: Mon, 4 Jan 2010 10:45:56 +0200
Subject: [PATCH 1/4] scsi_lib: request_queue is only needed inside
scsi_requeue_command
This is a pure cleanup. Just starting the engines for things
to come.
Signed-off-by: Boaz Harrosh
---
drivers/scsi/scsi_lib.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 62ec84b..dd834ca 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -491,10 +491,11 @@ void scsi_requeue_run_queue(struct work_struct *work)
* sector.
* Notes: Upon return, cmd is a stale pointer.
*/
-static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd)
+static void scsi_requeue_command(struct scsi_cmnd *cmd)
{
struct scsi_device *sdev = cmd->device;
struct request *req = cmd->request;
+ struct request_queue *q = req->q;
unsigned long flags;
/*
@@ -565,7 +566,6 @@ static void __scsi_release_buffers(struct scsi_cmnd *, int);
static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
int bytes, int requeue)
{
- struct request_queue *q = cmd->device->request_queue;
struct request *req = cmd->request;
/*
@@ -584,7 +584,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
* queue, and goose the queue again.
*/
scsi_release_buffers(cmd);
-scsi_requeue_command(q, cmd);
+scsi_requeue_command(cmd);
cmd = NULL;
}
return cmd;
@@ -779,7 +779,6 @@ static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
{
int result = cmd->result;
- struct request_queue *q = cmd->device->request_queue;
struct request *req = cmd->request;
int error = 0;
struct scsi_sense_hdr sshdr;
@@ -1003,7 +1002,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
scsi_print_command(cmd);
}
if (blk_end_request_err(req, error))
- scsi_requeue_command(q, cmd);
+ scsi_requeue_command(cmd);
else
scsi_next_command(cmd);
break;
@@ -1012,7 +1011,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
* A new command will be prepared and issued.
*/
scsi_release_buffers(cmd);
- scsi_requeue_command(q, cmd);
+ scsi_requeue_command(cmd);
break;
case ACTION_RETRY:
/* Retry the same command immediately */
--
1.8.5.3
>From aac579b02c7d3665665a1f1f7c064a82b70b873a Mon Sep 17 00:00:00 2001
From: Boaz Harrosh
Date: Mon, 4 Jan 2010 12:46:06 +0200
Subject: [PATCH 2/4] scsi_lib: Remove that __scsi_release_buffers contraption
Remove "do_bidi_check" by checking and nullifying cmd->request
when blk_end_* indicates the request is no longer valid.
Signed-off-by: Boaz Harrosh
---
drivers/scsi/scsi_lib.c | 41 +
1 file changed, 17 insertions(+), 24 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index dd834ca..c4cdc6a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -539,8 +539,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
scsi_run_queue(sdev->request_queue);
}
-static void __scsi_release_buffers(struct scsi_cmnd *, int);
-
/*
* Function:scsi_end_request()
*
@@ -591,11 +589,12 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
}
}
+ cmd->request = NULL;
/*
* This will goose the queue
[PATCH] aic79xx: fix misuse of static variables
The format strings for various printk()s make use of a temporary
variable that is declared 'static'. This is probably not intended,
so fix those.
Found in the PaX patch, written by the PaX Team.
Cc: PaX Team
Cc: Hannes Reinecke
Cc: "James E.J. Bottomley"
Signed-off-by: Mathias Krause
---
Remark: Compile tested only! I've no such hardware.
drivers/scsi/aic7xxx/aic79xx_pci.c | 18 +-
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/drivers/scsi/aic7xxx/aic79xx_pci.c
b/drivers/scsi/aic7xxx/aic79xx_pci.c
index 14b5f8d0e7..cc9bd26f5d 100644
--- a/drivers/scsi/aic7xxx/aic79xx_pci.c
+++ b/drivers/scsi/aic7xxx/aic79xx_pci.c
@@ -827,7 +827,7 @@ ahd_pci_intr(struct ahd_softc *ahd)
for (bit = 0; bit < 8; bit++) {
if ((pci_status[i] & (0x1 << bit)) != 0) {
- static const char *s;
+ const char *s;
s = pci_status_strings[bit];
if (i == 7/*TARG*/ && bit == 3)
@@ -887,23 +887,15 @@ ahd_pci_split_intr(struct ahd_softc *ahd, u_int intstat)
for (bit = 0; bit < 8; bit++) {
- if ((split_status[i] & (0x1 << bit)) != 0) {
- static const char *s;
-
- s = split_status_strings[bit];
- printk(s, ahd_name(ahd),
+ if ((split_status[i] & (0x1 << bit)) != 0)
+ printk(split_status_strings[bit], ahd_name(ahd),
split_status_source[i]);
- }
if (i > 1)
continue;
- if ((sg_split_status[i] & (0x1 << bit)) != 0) {
- static const char *s;
-
- s = split_status_strings[bit];
- printk(s, ahd_name(ahd), "SG");
- }
+ if ((sg_split_status[i] & (0x1 << bit)) != 0)
+ printk(split_status_strings[bit],
ahd_name(ahd), "SG");
}
}
/*
--
1.7.10.4
--
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 for-3.15] scsi/libiscsi: Fix static checker warning on bh locking
From: Shlomo Pongratz Commit 659743b "[SCSI] libiscsi: Reduce locking contention in fast path" introduced a new smatch warning on libiscsi.c "iscsi_xmit_task() warn: inconsistent returns bottom_half:: locked (1410 [(-61)]) unlocked (1425 [0], 1425 [s32min-(-1),1-s32max])", which we can eliminate by using non bh locking on the nested spin_lock call. Reported-by: Dan Carpenter Signed-off-by: Shlomo Pongratz Signed-off-by: Or Gerlitz --- drivers/scsi/libiscsi.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index 5b8605c..5087957 100644 --- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -1411,9 +1411,9 @@ static int iscsi_xmit_task(struct iscsi_conn *conn) conn->task = NULL; } /* regular RX path uses back_lock */ - spin_lock_bh(&conn->session->back_lock); + spin_lock(&conn->session->back_lock); __iscsi_put_task(task); - spin_unlock_bh(&conn->session->back_lock); + spin_unlock(&conn->session->back_lock); return rc; } -- 1.7.1 -- 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
