Re: [PATCH] tcmu: Fix trailing semicolon

[Michael Christie]

On 01/16/2018 09:34 AM, Luis de Bethencourt wrote:
> The trailing semicolon is an empty statement that does no operation.
> It is completely stripped out by the compiler. Removing it since it doesn't do
> anything.
> 
> Signed-off-by: Luis de Bethencourt 
> ---
> 
> Hi,
> 
> After fixing the same thing in drivers/staging/rtl8723bs/, Joe Perches
> suggested I fix it treewide [0].
> 
> Best regards 
> Luis
> 
> 
> [0] 
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-January/115410.html
> [1] 
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-January/115390.html
> 
>  drivers/target/target_core_user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_user.c 
> b/drivers/target/target_core_user.c
> index 60c8a87b7a88..f6164d294fb3 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -1564,7 +1564,7 @@ static int tcmu_wait_genl_cmd_reply(struct tcmu_dev 
> *udev)
>  
>   wake_up_all(>nl_cmd_wq);
>  
> - return ret;;
> + return ret;
>  }
>  
>  static int tcmu_netlink_event(struct tcmu_dev *udev, enum tcmu_genl_cmd cmd,
> 


Acked-by: Mike Christie 

Re: [Bug 111441] New: iscsi fails to attach to targets

[Michael Christie]

Can you send me your lio config?

Are you running lio on the same box as the initiator so it is also changing 
kernel version with each test?

It looks like the scsi scan code is not handling the lack of LUN 0 correctly. I 
think I know what patch is causing it, but cannot get my lio config to return 
the same info as you for the non existent LUNs, so I think I am hitting 
slightly different error paths.


> On Jan 28, 2016, at 8:55 PM, Serguei Bezverkhi (sbezverk) 
>  wrote:
> 
> HI Mike,
> 
> Thank you for looking into this issue.
> I reproduced this issue  this using both mainline compiled kernel and the one 
> posted by El Repo.  I used both of these kernels with RHEL 7.2, both kernels 
> showed exactly the same issue. When I rollback to the original kernel 
> 3.10.0-327.4.4, I do not see the issue.
> 
> As per your request attaching kernel config file. Please let me know if you 
> need any additional info or if you want to take a look at the router, I can 
> setup a webex meeting to show you the issue.
> 
> Thank you
> 
> Serguei
> 
> 
> 
> 
> Serguei Bezverkhi,
> TECHNICAL LEADER.SERVICES
> Global SP Services
> sbezv...@cisco.com
> Phone: +1 416 306 7312
> Mobile: +1 514 234 7374
> 
> CCIE (R,SP,Sec) - #9527
> 
> Cisco.com
> 
> 
> 
>  Think before you print.
> This email may contain confidential and privileged material for the sole use 
> of the intended recipient. Any review, use, distribution or disclosure by 
> others is strictly prohibited. If you are not the intended recipient (or 
> authorized to receive for the recipient), please contact the sender by reply 
> email and delete all copies of this message.
> Please click here for Company Registration Information.
> 
> 
> 
> 
> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org 
> [mailto:linux-scsi-ow...@vger.kernel.org] On Behalf Of Mike Christie
> Sent: Thursday, January 28, 2016 8:53 PM
> To: bugzilla-dae...@bugzilla.kernel.org; linux-scsi@vger.kernel.org
> Subject: Re: [Bug 111441] New: iscsi fails to attach to targets
> 
> On 01/28/2016 04:51 PM, bugzilla-dae...@bugzilla.kernel.org wrote:
>> https://bugzilla.kernel.org/show_bug.cgi?id=111441
>> 
>>Bug ID: 111441
>>   Summary: iscsi fails to attach to targets
>>   Product: IO/Storage
>>   Version: 2.5
>>Kernel Version: 4.4.0-1
>>  Hardware: x86-64
>>OS: Linux
>>  Tree: Mainline
>>Status: NEW
> 
> 
>> 4.4.0-1.el7.elrepo.x86_64 #1
> 
> I have not seen this oops before. We saw similar ones around 5 or 6 years 
> ago, but they were due to some sysfs or block or scsi issue (I cannot 
> remember exacty) and fixed there.
> 
> Is this a distro kernel or mainline. BZ says mainline, but the kernel name in 
> bug looks like a red hat related one. Where did you get it?
> 
> I do not hit this in 4.4 mainline. Send me your kernel .config, so I can make 
> sure I have the same options.
> 
> --
> 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


config-4.4.0
Description: Binary data
> 

Re: [Bug 111441] New: iscsi fails to attach to targets

[Michael Christie]

> On Jan 29, 2016, at 6:04 AM, Serguei Bezverkhi (sbezverk) 
>  wrote:
> 
> Actually this server uses both cases: Local taregts (since it is OpenStack 
> server) and remote targets as it tries to mount 4 remotefile systems.  
> 
> You are correct, I always use the same box I just change the kernel it is 
> using to boot. No other changes to the environment. I do not mind to load a 
> test kernel without that suspected patch, just get me the RPM.
> 

I do not know what you mean. I think the patch I sent will fix the sysfs errors 
caused due to alua not being setup properly on your system and scsi_dh_alua 
failing to attach. That patch should be applied to the 4.4 upstream kernel. Are 
you saying you want me to make you a kernel rpm?--
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/1] iscsi: fix regression caused by session lock patch

[Michael Christie]

> On Nov 15, 2015, at 4:10 AM, Or Gerlitz  wrote:
> 
> On Fri, Nov 13, 2015 at 6:51 PM, Mike Christie  wrote:
>> On 11/13/2015 09:06 AM, Or Gerlitz wrote:
 The patch has caused multiple regressions, did not even compile when
> sent to me, and was poorly reviewed and I have not heard from you guys
> in a week. Given the issues the patch has had and the current time, I do
> not feel comfortable with it anymore. I want to re-review it and fix it
> up when there is more time.
>>> Mike (Hi),
>>> 
>>> It's a complex patch that touches all the iscsi transports, and yes,
>>> when it was send to you the 1st time, there was build error on one of
>>> the offload transports (bad! but happens) and yes, as you pointed, one
>>> static checker fix + one bug fix for it went upstream after this has
>>> been merged, happens too.
>> 
>> A patch should not cause this many issues.
>> 
>>> What makes you say it was poorly reviewed?
>> 
>> I just did not do a good job at looking at the patch. I should have
>> caught all of these issues.
>> 
>> - The bnx2i cleanup_task bug should have been obvious, especially for me
>> because I had commented about the back lock and the abort path.
>> 
>> - This oops, was so basic. Incorrect locking around a linked list being
>> accessed from 2 threads is really one of those 1st year kernel
>> programmer things.
> 
> Mike, Chris
> 
> After the locking change, adding a task to any of the connection
> mgmtqueue, cmdqueue, or requeue lists is under the session forward lock.
> 
> Removing tasks from any of these lists in iscsi_data_xmit is under
> the session forward lock and **before** calling down to the transport
> to handle the task.
> 
> The iscsi_complete_task helper was added by Mike's commit
> 3bbaaad95fd38ded "[SCSI] libiscsi: handle cleanup task races"
> and is indeed typically called under the backward lock && has this section
> 
> +   if (!list_empty(>running))
> +   list_del_init(>running);
> 
> which per my reading of the code never comes into play, can you comment?


I had sent this to Sagi and your mellanox email the other day:


> The bug occurs when a target completes a command while we are still
> processing it. If we are doing a WRITE and the iscsi_task
> is on the cmdqueue because we are handling a R2T. The target shouldn't
> send a Check Condition at this time, but some do. If that happens, then
> iscsi_queuecommand could be adding a new task to the cmdqueue, while the
> recv path is handling the CC for the task with the outsanding R2T.  The
> recv path iscsi_complete_task call sees that task it on the cmdqueue and
> deletes it from the list at the same time iscsi_queuecommand is adding a new
> task.
> 
> This should not happen per the iscsi spec. There is some wording about
> waiting to finish the sequence in progress, but targets goof this up.




