Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1522609 > unrolled thread

Re: [PATCH v3 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

Started byJohannes Thumshirn <jthumshirn@suse.de>
First post2016-11-15 13:00 +0100
Last post2016-11-16 12:40 +0100
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 02/16] scsi: don't use fc_bsg_job::request and  fc_bsg_job::reply directly Johannes Thumshirn <jthumshirn@suse.de> - 2016-11-15 13:00 +0100
    Re: [PATCH v3 02/16] scsi: don't use fc_bsg_job::request and  fc_bsg_job::reply directly Steffen Maier <maier@linux.vnet.ibm.com> - 2016-11-15 15:40 +0100
      Re: [PATCH v3 02/16] scsi: don't use fc_bsg_job::request and  fc_bsg_job::reply directly Johannes Thumshirn <jthumshirn@suse.de> - 2016-11-15 15:50 +0100
      Re: [PATCH v3 02/16] scsi: don't use fc_bsg_job::request and  fc_bsg_job::reply directly Johannes Thumshirn <jthumshirn@suse.de> - 2016-11-15 16:40 +0100
        Re: [PATCH v3 02/16] scsi: don't use fc_bsg_job::request and  fc_bsg_job::reply directly Johannes Thumshirn <jthumshirn@suse.de> - 2016-11-16 12:40 +0100

#1522609 — Re: [PATCH v3 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

