Re: [dm-devel] [PATCH 05/15] dm: remove incomple BLOCK_PC support
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 05/15] dm: remove incomple BLOCK_PC support
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 05/15] dm: remove incomple BLOCK_PC support
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 05/15] dm: remove incomple BLOCK_PC support
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel