Re: [PATCH 1/2] libsas: Don't process sas events in static works

2017-05-21 Thread wangyijing 
Hi Dan, thanks for your review and comments!

在 2017/5/21 11:44, Dan Williams 写道:
> On Fri, May 19, 2017 at 11:39 PM, Yijing Wang  wrote:
>> Now libsas hotplug work is static, LLDD driver queue
>> the hotplug work into shost->work_q. If LLDD driver
>> burst post lots hotplug events to libsas, the hotplug
>> events may pending in the workqueue like
>>
>> shost->work_q
>> new work[PORTE_BYTES_DMAED] --> |[PHYE_LOSS_OF_SIGNAL][PORTE_BYTES_DMAED] -> 
>> processing
>> |<---wait worker to process>|
>> In this case, a new PORTE_BYTES_DMAED event coming, libsas try to queue it
>> to shost->work_q, but this work is already pending, so it would be lost.
>> Finally, libsas delete the related sas port and sas devices, but LLDD driver
>> expect libsas add the sas port and devices(last sas event).
>>
>> This patch remove the static defined hotplug work, and use dynamic work to
>> avoid missing hotplug events.
> 
> If we go this route we don't even need:
> 
> sas_port_event_fns
> sas_phy_event_fns
> sas_ha_event_fns

Yes, these three fns are not necessary, just for avoid lots kfree in 
phy/port/ha event fns.

> 
> ...just specify the target routine directly to INIT_WORK() and remove
> the indirection.
> 
> I also think for safety this should use a mempool that guarantees that
> events can continue to be processed under system memory pressure.

What I am worried about is it's would still fail if the mempool is used empty 
during memory pressure.

> Also, have you considered the case when a broken phy starts throwing a
> constant stream of events? Is there a point at which libsas should
> stop queuing events and disable the phy?

Not yet, I didn't find this issue in real case, but I agree, it's really a 
problem in some broken
hardware, I think it's not a easy problem, we could improve it step by step.

Thanks!
Yijing.

> 
> .
>

Re: [PATCH 13/25] tcm_qla2xxx: Do not allow aborted cmd to advance.

2017-05-21 Thread Nicholas A. Bellinger 
On Fri, 2017-05-19 at 14:53 -0700, Himanshu Madhani wrote:
> From: Quinn Tran 
> 
> In case of hardware queue full, commands can loop between
> TCM stack and tcm_qla2xx shim layers for retry. While command
> is waiting for retry, task mgmt can get ahead and abort the
> cmmand that encountered queue full condition. Fix this by
> dropping the command, if task mgmt has already started the
> command free process.
> 
> Cc: 
> Signed-off-by: Quinn Tran 
> Signed-off-by: Himanshu Madhani 
> ---
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
> b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index 7443e4efa3ae..07f8ad001bcb 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -686,6 +686,20 @@ static int tcm_qla2xxx_queue_status(struct se_cmd 
> *se_cmd)
>   struct qla_tgt_cmd, se_cmd);
>   int xmit_type = QLA_TGT_XMIT_STATUS;
>  
> + if (cmd->aborted) {
> + /*
> +  * Cmd can loop during Q-full. tcm_qla2xxx_aborted_task
> +  * can get ahead of this cmd. tcm_qla2xxx_aborted_task
> +  * already kick start the free.
> +  */
> + pr_debug(
> + "queue_data_in aborted cmd[%p] refcount %d transport_state 
> %x, t_state %x, se_cmd_flags %x\n",
> + cmd, kref_read(>se_cmd.cmd_kref),
> + cmd->se_cmd.transport_state, cmd->se_cmd.t_state,
> + cmd->se_cmd.se_cmd_flags);
> + return 0;
> + }
> +
>   cmd->bufflen = se_cmd->data_length;
>   cmd->sg = NULL;
>   cmd->sg_cnt = 0;

As your comment above mentions, there is a known issue in target-core
when queue-full occurs repeatably and a se_cmd descriptor is explicitly
stopped via session shutdown, TMR ABORT_TASK / LUN_RESET or otherwise.

We hit this scenario a while back in iser-target with iw_cxgb hw, which
due to a separate bug was constantly triggering queue-full under load to
uncover this same case.

So I'm still considering the different approaches to address this in
target-core proper, but don't have a problem with merging this as-is as
it won't logically conflict with any of those changes.

That said:

Acked-by: Nicholas Bellinger

Re: [PATCH 15/25] qla2xxx: Convert 32-bit LUN usage to 64-bit

2017-05-21 Thread Nicholas A. Bellinger 
On Fri, 2017-05-19 at 14:53 -0700, Himanshu Madhani wrote:
> From: Quinn Tran 
> 
> Convert 32bit LUN field to 64bit LUN.
> 
> Signed-off-by: Quinn Tran 
> Signed-off-by: Himanshu Madhani 
> ---
>  drivers/scsi/qla2xxx/qla_target.c  | 30 +-
>  drivers/scsi/qla2xxx/qla_target.h  |  4 ++--
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c |  2 +-
>  3 files changed, 16 insertions(+), 20 deletions(-)
> 

Aside from the typo to hard-code LUN=0 in __qlt_24xx_handle_abts() Bart
mentioned earlier, looks fine.

Acked-by: Nicholas Bellinger

Re: [PATCH 20/25] qla2xxx: Remove redundant code