FromJohannes Thumshirn <jthumshirn@suse.de>
Date2016-11-15 13:00 +0100
SubjectRe: [PATCH v3 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly
Message-ID<sDKz7-4pA-3@gated-at.bofh.it>
On Tue, Oct 25, 2016 at 09:43:14AM +0200, Johannes Thumshirn wrote:
> On Fri, Oct 14, 2016 at 09:38:21AM +0200, Johannes Thumshirn wrote:
> > On Thu, Oct 13, 2016 at 05:55:11PM +0200, Steffen Maier wrote:
> > > Hm, still behaves for me like I reported for v2:
> > > http://marc.info/?l=linux-scsi&m=147637177902937&w=2

[...]

> > 
> > The rational behind this is, in fc_req_to_bsgjob() we're assigning
> > job->request as req->cmd and job->request_len = req->cmd_len. But without
> > checkinf job->request_len we don't know whether we're save to touch
> > job->request (a.k.a. bsg_request).
> 
> Hi Steffen,
> Did you have any chance testing this? I hacked fcping to work with non-FCoE
> and rports as well and tested with FCoE and lpfc. No problems seen from my
> side. I've also pused the series (With this change folded in) to my git 
> tree at [1] if this helps you in any way.
> 
> [1] https://git.kernel.org/cgit/linux/kernel/git/jth/linux.git/log/?h=scsi-bsg-rewrite-v4
> 

So I finally have a test system up and running. I have good and bad news. The
good news is, I can't get the system crashing with my patches, the bad news is
I can't get zfcp_ping and zfcp_show to output something but HBA_STATUS_ERROR
with my patches and without.

And btw, I renamed the branch to fc-bsg-rewrite-v4 in case you want to clone
from it (it has patch 2/16 changed to the v3 submission).

Can you please have a look with your setup?

Thanks,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [next] | [standalone]

#1522775

FromSteffen Maier <maier@linux.vnet.ibm.com>
Date2016-11-15 15:40 +0100
Message-ID<sDN3X-69H-3@gated-at.bofh.it>
In reply to#1522609
Hi Johannes,

On 11/15/2016 12:56 PM, Johannes Thumshirn wrote:
> On Tue, Oct 25, 2016 at 09:43:14AM +0200, Johannes Thumshirn wrote:
>> On Fri, Oct 14, 2016 at 09:38:21AM +0200, Johannes Thumshirn wrote:
>>> On Thu, Oct 13, 2016 at 05:55:11PM +0200, Steffen Maier wrote:
>>>> Hm, still behaves for me like I reported for v2:
>>>> http://marc.info/?l=linux-scsi&m=147637177902937&w=2
>
> [...]
>
>>>
>>> The rational behind this is, in fc_req_to_bsgjob() we're assigning
>>> job->request as req->cmd and job->request_len = req->cmd_len. But without
>>> checkinf job->request_len we don't know whether we're save to touch
>>> job->request (a.k.a. bsg_request).
>>
>> Hi Steffen,
>> Did you have any chance testing this? I hacked fcping to work with non-FCoE
>> and rports as well and tested with FCoE and lpfc. No problems seen from my
>> side. I've also pused the series (With this change folded in) to my git
>> tree at [1] if this helps you in any way.
>>
>> [1] https://git.kernel.org/cgit/linux/kernel/git/jth/linux.git/log/?h=scsi-bsg-rewrite-v4
>>
>
> So I finally have a test system up and running. I have good and bad news. The
> good news is, I can't get the system crashing with my patches, the bad news is
> I can't get zfcp_ping and zfcp_show to output something but HBA_STATUS_ERROR
> with my patches and without.

Assuming you run the latest package version on s390x:

Do steps 2 and 3 of the procedure in
http://www.ibm.com/support/knowledgecenter/linuxonibm/com.ibm.linux.z.lhdd/lhdd_t_fcp_api_runappl.html
help?

The only other thing I can think of from the top of my head is that BSG 
ioctls are sensitive regarding ABI and I once had the kernel ioctl 
return EINVAL due to unmatching kernel-headers and libzfcphbaapi maps 
this EINVAL to HBA_STATUS_ERROR because there is no more specifically 
suitable HBA constant [old SUSE bugs 834498 and 834500].

> And btw, I renamed the branch to fc-bsg-rewrite-v4 in case you want to clone
> from it (it has patch 2/16 changed to the v3 submission).
>
> Can you please have a look with your setup?

I'm going to re-test hopefully within the next few days.

-- 
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[toc] | [prev] | [next] | [standalone]

#1522789

FromJohannes Thumshirn <jthumshirn@suse.de>
Date2016-11-15 15:50 +0100
Message-ID<sDNdE-6dg-23@gated-at.bofh.it>
In reply to#1522775
On Tue, Nov 15, 2016 at 03:31:27PM +0100, Steffen Maier wrote:
> Hi Johannes,
> 
> On 11/15/2016 12:56 PM, Johannes Thumshirn wrote:
> > On Tue, Oct 25, 2016 at 09:43:14AM +0200, Johannes Thumshirn wrote:
> > > On Fri, Oct 14, 2016 at 09:38:21AM +0200, Johannes Thumshirn wrote:
> > > > On Thu, Oct 13, 2016 at 05:55:11PM +0200, Steffen Maier wrote:
> > > > > Hm, still behaves for me like I reported for v2:
> > > > > http://marc.info/?l=linux-scsi&m=147637177902937&w=2
> > 
> > [...]
> > 
> > > > 
> > > > The rational behind this is, in fc_req_to_bsgjob() we're assigning
> > > > job->request as req->cmd and job->request_len = req->cmd_len. But without
> > > > checkinf job->request_len we don't know whether we're save to touch
> > > > job->request (a.k.a. bsg_request).
> > > 
> > > Hi Steffen,
> > > Did you have any chance testing this? I hacked fcping to work with non-FCoE
> > > and rports as well and tested with FCoE and lpfc. No problems seen from my
> > > side. I've also pused the series (With this change folded in) to my git
> > > tree at [1] if this helps you in any way.
> > > 
> > > [1] https://git.kernel.org/cgit/linux/kernel/git/jth/linux.git/log/?h=scsi-bsg-rewrite-v4
> > > 
> > 
> > So I finally have a test system up and running. I have good and bad news. The
> > good news is, I can't get the system crashing with my patches, the bad news is
> > I can't get zfcp_ping and zfcp_show to output something but HBA_STATUS_ERROR
> > with my patches and without.
> 
> Assuming you run the latest package version on s390x:
> 
> Do steps 2 and 3 of the procedure in
> http://www.ibm.com/support/knowledgecenter/linuxonibm/com.ibm.linux.z.lhdd/lhdd_t_fcp_api_runappl.html
> help?

No, I do have a correct /etc/hba.conf:
$ grep libzfcphbaapi /etc/hba.conf 
com.ibm.libzfcphbaapi /usr/lib64/libzfcphbaapi.so.0
and bumping the log level via export LIB_ZFCP_HBAAPI_LOG_LEVEL=1 just adds 
'(vlib.c:105)_initvlib: libzfcphbaapi.so loaded at Nov 15 15:43:23' to the 1st
line of zfcp_ping output.

Here's the full output:
$ zfcp_ping -v -a 0.0.fc00 -d 0x50050763051b473a
(vlib.c:105)_initvlib: libzfcphbaapi.so loaded at Nov 15 15:45:33
Sending PNG from BUS_ID=0.0.fc00 WWPN=0xc05076e0f3002344 ID=0x760c1c
dev=/dev/bsg/fc_host1 speed=8 GBit/s
--- REQUEST cmd = 0x0401 ---
03 00 00 00 fa 01 00 00 
04 01 00 01 00 00 00 00 
00 00 00 01 00 02 00 08 
50 05 07 63 05 1b 47 3a 
00 00 00 00 
--- RESPONSE rc = 0x1 ---
00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 
00 00 00 00 
--- REQUEST cmd = 0x0401 ---
03 00 00 00 fa 01 00 00 
04 01 00 01 00 00 00 00 
00 00 00 01 00 02 00 08 
50 05 07 63 05 1b 47 3a 
00 00 00 00 
--- RESPONSE rc = 0x1 ---
00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 
00 00 00 00 
--- REQUEST cmd = 0x0401 ---
03 00 00 00 fa 01 00 00 
04 01 00 01 00 00 00 00 
00 00 00 01 00 02 00 08 
50 05 07 63 05 1b 47 3a 
00 00 00 00 
--- RESPONSE rc = 0x1 ---
00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 
00 00 00 00 
Error received for FPNG request, aborting.

---------- ping statistics -----------
min/avg/max = 0.000/0.000/0.000 ms
--------------------------------------

I agree this looks a bit suspicious and I'll add some debugs in the kernel to
see what's going on.

> 
> The only other thing I can think of from the top of my head is that BSG
> ioctls are sensitive regarding ABI and I once had the kernel ioctl return
> EINVAL due to unmatching kernel-headers and libzfcphbaapi maps this EINVAL
> to HBA_STATUS_ERROR because there is no more specifically suitable HBA
> constant [old SUSE bugs 834498 and 834500].
> 
> > And btw, I renamed the branch to fc-bsg-rewrite-v4 in case you want to clone
> > from it (it has patch 2/16 changed to the v3 submission).
> > 
> > Can you please have a look with your setup?
> 
> I'm going to re-test hopefully within the next few days.

That'll be great, thanks.

Byte,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [next] | [standalone]

#1522834

FromJohannes Thumshirn <jthumshirn@suse.de>
Date2016-11-15 16:40 +0100
Message-ID<sDO02-6JS-15@gated-at.bofh.it>
In reply to#1522775
On Tue, Nov 15, 2016 at 03:31:27PM +0100, Steffen Maier wrote:
> Hi Johannes,
> 
> On 11/15/2016 12:56 PM, Johannes Thumshirn wrote:
> > On Tue, Oct 25, 2016 at 09:43:14AM +0200, Johannes Thumshirn wrote:
> > > On Fri, Oct 14, 2016 at 09:38:21AM +0200, Johannes Thumshirn wrote:
> > > > On Thu, Oct 13, 2016 at 05:55:11PM +0200, Steffen Maier wrote:
> > > > > Hm, still behaves for me like I reported for v2:
> > > > > http://marc.info/?l=linux-scsi&m=147637177902937&w=2
> > 
> > [...]
> > 
> > > > 
> > > > The rational behind this is, in fc_req_to_bsgjob() we're assigning
> > > > job->request as req->cmd and job->request_len = req->cmd_len. But without
> > > > checkinf job->request_len we don't know whether we're save to touch
> > > > job->request (a.k.a. bsg_request).
> > > 
> > > Hi Steffen,
> > > Did you have any chance testing this? I hacked fcping to work with non-FCoE
> > > and rports as well and tested with FCoE and lpfc. No problems seen from my
> > > side. I've also pused the series (With this change folded in) to my git
> > > tree at [1] if this helps you in any way.
> > > 
> > > [1] https://git.kernel.org/cgit/linux/kernel/git/jth/linux.git/log/?h=scsi-bsg-rewrite-v4
> > > 
> > 
> > So I finally have a test system up and running. I have good and bad news. The
> > good news is, I can't get the system crashing with my patches, the bad news is
> > I can't get zfcp_ping and zfcp_show to output something but HBA_STATUS_ERROR
> > with my patches and without.

Please ignore my last mails, apparently it's a wise idea to check which user
id one has before running zfcp_ping...

The good news for this is, I can now recreate the crashes you have and thus
have a chance to fix them :-)

