| Message ID | 1515575256-9949-1-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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@suse.com> > Cc: Steve Schremmer <sschremm@netapp.com> > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Jason Gunthorpe [mailto:jgg@ziepe.ca] > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 11, 2018 at 12:04:27AM +0000, Schremmer, Steven wrote: > > From: Jason Gunthorpe [mailto:jgg@ziepe.ca] > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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...
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
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.
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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
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.
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,
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@suse.com> Cc: Steve Schremmer <sschremm@netapp.com> --- drivers/infiniband/ulp/iser/iscsi_iser.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)