2017-05-21 Thread Nicholas A. Bellinger 
On Fri, 2017-05-19 at 23:43 +, Bart Van Assche wrote:
> On Fri, 2017-05-19 at 14:53 -0700, Himanshu Madhani wrote:
> > From: Quinn Tran 
> > 
> > During ABTS or Abort task, qla2xxx does a pre-search for
> > the se_cmd, based on command's tag. The same search is
> > performed by TCM. Remove the extra search from qla2xxx.
> > 
> > Signed-off-by: Quinn Tran 
> > Signed-off-by: Himanshu Madhani 
> > ---
> >  drivers/scsi/qla2xxx/qla_target.c | 29 -
> >  1 file changed, 4 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/scsi/qla2xxx/qla_target.c 
> > b/drivers/scsi/qla2xxx/qla_target.c
> > index 21e8993baf4b..b8e609ae6cff 100644
> > --- a/drivers/scsi/qla2xxx/qla_target.c
> > +++ b/drivers/scsi/qla2xxx/qla_target.c
> > @@ -1836,34 +1836,13 @@ static int __qlt_24xx_handle_abts(struct 
> > scsi_qla_host *vha,
> > struct abts_recv_from_24xx *abts, struct fc_port *sess)
> >  {
> > struct qla_hw_data *ha = vha->hw;
> > -   struct se_session *se_sess = sess->se_sess;
> > struct qla_tgt_mgmt_cmd *mcmd;
> > -   struct se_cmd *se_cmd;
> > int rc;
> > -   bool found_lun = false;
> > -   unsigned long flags;
> > -
> > -   spin_lock_irqsave(_sess->sess_cmd_lock, flags);
> > -   list_for_each_entry(se_cmd, _sess->sess_cmd_list, se_cmd_list) {
> > -   if (se_cmd->tag == abts->exchange_addr_to_abort) {
> > -   found_lun = true;
> > -   break;
> > -   }
> > -   }
> > -   spin_unlock_irqrestore(_sess->sess_cmd_lock, flags);
> >  
> > -   /* cmd not in LIO lists, look in qla list */
> > -   if (!found_lun) {
> > -   if (abort_cmd_for_tag(vha, abts->exchange_addr_to_abort)) {
> > -   /* send TASK_ABORT response immediately */
> > -   qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_CMPL, false);
> > -   return 0;
> > -   } else {
> > -   ql_dbg(ql_dbg_tgt_mgt, vha, 0xf081,
> > -   "unable to find cmd in driver or LIO for tag 
> > 0x%x\n",
> > -   abts->exchange_addr_to_abort);
> > -   return -ENOENT;
> > -   }
> > +   if (abort_cmd_for_tag(vha, abts->exchange_addr_to_abort)) {
> > +   /* send TASK_ABORT response immediately */
> > +   qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_CMPL, false);
> > +   return 0;
> > }
> >  
> > ql_dbg(ql_dbg_tgt_mgt, vha, 0xf00f,
> 
> Hello Himanshu and Quinn,
> 
> Please drop this patch. If a command has already been submitted to the LIO
> core and an ABTS is received then the LIO core should be requested to perform
> the abort. This patch changes the behavior of the qla2xxx target driver such
> that the LIO core is not informed at all if abort_cmd_for_tag() finds the
> command that has to be aborted in one of the command lists maintained by the
> qla2xxx driver. That can lead to the presence of overlapping writes in the
> command set on the target system and hence to data corruption.

This analysis is wrong.

The three lists abort_cmd_for_tag() walks from __qlt_24xx_handle_abts()
are used to track descriptors only before __qlt_do_work() is reached,
and before a descriptor is submitted into tcm_qla2xxx code.

Or rather, the three lists in abort_cmd_for_tag() only contain
qla_tgt_cmd or qla_tgt_sess_op descriptors that have not yet reached
qla_tgt_func_tmpl->handle_cmd() code.

Both qlt_do_work() and qlt_create_sess_from_atio() drop their respective
descriptors from ->cmd_list before dispatching into tcm_qla2xxx ->
target-core, which means there is no way for a descriptor to be part of
internal lists once __qlt_do_work() is called.

That said, the patch is correct and removes the redundant lookup.

Acked-by: Nicholas Bellinger

Re: [PATCH 06/25] qla2xxx: Reduce excessive debug print during 27xx fwdump.

2017-05-21 Thread kbuild test robot 
Hi Joe,

[auto build test WARNING on scsi/for-next]
[also build test WARNING on v4.12-rc1 next-20170519]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Himanshu-Madhani/qla2xxx-Bug-fixes-and-cleanups/20170521-131406
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   drivers/scsi/qla2xxx/qla_init.c: In function 'qla24xx_load_risc_flash':
>> drivers/scsi/qla2xxx/qla_init.c:6360:7: warning: format '%lx' expects 
>> argument of type 'long unsigned int', but argument 5 has type 'uint32_t' 
>> [-Wformat=]
  dlen - risc_size * sizeof(*dcode));
  ^
   drivers/scsi/qla2xxx/qla_init.c: In function 'qla24xx_load_risc_blob':
   drivers/scsi/qla2xxx/qla_init.c:6662:7: warning: format '%lx' expects 
argument of type 'long unsigned int', but argument 5 has type 'uint32_t' 
[-Wformat=]
  dlen - risc_size * sizeof(*fwcode));
  ^

