Re: [PATCH 05/15] dm: remove incomple BLOCK_PC support

[Christoph Hellwig]

On Thu, Jan 12, 2017 at 05:28:45PM -0500, Mike Snitzer wrote:
> What is "incomplete" about request-based DM's BLOCK_PC support?

BLOCK_PC requests are always aomething issued by the driver itself
(for a broad defintion of the driver, aka everything under the block
layer that works together is a driver for this purpose, e.g. all
of the SCSI subsystem).  If a driver doesn't issue BLOCK_PC
requests itself or through library functions only called from the
driver (e.g. scsi_cmd_ioctl) it is incomplete because it can't
be used.

> I'm also missing how you're saying the new blk-mq request-based DM will
> work with your new model.  I appreciate that we get the request from the
> underlying blk-mq request_queue and it'll be properly sized.  But
> wouldn't we need to pass data back up for these SCSI pass-through
> requests?  So wouldn't the top-level multipath request_queue need to
> setup cmd_size?

As said above - supporting BLOCK_PC for dm does not make sense, as
it's an internal passthrough mechanism for driver internal use.
It just happened we standardized it at the block layer because SCSI
commands are a standard supported by a few different drivers, e.g.
SCSI itself, the old ide code for ATAPI and CCISS and virtio_blk
which primarily are block drivers but allow some SCSI passthrough.

To be honest I'd love to just fold BLOCK_PC into the SCSI layer sooner
or later - the old IDE code and CCISS should die off sooner or later,
and virtio_blk scsi passthrough was a horrible idea to start with
(and I say that as the person who had the idea back then and implemented
it..)

> Sorry for the naive questions (that clearly speak to me not
> understanding how this aspect of the block and SCSI code work).. but I'd
> like to understand where DM will be lacking going forward.

At least in terms of BLOCK_PC nothing will change for dm, the code
was simply dead on arrival.  Maybe I should change the subject
to say that more clearly.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/15] dm: remove incomple BLOCK_PC support

[Mike Snitzer]

On Thu, Jan 12 2017 at  3:00am -0500,
Christoph Hellwig  wrote:

> On Wed, Jan 11, 2017 at 08:09:37PM -0500, Mike Snitzer wrote:
> > I'm not following your reasoning.
> > 
> > dm_blk_ioctl calls __blkdev_driver_ioctl and will call scsi_cmd_ioctl
> > (sd_ioctl -> scsi_cmd_blk_ioctl -> scsi_cmd_ioctl) if DM's underlying
> > block device is a scsi device.
> 
> Yes, it it does.  But scsi_cmd_ioctl as called from sd_ioctl will
> operate entirely on the SCSI request_queue - dm-mpath will never see
> the BLOCK_PC request generated by it.

I lost sight of the fact that BLOCK_PC requests are sent down via the
normal request submission (and not the ioctl path).  So my previous
reply wasn't relevant.

What is "incomplete" about request-based DM's BLOCK_PC support?

This code goes back to when request-based DM multipath was first
introduced via commit cec47e3d4a -- but I've never used the BLOCK_PC
requests for SCSI pass through myself.  I don't know who is using
it.. are you aware of some upper layer filesystem or userspace
submission path for these BLOCK_PC requests that they'd be passing
through DM?

I'm also missing how you're saying the new blk-mq request-based DM will
work with your new model.  I appreciate that we get the request from the
underlying blk-mq request_queue and it'll be properly sized.  But
wouldn't we need to pass data back up for these SCSI pass-through
requests?  So wouldn't the top-level multipath request_queue need to
setup cmd_size?

Sorry for the naive questions (that clearly speak to me not
understanding how this aspect of the block and SCSI code work).. but I'd
like to understand where DM will be lacking going forward.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/15] dm: remove incomple BLOCK_PC support

[Mike Snitzer]

On Tue, Jan 10 2017 at 10:06am -0500,
Christoph Hellwig  wrote:

> DM tries to copy a few fields around for BLOCK_PC requests, but given
> that no dm-target ever wires up scsi_cmd_ioctl BLOCK_PC can't actually
> be sent to dm.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/md/dm-rq.c | 16 
>  1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index 93f6e9f..3f12916 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -270,19 +270,6 @@ static void dm_end_request(struct request *clone, int 
> error)
>   struct mapped_device *md = tio->md;
>   struct request *rq = tio->orig;
>  
> - if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
> - rq->errors = clone->errors;
> - rq->resid_len = clone->resid_len;
> -
> - if (rq->sense)
> - /*
> -  * We are using the sense buffer of the original
> -  * request.
> -  * So setting the length of the sense data is enough.
> -  */
> - rq->sense_len = clone->sense_len;
> - }
> -
>   free_rq_clone(clone);
>   rq_end_stats(md, rq);
>   if (!rq->q->mq_ops)
> @@ -511,9 +498,6 @@ static int setup_clone(struct request *clone, struct 
> request *rq,
>   if (r)
>   return r;
>  
> - clone->cmd = rq->cmd;
> - clone->cmd_len = rq->cmd_len;
> - clone->sense = rq->sense;
>   clone->end_io = end_clone_request;
>   clone->end_io_data = tio;
>  

I'm not following your reasoning.

dm_blk_ioctl calls __blkdev_driver_ioctl and will call scsi_cmd_ioctl
(sd_ioctl -> scsi_cmd_blk_ioctl -> scsi_cmd_ioctl) if DM's underlying
block device is a scsi device.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html