[PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Hannes Reinecke]

The module parameter 'iser_pi_guard' has been disabled by commit
5bb6e543d2a7d58 ("IB/iser: DIX update"), but the functionality
to select the guard algorithm is still required.

Fixes: 5bb6e543d2a7d58 ("IB/iser: DIX update")
Signed-off-by: Hannes Reinecke <hare-IBi9RG/b67k@public.gmane.org>
Cc: Steve Schremmer <sschremm-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 19624e0..34c4d0a 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -649,8 +649,10 @@ static void iscsi_iser_cleanup_task(struct iscsi_task *task)
 			u32 sig_caps = ib_conn->device->ib_device->attrs.sig_prot_cap;
 
 			scsi_host_set_prot(shost, iser_dif_prot_caps(sig_caps));
-			scsi_host_set_guard(shost, SHOST_DIX_GUARD_IP |
-						   SHOST_DIX_GUARD_CRC);
+			if (iser_pi_guard)
+				scsi_host_set_guard(shost, SHOST_DIX_GUARD_IP);
+			else
+				scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);
 		}
 
 		if (iscsi_host_add(shost,
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Jason Gunthorpe]

On Wed, Jan 10, 2018 at 10:07:36AM +0100, Hannes Reinecke wrote:
> The module parameter 'iser_pi_guard' has been disabled by commit
> 5bb6e543d2a7d58 ("IB/iser: DIX update"), but the functionality
> to select the guard algorithm is still required.

Commit should explain why it is still required? What is the actual bug here?

Someone who understands this is going to have to Ack it for it to go
through the rdma tree..

> Fixes: 5bb6e543d2a7d58 ("IB/iser: DIX update")
> Signed-off-by: Hannes Reinecke <hare-IBi9RG/b67k@public.gmane.org>
> Cc: Steve Schremmer <sschremm-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
>  drivers/infiniband/ulp/iser/iscsi_iser.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
> index 19624e0..34c4d0a 100644
> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
> @@ -649,8 +649,10 @@ static void iscsi_iser_cleanup_task(struct iscsi_task *task)
>  			u32 sig_caps = ib_conn->device->ib_device->attrs.sig_prot_cap;
>  
>  			scsi_host_set_prot(shost, iser_dif_prot_caps(sig_caps));
> -			scsi_host_set_guard(shost, SHOST_DIX_GUARD_IP |
> -						   SHOST_DIX_GUARD_CRC);
> +			if (iser_pi_guard)
> +				scsi_host_set_guard(shost, SHOST_DIX_GUARD_IP);
> +			else
> +				scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);
>  		}
>  
>  		if (iscsi_host_add(shost,
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Schremmer, Steven]

> From: Jason Gunthorpe [mailto:jgg-uk2M96/98Pc@public.gmane.org]
> Sent: Wednesday, January 10, 2018 5:03 PM
> 
> On Wed, Jan 10, 2018 at 10:07:36AM +0100, Hannes Reinecke wrote:
> > The module parameter 'iser_pi_guard' has been disabled by commit
> > 5bb6e543d2a7d58 ("IB/iser: DIX update"), but the functionality
> > to select the guard algorithm is still required.
> 
> Commit should explain why it is still required? What is the actual bug here?
> 
> Someone who understands this is going to have to Ack it for it to go
> through the rdma tree..
> 
Without the module parameter, there is no way to actually use the CRC guard format.
Currently, iscsi_iser_session_create() indicates support for both IP and CRC formats, but 
sd_dif_config_host() always checks for IP support first, so CRC guard won't be used.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Jason Gunthorpe]

On Thu, Jan 11, 2018 at 12:04:27AM +0000, Schremmer, Steven wrote:
> > From: Jason Gunthorpe [mailto:jgg-uk2M96/98Pc@public.gmane.org]
> > Sent: Wednesday, January 10, 2018 5:03 PM
> > 
> > On Wed, Jan 10, 2018 at 10:07:36AM +0100, Hannes Reinecke wrote:
> > > The module parameter 'iser_pi_guard' has been disabled by commit
> > > 5bb6e543d2a7d58 ("IB/iser: DIX update"), but the functionality
> > > to select the guard algorithm is still required.
> > 
> > Commit should explain why it is still required? What is the actual bug here?
> > 
> > Someone who understands this is going to have to Ack it for it to go
> > through the rdma tree..
> > 
> Without the module parameter, there is no way to actually use the CRC guard format.
> Currently, iscsi_iser_session_create() indicates support for both IP and CRC formats, but 
> sd_dif_config_host() always checks for IP support first, so CRC guard won't be used.

The above paragraph would be a great addition to the commit message.

Still need an ack :)

