[PATCH 0/4] tcm: Unify virtual subsystem plugin emulation code

[PATCH 0/4] tcm: Unify virtual subsystem plugin emulation code

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Greetings all,

This series contains patches to unify TCM virtual subsystem plugin emulation
logic into generic TCM code, and to provide a method of emulation for control
CDBs that are emulated generically across subsystem plugin code or that may
require CDB specific context in the new struct se_subsystem_api_cdb set of
subsystem dependent CDB callers.

The first patch adds a generic function transport_emulate_control_cdb()
which is intended used by virtual subsystem plugin code.  This includes
the primary list of CDBs handled in this manner from existing IBLOCK,
FILEIO and RAMDISK *_emulate_scsi_cdb() subsystem dependent code, and the
struct se_subsystem_api_cdb define described above.

Thse second patch converts IBLOCK, FILEIO and RAMDISK subsystem code to use
the new ->emulate_inquiry(), ->emulate_read_cap(), ->emulate_read_cap16() and
->emulate_unmap() callers their respective existing subsystem dependent logic.

The third and forth patches move WRITE_SAME_* and SYNCHRONIZE_CACHE_* CDB
emulation logic into transport_emulate_control_cdb() for these two originally
specially handled cases with IBLOCK and FILEIO subsystem code.

So far this code has been tested on TCM_Loop SCSI LUN with IBLOCK, FILEIO and
IBLOCK backstores with TPU=1, TPWS=1 and WCE=1 on v2.6.36-rc6.

Many thanks go out to Christoph for a bit of friendly prodding to finally make
this conversion happen.  Thanks hch!

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>

Nicholas Bellinger (4):
  tcm: Unify subsystem plugin control CDB emulation
  tcm: Convert IBLOCK, FILEIO and RAMDISK subsystem code to
    transport_emulate_control_cdb()
  tcm: Move WRITE_SAME_* emulation into transport_emulate_control_cdb()
  tcm: Move SYNCHRONIZE_CACHE_* emulation into
    transport_emulate_control_cdb()

 drivers/target/target_core_file.c      |  163 ++++++++------------------
 drivers/target/target_core_iblock.c    |  102 +++-------------
 drivers/target/target_core_rd.c        |   91 ++-------------
 drivers/target/target_core_transport.c |  200 ++++++++++++++++++++++----------
 include/target/target_core_base.h      |    4 +-
 include/target/target_core_transport.h |   26 +++-
 6 files changed, 237 insertions(+), 349 deletions(-)

Re: [PATCH 0/4] tcm: Unify virtual subsystem plugin emulation code

[Christoph Hellwig]

What you did in this patch is a good step toward getting rid of the
scsi logic in the backend, but it's quite enough yet.  For one thing
the backends really shouldn't know anything about scsi commands, so
the call to transport_emulate_control_cdb should happen before even
calling into the backend.  Second your ->emulate_foo callbacks still
are far too SCSI-specific, e.g. WRITE SAME witht unmap bit and UNMAP
emulation really should just go into a single ->discard callback
for the backend.  And instead of calling into the backend again for
readcap emulation just set block_size and size attributs on a per-
device object and let common code handle it.  Similarly for inquity
just set the device type and other variables and handle it in common
code.  That avoid the special se_subsystem_api_cdb vector and simplifies
the code a lot conceptually a lot.  Together with calling
transport_emulate_control_cdb directly from core code before going
into ->do_task that does all the required work to make the backends
independent of the scsi protocol, so we could also use it e.g.
for ATA or virtio targets.

Re: [PATCH 0/4] tcm: Unify virtual subsystem plugin emulation code

[Nicholas A. Bellinger]

On Sun, 2010-10-10 at 09:32 +0200, Christoph Hellwig wrote:
> What you did in this patch is a good step toward getting rid of the
> scsi logic in the backend, but it's quite enough yet.  For one thing
> the backends really shouldn't know anything about scsi commands, so
> the call to transport_emulate_control_cdb should happen before even
> calling into the backend.

