On Wed, 2011-11-30 at 17:16 -0500, Christoph Hellwig wrote:
> On Wed, Nov 30, 2011 at 09:51:44PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
> >
> > Historically, pSCSI devices have been the ones that required target-core
> > to enforce a per se_device->depth_left. This patch changes target-core
> > to no longer (by default) enforce a per se_device->depth_left or sleep in
> > transport_tcq_window_closed() when we out of queue slots.
> >
> > It adds se_device->enforce_dev_tcq bitfield + se_subsystem_dev->su_dev_flags
> > SDF_ENFORCE_DEVICE_TCQ to signal the legacy usage in target-core with pSCSI.
> > So now aside from this special case, IBLOCK, FILEIO and RAMDISK with
> > ->enforce_dev_tcq=0 by default.
>
> What does pscsi actually need it for?
So like the commit says, this has been historically required for pSCSI
in order to enforce scsi_device->can_queue. This has been used by
typically by devices reporting ->can_queue=1, or a TCQ smaller than what
the fabric is capable of sending.
> I'd really much prefer either
> getting rid of this entirely, or if that isn't possible moving it into
> pscsi, including proper comments describing why it is needed there over
> adding more flags to subtly different behaviour.
>
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.
Moving it into pSCSI is an idea, but to make things work it would end up
duplicating alot of core code.
> > + /*
> > + * Only setup and enforce the se_device queue depth for
> > + * pSCSI backends using SDF_ENFORCE_DEVICE_TCQ.
> > + */
> > + if (se_dev->su_dev_flags & SDF_ENFORCE_DEVICE_TCQ) {
>
> The comment doesn't add much value..
>
Adding more context.
> > + 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.
--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
