Re: [PATCH 1/3] scsi_dh_rdac: switch to scsi_execute_req_flags()
On 11/03/2016 05:11 PM, Ewan D. Milne wrote: On Wed, 2016-11-02 at 22:27 +0100, Hannes Reinecke wrote: On 11/02/2016 04:44 PM, Ewan D. Milne wrote: In this and other places the patch changes the code from submitting the INQUIRY/EVPD command for the page it wants, to calling scsi_get_vpd_page(). scsi_get_vpd_page() will return -EINVAL if the device did not report in VPD page 0 that the requested page is in the list of supported pages. Did you actually verify that the RDAC returns all of these VPD pages in its VPD page 0 list? I mean, I know it's supposed to, but these are old devices. Errm. Old? Don't tell that to the E-Series folk; you'll never again get something sponsored :-) No, seriously: RDAC mode is continued to be supported even on the latest models, and all firmware revisions I've got support VPD page 0x0. And incidentally, this is the very same method we're using for basically all tools accessing VPD page 0x83, be it in the kernel or something like sg3_utils. _Not_ asking for page 0x0 lead to crashes on several older devices. You're right, I was curious, so I resurrected our old RDAC, and it does in fact report the necessary supported VPD pages: # sg_vpd -H /dev/sg4 Supported VPD pages VPD page: 00 20 00 00 12 00 80 83 85 86 87 b0 b1 c0 c1 c2 c3 ... 10 c4 c8 c9 ca d0 e0 .. My concern was that older devices might not do this, I think there have been some arrays that had hidden commands/pages. The original mpp_rdac (ie the multipath software from Engenio/LSI/NetApp) was using the 'C?' pages to identify the array, so they most certainly are present on all arrays. And to properly (and safely :-) identify you have to query page 0 to figure out if those pages are supported. Hence we're safe here. I could ask NetApp, though, for a final confirmation if required. Cheers, Hannes -- 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/3] scsi_dh_rdac: switch to scsi_execute_req_flags()
On Wed, 2016-11-02 at 22:27 +0100, Hannes Reinecke wrote: > On 11/02/2016 04:44 PM, Ewan D. Milne wrote: > > In this and other places the patch changes the code from submitting the > > INQUIRY/EVPD command for the page it wants, to calling scsi_get_vpd_page(). > > scsi_get_vpd_page() will return -EINVAL if the device did not report in > > VPD page 0 that the requested page is in the list of supported pages. > > Did you actually verify that the RDAC returns all of these VPD pages in > > its VPD page 0 list? > > > > I mean, I know it's supposed to, but these are old devices. > > > Errm. > > Old? Don't tell that to the E-Series folk; you'll never again get > something sponsored :-) > > No, seriously: RDAC mode is continued to be supported even on the latest > models, and all firmware revisions I've got support VPD page 0x0. > And incidentally, this is the very same method we're using for basically > all tools accessing VPD page 0x83, be it in the kernel or something like > sg3_utils. > _Not_ asking for page 0x0 lead to crashes on several older devices. You're right, I was curious, so I resurrected our old RDAC, and it does in fact report the necessary supported VPD pages: # sg_vpd -H /dev/sg4 Supported VPD pages VPD page: 00 20 00 00 12 00 80 83 85 86 87 b0 b1 c0 c1 c2 c3 ... 10 c4 c8 c9 ca d0 e0 .. My concern was that older devices might not do this, I think there have been some arrays that had hidden commands/pages. -Ewan -- 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/3] scsi_dh_rdac: switch to scsi_execute_req_flags()
On 11/02/2016 04:06 PM, Christoph Hellwig wrote:
On Tue, Nov 01, 2016 at 10:49:36PM +0100, Hannes Reinecke wrote:
Switch to scsi_execute_req_flags() instead of
using the block interface directly. This will set
REQ_QUIET and REQ_PREEMPT, but this is okay as
we're evaluating the errors anyway and should be
able to send the command even if the device is
quiesced.
Actually most users are switched to scsi_get_vpd_page if I read the patch
right, which is even better. And it seems like the remaining user of
scsi_execute_req_flags could be switch to use scsi_mode_select() as well,
but maybe we should leave that out for now.
I'd rather not do that now, as switching to scsi_mode_select() would
cause the failfast flags to be lost, and I'll have to think about the
implications of doing so.
+ if (scsi_execute_req_flags(sdev, cdb, DMA_TO_DEVICE,
+ >ctlr->mode_select, data_size, ,
+ RDAC_TIMEOUT * HZ,
+ RDAC_RETRIES, NULL, REQ_FAILFAST_MASK)) {
Pleae use the individual failfast flags - REQ_FAILFAST_MASK is just
for block layer use.
Okay.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] scsi_dh_rdac: switch to scsi_execute_req_flags()
On 11/02/2016 04:44 PM, Ewan D. Milne wrote:
On Tue, 2016-11-01 at 22:49 +0100, Hannes Reinecke wrote:
...
static int get_lun_info(struct scsi_device *sdev, struct rdac_dh_data *h,
char *array_name, u8 *array_id)
{
- int err, i;
- struct c8_inquiry *inqp;
+ int err = SCSI_DH_IO, i;
+ struct c8_inquiry *inqp = >inq.c8;
- err = submit_inquiry(sdev, 0xC8, sizeof(struct c8_inquiry), h);
- if (err == SCSI_DH_OK) {
- inqp = >inq.c8;
+ if (!scsi_get_vpd_page(sdev, 0xC8, (unsigned char *)inqp,
+ sizeof(struct c8_inquiry))) {
if (inqp->page_code != 0xc8)
return SCSI_DH_NOSYS;
if (inqp->page_id[0] != 'e' || inqp->page_id[1] != 'd' ||
@@ -453,20 +382,20 @@ static int get_lun_info(struct scsi_device *sdev, struct
rdac_dh_data *h,
*(array_name+ARRAY_LABEL_LEN-1) = '\0';
memset(array_id, 0, UNIQUE_ID_LEN);
memcpy(array_id, inqp->array_unique_id,
inqp->array_uniq_id_len);
+ err = SCSI_DH_OK;
}
return err;
}
In this and other places the patch changes the code from submitting the
INQUIRY/EVPD command for the page it wants, to calling scsi_get_vpd_page().
scsi_get_vpd_page() will return -EINVAL if the device did not report in
VPD page 0 that the requested page is in the list of supported pages.
Did you actually verify that the RDAC returns all of these VPD pages in
its VPD page 0 list?
I mean, I know it's supposed to, but these are old devices.
Errm.
Old? Don't tell that to the E-Series folk; you'll never again get
something sponsored :-)
No, seriously: RDAC mode is continued to be supported even on the latest
models, and all firmware revisions I've got support VPD page 0x0.
And incidentally, this is the very same method we're using for basically
all tools accessing VPD page 0x83, be it in the kernel or something like
sg3_utils.
_Not_ asking for page 0x0 lead to crashes on several older devices.
Cheers,
Hannes
--
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/3] scsi_dh_rdac: switch to scsi_execute_req_flags()
On Tue, 2016-11-01 at 22:49 +0100, Hannes Reinecke wrote:
...
> static int get_lun_info(struct scsi_device *sdev, struct rdac_dh_data *h,
> char *array_name, u8 *array_id)
> {
> - int err, i;
> - struct c8_inquiry *inqp;
> + int err = SCSI_DH_IO, i;
> + struct c8_inquiry *inqp = >inq.c8;
>
> - err = submit_inquiry(sdev, 0xC8, sizeof(struct c8_inquiry), h);
> - if (err == SCSI_DH_OK) {
> - inqp = >inq.c8;
> + if (!scsi_get_vpd_page(sdev, 0xC8, (unsigned char *)inqp,
> +sizeof(struct c8_inquiry))) {
> if (inqp->page_code != 0xc8)
> return SCSI_DH_NOSYS;
> if (inqp->page_id[0] != 'e' || inqp->page_id[1] != 'd' ||
> @@ -453,20 +382,20 @@ static int get_lun_info(struct scsi_device *sdev,
> struct rdac_dh_data *h,
> *(array_name+ARRAY_LABEL_LEN-1) = '\0';
> memset(array_id, 0, UNIQUE_ID_LEN);
> memcpy(array_id, inqp->array_unique_id,
> inqp->array_uniq_id_len);
> + err = SCSI_DH_OK;
> }
> return err;
> }
>
In this and other places the patch changes the code from submitting the
INQUIRY/EVPD command for the page it wants, to calling scsi_get_vpd_page().
scsi_get_vpd_page() will return -EINVAL if the device did not report in
VPD page 0 that the requested page is in the list of supported pages.
Did you actually verify that the RDAC returns all of these VPD pages in
its VPD page 0 list?
I mean, I know it's supposed to, but these are old devices.
-Ewan
--
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/3] scsi_dh_rdac: switch to scsi_execute_req_flags()
On Tue, Nov 01, 2016 at 10:49:36PM +0100, Hannes Reinecke wrote:
> Switch to scsi_execute_req_flags() instead of
> using the block interface directly. This will set
> REQ_QUIET and REQ_PREEMPT, but this is okay as
> we're evaluating the errors anyway and should be
> able to send the command even if the device is
> quiesced.
Actually most users are switched to scsi_get_vpd_page if I read the patch
right, which is even better. And it seems like the remaining user of
scsi_execute_req_flags could be switch to use scsi_mode_select() as well,
but maybe we should leave that out for now.
> + if (scsi_execute_req_flags(sdev, cdb, DMA_TO_DEVICE,
> +>ctlr->mode_select, data_size, ,
> +RDAC_TIMEOUT * HZ,
> +RDAC_RETRIES, NULL, REQ_FAILFAST_MASK)) {
Pleae use the individual failfast flags - REQ_FAILFAST_MASK is just
for block layer use.
--
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/3] scsi_dh_rdac: switch to scsi_execute_req_flags()
On 11/01/2016 03:47 PM, Christoph Hellwig wrote:
On Mon, Oct 31, 2016 at 06:59:28PM +0100, Hannes Reinecke wrote:
Switch to using scsi_execute_req_flags() instead of using the
block primitives.
__scsi_execute adds RQF_QUIET and RQF_PREEMPT to the request flags, which
would be a change in behavior. A little analysis on why that's safe or
even desireable would be nice. (This also applies to the other two patches
I think).
Hmm. Yeah, guess I'll need to reconcile that.
static void release_controller(struct kref *kref)
static int get_lun_info(struct scsi_device *sdev, struct rdac_dh_data *h,
char *array_name, u8 *array_id)
{
+ int err = SCSI_DH_IO, i;
struct c8_inquiry *inqp;
+ if (!scsi_get_vpd_page(sdev, 0xC8, (unsigned char *)h,
+ sizeof(struct c8_inquiry))) {
This looks completely bogus to me - h is a struct rdac_dh_data pointer,
which is an in-kernel data structure that scsi_get_vpd_page would
scramble over.
Indeed, you are right. I'll be fixing it up.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] scsi_dh_rdac: switch to scsi_execute_req_flags()
On Mon, Oct 31, 2016 at 06:59:28PM +0100, Hannes Reinecke wrote:
> Switch to using scsi_execute_req_flags() instead of using the
> block primitives.
__scsi_execute adds RQF_QUIET and RQF_PREEMPT to the request flags, which
would be a change in behavior. A little analysis on why that's safe or
even desireable would be nice. (This also applies to the other two patches
I think).
>
> static void release_controller(struct kref *kref)
> static int get_lun_info(struct scsi_device *sdev, struct rdac_dh_data *h,
> char *array_name, u8 *array_id)
> {
> + int err = SCSI_DH_IO, i;
> struct c8_inquiry *inqp;
>
> + if (!scsi_get_vpd_page(sdev, 0xC8, (unsigned char *)h,
> +sizeof(struct c8_inquiry))) {
This looks completely bogus to me - h is a struct rdac_dh_data pointer,
which is an in-kernel data structure that scsi_get_vpd_page would
scramble over.
--
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
