[0/4] target: make tpg/enable attribute

Message

Dmitry Bogdanov March 17, 2021, 10:46 a.m. UTC

All target transport drivers have its own 'tpg/enable' attribute
implementation. This produces unnecessary code duplication.
Also it makes difficult to control that attribute and to depend on that
attribute inside of target core module.

Replace tpg/enable attribute implementation of each transport to a
common implementation for all transports. Move enabling target logic to
a new fabric_ops callback.

This patchset is intended for scsi-queue.

Dmitry Bogdanov (4):
  target: core: add common tpg/enable attribute
  target: iscsi: replace enable attr to ops.enable
  target: qla2xx: replace enable attr to ops.enable
  target: sbp: replace enable attr to ops.enable

 drivers/scsi/qla2xxx/tcm_qla2xxx.c           | 73 +++-------------
 drivers/target/iscsi/iscsi_target_configfs.c | 91 +++++++-------------
 drivers/target/sbp/sbp_target.c              | 30 ++-----
 drivers/target/target_core_configfs.c        |  1 +
 drivers/target/target_core_fabric_configfs.c | 38 +++++++-
 drivers/target/target_core_internal.h        |  1 +
 drivers/target/target_core_tpg.c             | 42 +++++++++
 include/target/target_core_base.h            |  1 +
 include/target/target_core_fabric.h          |  1 +
 9 files changed, 129 insertions(+), 147 deletions(-)

Comments

Mike Christie March 17, 2021, 4:32 p.m. UTC | #1

On 3/17/21 5:46 AM, Dmitry Bogdanov wrote:
> All target transport drivers have its own 'tpg/enable' attribute
> implementation. This produces unnecessary code duplication
I don't think that is correct. Some drivers don't have en enable attr:

- vhost-scsi, xen and loop have a nexus attr that sort of leaves it in
the equivalent of enabled.

- usb has a nexus file like above, but after you write to it you still 
have to write to the enable attr.

- tcm_fc does not have an enable and has it's own initialization strategy.

For drivers that have an enable file your patches missed usb, srpt and
ibm_vscsi.

> Also it makes difficult to control that attribute and to depend on that
> attribute inside of target core module.

I agree with this :) It's a bit of mess, but I think it's sometimes due to
how the driver is implemented and how userspace has to set it up, so it's
not as simple as in this patchset due to having to support existing tools.

Dmitry Bogdanov March 18, 2021, 8:58 a.m. UTC | #2

Hi Mike,

Thank you for quick response.

I got my mistakes.
Will try to come up with a new solution.

BR,
 Dmitry

-----Original Message-----
From: Mike Christie <[e-mail]> 
Sent: Wednesday, March 17, 2021 7:32 PM
To: Dmitriy Bogdanov <[e-mail]>; Martin Petersen <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]; Nilesh Javali <[e-mail]>; Chris Boot <[e-mail]>
Subject: Re: [PATCH 0/4] target: make tpg/enable attribute

On 3/17/21 5:46 AM, Dmitry Bogdanov wrote:
> All target transport drivers have its own 'tpg/enable' attribute 
> implementation. This produces unnecessary code duplication
I don't think that is correct. Some drivers don't have en enable attr:

- vhost-scsi, xen and loop have a nexus attr that sort of leaves it in the equivalent of enabled.

- usb has a nexus file like above, but after you write to it you still have to write to the enable attr.

- tcm_fc does not have an enable and has it's own initialization strategy.

For drivers that have an enable file your patches missed usb, srpt and ibm_vscsi.

> Also it makes difficult to control that attribute and to depend on 
> that attribute inside of target core module.

I agree with this :) It's a bit of mess, but I think it's sometimes due to how the driver is implemented and how userspace has to set it up, so it's not as simple as in this patchset due to having to support existing tools.