| Message ID | 1484060780-15592-15-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 10, 2017 at 04:06:19PM +0100, Christoph Hellwig wrote: > Simply the boilerplate code needed for bsg nodes a bit. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- that reminds me of posting my SAS bsg-lib patch... Anyways looks good, Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
On Wed, Jan 11, 2017 at 09:42:44AM +0100, Johannes Thumshirn wrote: > On Tue, Jan 10, 2017 at 04:06:19PM +0100, Christoph Hellwig wrote: > > Simply the boilerplate code needed for bsg nodes a bit. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > that reminds me of posting my SAS bsg-lib patch... Yes. Having SAS use bsg-lib, and bsg-lib switched away from abusing struct request_queue would make this series a lot cleaner. So maybe we should get that into the scsi tree for 4.10 together with the prep patches in this series as a priority and defer the actual struct request changes once again. That should also give us some more time to sort out the dm-mpath story.. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 11, 2017 at 09:45:12AM +0100, Christoph Hellwig wrote: > On Wed, Jan 11, 2017 at 09:42:44AM +0100, Johannes Thumshirn wrote: > > On Tue, Jan 10, 2017 at 04:06:19PM +0100, Christoph Hellwig wrote: > > > Simply the boilerplate code needed for bsg nodes a bit. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > --- > > > > that reminds me of posting my SAS bsg-lib patch... > > Yes. Having SAS use bsg-lib, and bsg-lib switched away from abusing > struct request_queue would make this series a lot cleaner. > > So maybe we should get that into the scsi tree for 4.10 together > with the prep patches in this series as a priority and defer the actual > struct request changes once again. That should also give us some more > time to sort out the dm-mpath story.. I'll dig it up and RFC post it. It's currently untested though as I currently don't have a SMP capable SAS HBA here. Do you have an mptXsas available? Byte, Johannes
On 01/11/2017 09:45 AM, Christoph Hellwig wrote: > On Wed, Jan 11, 2017 at 09:42:44AM +0100, Johannes Thumshirn wrote: >> On Tue, Jan 10, 2017 at 04:06:19PM +0100, Christoph Hellwig wrote: >>> Simply the boilerplate code needed for bsg nodes a bit. >>> >>> Signed-off-by: Christoph Hellwig <hch@lst.de> >>> --- >> >> that reminds me of posting my SAS bsg-lib patch... > > Yes. Having SAS use bsg-lib, and bsg-lib switched away from abusing > struct request_queue would make this series a lot cleaner. > > So maybe we should get that into the scsi tree for 4.10 together > with the prep patches in this series as a priority and defer the actual > struct request changes once again. That should also give us some more > time to sort out the dm-mpath story.. I'd advocate to discuss this at LSF. Now that Mike moved the bio-based mpath stuff back in things got even more complex. I'll be posting a patchset for reimplementing multipath as a stand-alone driver shortly; that'll give us a good starting point on how we want multipath to evolve. Who knows; we might even manage to move multipath out of device-mapper altogether. That would make Mike very happy, and I wouldn't mind, either :-) Cheers, Hannes
On Wed, Jan 11, 2017 at 09:56:01AM +0100, Johannes Thumshirn wrote: > I'll dig it up and RFC post it. It's currently untested though as I > currently don't have a SMP capable SAS HBA here. > > Do you have an mptXsas available? Unfortunately not. But I think Hannes has, he has recently played around with the interrupt handling for the driver. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 11, 2017 at 09:59:17AM +0100, Hannes Reinecke wrote: > I'd advocate to discuss this at LSF. > Now that Mike moved the bio-based mpath stuff back in things got even > more complex. Yeah. If we'd _only_ have bio based support it would simplify things a lot, but as a third parallel path it's not exactly making things easier. > I'll be posting a patchset for reimplementing multipath as a stand-alone > driver shortly; that'll give us a good starting point on how we want > multipath to evolve. > > Who knows; we might even manage to move multipath out of device-mapper > altogether. > That would make Mike very happy, and I wouldn't mind, either :-) Heh. I'm curious how you want to do that while keeping existing setups working, though. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/11/2017 10:01 AM, Christoph Hellwig wrote: > On Wed, Jan 11, 2017 at 09:59:17AM +0100, Hannes Reinecke wrote: >> I'd advocate to discuss this at LSF. >> Now that Mike moved the bio-based mpath stuff back in things got even >> more complex. > > Yeah. If we'd _only_ have bio based support it would simplify things > a lot, but as a third parallel path it's not exactly making things easier. > >> I'll be posting a patchset for reimplementing multipath as a stand-alone >> driver shortly; that'll give us a good starting point on how we want >> multipath to evolve. >> >> Who knows; we might even manage to move multipath out of device-mapper >> altogether. >> That would make Mike very happy, and I wouldn't mind, either :-) > > Heh. I'm curious how you want to do that while keeping existing setups > working, though. which will become challenging, indeed. ATM it's just a testbed on how things could work; we've got most of the required infrastucture in the kernel nowadays, so that we can just drop most of the complexity from the present multipath-tools mess. In the end it might even boil down to update the existing device-mapper multipath implementation. We'll see. Cheers, Hannes
On Wed, Jan 11 2017 at 3:45am -0500, Christoph Hellwig <hch@lst.de> wrote: > On Wed, Jan 11, 2017 at 09:42:44AM +0100, Johannes Thumshirn wrote: > > On Tue, Jan 10, 2017 at 04:06:19PM +0100, Christoph Hellwig wrote: > > > Simply the boilerplate code needed for bsg nodes a bit. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > --- > > > > that reminds me of posting my SAS bsg-lib patch... > > Yes. Having SAS use bsg-lib, and bsg-lib switched away from abusing > struct request_queue would make this series a lot cleaner. > > So maybe we should get that into the scsi tree for 4.10 together > with the prep patches in this series as a priority and defer the actual > struct request changes once again. That should also give us some more > time to sort out the dm-mpath story.. I'm not aware of the story you're referring to. I'm missing the actual challenge you're facing. But I've seen you reference the need to stop multipath from allocating its own requests. Are you referring to old request_fn request-based multipath's clone_old_rq:alloc_old_clone_request? Or how blk-mq request-based multipath gets a request from the blk-mq tag space (via blk_mq_alloc_request)? Or both? How is that holding you back? -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 11 2017 at 4:37am -0500, Hannes Reinecke <hare@suse.de> wrote: > On 01/11/2017 10:01 AM, Christoph Hellwig wrote: > > On Wed, Jan 11, 2017 at 09:59:17AM +0100, Hannes Reinecke wrote: > >> I'd advocate to discuss this at LSF. > >> Now that Mike moved the bio-based mpath stuff back in things got even > >> more complex. > > > > Yeah. If we'd _only_ have bio based support it would simplify things > > a lot, but as a third parallel path it's not exactly making things easier. > > > >> I'll be posting a patchset for reimplementing multipath as a stand-alone > >> driver shortly; that'll give us a good starting point on how we want > >> multipath to evolve. > >> > >> Who knows; we might even manage to move multipath out of device-mapper > >> altogether. > >> That would make Mike very happy, and I wouldn't mind, either :-) > > > > Heh. I'm curious how you want to do that while keeping existing setups > > working, though. > > which will become challenging, indeed. > > ATM it's just a testbed on how things could work; we've got most of the > required infrastucture in the kernel nowadays, so that we can just drop > most of the complexity from the present multipath-tools mess. > > In the end it might even boil down to update the existing device-mapper > multipath implementation. We'll see. Honestly, at this point I'm pretty confident you guys will never actually produce a new NIH multipath. You'll just threaten to for _years_. Joking aside, I think DM multipath would benefit from a more focused separation from all the SCSI cruft that wormed its way in. Factoring that stuff out so that multipath can be more easily adapted/optimized for use with leaner transports like NVMe-over-fabrics. But NVMe-over-fabrics setups are still pretty rare (only a handful of vendors are stepping up with requirements to support it) so details on what change is needed to DM multipath is largely unknown to me. But may become clearer in the coming month. Mike -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 11, 2017 at 05:01:22PM -0500, Mike Snitzer wrote: > But I've seen you reference the need to stop multipath from allocating > its own requests. Are you referring to old request_fn request-based > multipath's clone_old_rq:alloc_old_clone_request? Yes, that one is the issue. It allocates a struct request "blind", that is without known what queue it goes to. With this queue (or blk-mq for that matter) we need to know the queue, because the request structures might have additional data behind it and require additional initialization for drivers that require per-request data. We make use of the per-request data for SCSI passthrough in this patch. > Or how blk-mq request-based multipath gets a request from the blk-mq tag > space (via blk_mq_alloc_request)? That's fine because it works on the queue of the device that I/O is submitted to. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/block/bsg-lib.c b/block/bsg-lib.c index 9d652a9..c74acf4 100644 --- a/block/bsg-lib.c +++ b/block/bsg-lib.c @@ -177,7 +177,7 @@ static int bsg_create_job(struct device *dev, struct request *req) * * Drivers/subsys should pass this to the queue init function. */ -void bsg_request_fn(struct request_queue *q) +static void bsg_request_fn(struct request_queue *q) __releases(q->queue_lock) __acquires(q->queue_lock) { @@ -214,24 +214,24 @@ void bsg_request_fn(struct request_queue *q) put_device(dev); spin_lock_irq(q->queue_lock); } -EXPORT_SYMBOL_GPL(bsg_request_fn); /** * bsg_setup_queue - Create and add the bsg hooks so we can receive requests * @dev: device to attach bsg device to - * @q: request queue setup by caller * @name: device to give bsg device * @job_fn: bsg job handler * @dd_job_size: size of LLD data needed for each job - * - * The caller should have setup the reuqest queue with bsg_request_fn - * as the request_fn. */ -int bsg_setup_queue(struct device *dev, struct request_queue *q, - char *name, bsg_job_fn *job_fn, int dd_job_size) +struct request_queue *bsg_setup_queue(struct device *dev, char *name, + bsg_job_fn *job_fn, int dd_job_size) { + struct request_queue *q; int ret; + q = blk_init_queue(bsg_request_fn, NULL); + if (!q) + return ERR_PTR(-ENOMEM); + q->queuedata = dev; q->bsg_job_size = dd_job_size; q->bsg_job_fn = job_fn; @@ -243,9 +243,10 @@ int bsg_setup_queue(struct device *dev, struct request_queue *q, if (ret) { printk(KERN_ERR "%s: bsg interface failed to " "initialize - register queue\n", dev->kobj.name); - return ret; + blk_cleanup_queue(q); + return ERR_PTR(ret); } - return 0; + return q; } EXPORT_SYMBOL_GPL(bsg_setup_queue); diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c index afcedec..13dcb9b 100644 --- a/drivers/scsi/scsi_transport_fc.c +++ b/drivers/scsi/scsi_transport_fc.c @@ -3765,7 +3765,6 @@ fc_bsg_hostadd(struct Scsi_Host *shost, struct fc_host_attrs *fc_host) struct device *dev = &shost->shost_gendev; struct fc_internal *i = to_fc_internal(shost->transportt); struct request_queue *q; - int err; char bsg_name[20]; fc_host->rqst_q = NULL; @@ -3776,24 +3775,14 @@ fc_bsg_hostadd(struct Scsi_Host *shost, struct fc_host_attrs *fc_host) snprintf(bsg_name, sizeof(bsg_name), "fc_host%d", shost->host_no); - q = blk_init_queue(bsg_request_fn, NULL); - if (!q) { - dev_err(dev, - "fc_host%d: bsg interface failed to initialize - no request queue\n", - shost->host_no); - return -ENOMEM; - } - - __scsi_init_queue(shost, q); - err = bsg_setup_queue(dev, q, bsg_name, fc_bsg_dispatch, - i->f->dd_bsg_size); - if (err) { + q = bsg_setup_queue(dev, bsg_name, fc_bsg_dispatch, i->f->dd_bsg_size); + if (IS_ERR(q)) { dev_err(dev, "fc_host%d: bsg interface failed to initialize - setup queue\n", shost->host_no); - blk_cleanup_queue(q); - return err; + return PTR_ERR(q); } + __scsi_init_queue(shost, q); blk_queue_rq_timed_out(q, fc_bsg_job_timeout); blk_queue_rq_timeout(q, FC_DEFAULT_BSG_TIMEOUT); fc_host->rqst_q = q; @@ -3825,27 +3814,18 @@ fc_bsg_rportadd(struct Scsi_Host *shost, struct fc_rport *rport) struct device *dev = &rport->dev; struct fc_internal *i = to_fc_internal(shost->transportt); struct request_queue *q; - int err; rport->rqst_q = NULL; if (!i->f->bsg_request) return -ENOTSUPP; - q = blk_init_queue(bsg_request_fn, NULL); - if (!q) { - dev_err(dev, "bsg interface failed to initialize - no request queue\n"); - return -ENOMEM; - } - - __scsi_init_queue(shost, q); - err = bsg_setup_queue(dev, q, NULL, fc_bsg_dispatch, i->f->dd_bsg_size); - if (err) { + q = bsg_setup_queue(dev, NULL, fc_bsg_dispatch, i->f->dd_bsg_size); + if (IS_ERR(q)) { dev_err(dev, "failed to setup bsg queue\n"); - blk_cleanup_queue(q); - return err; + return PTR_ERR(q); } - + __scsi_init_queue(shost, q); blk_queue_prep_rq(q, fc_bsg_rport_prep); blk_queue_rq_timed_out(q, fc_bsg_job_timeout); blk_queue_rq_timeout(q, BLK_DEFAULT_SG_TIMEOUT); diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c index 04ebe6e..568c9f2 100644 --- a/drivers/scsi/scsi_transport_iscsi.c +++ b/drivers/scsi/scsi_transport_iscsi.c @@ -1537,25 +1537,18 @@ iscsi_bsg_host_add(struct Scsi_Host *shost, struct iscsi_cls_host *ihost) struct iscsi_internal *i = to_iscsi_internal(shost->transportt); struct request_queue *q; char bsg_name[20]; - int ret; if (!i->iscsi_transport->bsg_request) return -ENOTSUPP; snprintf(bsg_name, sizeof(bsg_name), "iscsi_host%d", shost->host_no); - - q = blk_init_queue(bsg_request_fn, NULL); - if (!q) - return -ENOMEM; - - __scsi_init_queue(shost, q); - ret = bsg_setup_queue(dev, q, bsg_name, iscsi_bsg_host_dispatch, 0); - if (ret) { + q = bsg_setup_queue(dev, bsg_name, iscsi_bsg_host_dispatch, 0); + if (IS_ERR(q)) { shost_printk(KERN_ERR, shost, "bsg interface failed to " "initialize - no request queue\n"); - blk_cleanup_queue(q); - return ret; + return PTR_ERR(q); } + __scsi_init_queue(shost, q); ihost->bsg_q = q; return 0; diff --git a/include/linux/bsg-lib.h b/include/linux/bsg-lib.h index 657a718..e34dde2 100644 --- a/include/linux/bsg-lib.h +++ b/include/linux/bsg-lib.h @@ -66,9 +66,8 @@ struct bsg_job { void bsg_job_done(struct bsg_job *job, int result, unsigned int reply_payload_rcv_len); -int bsg_setup_queue(struct device *dev, struct request_queue *q, char *name, - bsg_job_fn *job_fn, int dd_job_size); -void bsg_request_fn(struct request_queue *q); +struct request_queue *bsg_setup_queue(struct device *dev, char *name, + bsg_job_fn *job_fn, int dd_job_size); void bsg_job_put(struct bsg_job *job); int __must_check bsg_job_get(struct bsg_job *job);
Simply the boilerplate code needed for bsg nodes a bit. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/bsg-lib.c | 21 +++++++++++---------- drivers/scsi/scsi_transport_fc.c | 36 ++++++++---------------------------- drivers/scsi/scsi_transport_iscsi.c | 15 ++++----------- include/linux/bsg-lib.h | 5 ++--- 4 files changed, 25 insertions(+), 52 deletions(-)