--
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 0/7] be2iscsi driver update to 10.6.0.0

[Michael Christie]

On Apr 24, 2015, at 9:46 PM, John Soni Jose sony.joh...@emulex.com wrote:

 The patchset updates be2iscsi driver to 10.6.0.0 version.
 These patches are generated aganist scsi for-next branch
 
 John Soni Jose (7):
  be2iscsi : Fix the retry count for boot targets
  be2iscsi : Fix the PCI request region reserving.
  be2iscsi : Fix memory leak in the unload path
  be2iscsi : Fix memory check before unmapping.
  be2iscsi : Update the copyright year
  be2iscsi : Logout of FW Boot Session
  be2iscsi : Bump the driver version
 
 drivers/scsi/be2iscsi/be.h   |2 +-
 drivers/scsi/be2iscsi/be_cmds.c  |6 ++-
 drivers/scsi/be2iscsi/be_cmds.h  |   14 ++-
 drivers/scsi/be2iscsi/be_iscsi.c |2 +-
 drivers/scsi/be2iscsi/be_iscsi.h |2 +-
 drivers/scsi/be2iscsi/be_main.c  |   78 +
 drivers/scsi/be2iscsi/be_main.h  |8 +++-
 drivers/scsi/be2iscsi/be_mgmt.c  |   71 ++-
 drivers/scsi/be2iscsi/be_mgmt.h  |5 ++-
 9 files changed, 162 insertions(+), 26 deletions(-)
 

Looks ok to me.

Reviewed-by: Mike Christie micha...@cs.wisc.edu--
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/1] be2iscsi: add block valid bit to iBFT flag

[Michael Christie]

On Jan 15, 2015, at 7:26 PM, Minh Duc Tran minhduc.t...@emulex.com wrote:

 -Original Message-
 From: Mike Christie [mailto:micha...@cs.wisc.edu] 
 Sent: Thursday, January 15, 2015 3:17 PM
 To: Minh Duc Tran; linux-scsi@vger.kernel.org
 Cc: Jayamohan Kallickal
 Subject: Re: [PATCH 1/1] be2iscsi: add block valid bit to iBFT flag
 
 On 01/12/2015 05:05 PM, Minh Duc Tran wrote:
 
 From: Minh Duc Tran
 Sent: Sunday, November 09, 2014 10:52 PM
 To: 'linux-scsi@vger.kernel.org'
 Cc: Mike Christie (micha...@cs.wisc.edu); Jayamohan Kallickal
 Subject: [PATCH 1/1] be2iscsi: add block valid bit to iBFT flag
 
 From: Minh Tran minhduc.t...@emulex.com
 
  We are starting to see problems with certain open-iscsi 
 versions out there checking block valid bit.  Iscsi boot target login will 
 not happen without this bit being set.
 
 Signed-off-by: Minh Tran minhduc.t...@emulex.com
 ---
 scsi/be2iscsi/be_main.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/scsi/be2iscsi/be_main.c b/scsi/be2iscsi/be_main.c index 
 30d74a0..aacf223 100644
 --- a/scsi/be2iscsi/be_main.c
 +++ b/scsi/be2iscsi/be_main.c
 @@ -429,7 +429,7 @@ static ssize_t beiscsi_show_boot_tgt_info(void *data, 
 int type, char *buf)
auth_data.chap.intr_secret);
   break;
   case ISCSI_BOOT_TGT_FLAGS:
 -   rc = sprintf(str, 2\n);
 +   rc = sprintf(str, 3\n);
   break;
   case ISCSI_BOOT_TGT_NIC_ASSOC:
   rc = sprintf(str, 0\n); @@ -466,7 +466,7 @@ static 
 ssize_t beiscsi_show_boot_eth_info(void *data, int type, char *buf)
 
   switch (type) {
   case ISCSI_BOOT_ETH_FLAGS:
 -   rc = sprintf(str, 2\n);
 +   rc = sprintf(str, 3\n);
   break;
   case ISCSI_BOOT_ETH_INDEX:
   rc = sprintf(str, 0\n);
 --
 1.7.1
 
 Hi Mike,
 I think we have lost track of this patch.  I've checked 3.19-rc3 today and 
 it's not in yet.  Can we get this in as soon as possible?
 
 
 The thread got a little busted in my mailer, so I might have missed 
 something.
 
 Did you see my review comments:
 https://www.marc.info/?l=linux-scsim=141564103903024w=2
 here?
 
 I am not seeing a follow up patch or comment from you saying you did not 
 want to do it that way. Did you send that to the list in a new thread?
 If so, I lost it.
 
 Mike, nothing is pending from me for this thread.  I've responded that this 
 issue is seen with Suse (sles12).  I think the missing patch you have been 
 waiting for is from Qlogic for ql4_def.h.
 Vikas Chaudhary from Q was supposed to send you but my mailer said he is no 
 longer at Q.

I think we are misunderstanding each other on my comments. You need to modify 
your patch so it fixes the problem in a general way for all drivers.

--
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] iSCSI MQ adoption via MCS discussion

[Michael Christie]

On Jan 8, 2015, at 11:03 PM, Nicholas A. Bellinger n...@linux-iscsi.org wrote:

 On Thu, 2015-01-08 at 15:22 -0800, James Bottomley wrote:
 On Thu, 2015-01-08 at 14:57 -0800, Nicholas A. Bellinger wrote:
 On Thu, 2015-01-08 at 14:29 -0800, James Bottomley wrote:
 On Thu, 2015-01-08 at 14:16 -0800, Nicholas A. Bellinger wrote:
 
 SNIP
 
 The point is that a simple session wide counter for command sequence
 number assignment is significantly less overhead than all of the
 overhead associated with running a full multipath stack atop multiple
 sessions.
 
 I don't see how that's relevant to issue speed, which was the measure we
 were using: The layers above are just a hopper.  As long as they're
 loaded, the MQ lower layer can issue at full speed.  So as long as the
 multipath hopper is efficient enough to keep the queues loaded there's
 no speed degradation.
 
 The problem with a sequence point inside the MQ issue layer is that it
 can cause a stall that reduces the issue speed. so the counter sequence
 point causes a degraded issue speed over the multipath hopper approach
 above even if the multipath approach has a higher CPU overhead.
 
 Now, if the system is close to 100% cpu already, *then* the multipath
 overhead will try to take CPU power we don't have and cause a stall, but
 it's only in the flat out CPU case.
 
 Not to mention that our iSCSI/iSER initiator is already taking a session
 wide lock when sending outgoing PDUs, so adding a session wide counter
 isn't adding any additional synchronization overhead vs. what's already
 in place.
 
 I'll leave it up to the iSER people to decide whether they're redoing
 this as part of the MQ work.
 
 
 Session wide command sequence number synchronization isn't something to
 be removed as part of the MQ work.  It's a iSCSI/iSER protocol
 requirement.
 
 That is, the expected + maximum sequence numbers are returned as part of
 every response PDU, which the initiator uses to determine when the
 command sequence number window is open so new non-immediate commands may
 be sent to the target.
 
 So, given some manner of session wide synchronization is required
 between different contexts for the existing single connection case to
 update the command sequence number and check when the window opens, it's
 a fallacy to claim MC/S adds some type of new initiator specific
 synchronization overhead vs. single connection code.

I think you are assuming we are leaving the iscsi code as it is today.

For the non-MCS mq session per CPU design, we would be allocating and binding 
the session and its resources to specific CPUs. They would only be accessed by 
the threads on that one CPU, so we get our serialization/synchronization from 
that. That is why we are saying we do not need something like 
atomic_t/spin_locks for the sequence number handling for this type of 
implementation.

If we just tried to do this with the old code where the session could be 
accessed on multiple CPUs then you are right, we need locks/atomics like how we 
do in the MCS case.

--
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: iSCSI request keep rejected by microsoft iSCSI target because of write_same check

[Michael Christie]

Hey Vaughan,

This was the issue we worked with Microsoft on wasn’t it? You guys figured out 
it was fixed in their target in a recent release right?

On Dec 1, 2014, at 3:01 AM, vaughan vaughan@oracle.com wrote:

 On 12/01/2014 03:25 PM, j...@deti74.ru wrote:
 I has some sproblem and only one ISCSI target - Microsoft SWT
 I disable write_same for iscsi by patching kernel (3.17.0)
 This method is not good.  Below is the answer from Mike when I asked if
 we can disable write_same in the template before.
 
 If you wanted to disable commands you would want to do it per device or
 target at the scsi layer, because there are or will be iscsi targets
 that support it.
 
 Also Martin suggests the target should return a check condition of
 ILLEGAL REQUEST if it doesn't support a specific command.
 It's a bug that only happens with Microsoft target, not others like tgtd.
 
 
 
 Patch is simple add .no_write_same = 1 param, into scsi_host_template
 struct.
 
 file drivers/scsi/iscsi_tcp.c :
 
 static struct scsi_host_template iscsi_sw_tcp_sht = {
  .module= THIS_MODULE,
  .name= iSCSI Initiator over TCP/IP,
  .queuecommand   = iscsi_queuecommand,
  .change_queue_depth= iscsi_change_queue_depth,
  .can_queue= ISCSI_DEF_XMIT_CMDS_MAX - 1,
  .sg_tablesize= 4096,
  .max_sectors= 0x,
  .cmd_per_lun= ISCSI_DEF_CMD_PER_LUN,
  .eh_abort_handler   = iscsi_eh_abort,
  .eh_device_reset_handler= iscsi_eh_device_reset,
  .eh_target_reset_handler = iscsi_eh_recover_target,
  .use_clustering = DISABLE_CLUSTERING,
 + .no_write_same= 1,
  .slave_alloc= iscsi_sw_tcp_slave_alloc,
  .slave_configure= iscsi_sw_tcp_slave_configure,
  .target_alloc= iscsi_target_alloc,
  .proc_name= iscsi_tcp,
  .this_id= -1,
  };
 
 
 суббота, 11 января 2014 г., 6:22:46 UTC+5 пользователь Vaughan Cao
 написал:
 
 
On 2014年01月11日 04:24, Mike Christie wrote:
 On 01/10/2014 02:09 AM, vaughan wrote:
 On 01/10/2014 03:41 PM, Mike Christie wrote:
 On 1/10/14 12:11 AM, vaughan wrote:
 I haven't figure out why it's rejected with bookmark
invalid(9)
 reason, rather than command not supported. IMO bookmark
invalid is
 used when minor protocol conflict such as final flag not set
with
 non-write command. However, I haven't find error of this kind in
 report_opcode, so I guess it's not supported on the target.
 
 Is it possible to get a wireshark/tcpdump trace? It does not
have to
 be during boot. We just need to see what commands are sent and
the
 response the target is returning.
 
 I forgot we know some microsoft iscsi target people. We can
just email
 them with the trace to confirm what is going on with the
target. The
 trace seems to be easier for them than them interpreting linux
kernel
 logs.
 I enabled debug_iscsi_tcp, here is a more detailed log in
normal connection.
 Does conn error (1020) mean it's target peer who disconnect the
 connection at the same time of reject report_opcode?
 Yes.
 
 If it is, I think iSCSI boot failure can't be avoided without
disable
 write_same check on OEL.
 Yes, you are right. Due to how more distros do boot, iscsid will
not be
 up and you will hang. If are talking about disablement though I
think it
 should not be done at the iscsi layer. It should be some sort of
 white/black list at the scsi device layer or something like that.
 
 However, I will ping Microsoft and cc you and we can see what is
up for
 sure. Maybe we will get lucky and they will have a release with
a fix on
 their side.
 
Target:
   Windows Server 2008 R2 DataCenter
   Microsoft iSCSI Software Target: 3.3.16563.
Initiator:
   kernel with write_same check in sd, such as Kernel
3.11.10-200.fc19.x86_64  
 
 
 -- 
 You received this message because you are subscribed to the Google Groups 
 open-iscsi group.
 To unsubscribe from this group and stop receiving emails from it, send an 
 email to open-iscsi+unsubscr...@googlegroups.com.
 To post to this group, send email to open-is...@googlegroups.com.
 Visit this group at http://groups.google.com/group/open-iscsi.
 For more options, visit https://groups.google.com/d/optout.
 

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

[Michael Christie]

On Jun 25, 2014, at 6:35 AM, Christoph Hellwig h...@infradead.org wrote:

 On Wed, Jun 25, 2014 at 01:32:39PM +0300, Sagi Grimberg wrote:
 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 confirms what I wrote above, so AFAICT no additional fix is required
 for libiscsi wrt bidi commands support.
 
 Good to know.  I'd really prefer just going with the fix from Martin
 that I have already queued up for 3.16, and then we can have the fully
 fledged out new scsi_transfer_length() for 3.17.
 

I am fine with this too.

I was way off track. I was more concerned with not wanting to use multiple 
functions/macros to get the transfer len. That should definitely be done when 
there is more time.
--
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

[Michael Christie]

On Jun 24, 2014, at 7:53 AM, Martin K. Petersen martin.peter...@oracle.com 
wrote:

 Mike == Mike Christie micha...@cs.wisc.edu writes:
 
 Mike The problem is WRITE_SAME requests are setup so that
 Mike req-__data_len is the value of the entire request when the setup
 Mike is completed but during the setup process it's value changes
 
 Oh, I see. So things break because iSCSI uses scsi_transfer_length()
 where the scatterlist length was used in the past.
 
 How about this?
 
 
 SCSI: Use SCSI data buffer length to extract transfer size
 
 Commit 8846bab180fa introduced a helper that can be used to query the
 wire transfer size for a SCSI command taking protection information into
 account.
 
 However, some commands do not have a 1:1 mapping between the block range
 they work on and the payload size (discard, write same). After the
 scatterlist has been set up these requests use __data_len to store the
 number of bytes to report completion on. This means that callers of
 scsi_transfer_length() would get the wrong byte count for these types of
 requests.
 
 To overcome this we make scsi_transfer_length() use the scatterlist
 length in the scsi_data_buffer as basis for the wire transfer
 calculation instead of __data_len.
 
 Reported-by: Christoph Hellwig h...@infradead.org
 Debugged-by: Mike Christie micha...@cs.wisc.edu
 Signed-off-by: Martin K. Petersen martin.peter...@oracle.com
 
 diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
 index 42ed789ebafc..e0ae71098144 100644
 --- a/include/scsi/scsi_cmnd.h
 +++ b/include/scsi/scsi_cmnd.h
 @@ -318,7 +318,7 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd, 
 char status)
 
 static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
 {
 - unsigned int xfer_len = blk_rq_bytes(scmd-request);
 + unsigned int xfer_len = scsi_out(scmd)-length;
   unsigned int prot_op = scsi_get_prot_op(scmd);
   unsigned int sector_size = scmd-device-sector_size;


Do we need to check for the data direction. Something like

if (scmd-sc_data_direction == DMA_TO_DEVICE)
xfer_len = scsi_out(scmnd)-length;
else
xfer_len = scsi_in(scmnd)-length;

--
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/1] be2iscsi: remove potential junk pointer free

[Michael Christie]