BTW - not knowing anything, why isn't this knob in the core sd code?
Other drivers need it too from whatI could see?

We really don't like module parameters in the kernel.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Bart Van Assche]

On Thu, 2018-01-11 at 09:58 -0700, Jason Gunthorpe wrote:
> On Thu, Jan 11, 2018 at 12:04:27AM +0000, Schremmer, Steven wrote:
> > Without the module parameter, there is no way to actually use the CRC guard format.
> > Currently, iscsi_iser_session_create() indicates support for both IP and CRC formats, but 
> > sd_dif_config_host() always checks for IP support first, so CRC guard won't be used.
> 
> The above paragraph would be a great addition to the commit message.
> 
> Still need an ack :)
> 
> BTW - not knowing anything, why isn't this knob in the core sd code?
> Other drivers need it too from whatI could see?
> 
> We really don't like module parameters in the kernel.

Has it been considered to specify which checksum type to use through iscsiadm
to the iSER initiator driver? I think that would avoid that we have to resurrect
the kernel module parameter.

Thanks,

Bart.

Re: [PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Sagi Grimberg]

Hi Steven and Hannes,

>> On Wed, Jan 10, 2018 at 10:07:36AM +0100, Hannes Reinecke wrote:
>>> The module parameter 'iser_pi_guard' has been disabled by commit
>>> 5bb6e543d2a7d58 ("IB/iser: DIX update"), but the functionality
>>> to select the guard algorithm is still required.
>>
>> Commit should explain why it is still required? What is the actual bug here?
>>
>> Someone who understands this is going to have to Ack it for it to go
>> through the rdma tree..
>>
> Without the module parameter, there is no way to actually use the CRC guard format.
> Currently, iscsi_iser_session_create() indicates support for both IP and CRC formats, but
> sd_dif_config_host() always checks for IP support first, so CRC guard won't be used.

Isn't a bit backwards that each individual driver needs this knob to
modify the block layer behavior? I think a better approach would be to
get rid of the drivers modparams and simply add a block sysfs knob that
would take the knob guard if supported...

Thoughts?

CCing Martin...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Martin K. Petersen]

Sagi,

> Isn't a bit backwards that each individual driver needs this knob to
> modify the block layer behavior? I think a better approach would be to
> get rid of the drivers modparams and simply add a block sysfs knob
> that would take the knob guard if supported...

Originally the IP checksum thing was an optimization for a single
device. But others adopted it as well so it grew from being a driver
tweak to a common feature.

I don't have a problem adding a way to toggle it at the block layer. But
it would have to be an additional knob. We can't nuke the module
parameters without breaking a ton of stuff...

-- 
Martin K. Petersen	Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Hannes Reinecke]

On 01/16/2018 03:57 AM, Martin K. Petersen wrote:
> 
> Sagi,
> 
>> Isn't a bit backwards that each individual driver needs this knob to
>> modify the block layer behavior? I think a better approach would be to
>> get rid of the drivers modparams and simply add a block sysfs knob
>> that would take the knob guard if supported...
> 
> Originally the IP checksum thing was an optimization for a single
> device. But others adopted it as well so it grew from being a driver
> tweak to a common feature.
> 
> I don't have a problem adding a way to toggle it at the block layer. But
> it would have to be an additional knob. We can't nuke the module
> parameters without breaking a ton of stuff...
> 
?

So what now?
Do we need to keep the parameter?
If so we really should be re-enable it; ATM it's just a no-op leading
the user to believe something has actually happened.

I'm fine with adding a knob in sysfs to enable things, but I'm a bit
puzzled now what will happen with this parameter...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare-IBi9RG/b67k@public.gmane.org			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Martin K. Petersen]

Hannes,

> Do we need to keep the parameter?
> If so we really should be re-enable it; ATM it's just a no-op leading
> the user to believe something has actually happened.

The driver module parameters existed mainly for the purpose of hw
testing. One could disable IP checksum and revert to CRC to ensure both
code paths were working properly. But it wasn't really meant to be
something ordinary users would ever muck with. Why would anybody use the
much slower CRC when IP checksum was supported by the hardware? (*)

The reason the kernel interface changed from a per-driver to a per-I/O
flag for checksum selection was that there are a few things you can't
express when checksum conversion is taking place. For instance, say
you're writing a RAID parity disk. The 8 additional bytes on the parity
disk are not valid T10 PI but rather the XOR of the PI across the
stripe. So you need to be able to disable checking and conversion on a
per-I/O basis when accessing the parity disk. Deliberately writing "bad"
PI for test purposes is another example where conversion must be
disabled.

