Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
On 10/23/2017 01:17 AM, Martin K. Petersen wrote: > > Christoph, > >>> Yes, I expected the bsg bits to go through Jens' tree. >> >> Ok, then I misremembered it, and we'll have to delay the remaining >> patches until the next merge window, as they depend on the previous >> ones. > > I don't mind taking them through SCSI if Jens agrees. I'm fine with that, as long as the last issue Benjamin brought up has been fixed up. -- Jens Axboe
Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
> +static int bsg_transport_complete_rq(struct request *rq, struct sg_io_v4
> *hdr)
> +{
> + struct bsg_job *job = blk_mq_rq_to_pdu(rq);
> + int ret = 0;
> +
> + /*
> + * The assignments below don't make much sense, but are kept for
> + * bug by bug backwards compatibility:
> + */
> + hdr->device_status = job->result & 0xff;
> + hdr->transport_status = host_byte(job->result);
> + hdr->driver_status = driver_byte(job->result);
> + hdr->info = 0;
> + if (hdr->device_status || hdr->transport_status || hdr->driver_status)
> + hdr->info |= SG_INFO_CHECK;
> + hdr->response_len = 0;
> +
> + if (job->result < 0) {
> + /* we're only returning the result field in the reply */
> + job->reply_len = sizeof(u32);
> + ret = job->result;
> + }
> +
> + if (job->reply_len && hdr->response) {
> + int len = min(hdr->max_response_len, job->reply_len);
> +
> + if (copy_to_user(ptr64(hdr->response), job->reply, len))
> + ret = -EFAULT;
> + else
> + hdr->response_len = len;
> + }
> +
> + /* we assume all request payload was transferred, residual == 0 */
> + hdr->dout_resid = 0;
> +
> + if (rq->next_rq) {
> + unsigned int rsp_len = blk_rq_bytes(rq->next_rq);
> +
> + if (WARN_ON(job->reply_payload_rcv_len > rsp_len))
This gives my a lot of new Warnings when running my tests on zFCP, non
of which happen when I run on 4.13.
After browsing the source some, I figured this is because in comparison
to the old code, blk_rq_bytes() is now called after we finished the
blk-request already and blk_update_bidi_request()+blk_update_request()
was already called. This will update rq->__data_len, and thus the call
here to get rsp_len will get a wrong value. Thus the warnings, and the
following calculation is actually wrong.
I figure you can just replace this call for blk_rq_bytes() with
'job->reply_payload.payload_len'. Its essentially the same value, but
the later is not changed after the job is committed as far as I could see
in the source. Driver makes use of it, but only reading as far as I
could make out after browsing the code for a bit.
I did a quick test with that change in place and that seems to work fine
now. As far as my tests go, they behave as they did before.
Beste Grüße / Best regards,
- Benjamin Block
> + hdr->din_resid = 0;
> + else
> + hdr->din_resid = rsp_len - job->reply_payload_rcv_len;
> + } else {
> + hdr->din_resid = 0;
> + }
> +
> + return ret;
> +}
> +
--
Linux on z Systems Development / IBM Systems & Technology Group
IBM Deutschland Research & Development GmbH
Vorsitz. AufsR.: Martina Koederitz /Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
Christoph, >> Yes, I expected the bsg bits to go through Jens' tree. > > Ok, then I misremembered it, and we'll have to delay the remaining > patches until the next merge window, as they depend on the previous > ones. I don't mind taking them through SCSI if Jens agrees. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
On Mon, Oct 23, 2017 at 02:16:03AM -0400, Martin K. Petersen wrote: > > Benjamin, > > >> Not sure it's worth it especially now that Martin has merged the patch. > > > > He did? I only saw a mail that he picked patches 2-5. So all the bsg > > changes are still open I think. > > Yes, I expected the bsg bits to go through Jens' tree. Ok, then I misremembered it, and we'll have to delay the remaining patches until the next merge window, as they depend on the previous ones.
Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
Benjamin, >> Not sure it's worth it especially now that Martin has merged the patch. > > He did? I only saw a mail that he picked patches 2-5. So all the bsg > changes are still open I think. Yes, I expected the bsg bits to go through Jens' tree. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
On Fri, Oct 20, 2017 at 06:26:30PM +0200, Christoph Hellwig wrote:
> On Thu, Oct 19, 2017 at 05:59:33PM +0200, Benjamin Block wrote:
> > > +#define ptr64(val) ((void __user *)(uintptr_t)(val))
> >
> > Better to reflect the special property, that it is a user pointer, in
> > the name of the macro. Maybe something like user_ptr(64). The same
> > comment for the same macro in bsg.c.
>
> Not sure it's worth it especially now that Martin has merged the patch.
He did? I only saw a mail that he picked patches 2-5. So all the bsg
changes are still open I think.
(Maybe I just missed that, I haven't exactly followed the list very
closely as of late)
> But given how many interface we have all over the kernel that use a u64
> to store a user pointer in ioctls and similar it might make sense to
> lift a helper like this to a generic header. In that case we'll need
> a more descriptive name for sure.
>
> > > +static int bsg_transport_check_proto(struct sg_io_v4 *hdr)
> > > +{
> > > + if (hdr->protocol != BSG_PROTOCOL_SCSI ||
> > > + hdr->subprotocol != BSG_SUB_PROTOCOL_SCSI_TRANSPORT)
> > > + return -EINVAL;
> > > + if (!capable(CAP_SYS_RAWIO))
> > > + return -EPERM;
> >
> > Any particular reason why this is not symmetric with bsg_scsi? IOW
> > permission checking done in bsg_transport_fill_hdr(), like it is done in
> > bsg_scsi_fill_hdr()?
> >
> > We might save some time copying memory with this (we also only talk
> > about ~20 bytes here), but on the other hand the interface would be more
> > clean otherwise IMO (if we already do restructure the interface) -
> > similar callbacks have similar responsibilities.
>
> I could move the capable check around, no sure why I had done it that
> way, it's been a while. Probably because blk_verify_command needs the
> CDB while a simple capable() check does not.
That was my guess, too. I just though it would be more consistent otherwise.
Its not a big thing, really.
Beste Grüße / Best regards,
- Benjamin Block
--
Linux on z Systems Development / IBM Systems & Technology Group
IBM Deutschland Research & Development GmbH
Vorsitz. AufsR.: Martina Koederitz /Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
On Thu, Oct 19, 2017 at 05:59:33PM +0200, Benjamin Block wrote:
> > +#define ptr64(val) ((void __user *)(uintptr_t)(val))
>
> Better to reflect the special property, that it is a user pointer, in
> the name of the macro. Maybe something like user_ptr(64). The same
> comment for the same macro in bsg.c.
Not sure it's worth it especially now that Martin has merged the patch.
But given how many interface we have all over the kernel that use a u64
to store a user pointer in ioctls and similar it might make sense to
lift a helper like this to a generic header. In that case we'll need
a more descriptive name for sure.
> > +static int bsg_transport_check_proto(struct sg_io_v4 *hdr)
> > +{
> > + if (hdr->protocol != BSG_PROTOCOL_SCSI ||
> > + hdr->subprotocol != BSG_SUB_PROTOCOL_SCSI_TRANSPORT)
> > + return -EINVAL;
> > + if (!capable(CAP_SYS_RAWIO))
> > + return -EPERM;
>
> Any particular reason why this is not symmetric with bsg_scsi? IOW
> permission checking done in bsg_transport_fill_hdr(), like it is done in
> bsg_scsi_fill_hdr()?
>
> We might save some time copying memory with this (we also only talk
> about ~20 bytes here), but on the other hand the interface would be more
> clean otherwise IMO (if we already do restructure the interface) -
> similar callbacks have similar responsibilities.
I could move the capable check around, no sure why I had done it that
way, it's been a while. Probably because blk_verify_command needs the
CDB while a simple capable() check does not.
> If I understand this right, the this reflects the old code, if only
> written down a little different.
>
> But I wonder why we do that? Wouldn't that be interesting to know for
> uspace, if more was received than it allocated space for? Isn't that the
> typical residual over run case (similar to LUN scanning in SCSI common
> code), and din_resid is signed after all? Well I guess it could be an
> ABI break, I don't know.
>
> Ah well, at least the documentation for 'struct sg_io_v4' makes no such
> restrictions (that it can not be below 0).
>
> Just a thought I had while reading it.
Maybe it would, but I really didn't want to change behavior. If we
were to redo transport passthrough I would do it totally different today.
> > + job->reply = kzalloc(SCSI_SENSE_BUFFERSIZE, gfp);
>
> One suggestion here. Maybe we could get rid of this implicit knowledge
> about SCSI_SENSE_BUFFERSIZE being the max size for a bsg-reply?
> Especially if we use this patch and get rid of other similarities (like
> using scsi_request).
>
> Maybe we can just define a extra macro in bsg-lib.c, or in one of the
> headers, and define its size to be SCSI_SENSE_BUFFERSIZE (for now) and
> then use that in all cases.
>
> I tried something similar some time ego if you remember, but I couldn't
> follow up because other stuff got more important in the meantime. One
> could also static check the transport reply-types against that.
>
> This way, should need to change that value for a sepcific transport, we
> only need to change one knob, and not 10 (I guess SCSI_SENSE_BUFFERSIZE
> could not be changed for such cases ;) ).
There shouldn't be any dependencies on SCSI_SENSE_BUFFERSIZE left,
so yes, this could be cleaned up. Great opportunity for a follow on
patch.
> > - /* if the LLD has been removed then the bsg_unregister_queue will
> > -* eventually be called and the class_dev was freed, so we can no
> > -* longer use this request_queue. Return no such address.
> > -*/
>
> Why remove the comment? Has that changed?
Nothing, but then again it's standard behavior so the comment doesn't
really add any value.
> > + rq->timeout = msecs_to_jiffies(hdr->timeout);
> > + if (!rq->timeout)
> > + rq->timeout = rq->q->sg_timeout;
>
> No need to use the rq pointer, you already have a variable q with the
> same content.
True.
> > - ret = blk_rq_map_user(q, rq, NULL, dxferp, dxfer_len,
> > - GFP_KERNEL);
> > - if (ret)
> > - goto out;
> > + ret = blk_rq_map_user(q, rq, NULL, ptr64(hdr->din_xferp),
> > + hdr->din_xfer_len, GFP_KERNEL);
> > + } else {
> > + ret = blk_rq_map_user(q, rq, NULL, NULL, 0, GFP_KERNEL);
>
> Why do we behave differently in this case now? To prevent special
> handling elsewhere? Otherwise it seems a bit pointless/error-prone
> mapping zero length to nothing.
Yes, this could be removed again. I'll send a follow up.
Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
Hey Christoph,
better late than never I guess.
On Tue, Oct 03, 2017 at 12:48:45PM +0200, Christoph Hellwig wrote:
> The current BSG design tries to shoe-horn the transport-specific passthrough
> commands into the overall framework for SCSI passthrough requests. This
> has a couple problems:
>
> - each passthrough queue has to set the QUEUE_FLAG_SCSI_PASSTHROUGH flag
>despite not dealing with SCSI commands at all. Because of that these
>queues could also incorrectly accept SCSI commands from in-kernel
>users or through the legacy SCSI_IOCTL_SEND_COMMAND ioctl.
> - the real SCSI bsg queues also incorrectly accept bsg requests of the
>BSG_SUB_PROTOCOL_SCSI_TRANSPORT type
> - the bsg transport code is almost unredable because it tries to reuse
>different SCSI concepts for its own purpose.
>
> This patch instead adds a new bsg_ops structure to handle the two cases
> differently, and thus solves all of the above problems. Another side
> effect is that the bsg-lib queues also don't need to embedd a
> struct scsi_request anymore.
>
> Signed-off-by: Christoph Hellwig
> ---
> block/bsg-lib.c | 158 +++
> block/bsg.c | 257
> +-
> drivers/scsi/scsi_lib.c | 4 +-
> drivers/scsi/scsi_sysfs.c | 3 +-
> drivers/scsi/scsi_transport_sas.c | 1 -
> include/linux/bsg-lib.h | 4 +-
> include/linux/bsg.h | 35 --
> 7 files changed, 251 insertions(+), 211 deletions(-)
>
> diff --git a/block/bsg-lib.c b/block/bsg-lib.c
> index 6299526bd2c3..99b459e21782 100644
> --- a/block/bsg-lib.c
> +++ b/block/bsg-lib.c
> @@ -27,6 +27,94 @@
> #include
> #include
> #include
> +#include
> +
> +#define ptr64(val) ((void __user *)(uintptr_t)(val))
Better to reflect the special property, that it is a user pointer, in
the name of the macro. Maybe something like user_ptr(64). The same
comment for the same macro in bsg.c.
> +
> +static int bsg_transport_check_proto(struct sg_io_v4 *hdr)
> +{
> + if (hdr->protocol != BSG_PROTOCOL_SCSI ||
> + hdr->subprotocol != BSG_SUB_PROTOCOL_SCSI_TRANSPORT)
> + return -EINVAL;
> + if (!capable(CAP_SYS_RAWIO))
> + return -EPERM;
Any particular reason why this is not symmetric with bsg_scsi? IOW
permission checking done in bsg_transport_fill_hdr(), like it is done in
bsg_scsi_fill_hdr()?
We might save some time copying memory with this (we also only talk
about ~20 bytes here), but on the other hand the interface would be more
clean otherwise IMO (if we already do restructure the interface) -
similar callbacks have similar responsibilities.
> + return 0;
> +}
> +
> +static int bsg_transport_fill_hdr(struct request *rq, struct sg_io_v4 *hdr,
> + fmode_t mode)
> +{
> + struct bsg_job *job = blk_mq_rq_to_pdu(rq);
> +
> + job->request_len = hdr->request_len;
> + job->request = memdup_user(ptr64(hdr->request), hdr->request_len);
> + if (IS_ERR(job->request))
> + return PTR_ERR(job->request);
> + return 0;
> +}
> +
> +static int bsg_transport_complete_rq(struct request *rq, struct sg_io_v4
> *hdr)
> +{
> + struct bsg_job *job = blk_mq_rq_to_pdu(rq);
> + int ret = 0;
> +
> + /*
> + * The assignments below don't make much sense, but are kept for
> + * bug by bug backwards compatibility:
> + */
> + hdr->device_status = job->result & 0xff;
> + hdr->transport_status = host_byte(job->result);
> + hdr->driver_status = driver_byte(job->result);
> + hdr->info = 0;
> + if (hdr->device_status || hdr->transport_status || hdr->driver_status)
> + hdr->info |= SG_INFO_CHECK;
> + hdr->response_len = 0;
> +
> + if (job->result < 0) {
> + /* we're only returning the result field in the reply */
> + job->reply_len = sizeof(u32);
> + ret = job->result;
> + }
> +
> + if (job->reply_len && hdr->response) {
> + int len = min(hdr->max_response_len, job->reply_len);
> +
> + if (copy_to_user(ptr64(hdr->response), job->reply, len))
> + ret = -EFAULT;
> + else
> + hdr->response_len = len;
very very minor nitpick: this is reversed with the handling in
bsg_scsi_complete_rq().. could be identical.
> + }
> +
> + /* we assume all request payload was transferred, residual == 0 */
> + hdr->dout_resid = 0;
> +
> + if (rq->next_rq) {
> + unsigned int rsp_len = blk_rq_bytes(rq->next_rq);
> +
> + if (WARN_ON(job->reply_payload_rcv_len > rsp_len))
> + hdr->din_resid = 0;
If I understand this right, the this reflects the old code, if only
written down a little different.
But I wonder why we do that? Wouldn't that be interesting to know for
uspace, if more was received than it allocated space for? Isn't that the
typical residual over run c
Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
On Wed, Oct 04, 2017 at 09:20:59AM +0200, Christoph Hellwig wrote: > It's a different level of callback - ops are the type of request > passed through (scsi vs transport) and ->release is s whacky > implementation detail of the SAS passthrough. If at all ->release > should go away eventually by cleaning that mess up. OK then, Reviewed-by: Johannes Thumshirn -- Johannes Thumshirn Storage jthumsh...@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
Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
On Wed, Oct 04, 2017 at 09:18:11AM +0200, Johannes Thumshirn wrote: > Wouldn't it make sense to put the ->release() method into bsg_ops as > well? The current prototype of bsg_register_queue isn't exactly what I > would call a sane API. It's a different level of callback - ops are the type of request passed through (scsi vs transport) and ->release is s whacky implementation detail of the SAS passthrough. If at all ->release should go away eventually by cleaning that mess up.
Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
Christoph Hellwig writes: [...] > @@ -965,7 +932,8 @@ void bsg_unregister_queue(struct request_queue *q) > EXPORT_SYMBOL_GPL(bsg_unregister_queue); > > int bsg_register_queue(struct request_queue *q, struct device *parent, > -const char *name, void (*release)(struct device *)) > + const char *name, const struct bsg_ops *ops, > + void (*release)(struct device *)) Wouldn't it make sense to put the ->release() method into bsg_ops as well? The current prototype of bsg_register_queue isn't exactly what I would call a sane API. -- Johannes Thumshirn Storage jthumsh...@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
Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
Christoph Hellwig writes: [...] > @@ -965,7 +932,8 @@ void bsg_unregister_queue(struct request_queue *q) > EXPORT_SYMBOL_GPL(bsg_unregister_queue); > > int bsg_register_queue(struct request_queue *q, struct device *parent, > -const char *name, void (*release)(struct device *)) > + const char *name, const struct bsg_ops *ops, > + void (*release)(struct device *)) Wouldn't it make sense to put the ->release() method into bsg_ops as well? The current prototype of bsg_register_queue isn't exactly what I would call a sane API. -- Johannes Thumshirn Storage jthumsh...@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
Re: [PATCH 9/9] bsg: split handling of SCSI CDBs vs transport requeues
On 10/03/2017 12:48 PM, Christoph Hellwig wrote: > The current BSG design tries to shoe-horn the transport-specific passthrough > commands into the overall framework for SCSI passthrough requests. This > has a couple problems: > > - each passthrough queue has to set the QUEUE_FLAG_SCSI_PASSTHROUGH flag >despite not dealing with SCSI commands at all. Because of that these >queues could also incorrectly accept SCSI commands from in-kernel >users or through the legacy SCSI_IOCTL_SEND_COMMAND ioctl. > - the real SCSI bsg queues also incorrectly accept bsg requests of the >BSG_SUB_PROTOCOL_SCSI_TRANSPORT type > - the bsg transport code is almost unredable because it tries to reuse >different SCSI concepts for its own purpose. > > This patch instead adds a new bsg_ops structure to handle the two cases > differently, and thus solves all of the above problems. Another side > effect is that the bsg-lib queues also don't need to embedd a > struct scsi_request anymore. > > Signed-off-by: Christoph Hellwig > --- > block/bsg-lib.c | 158 +++ > block/bsg.c | 257 > +- > drivers/scsi/scsi_lib.c | 4 +- > drivers/scsi/scsi_sysfs.c | 3 +- > drivers/scsi/scsi_transport_sas.c | 1 - > include/linux/bsg-lib.h | 4 +- > include/linux/bsg.h | 35 -- > 7 files changed, 251 insertions(+), 211 deletions(-) > Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
