[PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[Maurizio Lombardi]

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 5248c88..1903ae5 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -25,6 +25,7 @@
 #include <scsi/scsi.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_dh.h>
+#include <scsi/scsi_dbg.h>
 
 #define ALUA_DH_NAME "alua"
 #define ALUA_DH_VER "1.3"
@@ -163,9 +164,12 @@ static int submit_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 
 	err = blk_execute_rq(rq->q, NULL, rq, 1);
 	if (err == -EIO) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: evpd inquiry failed with %x\n",
-			    ALUA_DH_NAME, rq->errors);
+		sdev_printk(KERN_INFO, sdev, "%s: evpd inquiry failed\n",
+			    ALUA_DH_NAME);
+		scsi_show_result(rq->errors);
+		if (driver_byte(rq->errors) & DRIVER_SENSE)
+			__scsi_print_sense("alua vpd_inquiry", rq->sense,
+					   rq->sense_len);
 		h->senselen = rq->sense_len;
 		err = SCSI_DH_IO;
 	}
@@ -206,9 +210,11 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 
 	err = blk_execute_rq(rq->q, NULL, rq, 1);
 	if (err == -EIO) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: rtpg failed with %x\n",
-			    ALUA_DH_NAME, rq->errors);
+		sdev_printk(KERN_INFO, sdev, "%s: rtpg failed\n", ALUA_DH_NAME);
+		scsi_show_result(rq->errors);
+		if (driver_byte(rq->errors) & DRIVER_SENSE)
+			__scsi_print_sense("alua submit_rtpg", rq->sense,
+					   rq->sense_len);
 		h->senselen = rq->sense_len;
 		err = SCSI_DH_IO;
 	}
-- 
Maurizio Lombardi

Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[Christoph Hellwig]

On Tue, Apr 22, 2014 at 01:44:16PM +0200, Maurizio Lombardi wrote:
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
>  drivers/scsi/device_handler/scsi_dh_alua.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)


Looks useful, but why do we have basically two copies of the same
code for different commands? Looking at them in detail they mostly look
like copies of scsi_execute/scsi_execute_req_flags and should be switched
to that.

Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[Maurizio Lombardi]

Hi,

On Fri, Apr 25, 2014 at 03:12:19PM +0200, Christoph Hellwig wrote:
> 
> Looks useful, but why do we have basically two copies of the same
> code for different commands? Looking at them in detail they mostly look
> like copies of scsi_execute/scsi_execute_req_flags and should be switched
> to that.

Yes I think you are right, I'm trying to write a patch to get rid of all
this duplicated code.
I'll publish a patchset next week.

Thanks,
Maurizio Lombardi

Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[Maurizio Lombardi]

Hi Christoph,

On Fri, Apr 25, 2014 at 03:12:19PM +0200, Christoph Hellwig wrote:
> 
> Looks useful, but why do we have basically two copies of the same
> code for different commands? Looking at them in detail they mostly look
> like copies of scsi_execute/scsi_execute_req_flags and should be switched
> to that.

I've tried to write a patch but unfortunately there are some problems with that.
For example, look at the submit_rtpg() function:

If the blk_execute_rq() function returns an error, the h->sense buffer
is updated.

        rq->sense = h->sense;  <---------
        memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
        rq->sense_len = h->senselen = 0;

        err = blk_execute_rq(rq->q, NULL, rq, 1);
        if (err == -EIO) {
                sdev_printk(KERN_INFO, sdev,
                            "%s: rtpg failed with %x\n",
                            ALUA_DH_NAME, rq->errors);
                h->senselen = rq->sense_len;  <--------
                err = SCSI_DH_IO;
        }

If I convert submit_rtpg() to make use of scsi_execute() then I'll not be
able to read the updated sense buffer.

scsi_execute() accepts a pointer to the sense buffer as parameter, but
it does not update its content in case blk_execute_rq() returns an error.

Maurizio Lombardi

Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[Christoph Hellwig]

On Tue, Apr 29, 2014 at 11:30:35AM +0200, Maurizio Lombardi wrote:
> If I convert submit_rtpg() to make use of scsi_execute() then I'll not be
> able to read the updated sense buffer.
> 
> scsi_execute() accepts a pointer to the sense buffer as parameter, but
> it does not update its content in case blk_execute_rq() returns an error.

What kind of update do you want?  All the sense buffer updates are done
in the low-level code, and then you could update anything additional
you would need in the caller as you get the whole sense buffer when
using scsi_execute() (for scsi_execute_req_flags it's harder due to the
normalized sense buffer).

Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[Maurizio Lombardi]

Hi,

On Tue, Apr 29, 2014 at 08:00:41PM +0200, Christoph Hellwig wrote:
> 
> What kind of update do you want?  All the sense buffer updates are done
> in the low-level code, and then you could update anything additional
> you would need in the caller as you get the whole sense buffer when
> using scsi_execute() (for scsi_execute_req_flags it's harder due to the
> normalized sense buffer).

Hannes Reinecke is also working on those functions, I'll have to collaborate
with him to minimize the conflicts with the patchset he is preparing.

In the meantime, do you think it is possible to merge the first version
of the patch (http://www.spinics.net/lists/linux-scsi/msg73984.html)?
We could proceed to rewrite the functions later.

Thanks,
Maurizio Lombardi

Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[Christoph Hellwig]

On Tue, May 13, 2014 at 03:10:15PM +0200, Maurizio Lombardi wrote:
> Hannes Reinecke is also working on those functions, I'll have to collaborate
> with him to minimize the conflicts with the patchset he is preparing.
> 
> In the meantime, do you think it is possible to merge the first version
> of the patch (http://www.spinics.net/lists/linux-scsi/msg73984.html)?
> We could proceed to rewrite the functions later.

I'm fine with adding the pints, but what work is Hannes doing with
scsi_execute at the moment?

Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[Hannes Reinecke]

On 05/13/2014 04:07 PM, Christoph Hellwig wrote:
> On Tue, May 13, 2014 at 03:10:15PM +0200, Maurizio Lombardi wrote:
>> Hannes Reinecke is also working on those functions, I'll have to collaborate
>> with him to minimize the conflicts with the patchset he is preparing.
>>
>> In the meantime, do you think it is possible to merge the first version
>> of the patch (http://www.spinics.net/lists/linux-scsi/msg73984.html)?
>> We could proceed to rewrite the functions later.
>
> I'm fine with adding the pints, but what work is Hannes doing with
> scsi_execute at the moment?
>
I'm not doing any work with scsi_execute.
I do, however, have a patchset for rewriting scsi_dh_alua.
Which touches those areas.

If you were to go ahead with the patch please strip the first 
scsi_execute() altogether and use the VPD information already provided 
by the scsi device.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[Maurizio Lombardi]

On Tue, May 13, 2014 at 04:14:47PM +0200, Hannes Reinecke wrote:
> If you were to go ahead with the patch please strip the first
> scsi_execute() altogether and use the VPD information already
> provided by the scsi device.

The patch I'm asking to merge just adds some printks, nothing else,
it does not make use of scsi_execute():

http://www.spinics.net/lists/linux-scsi/msg73984.html

Maurizio Lombardi

Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[Christoph Hellwig]

On Tue, May 13, 2014 at 04:24:02PM +0200, Maurizio Lombardi wrote:
> On Tue, May 13, 2014 at 04:14:47PM +0200, Hannes Reinecke wrote:
> > If you were to go ahead with the patch please strip the first
> > scsi_execute() altogether and use the VPD information already
> > provided by the scsi device.
> 
> The patch I'm asking to merge just adds some printks, nothing else,
> it does not make use of scsi_execute():
> 
> http://www.spinics.net/lists/linux-scsi/msg73984.html

We now have a readily available copy of EVPD page 0x83 hanging off
the scsi device, so the alua device handler should use it.

Hannes, how about you handle that if you're doing major surgery in that
area anwyay?

Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[Maurizio Lombardi]

On Thu, May 15, 2014 at 08:41:10AM +0200, Christoph Hellwig wrote:
> 
> We now have a readily available copy of EVPD page 0x83 hanging off
> the scsi device, so the alua device handler should use it.
> 
> Hannes, how about you handle that if you're doing major surgery in that
> area anwyay?

Hannes?

Did you notice this email?

Thanks,
Maurizio Lombardi

Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.

[Hannes Reinecke]

On 06/04/2014 09:51 AM, Maurizio Lombardi wrote:
> On Thu, May 15, 2014 at 08:41:10AM +0200, Christoph Hellwig wrote:
>>
>> We now have a readily available copy of EVPD page 0x83 hanging off
>> the scsi device, so the alua device handler should use it.
>>
>> Hannes, how about you handle that if you're doing major surgery in that
>> area anwyay?
>
> Hannes?
>
> Did you notice this email?
>
Oh, yes, I did.
Will be sending a patch shortly.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html