vim +6360 drivers/scsi/qla2xxx/qla_init.c

  6344  qla24xx_read_flash_data(vha, dcode, faddr, risc_size);
  6345  for (i = 0; i < risc_size; i++)
  6346  dcode[i] = le32_to_cpu(dcode[i]);
  6347  
  6348  if (!qla27xx_fwdt_template_valid(dcode)) {
  6349  ql_log(ql_log_warn, vha, 0x0165,
  6350  "Failed fwdump template validate\n");
  6351  goto default_template;
  6352  }
  6353  
  6354  dlen = qla27xx_fwdt_template_size(dcode);
  6355  ql_dbg(ql_dbg_init, vha, 0x0166,
  6356  "-> template size %x bytes\n", dlen);
  6357  if (dlen > risc_size * sizeof(*dcode)) {
  6358  ql_log(ql_log_warn, vha, 0x0167,
  6359  "Failed fwdump template exceeds array by %lx 
bytes\n",
> 6360  dlen - risc_size * sizeof(*dcode));
  6361  goto default_template;
  6362  }
  6363  ha->fw_dump_template_len = dlen;
  6364  return rval;
  6365  
  6366  default_template:
  6367  ql_log(ql_log_warn, vha, 0x0168, "Using default fwdump 
template\n");
  6368  if (ha->fw_dump_template)

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Viable partnership request.

2017-05-21 Thread Mr Albert Yang 
Compliment of the day,


I have access to very vital information that can be used to move a huge amount 
of money . I have done my homework very well and i have the machineries in 
place to get it done since I am still in active service. If it was possible for 
me to do it alone I would not have bothered contacting you, ultimately I need a 
honest foreigner to play an important role in the completion of this business 
deal.


Regards,
Albert.

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

2017-05-21 Thread Bart Van Assche 
On Sun, 2017-05-21 at 08:50 +0200, Christoph Hellwig wrote:
> Although we really need to stop abusing requests/cmds for EH..
> (Hannes, time to dust off your old patches!)

Hello Christoph and Hannes,

How about passing a struct scsi_device pointer to the eh_*_reset_handler
callbacks instead of a struct scsi_cmnd pointer? Most SCSI LLD
eh_*_reset_handler implementations don't do anything with the information
passed through the struct scsi_cmnd pointer except reading the SCSI device
pointer and logging the SCSI command CDB. Hannes, is this the same as your
proposal? Do you want to work on this or do you perhaps expect me to prepare
patches to implement that change?

Bart.

Re: [PATCH 02/18] bsg: Check private request size before attaching to a queue

2017-05-21 Thread Bart Van Assche 
On Sun, 2017-05-21 at 08:32 +0200, Christoph Hellwig wrote:
> And btw, I didn't get your cover letter [0/18], did that get lost
> somewhere?

Hello Christoph,

Thanks for the review comments. The cover letter should have made it to at
least the linux-scsi mailing list since it shows up in at least one archive of
that mailing list: https://www.spinics.net/lists/linux-scsi/msg108940.html.

Bart.

Re: [PATCH 06/25] qla2xxx: Reduce excessive debug print during 27xx fwdump.

2017-05-21 Thread kbuild test robot 
Hi Joe,

[auto build test WARNING on scsi/for-next]
[also build test WARNING on v4.12-rc1 next-20170519]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Himanshu-Madhani/qla2xxx-Bug-fixes-and-cleanups/20170521-131406
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/scsi/qla2xxx/qla_init.c: In function 'qla24xx_load_risc_flash':
>> drivers/scsi/qla2xxx/qla_init.c:6359:50: warning: format '%lx' expects 
>> argument of type 'long unsigned int', but argument 5 has type 'uint32_t {aka 
>> unsigned int}' [-Wformat=]
  "Failed fwdump template exceeds array by %lx bytes\n",
 ^
   drivers/scsi/qla2xxx/qla_init.c: In function 'qla24xx_load_risc_blob':
   drivers/scsi/qla2xxx/qla_init.c:6661:50: warning: format '%lx' expects 
argument of type 'long unsigned int', but argument 5 has type 'uint32_t {aka 
unsigned int}' [-Wformat=]
  "Failed fwdump template exceeds array by %lx bytes\n",
 ^