Hmmm, good point.  I was trying to avoid doing this initially because it
adds more complexity when calling ->do_task() in target_core_transport.c
__transport_execute_tasks(), but I would agree the tradeoff here to make
IBLOCK, FILEIO and RAMDISK subsystem backend code SCSI control CDB
emulation independent is worth it.

> Second your ->emulate_foo callbacks still
> are far too SCSI-specific, e.g. WRITE SAME witht unmap bit and UNMAP
> emulation really should just go into a single ->discard callback
> for the backend.  And instead of calling into the backend again for
> readcap emulation just set block_size and size attributs on a per-
> device object and let common code handle it.  Similarly for inquity
> just set the device type and other variables and handle it in common
> code.

Ok, I think this makes sense and should be easy enough for the cases
that have been listed here.  However, there are still cases (namely DIF)
that containing SCSI specific bits and would still be going into IBLOCK
and FILEIO.

> That avoid the special se_subsystem_api_cdb vector and simplifies
> the code a lot conceptually a lot.  Together with calling
> transport_emulate_control_cdb directly from core code before going
> into ->do_task that does all the required work to make the backends
> independent of the scsi protocol, so we could also use it e.g.
> for ATA or virtio targets.

Makes sense, I will drop struct se_subsystem_api_cdb, et al and post a
followup patch series this afternoon.

Thanks!

--nab

Re: [PATCH 0/4] tcm: Unify virtual subsystem plugin emulation code

[Vladislav Bolkhovitin]

Christoph Hellwig, on 10/10/2010 11:32 AM wrote:
> What you did in this patch is a good step toward getting rid of the
> scsi logic in the backend, but it's quite enough yet.  For one thing
> the backends really shouldn't know anything about scsi commands, so
> the call to transport_emulate_control_cdb should happen before even
> calling into the backend.  Second your ->emulate_foo callbacks still
> are far too SCSI-specific, e.g. WRITE SAME witht unmap bit and UNMAP
> emulation really should just go into a single ->discard callback
> for the backend.  And instead of calling into the backend again for
> readcap emulation just set block_size and size attributs on a per-
> device object and let common code handle it.  Similarly for inquity
> just set the device type and other variables and handle it in common
> code.  That avoid the special se_subsystem_api_cdb vector and simplifies
> the code a lot conceptually a lot. Together with calling
> transport_emulate_control_cdb directly from core code before going
> into ->do_task that does all the required work to make the backends
> independent of the scsi protocol, so we could also use it e.g.
> for ATA or virtio targets.

I strongly disagree with this approach to allow non-SCSI transports to
use facilities provided by a generic (SCSI) target subsystem. This
approach is bad, because you enforce the SCSI target subsystem to
implement in its interface "the least common denominator" of all
services non-SCSI transports can requests, which leads to:

1. The SCSI target subsystem code overcomplication, so making it much
harder to audit and more buggy.

2. You creates hidden inter-transport dependencies inside of the target
subsystem, so by fixing services for one transport you can break another.

3. By introducing additional processing to hide SCSI internals you hurt
performance, especially for the mainline SCSI case.

Moreover, my experience tells me that fully hiding SCSI internals of the
target subsystem is hardly feasible at all.

There is another, much better approach to allow non-SCSI transports to
use facilities provided by the SCSI target subsystem: leave the generic
target subsystem fully SCSI-centric and create for each non-SCSI
transport a helper library which would allow to translate its requests
to the corresponding SCSI requests and SCSI sense to their error codes.

All non-SCSI transports I know, including ATA, are, basically, subsets
of SCSI facilities, so translation of their requests to the
corresponding SCSI commands would be a straightforward task. For
instance, discard would be translated to SCSI UNMAP.

This approach would bring:

1. Clear, easy auditable target subsystem's code.

2. Clear separation of SCSI and non-SCSI processing among different SCSI
and non-SCSI target drivers.

3. The best performance in all cases.

Vlad