On Jun 6, 2014, at 7:22 AM, Tomas Henzl the...@redhat.com wrote:

 commit 0e7c60c [SCSI] be2iscsi: fix memory leak in error path
 fixed an potential junk pointer free if  mgmt_get_if_info() returned an error
 
 fix it on one more place
 
 Signed-off-by: Tomas Henzl the...@redhat.com
 ---
 diff --git a/drivers/scsi/be2iscsi/be_mgmt.c b/drivers/scsi/be2iscsi/be_mgmt.c
 index 6045aa7..07934b0 100644
 --- a/drivers/scsi/be2iscsi/be_mgmt.c
 +++ b/drivers/scsi/be2iscsi/be_mgmt.c
 @@ -1008,10 +1008,8 @@ int mgmt_set_ip(struct beiscsi_hba *phba,
   BE2_IPV6 : BE2_IPV4 ;
 
   rc = mgmt_get_if_info(phba, ip_type, if_info);
 - if (rc) {
 - kfree(if_info);
 + if (rc)
   return rc;
 - }
 
   if (boot_proto == ISCSI_BOOTPROTO_DHCP) {
   if (if_info-dhcp_state) {

Reviewed-by: Mike Christie micha...@cs.wisc.edu

--
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: CmdSN greather than MaxCmdSN protocol error in LIO Iser

[Michael Christie]

On Nov 12, 2013, at 12:18 PM, Moussa Ba (moussaba) mouss...@micron.com wrote:
 
 I am attaching error codes on the inititator that seem to coincide with the 
 timeout errors.
 
 [  261.855254]  connection6:0: pdu (op 0x65 itt 0x1) rejected. Reason code 0x4
 [  261.855266]  connection6:0: pdu (op 0xa itt 0x1) rejected. Reason code 0x4
 [  261.855268]  connection6:0: pdu (op 0x51 itt 0x1) rejected. Reason code 0x4
 


The initiator does not handle rejects. This is actually the 2nd time I have
seen one in 10 years. It logs them just lets the command timeout.--
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: CmdSN greather than MaxCmdSN protocol error in LIO Iser

[Michael Christie]

 On Nov 11, 2013, at 7:31 PM, Nicholas A. Bellinger n...@linux-iscsi.org 
 wrote:
 
 On Mon, 2013-11-11 at 16:48 -0800, Nicholas A. Bellinger wrote:
 On Mon, 2013-11-11 at 16:42 -0800, Nicholas A. Bellinger wrote:
 On Mon, 2013-11-11 at 13:17 -0800, Nicholas A. Bellinger wrote:
 Hi Moussa  Co,
 
 On Mon, 2013-11-11 at 19:05 +, Moussa Ba (moussaba) wrote:
 My system setup is as follows:
 
 Target: CentOS 6.4 - LIO Target running 3.12 kernel, 8 PCIe SSD in one 
 volume group, 8 logical volumes, 8 targets, 1 LUN/target.
 Initiator: CentOS 6.4 running 3.11 Kernel (Also ran 2.6.32-358), ISER 
 based initiator over Mellanox 40Gbit ConnectX HCA
 
 When running performance tests on the initiator, I am running into
 fio timeouts that lead to ABORT_TASK commands on the target.  In other
 words, fio fails to reap all the io threads almost as if it is
 waiting for lost IOs to complete. This is happening on random write IO
 operations.  Some context, we are generating about 576KIOPS 4KB block
 sizes using 8 LUNS.  The PCIe SSD have a write buffer that can absorb
 writes with an end to end latency on the initiator of 44us.  We are
 not currently seeing any errors on read IOs, which tend to have 2X+
 the latency of writes.  
 
 See below for the dmesg on the target side.
 Timeout Condition occurs at 154 which is the Protocol Error after fio is 
 interrupted or runs to completion.  
 [  154.453663] Received CmdSN: 0x000fcbb7 is greater than MaxCmdSN: 
 0x000fcbb6, protocol error.
 [  154.453673] Received CmdSN: 0x000fcbb8 is greater than MaxCmdSN: 
 0x000fcbb6, protocol error.
 
 (CC'ing Mike)
 
 As mentioned off-list, this would tend to indicate some manner of
 open-iscsi bug, as it's never legal for an initiator to send a CmdSN
 greater than the MaxCmdSN that's currently being sent in target outgoing
 response PDUs.
 
 Mike, any idea as to how this might be happening on the initiator
 side..?
 
 SNIP
 
 M, just noticed a bit of very suspicious logic in open-iscsi.
 (CC'ing linux-scsi)
 
 Namely, how drivers/scsi/libiscsi.c:iscsi_data_xmit() attempts to drain
 conn-cmdqueue and hence call iscsi_prep_scsi_cmd_pdu() + increment
 session-cmdsn, without ever checking to see if the CmdSN window may
 have already closed..
 
 The only CmdSN window check that I see in the I/O dispatch codepath is
 in iscsi_queuecommand() - iscsi_check_cmdsn_window_closed(), but once
 iscsi_conn_queue_work() is invoked and process context in
 iscsi_xmitworker() - iscsi_data_xmit() starts to execute, there is
 AFAICT no further CmdSN window open checks to ensure the initiator does
 not overflow MaxCmdSN..
 
 This would very much line up with the type of bug that is being
 reported.  Before trying to determine what a possible fix might look
 like, can you try the following libiscsi patch to see if your able to
 hit either of the BUG's below..?
 
 Thanks,
 
 --nab
 
 diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
 index e399561..9106f63 100644
 --- a/drivers/scsi/libiscsi.c
 +++ b/drivers/scsi/libiscsi.c
 @@ -1483,6 +1483,12 @@ check_mgmt:
fail_scsi_task(conn-task, DID_IMM_RETRY);
continue;
}
 +
 +   if (iscsi_check_cmdsn_window_closed(conn)) {
 +   printk(CmdSN window already closed in 
 iscsi_data_xmit #1\n);
 +   BUG();
 +   }
 +
rc = iscsi_prep_scsi_cmd_pdu(conn-task);
if (rc) {
if (rc == -ENOMEM || rc == -EACCES) {
 @@ -1518,6 +1524,11 @@ check_mgmt:
if (iscsi_check_tmf_restrictions(task, ISCSI_OP_SCSI_DATA_OUT))
break;
 
 +   if (iscsi_check_cmdsn_window_closed(conn)) {
 +   printk(CmdSN window already closed in 
 iscsi_data_xmit #2\n);
 +   BUG();
 +   }
 +
conn-task = task;
list_del_init(conn-task-running);
conn-task-state = ISCSI_TASK_RUNNING;
 
 

If you hit a bug there then it is probably the target or if you are using the 
new libiscsi back/frwd lock patches it could be related to them. We should not 
need to check above because we never queue more cmds from the scsi layer than 
the window will allow at the time. If the window is 10, the queued_cmdsn check 
should make sure we have only accepted 10 commands into the iscsi layer so we 
should not need to recheck above.

You should just enable libiscsi debug printk in iscsi_check_cmdsn_window so we 
can see the sn related values at the time of queuing. You should enable that 
printk whithout your 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: CmdSN greather than MaxCmdSN protocol error in LIO Iser

[Michael Christie]

 On Nov 11, 2013, at 8:32 PM, Michael Christie micha...@cs.wisc.edu wrote:
 
 
 
 On Nov 11, 2013, at 7:31 PM, Nicholas A. Bellinger n...@linux-iscsi.org 
 wrote:
 
 On Mon, 2013-11-11 at 16:48 -0800, Nicholas A. Bellinger wrote:
 On Mon, 2013-11-11 at 16:42 -0800, Nicholas A. Bellinger wrote:
 On Mon, 2013-11-11 at 13:17 -0800, Nicholas A. Bellinger wrote:
 Hi Moussa  Co,
 
 On Mon, 2013-11-11 at 19:05 +, Moussa Ba (moussaba) wrote:
 My system setup is as follows:
 
 Target: CentOS 6.4 - LIO Target running 3.12 kernel, 8 PCIe SSD in one 
 volume group, 8 logical volumes, 8 targets, 1 LUN/target.
 Initiator: CentOS 6.4 running 3.11 Kernel (Also ran 2.6.32-358), ISER 
 based initiator over Mellanox 40Gbit ConnectX HCA
 
 When running performance tests on the initiator, I am running into
 fio timeouts that lead to ABORT_TASK commands on the target.  In other
 words, fio fails to reap all the io threads almost as if it is
 waiting for lost IOs to complete. This is happening on random write IO
 operations.  Some context, we are generating about 576KIOPS 4KB block
 sizes using 8 LUNS.  The PCIe SSD have a write buffer that can absorb
 writes with an end to end latency on the initiator of 44us.  We are
 not currently seeing any errors on read IOs, which tend to have 2X+
 the latency of writes.  
 
 See below for the dmesg on the target side.
 Timeout Condition occurs at 154 which is the Protocol Error after fio is 
 interrupted or runs to completion.  
 [  154.453663] Received CmdSN: 0x000fcbb7 is greater than MaxCmdSN: 
 0x000fcbb6, protocol error.
 [  154.453673] Received CmdSN: 0x000fcbb8 is greater than MaxCmdSN: 
 0x000fcbb6, protocol error.
 
 (CC'ing Mike)
 
 As mentioned off-list, this would tend to indicate some manner of
 open-iscsi bug, as it's never legal for an initiator to send a CmdSN
 greater than the MaxCmdSN that's currently being sent in target outgoing
 response PDUs.
 
 Mike, any idea as to how this might be happening on the initiator
 side..?
 
 SNIP
 
 M, just noticed a bit of very suspicious logic in open-iscsi.
 (CC'ing linux-scsi)
 
 Namely, how drivers/scsi/libiscsi.c:iscsi_data_xmit() attempts to drain
 conn-cmdqueue and hence call iscsi_prep_scsi_cmd_pdu() + increment
 session-cmdsn, without ever checking to see if the CmdSN window may
 have already closed..
 
 The only CmdSN window check that I see in the I/O dispatch codepath is
 in iscsi_queuecommand() - iscsi_check_cmdsn_window_closed(), but once
 iscsi_conn_queue_work() is invoked and process context in
 iscsi_xmitworker() - iscsi_data_xmit() starts to execute, there is
 AFAICT no further CmdSN window open checks to ensure the initiator does
 not overflow MaxCmdSN..
 
 This would very much line up with the type of bug that is being
 reported.  Before trying to determine what a possible fix might look
 like, can you try the following libiscsi patch to see if your able to
 hit either of the BUG's below..?
 
 Thanks,
 
 --nab
 
 diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
 index e399561..9106f63 100644
 --- a/drivers/scsi/libiscsi.c
 +++ b/drivers/scsi/libiscsi.c
 @@ -1483,6 +1483,12 @@ check_mgmt:
   fail_scsi_task(conn-task, DID_IMM_RETRY);
   continue;
   }
 +
 +   if (iscsi_check_cmdsn_window_closed(conn)) {
 +   printk(CmdSN window already closed in 
 iscsi_data_xmit #1\n);
 +   BUG();
 +   }
 +
   rc = iscsi_prep_scsi_cmd_pdu(conn-task);
   if (rc) {
   if (rc == -ENOMEM || rc == -EACCES) {
 @@ -1518,6 +1524,11 @@ check_mgmt:
   if (iscsi_check_tmf_restrictions(task, ISCSI_OP_SCSI_DATA_OUT))
   break;
 
 +   if (iscsi_check_cmdsn_window_closed(conn)) {
 +   printk(CmdSN window already closed in 
 iscsi_data_xmit #2\n);
 +   BUG();
 +   }
 +
   conn-task = task;
   list_del_init(conn-task-running);
   conn-task-state = ISCSI_TASK_RUNNING;
 
 If you hit a bug there then it is probably the target or if you are using the 
 new libiscsi back/frwd lock patches it could be related to them. We should 
 not need to check above because we never queue more cmds from the scsi layer 
 than the window will allow at the time. If the window is 10, the queued_cmdsn 
 check should make sure we have only accepted 10 commands into the iscsi layer 
 so we should not need to recheck above.
 
 You should just enable libiscsi debug printk in iscsi_check_cmdsn_window so 
 we can see the sn related values at the time of queuing. You should enable 
 that printk whithout your patch.

Actually that will not help because we always have the queued cmdsn lower than 
the max in that path. I would do your patch but then also print the queued 
cmdsn, cmdsn and also the maxcmdsn in the check function.


I am not by my computer so I cannot send

Re: [PATCH v11 6/9] Make scsi_remove_host() wait until error handling finished

[Michael Christie]

On Jun 25, 2013, at 10:31 AM, Bart Van Assche bvanass...@acm.org wrote:

 On 06/25/13 15:45, James Bottomley wrote:
 On Tue, 2013-06-25 at 11:01 +0200, Bart Van Assche wrote:
 There is a difference though between moving the EH kthread_stop() call
 and the patch at the start of this thread: moving the EH kthread_stop()
 call does not prevent that an ioctl like SG_SCSI_RESET triggers an eh_*
 callback after scsi_remove_host() has finished. However, the
 scsi_begin_eh() / scsi_end_eh() functions do prevent that an ioctl can
 cause an eh_* callback to be invoked after scsi_remove_device() finished.
 
 OK, but this doesn't tell me what you're trying to achieve.
 
 An eh function is allowable as long as the host hadn't had the release
 callback executed.  That means you must have to have a reference to the
 device/host to execute the eh function, which is currently guaranteed
 for all invocations.
 
 That raises a new question: how is an LLD expected to clean up resources 
 without triggering a race condition ? What you wrote means that it's not safe 
 for an LLD to start cleaning up the resources needed by the eh_* callbacks 
 immediately after scsi_remove_device() returns since it it not guaranteed 
 that at that time all references to the device have already been dropped.
 


A callback in the device/target/host (whatever is needed) release function 
would do this right? If I understand James right, I think he suggested 
something like this in another mail.

--
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 v11 6/9] Make scsi_remove_host() wait until error handling finished

[Michael Christie]

On Jun 24, 2013, at 9:26 PM, Mike Christie micha...@cs.wisc.edu wrote:

 On 06/24/2013 05:27 PM, James Bottomley wrote:
 However, what's the reasoning behind wanting to do this?  In theory all
 necessary resources for the eh thread should only be freed in the
 release callback.  That means they aren't freed until all error recovery
 completes.
 
 I think it makes it easier to handle cleanup of driver resources
 needed
 for aborts/resets for some drivers. If after host removal, the scsi eh
 can call into the driver after scsi_remove_host is called then we have
 to set some internal bits to fail future eh callback calls and handle
 waiting on or flushing running eh operations. If we know that after
 scsi_host_remove is called the eh callbacks will not be running and
 will
 not be called we can just free the driver resources.
 
 For iscsi and I think drivers that do scsi_remove_target it would be
 helpful to have something similar at the target level.
 
 I'm wary of this because it combines two models: a definite state model
 (where we move from state to state waiting for the completions) with an
 event driven one (in theory the current one); such combinations rarely
 lead to good things because you get a mixture of actions causing state
 transitions some of which are waited for and some of which have an async
 transition and that ends up confusing the heck out of everybody no
 matter how carefully documented.  Can you give me some use cases of what
 you're trying to achieve?  Could it be as simple as an event that fires
 on release?
 
 
 The problem that we hit in iscsi is that we will call scsi_remove_target
 (we used to call scsi_remove_host when we always did a host per target
 so we hit the problem at that level before). That will complete, but the
 scsi eh might still be trying to abort/reset devices accessed through
 that target. To avoid freeing resources that the iscsi scsi eh might be
 using, we set internal state bits and wait on host_busy to go to zero
 before we tear down the iscsi session, conn and task structs.
 
 I think Bart was hitting a similar issue but a level up in the host
 removal case, because srp always does a host per target and so it just
 does a scsi_remove_host..

I take this back. I don't think it is a issue anymore and I think I can remove 
the iscsi hack. With the blk_cleanup_queue/blk_drain_queue code I think the 
target and the removal of its devices will not complete until the scsi eh is 
completed. The blk_drain_queue code will now wait for the eh to complete 
because the rq counters will be incremented.--
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: use 64-bit value for 'max_luns'

[Michael Christie]

On Feb 19, 2013, at 2:18 AM, Hannes Reinecke h...@suse.de wrote:

 diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
 index b44c1cf..95105c0 100644
 --- a/drivers/scsi/cxgbi/libcxgbi.c
 +++ b/drivers/scsi/cxgbi/libcxgbi.c
 @@ -245,7 +245,7 @@ void cxgbi_hbas_remove(struct cxgbi_device *cdev)
 }
 EXPORT_SYMBOL_GPL(cxgbi_hbas_remove);
 
 -int cxgbi_hbas_add(struct cxgbi_device *cdev, unsigned int max_lun,
 +int cxgbi_hbas_add(struct cxgbi_device *cdev, uint64_t max_lun,
   unsigned int max_id, struct scsi_host_template *sht,
   struct scsi_transport_template *stt)
 {
 diff --git a/drivers/scsi/cxgbi/libcxgbi.h b/drivers/scsi/cxgbi/libcxgbi.h
 index 80fa99b..d3b27f8 100644
 --- a/drivers/scsi/cxgbi/libcxgbi.h
 +++ b/drivers/scsi/cxgbi/libcxgbi.h
 @@ -692,7 +692,7 @@ struct cxgbi_device *cxgbi_device_register(unsigned int, 
 unsigned int);
 void cxgbi_device_unregister(struct cxgbi_device *);
 void cxgbi_device_unregister_all(unsigned int flag);
 struct cxgbi_device *cxgbi_device_find_by_lldev(void *);
 -int cxgbi_hbas_add(struct cxgbi_device *, unsigned int, unsigned int,
 +int cxgbi_hbas_add(struct cxgbi_device *, uint64_t, unsigned int,
   struct scsi_host_template *,
   struct scsi_transport_template *);
 void cxgbi_hbas_remove(struct cxgbi_device *);
 diff --git a/drivers/scsi/dc395x.c b/drivers/scsi/dc395x.c
 index dfe90ce..9acdb9c 100644


Could it be possible to use u64, so it matches the rest of the driver coding 
style? The rest of the driver uses the u* definitions instead f the uint*_t 
ones.

I think it could just be edited when the patch is merged instead of having to 
send a whole new patchset. We can also just fix it up later of the patches get 
merged as is.--
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] SG_SCSI_RESET ioctl: add no_escalate values

[Michael Christie]

On Feb 15, 2013, at 5:32 PM, Douglas Gilbert dgilb...@interlog.com wrote:

 On 13-02-15 04:48 PM, Mike Christie wrote:
 On 02/15/2013 01:39 PM, Douglas Gilbert wrote:
 Further to the thread titled: [PATCH] SG_SCSI_RESET ioctl should
 only perform requested operation by Jeremy Linton a patch
 is presented that adds no_escalate versions to the existing
 ioctl. This should not break any existing code.
 
 This patches applies to lk 3.7.7 and lk 3.8.0-rc7 . I will extend
 sg_reset in the sg3_utils package to use it.
 
 ChangeLog:
   - modify SG_SCSI_RESET ioctl so the SG_SCSI_RESET_NO_ESCALATE
 value may be added to the existing values. If so the existing
 device-target-bus-host escalation does not occur.
   - modify scsi_reset_provider() in the scsi_error.c file in a
 similar way to support this new functionality.
 
 Signed-off-by: Douglas Gilbert dgilb...@interlog.com
 
 Some drivers rely on more invasive eh callbacks to be called if they
 return FAILED in a eh callbacks. Is there a way for drivers to tell
 scsi-ml it needs the old behavior?
 
 Mike,
 The old behaviour hasn't changed both at the
 ioctl(SG_SCSI_RESET) level and the underlying kernel
 scsi_reset_provider() function. In both cases extra
 values have been added: to third argument of the ioctl
 and the second argument ('flag') of the
 scsi_reset_provider() function. The existing values
 will do exactly the same thing (i.e. escalate) with
 the same return values.
 
 The only way it is different is that values that were
 previously errors (precisely 0x101 to 0x104) now cause a
 non-escalating device(LU)/target/bus reset.

Sorry for the confusion. I should have written is there a way for drivers to 
tell scsi-ml it only supports the old behavior and does not support the new 
calls? If not then I am guessing this is mandatory to support and we need to 
fix the drivers right?

If userspace sends a command that performs the non-escalting *reset to some 
drivers, it is going to leave them in a state where they may not process IO or 
will crash.--
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] SG_SCSI_RESET ioctl: add no_escalate values

[Michael Christie]

On Feb 15, 2013, at 11:45 PM, Michael Christie micha...@cs.wisc.edu wrote:
 Some drivers rely on more invasive eh callbacks to be called if they
 return FAILED in a eh callbacks.


 If userspace sends a command that performs the non-escalting *reset to some 
 drivers, it is going to leave them in a state where they may not process IO 
 or will crash.

Actually, I take those 2 comments back. I think it is just some drivers abort 
callbacks that might be issues, so we should be safe. I did not look at all 
drivers though.--
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] Use a more selective error recovery strategy based on device capabilities

[Michael Christie]

On Feb 13, 2013, at 9:46 AM, Jeremy Linton jlin...@tributary.com wrote:

 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1
 
 On 2/13/2013 7:06 AM, Hannes Reinecke wrote:
 
 But unfortunately it failed the reality check; of my zoo of storage arrays
 only NetApp OnTap 8.X and HP P2000 supports the REPORT SUPPORTED TASK
 MANAGEMENT FUNCTIONS command. None of the others (HP EVA, NetApp E-Series,
 EMC Clariion CX-3) do.
 
   I'm not really surprised. I've had better luck with devices that report
 compliance with more recent versions of the specification. Of the those, I
 would guess slightly more than 50% support it. That includes tape drives,
 changer devices, and raid controllers. Of the ones I've tested that support
 it, they almost always report 0xDA for the first byte. Which correspond to
 abort task, abort task set, clear task set, lun reset and target reset.
 


For the case where report supported TMFs is not supported can we just have the 
LLD return some new return code from the eh callback when it gets 
FUNCTION_REJECTED. scsi-ml would then clear the eh_*_ok bit, so at least it 
would not be called again.--
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][RFC] scsi_transport_fc: Implement I_T nexus reset

[Michael Christie]

On Dec 7, 2012, at 8:51 AM, Hannes Reinecke h...@suse.de wrote:
 
 diff --git a/drivers/scsi/bfa/bfad_im.c b/drivers/scsi/bfa/bfad_im.c
 index 8f92732..d6555aa 100644
 --- a/drivers/scsi/bfa/bfad_im.c
 +++ b/drivers/scsi/bfa/bfad_im.c
 @@ -793,7 +793,7 @@ struct scsi_host_template bfad_im_scsi_host_template = {
   .queuecommand = bfad_im_queuecommand,
   .eh_abort_handler = bfad_im_abort_handler,
   .eh_device_reset_handler = bfad_im_reset_lun_handler,
 - .eh_bus_reset_handler = bfad_im_reset_bus_handler,
 + .eh_bus_reset_handler = fc_eh_reset_it_nexus_handler,
 
   .slave_alloc = bfad_im_slave_alloc,
   .slave_configure = bfad_im_slave_configure,
 @@ -815,7 +815,7 @@ struct scsi_host_template bfad_im_vport_template = {
   .queuecommand = bfad_im_queuecommand,
   .eh_abort_handler = bfad_im_abort_handler,
   .eh_device_reset_handler = bfad_im_reset_lun_handler,
 - .eh_bus_reset_handler = bfad_im_reset_bus_handler,
 + .eh_bus_reset_handler = fc_eh_reset_it_nexus_handler,
 
   .slave_alloc = bfad_im_slave_alloc,
   .slave_configure = bfad_im_slave_configure,
 diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
 index 60e5a17..2fd67c1 100644
 --- a/drivers/scsi/lpfc/lpfc_scsi.c
 +++ b/drivers/scsi/lpfc/lpfc_scsi.c
 @@ -5136,7 +5136,7 @@ struct scsi_host_template lpfc_template = {
   .eh_abort_handler   = lpfc_abort_handler,
   .eh_device_reset_handler = lpfc_device_reset_handler,
   .eh_target_reset_handler = lpfc_target_reset_handler,
 - .eh_bus_reset_handler   = lpfc_bus_reset_handler,
 + .eh_bus_reset_handler   = fc_eh_reset_it_nexus_handler,
   .eh_host_reset_handler  = lpfc_host_reset_handler,
   .slave_alloc= lpfc_slave_alloc,
   .slave_configure= lpfc_slave_configure,
 @@ -5160,7 +5160,7 @@ struct scsi_host_template lpfc_vport_template = {
   .eh_abort_handler   = lpfc_abort_handler,
   .eh_device_reset_handler = lpfc_device_reset_handler,
   .eh_target_reset_handler = lpfc_target_reset_handler,
 - .eh_bus_reset_handler   = lpfc_bus_reset_handler,
 + .eh_bus_reset_handler   = fc_eh_reset_it_nexus_handler,
   .slave_alloc= lpfc_slave_alloc,
   .slave_configure= lpfc_slave_configure,
   .slave_destroy  = lpfc_slave_destroy,
 diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
 index 3a1661c..5d59284 100644
 --- a/drivers/scsi/qla2xxx/qla_os.c
 +++ b/drivers/scsi/qla2xxx/qla_os.c
 @@ -246,7 +246,7 @@ struct scsi_host_template qla2xxx_driver_template = {
   .eh_abort_handler   = qla2xxx_eh_abort,
   .eh_device_reset_handler = qla2xxx_eh_device_reset,
   .eh_target_reset_handler = qla2xxx_eh_target_reset,
 - .eh_bus_reset_handler   = qla2xxx_eh_bus_reset,
 + .eh_bus_reset_handler   = fc_eh_reset_it_nexus_handler,
   .eh_host_reset_handler  = qla2xxx_eh_host_reset,


Hey,

One other comment. I do not think we can use the bus reset callout as is. I 
think for all fc drivers but mptfc, the channel is always 0. All targets on 
those hosts have the same channel value, and it seems  this code in 
scsi_error.c:scsi_eh_bus_reset():

rtn = scsi_try_bus_reset(chan_scmd);
if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
if (channel == scmd_channel(scmd)) {

would end up matching all the commands on the host instead of just the commands 
on the specific target that was passed into scsi_try_bus_reset if SUCCESS or 
FAST_IO_FAIL was returned by scsi_try_bus_reset. If there were 2 faulty targets 
on a host, you would end up calling fc_eh_reset_it_nexus_handler on only 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

Re: [PATCH 0/7] qla4xxx: Updates for scsi misc branch

[Michael Christie]

On Oct 10, 2012, at 6:24 AM, vikas.chaudh...@qlogic.com wrote:

 From: Vikas Chaudhary vikas.chaudh...@qlogic.com
 
 James,
 
 Please apply the following patches to the scsi tree at your earliest
 convenience.
 
 Thanks,
 Vikas.
 

I think some of these should be common operations instead of vendor ones.


 Adheer Chandravanshi (1):
  qla4xxx: Added new BSG command QLISCSI_VND_ABOUT_FIRMWARE
 

What info does this provide?


 Harish Zunjarrao (3):
  qla4xxx: Allow reset in link down case
  qla4xxx: Invoke Set Address Control Block using BSG

Does this set the networking info?


  qla4xxx: Invoke DisableACB using BSG

What does this do? Disable the networking?



 Manish Rangankar (2):
  qla4xxx: Add get default DDB support using BSG.

What does this do? Get the default DDB settings?

  qla4xxx: Add get DDB support using BSG.



--
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 07/17] be2iscsi: Fix the kernel panic in blkiopoll disable mode

[Michael Christie]

On Sep 28, 2012, at 8:32 PM, John Soni Jose sony.joh...@emulex.com wrote:

 From: Jayamohan Kallickal jayamohan.kallic...@emulex.com
 
 Fix kernel panic issue while running IO in blk_iopoll disable mode.

What was the bug exactly?

 Creating UNBOUND WQ for each EQ in the driver.

What is the benefit of doing this vs per hba like before?

Why WQ_UNBOUND? It seems other drivers prefer it the other way.
--
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] iscsi: Don't disable BH on BH context

[Michael Christie]

On Aug 20, 2012, at 8:28 PM, Ying Xue ying@windriver.com wrote:

 Since we have already in BH context when iscsi_sw_tcp_write_space()
 is called, it's unnecessary to disable BH.

Hey,

I do don't think this is right anymore. It looks like it can be called in 
sock_setsockopt.

 
 Signed-off-by: Ying Xue ying@windriver.com
 Acked-by: Michael Christie micha...@cs.wisc.edu
 ---
 drivers/scsi/iscsi_tcp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
 index 9220861..d763857 100644
 --- a/drivers/scsi/iscsi_tcp.c
 +++ b/drivers/scsi/iscsi_tcp.c
 @@ -194,7 +194,7 @@ static void iscsi_sw_tcp_write_space(struct sock *sk)
   struct iscsi_sw_tcp_conn *tcp_sw_conn;
   void (*old_write_space)(struct sock *);
 
 - read_lock_bh(sk-sk_callback_lock);
 + read_lock(sk-sk_callback_lock);
   conn = sk-sk_user_data;
   if (!conn) {
   read_unlock_bh(sk-sk_callback_lock);
 @@ -204,7 +204,7 @@ static void iscsi_sw_tcp_write_space(struct sock *sk)
   tcp_conn = conn-dd_data;
   tcp_sw_conn = tcp_conn-dd_data;
   old_write_space = tcp_sw_conn-old_write_space;
 - read_unlock_bh(sk-sk_callback_lock);
 + read_unlock(sk-sk_callback_lock);
 
   old_write_space(sk);
 
 -- 
 1.7.11
 
 --
 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 0/12] qla4xxx: Updates for scsi misc branch.

[Michael Christie]

On Aug 22, 2012, at 6:54 AM, vikas.chaudh...@qlogic.com wrote:
 Vikas Chaudhary (12):
  qla4xxx: Update function name from 8xxx to 82xx
  qla4xxx: Update structure and variable names
  qla4xxx: Update func name from ql4_ to qla4_
  qla4xxx: Rename macros from 82XX to 8XXX
  qla4xxx: Clean-up and optimize macros
  qla4xxx: Added new functions in isp_ops
  qla4xxx: Replace all !is_qla8022() with is_qla40XX()
  qla4xxx: Set IDC version in correct way
  qla4xxx: Added new function qla4_8xxx_get_minidump
  qla4xxx: Added support for ISP83XX
  qla4xxx: Update Copyright header
  qla4xxx: Update driver version to 5.03.00-k0


Patches look ok overall. There are some minor coding style issues in patch 10.
- do not need a return statement at the end of a function that does not 
return something
- some people do not like a extra newlines like this:

x = something;

if (x == something else)

but I think it would be ok to clean those up later since they are minor.

Reviewed-by: Mike Christie micha...@cs.wisc.edu

--
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] Fix a use-after-free triggered by device removal

[Michael Christie]

On Sep 3, 2012, at 9:12 AM, Bart Van Assche bvanass...@acm.org wrote:

 If the put_device() call in scsi_request_fn() drops the sdev refcount
 to zero then the spin_lock() call after the put_device() call triggers
 a use-after-free. Avoid that by making sure that blk_cleanup_queue()
 can only finish after all active scsi_request_fn() calls have returned.



If we have this patch
http://marc.info/?l=linux-scsim=134453905402413w=2
it seems we have all the scsi layer callers of the request_fn/*blk_run_queue 
holding a reference to the device when they make the call. Right, or are there 
some other places missing?

What are the other places we can call the request_fn without already holding a 
reference to the device? Is it the block layer? Is that why we need 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] scsi_dh_alua: Enable STPG for unavailable ports

[Michael Christie]

On Aug 24, 2012, at 4:08 AM, Bart Van Assche bvanass...@acm.org wrote:
 diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
 b/drivers/scsi/device_handler/scsi_dh_alua.c
 index 08d80a6..a05d469 100644
 --- a/drivers/scsi/device_handler/scsi_dh_alua.c
 +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
 @@ -641,8 +641,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct 
 alua_dh_data *h)
   h-state = TPGS_STATE_STANDBY;
   break;
   case TPGS_STATE_OFFLINE:
 - case TPGS_STATE_UNAVAILABLE:
 - /* Path unusable for unavailable/offline */
 + /* Path unusable */
   err = SCSI_DH_DEV_OFFLINED;
   break;
   default:

We must not have tested unavailable state very much. 3rd bug in a short time. 
Looks ok to me.

Reviewed-by: Mike Christie micha...@cs.wisc.edu


--
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: return TARGET_ERROR when a device has been disconnected

[Michael Christie]

On Aug 16, 2012, at 1:40 AM, Hannes Reinecke h...@suse.de wrote:

 When we receive a sense code of
 NOT READY, LOGICAL UNIT NOT SUPPORTED
 the device has been disconnected and any retry on other paths
 would be pointless. So return TARGET_ERROR here.


What target is this with? What about {ILLEGAL_REQUEST,  LOGICAL UNIT NOT 
SUPPORTED}? Would you want to do the same behavior for that error?

I ask because I have seen targets return {ILLEGAL_REQUEST,  LOGICAL UNIT NOT 
SUPPORTED} when you only unexport the LUN from a path/port on the target. I was 
wondering if some might do the same for {NOT_READY, LOGICAL UNIT NOT 
SUPPORTED}? In those cases, we would not want multipath to fail IO.

In these type of cases should the target be returning something else?--
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] iscsi_tcp: add write permission to max_lun

[Michael Christie]

On Aug 15, 2012, at 6:30 AM, Rob Evers rev...@redhat.com wrote:

 Add root write permission to iscsi_tcp max_lun parameter
 
 Signed-off-by: Rob Evers rev...@redhat.com
 ---
 drivers/scsi/iscsi_tcp.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
 
 diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
 index 9220861..e8609a4 100644
 --- a/drivers/scsi/iscsi_tcp.c
 +++ b/drivers/scsi/iscsi_tcp.c
 @@ -56,7 +56,7 @@ static struct scsi_host_template iscsi_sw_tcp_sht;
 static struct iscsi_transport iscsi_sw_tcp_transport;
 
 static unsigned int iscsi_max_lun = 512;
 -module_param_named(max_lun, iscsi_max_lun, uint, S_IRUGO);
 +module_param_named(max_lun, iscsi_max_lun, uint, S_IRUGO|S_IWUSR);



Looks ok.

Reviewed-by: Mike Christie micha...@cs.wisc.edu
--
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] increase iscsi_tcp max_lun to ~0, don't care

[Michael Christie]

On Aug 15, 2012, at 5:39 PM, Rob Evers rev...@redhat.com wrote:

 
 Signed-off-by: Rob Evers rev...@redhat.com
 ---
 drivers/scsi/iscsi_tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
 index 9220861..1b91ca0 100644
 --- a/drivers/scsi/iscsi_tcp.c
 +++ b/drivers/scsi/iscsi_tcp.c
 @@ -55,7 +55,7 @@ static struct scsi_transport_template 
 *iscsi_sw_tcp_scsi_transport;
 static struct scsi_host_template iscsi_sw_tcp_sht;
 static struct iscsi_transport iscsi_sw_tcp_transport;
 
 -static unsigned int iscsi_max_lun = 512;
 +static unsigned int iscsi_max_lun = ~0;
 module_param_named(max_lun, iscsi_max_lun, uint, S_IRUGO);

Ok. Thanks.

Reviewed-by: Mike Christie micha...@cs.wisc.edu

--
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/7] Removing the iscsi_data_pdu setting.

[Michael Christie]

On Aug 13, 2012, at 5:29 AM, John Soni Jose sony.joh...@emulex.com wrote:

 The setting of iscsi_data_pdu is not required anymore,
 as this was required for BE1 adapters only.
 

Were those released and supported by the upstream kernel in previous kernels?
--
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] libiscsi: avoid unnecessary multiple NULL assignments

[Michael Christie]

Replacing linux-scsi list with linux-kernel.

On Aug 12, 2012, at 9:15 AM, Masatake YAMATO yam...@redhat.com wrote:

 In iscsi_free_task, NULL is assigned to task-sc twice: before and
 after kfifo_in invocatoin. Allocating and freeing iscsi_task are guarded
 with session-lock, so multiple NULL assignments cause no trouble. But
 people reading the source code may be confused.
 
 The second NULL assignment comes from commit:
 
3e5c28ad0391389959ccae81c938c7533efb3490
 
 It seems that the line after kfifo_in invocation was introduced
 accidentally.
 
 Signed-off-by: Masatake YAMATO yam...@redhat.com
 ---
 drivers/scsi/libiscsi.c | 1 -
 1 file changed, 1 deletion(-)
 
 diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
 index 82c3fd4..7aacf3a 100644
 --- a/drivers/scsi/libiscsi.c
 +++ b/drivers/scsi/libiscsi.c
 @@ -500,21 +500,20 @@ static void iscsi_free_task(struct iscsi_task *task)
   task-sc = NULL;
   /*
* login task is preallocated so do not free
*/
   if (conn-login_task == task)
   return;
 
   kfifo_in(session-cmdpool.queue, (void*)task, sizeof(void*));
 
   if (sc) {
 - task-sc = NULL;
   /* SCSI eh reuses commands to verify us */
   sc-SCp.ptr = NULL;
   /*
* queue command may call this to free the task, so
* it will decide how to return sc to scsi-ml.
*/
   if (oldstate != ISCSI_TASK_REQUEUE_SCSIQ)
   sc-scsi_done(sc);
   }

Looks ok to me.

Reviewed-by: Mike Christie micha...@cs.wisc.edu
--
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_lib: Set the device state from transport-offline to running

[Michael Christie]

On Aug 9, 2012, at 3:51 AM, vikas.chaudh...@qlogic.com wrote:

 From: Vikas Chaudhary vikas.chaudh...@qlogic.com
 
 FC and iSCSI class set SCSI devices to transport-offline state after
 fast_io_fail/replacement_timeout has fired, but after relogin, function
 scsi_internal_device_unblock() is not setting scsi device state to running.
 Due to this the devices even after being relogged in remain offline.
 
 Signed-off-by: Vikas Chaudhary vikas.chaudh...@qlogic.com
 ---
 drivers/scsi/scsi_lib.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
 
 diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
 index ffd7773..4ba3719 100644
 --- a/drivers/scsi/scsi_lib.c
 +++ b/drivers/scsi/scsi_lib.c
 @@ -2470,7 +2470,8 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
* Try to transition the scsi device to SDEV_RUNNING or one of the
* offlined states and goose the device queue if successful.
*/
 - if (sdev-sdev_state == SDEV_BLOCK)
 + if ((sdev-sdev_state == SDEV_BLOCK) ||
 + (sdev-sdev_state == SDEV_TRANSPORT_OFFLINE))
   sdev-sdev_state = new_state;
   else if (sdev-sdev_state == SDEV_CREATED_BLOCK) {
   if (new_state == SDEV_TRANSPORT_OFFLINE ||


Dumb mistake on my part. Thank you for fixing this.

Reviewed-by: Mike Christie micha...@cs.wisc.edu

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