vim +6359 drivers/scsi/qla2xxx/qla_init.c

  6343  dcode = ha->fw_dump_template;
  6344  qla24xx_read_flash_data(vha, dcode, faddr, risc_size);
  6345  for (i = 0; i < risc_size; i++)
  6346  dcode[i] = le32_to_cpu(dcode[i]);
  6347  
  6348  if (!qla27xx_fwdt_template_valid(dcode)) {
  6349  ql_log(ql_log_warn, vha, 0x0165,
  6350  "Failed fwdump template validate\n");
  6351  goto default_template;
  6352  }
  6353  
  6354  dlen = qla27xx_fwdt_template_size(dcode);
  6355  ql_dbg(ql_dbg_init, vha, 0x0166,
  6356  "-> template size %x bytes\n", dlen);
  6357  if (dlen > risc_size * sizeof(*dcode)) {
  6358  ql_log(ql_log_warn, vha, 0x0167,
> 6359  "Failed fwdump template exceeds array by %lx 
> bytes\n",
  6360  dlen - risc_size * sizeof(*dcode));
  6361  goto default_template;
  6362  }
  6363  ha->fw_dump_template_len = dlen;
  6364  return rval;
  6365  
  6366  default_template:
  6367  ql_log(ql_log_warn, vha, 0x0168, "Using default fwdump 
template\n");

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH v2 3/4] scsi_dh_alua: do not call BUG_ON when updating port group

2017-05-21 Thread Christoph Hellwig 
On Mon, May 15, 2017 at 08:30:19PM +0200, Martin Wilck wrote:
> please be assured that I'm not trying to paper over anything. Your
> concern about sdev->handler_data is justified. While I think that it's
> a separate issue from what my patches were supposed to address, let me
> see if I can come up with something more comprehensive.
> 
> It will take time, though, until I fully comprehend the locking concept
> of scsi_dh_alua.c.

I think a simple call_rcu for the cleanup in alua_bus_detach should
do the job.  And I agree with Bart that we need to sort this out
properly..

Re: [PATCH blktests 0/3] Add SCSI generic test group

2017-05-21 Thread Christoph Hellwig 
On Thu, May 18, 2017 at 03:29:45PM +0200, Johannes Thumshirn wrote:
> On 05/18/2017 03:19 PM, Christoph Hellwig wrote:
> > All SG_IO test should also apply to block device nodes that support
> > the ioctl..
> > 
> 
> But these are not necessarily SG_IO tests, are they?
> 
> The test included is doesn't hit the SG_IO path in the sg driver, but
> the sg_read path.

Oh, you're right.

> 
> Of cause we can make a generic sg_io tool but IMHO it's quite convenient
> to just throw in the reproducers we get.
> 
> And as it exercises the /dev/sg character device I decided to make a new
> group.

Yeah, makes sense.  And any SG_IO test should be able to run on
both block device nodes and sg nodes.

Re: [PATCH v2] Use ctlr directly in rdac_failover_get()

2017-05-21 Thread Christoph Hellwig 
Looks fine,

Reviewed-by: Christoph Hellwig

Re: [PATCH 01/18] block: Introduce blk_queue_cmd_size()

2017-05-21 Thread Christoph Hellwig 
On Fri, May 19, 2017 at 11:29:59AM -0700, Bart Van Assche wrote:
> This function will be used by later patches in this series.

And it could already be used to simplify blk_alloc_flush_queue a bit..

Reviewed-by: Christoph Hellwig

Re: [PATCH 18/18] scsi_setup_fs_cmnd(): Call scsi_req_init() instead of open-coding it

2017-05-21 Thread Christoph Hellwig 
Looks fine,

Reviewed-by: Christoph Hellwig

Re: [PATCH 16/18] scsi-mq: Make behavior scsi_mq_prep_fn() closer to that of scsi_prep_fn()

2017-05-21 Thread Christoph Hellwig 
Looks good,

Reviewed-by: Christoph Hellwig

Re: [PATCH 17/18] scsi: Consolidate more initialization code

2017-05-21 Thread Christoph Hellwig 
Looks good,

Reviewed-by: Christoph Hellwig

Re: [PATCH 15/18] scsi: storvsc: Initialize driver-private command before using it

2017-05-21 Thread Christoph Hellwig 
On Fri, May 19, 2017 at 11:30:13AM -0700, Bart Van Assche wrote:
> The storvsc driver is the only SCSI LLD that uses driver-private
> command data and that does not zero-initialize that data before
> reading it. Make this driver consistent with the other SCSI LLDs
> that use driver-private command data.

Well.  Either we add zeroing to storvsc and remove it from common
code, or we remove the zeroing from the drivers.

We shouldn't do both.  Given that we already zero the remaining
command it seems to me like keeping the zeroing in common code
would be preferred, but I'm open to discussion.

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

2017-05-21 Thread Christoph Hellwig 
Looks fine,

Reviewed-by: Christoph Hellwig 

Although we really need to stop abusing requests/cmds for EH..
(Hannes, time to dust off your old patches!)

Re: [PATCH 14/18] scsi: Make scsi_initialize_rq() zero the entire struct scsi_cmnd

2017-05-21 Thread Christoph Hellwig 
On Fri, May 19, 2017 at 11:30:12AM -0700, Bart Van Assche wrote:
> This simplifies the memset() call in scsi_initialize_rq() and avoids
> that any stale data is left behind in struct scsi_request.
> 
> Signed-off-by: Bart Van Assche 
> Cc: Christoph Hellwig 
> Cc: Hannes Reinecke 
> ---
>  drivers/scsi/scsi_lib.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index eeb668935836..791bae192bfb 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1160,11 +1160,9 @@ static void scsi_initialize_rq(struct request *rq)
>   void *buf = cmd->sense_buffer;
>   void *prot = cmd->prot_sdb;
>  
> - /* zero out the cmd, except for the embedded scsi_request */
> - memset((char *)cmd + sizeof(cmd->req), 0,
> - sizeof(*cmd) - sizeof(cmd->req) + dev->host->hostt->cmd_size);
> + memset(cmd, 0, blk_queue_cmd_size(rq->q));
>   scsi_req_init(>req);
> - cmd->req.sense = cmd->sense_buffer;
> + cmd->req.sense = buf;
>   cmd->device = dev;
>   cmd->sense_buffer = buf;

maybe move the two sense buffer initializations together?

Otherwise this looks fine:

Reviewed-by: Christoph Hellwig

Re: [PATCH 13/18] scsi: Move sense buffer pointer initialization into scsi_initialize_rq()

2017-05-21 Thread Christoph Hellwig 
On Fri, May 19, 2017 at 11:30:11AM -0700, Bart Van Assche wrote:
> This patch is a preparation for the next patch that will zero
> the struct scsi_request embedded in struct scsi_cmnd before
> calling scsi_req_init().

Looks fine,

Reviewed-by: Christoph Hellwig

Re: [PATCH 12/18] scsi: Inline scsi_init_command()

2017-05-21 Thread Christoph Hellwig 
Looks fine,

Reviewed-by: Christoph Hellwig

Re: [PATCH 11/18] scsi: Move most of scsi_init_command() into scsi_initialize_rq()

2017-05-21 Thread Christoph Hellwig 
On Fri, May 19, 2017 at 11:30:09AM -0700, Bart Van Assche wrote:
> Move the initializations that only have to be performed once and
> not every time a request is prepared from scsi_init_command()
> into scsi_initialize_rq(). This patch also moves the
> jiffies_at_alloc assignment such that it gets back the meaning it
> had before commit e9c787e65c0c, namely the value of the jiffies
> counter at request allocation time.

Looks fine,

Reviewed-by: Christoph Hellwig

Re: [PATCH 11/18] scsi: Move most of scsi_init_command() into scsi_initialize_rq()

2017-05-21 Thread Christoph Hellwig 
On Sun, May 21, 2017 at 08:45:23AM +0200, Christoph Hellwig wrote:
> On Fri, May 19, 2017 at 11:30:09AM -0700, Bart Van Assche wrote:
> > Move the initializations that only have to be performed once and
> > not every time a request is prepared from scsi_init_command()
> > into scsi_initialize_rq(). This patch also moves the
> > jiffies_at_alloc assignment such that it gets back the meaning it
> > had before commit e9c787e65c0c, namely the value of the jiffies
> > counter at request allocation time.
> 
> How does this account for the kmalloced request in scsi_ioctl_reset()?