Byte,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [next] | [standalone]

#1523396

FromJohannes Thumshirn <jthumshirn@suse.de>
Date2016-11-16 12:40 +0100
Message-ID<sE6Jk-2bm-19@gated-at.bofh.it>
In reply to#1522834
On Tue, Nov 15, 2016 at 04:39:33PM +0100, Johannes Thumshirn wrote:
> On Tue, Nov 15, 2016 at 03:31:27PM +0100, Steffen Maier wrote:
> > Hi Johannes,
> > 
> > On 11/15/2016 12:56 PM, Johannes Thumshirn wrote:
> > > On Tue, Oct 25, 2016 at 09:43:14AM +0200, Johannes Thumshirn wrote:
> > > > On Fri, Oct 14, 2016 at 09:38:21AM +0200, Johannes Thumshirn wrote:
> > > > > On Thu, Oct 13, 2016 at 05:55:11PM +0200, Steffen Maier wrote:
> > > > > > Hm, still behaves for me like I reported for v2:
> > > > > > http://marc.info/?l=linux-scsi&m=147637177902937&w=2
> > > 
> > > [...]
> > > 
> > > > > 
> > > > > The rational behind this is, in fc_req_to_bsgjob() we're assigning
> > > > > job->request as req->cmd and job->request_len = req->cmd_len. But without
> > > > > checkinf job->request_len we don't know whether we're save to touch
> > > > > job->request (a.k.a. bsg_request).
> > > > 
> > > > Hi Steffen,
> > > > Did you have any chance testing this? I hacked fcping to work with non-FCoE
> > > > and rports as well and tested with FCoE and lpfc. No problems seen from my
> > > > side. I've also pused the series (With this change folded in) to my git
> > > > tree at [1] if this helps you in any way.
> > > > 
> > > > [1] https://git.kernel.org/cgit/linux/kernel/git/jth/linux.git/log/?h=scsi-bsg-rewrite-v4
> > > > 
> > > 
> > > So I finally have a test system up and running. I have good and bad news. The
> > > good news is, I can't get the system crashing with my patches, the bad news is
> > > I can't get zfcp_ping and zfcp_show to output something but HBA_STATUS_ERROR
> > > with my patches and without.
> 
> Please ignore my last mails, apparently it's a wise idea to check which user
> id one has before running zfcp_ping...
> 
> The good news for this is, I can now recreate the crashes you have and thus
> have a chance to fix them :-)

So JFTR, I was able to fix the 1 problem introduced by this patch, it's the
follwoing hunk:
@@ -3726,9 +3729,9 @@ fc_req_to_bsgjob(struct Scsi_Host *shost, struct fc_rport *rport,
        if (i->f->dd_bsg_size)
                job->dd_data = (void *)&job[1];
        spin_lock_init(&job->job_lock);
-       job->request = (struct fc_bsg_request *)req->cmd;
+       bsg_request = (struct fc_bsg_request *)req->cmd;
        job->request_len = req->cmd_len;
-       job->reply = req->sense;
+       bsg_reply = req->sense;
        job->reply_len = SCSI_SENSE_BUFFERSIZE; /* Size of sense buffer
                                                 * allocated */
        if (req->bio) {

But as fc_req_to_bsgjob() get's deleted in Patch 15/16 the problem is
re-introduced. Unfortunately the fix isn't as trivial as for 2/16 so I'm still
trying to nail it down.

Thanks,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web