On Thu, 2011-12-01 at 02:14 -0500, Christoph Hellwig wrote:
> On Wed, Nov 30, 2011 at 02:26:57PM -0800, Nicholas A. Bellinger wrote:
> > If pSCSI's usage of blk_execute_rq_nowait() will enforce
> > scsi_device->can_queue for us in order to prevent backend queue
> > overflow, then it can be removed.
> >
> > All of the old scsi_execute_req() usage always needed this, so I was
> > trying to be safe here.
>
> Both enforce the can_queue limit, and as far as I know always have.
>
Then I must be confusing this with the pre scsi_execute_req() ->
Scsi_Request + scsi_do_req() usage then, because at some point years ago
this was a requirement for passthrough with more than a few PSCSI
backends.
If it's really safe for all pSCSI backends to queue as many tasks as
possible with modern code and let blk_execute_rq_nowait() handle it,
then I'm more than happy to drop the extra bits for this here all
together.
> > Adding more context.
>
> Let's remove the part just restatation the obvious facts from the code
> first..
>
> > > > + if (dev->enforce_dev_tcq) {
> > > > + if (!dev->depth_left) {
> > > > + spin_unlock_irq(&dev->execute_task_lock);
> > > > + return transport_tcq_window_closed(dev);
> > > > + }
> > > > + dev->dev_tcq_window_closed = 0;
> > > > + }
> > >
> > > Also note that the current transport_tcq_window_closed code is
> > > really dumb. There is absolutely no need for the sleep, instead
> > > the code incrementing depth_left should do a wakeup on
> > > dev->dev_queue_obj.thread_wq.
> >
> > It already does do a wakeup on thread_wq after the sleep.
>
> Which as I said is dumb. The wakeup needs to be happen from the thread
> incrementing depth_left, that is freeing the ressource - a busywait
> in the submission path does not help at all.
>
I am happy to see this go if it's really not necessary for pSCSI
anymore.
Anyways, will need to poke at this with some older devices to make sure
it work's in practice, but if your really sure w/ blk_execute_rq_nowait
is enforcing scsi_device->queue_depth, I'll go ahead and drop the legacy
usage with ->depth_left in target-core now.
Thanks Christoph,
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