Anyway. I think Sagi's iser_pi_guard patch was a result of that
transition from driver global checksum setting to per-I/O ditto.

I don't have a problem with having a block-level preference knob
(slightly convoluted to implement since they are different integrity
profiles). But I do question whether it is worth the hassle. What
exactly is your use case that requires twiddling this?

(*) The modern PCLMULQDQ-optimized CRC calculation blurs that picture
slightly.

-- 
Martin K. Petersen	Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Sagi Grimberg]

> Sagi,
> 
>> Isn't a bit backwards that each individual driver needs this knob to
>> modify the block layer behavior? I think a better approach would be to
>> get rid of the drivers modparams and simply add a block sysfs knob
>> that would take the knob guard if supported...
> 
> Originally the IP checksum thing was an optimization for a single
> device. But others adopted it as well so it grew from being a driver
> tweak to a common feature.
> 
> I don't have a problem adding a way to toggle it at the block layer. But
> it would have to be an additional knob.

Personally I think that the clean way to have it is that the driver
exposes capabilities and the core selects the method instead of
having each driver expose what it supports based on the desired method.

But I really don't care that much, if its not something we consider
worth doing then I can easily ack this patch.

> We can't nuke the module parameters without breaking a ton of stuff...

Correct, but we can gradually deprecate them..
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Sagi Grimberg]

> Hannes,
> 
>> Do we need to keep the parameter?
>> If so we really should be re-enable it; ATM it's just a no-op leading
>> the user to believe something has actually happened.
> 
> The driver module parameters existed mainly for the purpose of hw
> testing. One could disable IP checksum and revert to CRC to ensure both
> code paths were working properly. But it wasn't really meant to be
> something ordinary users would ever muck with. Why would anybody use the
> much slower CRC when IP checksum was supported by the hardware? (*)

Agree, but I definitely might not see the full picture here.

> Anyway. I think Sagi's iser_pi_guard patch was a result of that
> transition from driver global checksum setting to per-I/O ditto.

Correct, and due to your comment above.

> I don't have a problem with having a block-level preference knob
> (slightly convoluted to implement since they are different integrity
> profiles). But I do question whether it is worth the hassle. What
> exactly is your use case that requires twiddling this?

I'm not sure its worth it either, I can easily ack this one if that's
the case.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Schremmer, Steven]

> From: Sagi Grimberg [mailto:sagi@grimberg.me]
> Sent: Wednesday, January 17, 2018 3:59 AM
> > Hannes,
> >
> >> Do we need to keep the parameter?
> >> If so we really should be re-enable it; ATM it's just a no-op leading
> >> the user to believe something has actually happened.
> >
> > The driver module parameters existed mainly for the purpose of hw
> > testing. One could disable IP checksum and revert to CRC to ensure both
> > code paths were working properly. But it wasn't really meant to be
> > something ordinary users would ever muck with. Why would anybody use the
> > much slower CRC when IP checksum was supported by the hardware? (*)
> 
> Agree, but I definitely might not see the full picture here.
If the target uses CRC, the host will need a way to match.
AFAIK, there is no way in the SCSI protocol to determine this, so admin
has to set it up.

> 
> > Anyway. I think Sagi's iser_pi_guard patch was a result of that
> > transition from driver global checksum setting to per-I/O ditto.
> 
> Correct, and due to your comment above.
> 
> > I don't have a problem with having a block-level preference knob
> > (slightly convoluted to implement since they are different integrity
> > profiles). But I do question whether it is worth the hassle. What
> > exactly is your use case that requires twiddling this?
A target that can do CRC Guard checks internally, but doesn't use IP
checksum.

> 
> I'm not sure its worth it either, I can easily ack this one if that's
> the case.
This patch would restore the previous knob which meets the need
for now. If a better method is developed in the future, then great.

Thanks,
Steve

Re: [PATCH] iscsi_iser: Re-enable 'iser_pi_guard' module parameter

[Martin K. Petersen]

Steven,

> If the target uses CRC, the host will need a way to match.  AFAIK,
> there is no way in the SCSI protocol to determine this,

The IP checksum feature is entirely DIX, so no.

There is no guard format feature discovery taking place since only the
initiator supports IP checksum. It is always T10 CRC on the wire between
initiator and target on SAS/FC.

> This patch would restore the previous knob which meets the need
> for now. If a better method is developed in the future, then great.

I'm not against moving the checksum selection up the stack in
principle. But it's a lot of churn for little gain, IMHO. So I think I'm
in favor of reviving the iser_pi_guard parameter. At least in the short
term.

-- 
Martin K. Petersen	Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html