Oh, because that currently manually calls blk_rq_init..

Re: [PATCH 11/18] scsi: Move most of scsi_init_command() into scsi_initialize_rq()

2017-05-21 Thread Christoph Hellwig 
On Fri, May 19, 2017 at 11:30:09AM -0700, Bart Van Assche wrote:
> Move the initializations that only have to be performed once and
> not every time a request is prepared from scsi_init_command()
> into scsi_initialize_rq(). This patch also moves the
> jiffies_at_alloc assignment such that it gets back the meaning it
> had before commit e9c787e65c0c, namely the value of the jiffies
> counter at request allocation time.

How does this account for the kmalloced request in scsi_ioctl_reset()?

Re: [PATCH 10/18] scsi: Only add commands to the device command list if required by the LLD

2017-05-21 Thread Christoph Hellwig 
Looks fine,

Reviewed-by: Christoph Hellwig

Re: [PATCH 09/18] scsi: Change argument type of scsi_req_init()

2017-05-21 Thread Christoph Hellwig 
Looks fine,

Reviewed-by: Christoph Hellwig

Re: [PATCH 08/18] block: Make scsi_req_init() calls implicit

2017-05-21 Thread Christoph Hellwig 
On Fri, May 19, 2017 at 11:30:06AM -0700, Bart Van Assche wrote:
> Instead of explicitly calling scsi_req_init(), let
> blk_get_request() call that function from inside blk_rq_init().
> Add an .initialize_rq_fn() callback function to the block drivers
> that need it.

Thanks Bart,

this looks like a very nice cleanup.

> Merge the IDE .init_rq_fn() function into
> .initialize_rq_fn() because it is too small to keep it as a
> separate function.

Although we can't do this IFF we don't call .initialize_rq_fn from
the fast path (which I think we should).

Re: [PATCH 07/18] block: Introduce request_queue.initialize_rq_fn()

2017-05-21 Thread Christoph Hellwig 
On Fri, May 19, 2017 at 11:30:05AM -0700, Bart Van Assche wrote:
> Several block drivers need to initialize the driver-private data
> after having called blk_get_request() and before .prep_rq_fn() is
> called, e.g. when submitting a REQ_OP_SCSI_* request. Avoid that
> that initialization code has to be repeated after every
> blk_get_request() call by adding a new callback function to struct
> request_queue.
> 
> Signed-off-by: Bart Van Assche 
> Cc: Jens Axboe 
> Cc: Christoph Hellwig 
> Cc: Omar Sandoval 
> Cc: Hannes Reinecke 
> Cc: linux-bl...@vger.kernel.org
> ---
>  block/blk-core.c   | 3 +++
>  block/blk-mq.c | 3 +++
>  include/linux/blkdev.h | 4 
>  3 files changed, 10 insertions(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index a69d420b7ff0..f2540d164679 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -126,6 +126,9 @@ void blk_rq_init(struct request_queue *q, struct request 
> *rq)
>   rq->start_time = jiffies;
>   set_start_time_ns(rq);
>   rq->part = NULL;
> +
> + if (q->initialize_rq_fn)
> + q->initialize_rq_fn(rq);

Can we keep this out of the fast path and only do it from the
blk_get_request / blk_mq_alloc_request path?  And while were at it
I think those two should be merged as far as the public interface
goes, that is we should also expose the flags argument for the
legacy path.

Re: [PATCH 02/18] bsg: Check private request size before attaching to a queue

2017-05-21 Thread Christoph Hellwig 
Hi Bart,

I think this is the wrong kind of check - while we do care about the
size of the queue, we only do it as a side effect of the queue
being able to handle REQ_OP_SCSI_IN/REQ_OP_SCSI_OUT commands.

I think we'll need a flag for those in the queue instead.

And btw, I didn't get your cover letter [0/18], did that get lost
somewhere?