Re: [RFC 04/22] tcm: Add v4 base data structures and struct target_core_fabric_ops

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2010-09-02 at 01:56 -0400, Konrad Rzeszutek Wilk wrote:
> On Monday 30 August 2010 05:20:40 Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
> 
> Hey Nicholas,
> 
> I did a cursory review .
> 
> >
> > This patch adds TCM Core v4 base data structures definitions + macros
> > and fabric module function pointer API in struct target_core_fabric_ops.
> >
> > Signed-off-by: Nicholas A. Bellinger <nab@xxxxxxxxxxxxxxx>
> > ---
> >  include/target/target_core_base.h       |  985
> > +++++++++++++++++++++++++++++++ include/target/target_core_fabric_ops.h |  
> > 89 +++
> >  2 files changed, 1074 insertions(+), 0 deletions(-)
> >  create mode 100644 include/target/target_core_base.h
> >  create mode 100644 include/target/target_core_fabric_ops.h
> >
> > diff --git a/include/target/target_core_base.h
> > b/include/target/target_core_base.h new file mode 100644
> > index 0000000..f4d0c70
> > --- /dev/null
> > +++ b/include/target/target_core_base.h
> > @@ -0,0 +1,985 @@
> > +#ifndef TARGET_CORE_BASE_H
> > +#define TARGET_CORE_BASE_H
> > +
> > +#include <linux/in.h>
> > +#include <linux/configfs.h>
> > +#include <net/sock.h>
> > +#include <net/tcp.h>
> > +#include "target_core_mib.h"
> > +
> > +#define TARGET_CORE_MOD_VERSION		"v4.0.0-rc3"
> > +#define SHUTDOWN_SIGS	(sigmask(SIGKILL)|sigmask(SIGINT)|sigmask(SIGABRT))
> > +
> > +/* SCSI Command Descriptor Block Size a la SCSI's MAX_COMMAND_SIZE */
> > +#define SCSI_CDB_SIZE			16
> > +#define TRANSPORT_IOV_DATA_BUFFER	5
> > +
> > +/* Maximum Number of LUNs per Target Portal Group */
> > +#define TRANSPORT_MAX_LUNS_PER_TPG	    256
> > +
> > +/* From include/scsi/scsi_cmnd.h:SCSI_SENSE_BUFFERSIZE */
> > +#define TRANSPORT_SENSE_BUFFER              SCSI_SENSE_BUFFERSIZE
> > +
> > +#define SPC_SENSE_KEY_OFFSET			2
> > +#define SPC_ASC_KEY_OFFSET			12
> > +#define SPC_ASCQ_KEY_OFFSET			13
> > +
> 
> .. from here on
> > +/* Currently same as ISCSI_IQN_LEN */
> > +#define TRANSPORT_IQN_LEN			224
> > +#define LU_GROUP_NAME_BUF			256
> > +#define TG_PT_GROUP_NAME_BUF			256
> > +/* Used to parse VPD into struct t10_vpd */
> > +#define VPD_TMP_BUF_SIZE			128
> > +/* Used for target_core-pscsi.c:pscsi_transport_complete() */
> > +#define VPD_BUF_LEN				256
> > +/* Used for struct se_subsystem_dev-->se_dev_alias, must be less than
> > +   PAGE_SIZE */
> > +#define SE_DEV_ALIAS_LEN			512
> > +/* Used for struct se_subsystem_dev->se_dev_udev_path[], must be less than
> > +   PAGE_SIZE */
> > +#define SE_UDEV_PATH_LEN			512
> > +/* Used for struct se_dev_snap_attrib->contact */
> > +#define SNAP_CONTACT_LEN			128
> > +/* Used for struct se_dev_snap_attrib->lv_group */
> > +#define SNAP_GROUP_LEN				128
> > +/* Used for struct se_dev_snap_attrib->lvc_size */
> > +#define SNAP_LVC_LEN				32
> > +/* Used by struct t10_reservation_template->pr_[i,t]_port[] */
> > +#define PR_APTPL_MAX_IPORT_LEN			256
> > +#define PR_APTPL_MAX_TPORT_LEN			256
> > +/* Used by struct t10_reservation_template->pr_aptpl_buf_len */
> > +#define PR_APTPL_BUF_LEN			8192
> > +/* Used by struct t10_alua_tg_pt_gp->tg_pt_gp_md_buf_len */
> > +#define ALUA_MD_BUF_LEN				1024
> > +/* Used by struct t10_pr_registration->pr_reg_isid */
> > +#define PR_REG_ISID_LEN				16
> > +/* PR_REG_ISID_LEN + ',i,0x' */
> > +#define PR_REG_ISID_ID_LEN			(PR_REG_ISID_LEN + 5)
> 
> to here I am unsure which one of these are defined by a spec (like the IQN 
> length) and which ones are Linux nomenclature. Perhaps it might be prudent to 
> split them in two camps.

Hmm, TRANSPORT_IQN_LEN is indeed a special case for the Initiator Port
WWPN in struct se_node_acl->initiatorname[].  We could always make this
fabric module size dependent and do a special allocation for this, but I
am not the benefit is really worth it.

>  And how did you come up with some of the length 
> values that don't correspond to a spec?

Well in the case of the PR APTPL defs, I tried to estimate what I
thought would be required for future fabrics.  For the others like the
LU_GROUP_NAME_BUF and TG_PT_GROUP_NAME_BUF related to ALUA, I thought
these where reasonable limits.

> > +
> > +/* used by PSCSI and iBlock Transport drivers */
> > +#define READ_BLOCK_LEN          		6
> > +#define READ_CAP_LEN            		8
> > +#define READ_POSITION_LEN       		20
> > +#define INQUIRY_LEN				36
> > +#define INQUIRY_VPD_SERIAL_LEN			254
> > +#define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN	254
> > +
> > +/* struct se_cmd->data_direction */
> > +#define SE_DIRECTION_NONE			0
> > +#define SE_DIRECTION_READ			1
> > +#define SE_DIRECTION_WRITE			2
> > +#define SE_DIRECTION_BIDI			3
> 
> Ugh, how about an enum instead?

Sure, fair enough.

> 
> > +
> > +/* struct se_hba->hba_flags */
> > +#define HBA_FLAGS_INTERNAL_USE			0x00000001
> > +#define HBA_FLAGS_PSCSI_MODE			0x00000002
> > +
> > +/* struct se_hba->hba_status and iscsi_tpg_hba->thba_status */
> > +#define HBA_STATUS_FREE				0x00000001
> > +#define HBA_STATUS_ACTIVE			0x00000002
> > +#define HBA_STATUS_INACTIVE			0x00000004
> > +#define HBA_STATUS_SHUTDOWN			0x00000008
> > +
> 
> > +/* struct se_lun->lun_status */
> > +#define TRANSPORT_LUN_STATUS_FREE		0
> > +#define TRANSPORT_LUN_STATUS_ACTIVE		1
> > +
> > +/* struct se_lun->lun_type */
> > +#define TRANSPORT_LUN_TYPE_NONE			0
> > +#define TRANSPORT_LUN_TYPE_DEVICE		1
> > +
> > +/* struct se_portal_group->se_tpg_type */
> > +#define TRANSPORT_TPG_TYPE_NORMAL		0
> > +#define TRANSPORT_TPG_TYPE_DISCOVERY		1
> 
> These sound quite generic. Perhaps they should be moved to a more generic 
> header file?

Ok, these can go into include/target/target_core_tpg.h..

> > +
> > +/* Used for se_node_acl->nodeacl_flags */
> 
> That "nodeacl" prefix for "flags" looks unnecessary. Why not
> just have it defined as "se_node_acl->flags"

I like having the prefix so that the code can be easily grepped.

> > +#define NAF_DYNAMIC_NODE_ACL                    0x01
> 
> > +
> > +/* struct se_map_sg->map_flags */
> Ditto

Same for this..

> > +#define MAP_SG_KMAP				0x01
> 
> > +
> > +/* Used for generate timer flags */
> > +#define TF_RUNNING				0x01
> > +#define TF_STOP					0x02
> > +
> > +/* Special transport agnostic struct se_cmd->t_states */
> > +#define TRANSPORT_NO_STATE			240
> > +#define TRANSPORT_NEW_CMD			241
> > +#define TRANSPORT_DEFERRED_CMD			242
> > +#define TRANSPORT_WRITE_PENDING			243
> > +#define TRANSPORT_PROCESS_WRITE			244
> > +#define TRANSPORT_PROCESSING			245
> > +#define TRANSPORT_COMPLETE_OK			246
> > +#define TRANSPORT_COMPLETE_FAILURE		247
> > +#define TRANSPORT_COMPLETE_TIMEOUT		248
> > +#define TRANSPORT_PROCESS_TMR			249
> > +#define TRANSPORT_TMR_COMPLETE			250
> > +#define TRANSPORT_ISTATE_PROCESSING 		251
> > +#define TRANSPORT_ISTATE_PROCESSED  		252
> > +#define TRANSPORT_KILL				253
> > +#define TRANSPORT_REMOVE			254
> > +#define TRANSPORT_FREE				255
> 
> These really look generic to me. Perhaps you can move them to a separate 
> header file?

These are used by TCM Core and fabric modules to signal state, as these
are used in core code by target_core_transport.h code, moving these to
target_core_transport.h is fine by me..

> > +
> > +#define SCF_SUPPORTED_SAM_OPCODE                0x00000001
> > +#define SCF_TRANSPORT_TASK_SENSE                0x00000002
> > +#define SCF_EMULATED_TASK_SENSE                 0x00000004
> > +#define SCF_SCSI_DATA_SG_IO_CDB                 0x00000008
> > +#define SCF_SCSI_CONTROL_SG_IO_CDB              0x00000010
> > +#define SCF_SCSI_CONTROL_NONSG_IO_CDB           0x00000020
> > +#define SCF_SCSI_NON_DATA_CDB                   0x00000040
> > +#define SCF_SCSI_CDB_EXCEPTION                  0x00000080
> > +#define SCF_SCSI_RESERVATION_CONFLICT           0x00000100
> > +#define SCF_CMD_PASSTHROUGH                     0x00000200
> > +#define SCF_CMD_PASSTHROUGH_NOALLOC             0x00000400
> > +#define SCF_SE_CMD_FAILED                       0x00000800
> > +#define SCF_SE_LUN_CMD                          0x00001000
> > +#define SCF_SE_ALLOW_EOO                        0x00002000
> > +#define SCF_SE_DISABLE_ONLINE_CHECK             0x00004000
> > +#define SCF_SENT_CHECK_CONDITION		0x00008000
> > +#define SCF_OVERFLOW_BIT                        0x00010000
> > +#define SCF_UNDERFLOW_BIT                       0x00020000
> > +#define SCF_SENT_DELAYED_TAS			0x00040000
> > +#define SCF_ALUA_NON_OPTIMIZED			0x00080000
> > +#define SCF_DELAYED_CMD_FROM_SAM_ATTR		0x00100000
> > +#define SCF_PASSTHROUGH_SG_TO_MEM		0x00200000
> > +#define SCF_PASSTHROUGH_CONTIG_TO_SG		0x00400000
> > +#define SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC	0x00800000
> > +#define SCF_EMULATE_SYNC_CACHE			0x01000000
> > +#define SCF_EMULATE_CDB_ASYNC			0x02000000
> 
> Maybe an explanation what 'SCF' stands for?

Sure, I will add a comment here.. "Se_Cmd Flags"

> > +
> > +/* struct se_device->type */
> > +#define PSCSI					1
> > +#define STGT					2
> > +#define PATA					3
> > +#define IBLOCK					4
> > +#define RAMDISK_DR				5
> > +#define RAMDISK_MCP				6
> > +#define FILEIO					7
> > +#define VROM					8
> > +#define VTAPE					9
> > +#define MEDIA_CHANGER				10
> 
> Enum?

Sure.

> > +
> > +/* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
> > +#define TRANSPORT_LUNFLAGS_NO_ACCESS		0x00000000
> > +#define TRANSPORT_LUNFLAGS_INITIATOR_ACCESS	0x00000001
> > +#define TRANSPORT_LUNFLAGS_READ_ONLY		0x00000002
> > +#define TRANSPORT_LUNFLAGS_READ_WRITE		0x00000004
> > +
> > +/* struct se_device->dev_status */
> > +#define TRANSPORT_DEVICE_ACTIVATED		0x01
> > +#define	TRANSPORT_DEVICE_DEACTIVATED		0x02
> > +#define TRANSPORT_DEVICE_QUEUE_FULL		0x04
> > +#define	TRANSPORT_DEVICE_SHUTDOWN		0x08
> > +#define TRANSPORT_DEVICE_OFFLINE_ACTIVATED	0x10
> > +#define TRANSPORT_DEVICE_OFFLINE_DEACTIVATED	0x20
> 
> Hmm.. Maybe it might be prudent to restructure this file altogether so that
> the #defines are right next to the variables. Something akin to how 'ehci.h' 
> has it setup?
> 

Hmmmm, fair point.  That would make things a bit easier to read.

> It would make it so much simpler to figure out what are the possible states of 
> a variable in a structure.
> 
> > +
> > +#define	DEV_STATUS_THR_TUR			1
> > +#define DEV_STATUS_THR_TAKE_ONLINE		2
> > +#define DEV_STATUS_THR_TAKE_OFFLINE		3
> > +#define DEV_STATUS_THR_SHUTDOWN			4
> 
> For most if not all of the #define you mentioned to what the variable the 
> #defines correspond to. You missed it for these four above.
> 

Actually, this is dead code from 2.x that I will drop.

> > +
> > +/* struct se_dev_entry->deve_flags */
> > +#define DEF_PR_REGISTERED			0x01
> > +
> > +/* Used for struct t10_pr_registration->pr_reg_flags */
> > +#define PRF_ISID_PRESENT_AT_REG			0x01
> 
> > +
> > +/* transport_send_check_condition_and_sense() */
> 
> That is one lengthy function name, but I don't see it in this patch?
> 

This one lives in drivers/target/target_core_transport.c

> > +#define NON_EXISTENT_LUN			0x1
> > +#define UNSUPPORTED_SCSI_OPCODE			0x2
> > +#define INCORRECT_AMOUNT_OF_DATA		0x3
> > +#define UNEXPECTED_UNSOLICITED_DATA		0x4
> > +#define SERVICE_CRC_ERROR			0x5
> > +#define SNACK_REJECTED				0x6
> > +#define SECTOR_COUNT_TOO_MANY			0x7
> > +#define INVALID_CDB_FIELD			0x8
> > +#define INVALID_PARAMETER_LIST			0x9
> > +#define LOGICAL_UNIT_COMMUNICATION_FAILURE	0xa
> > +#define UNKNOWN_MODE_PAGE			0xb
> > +#define WRITE_PROTECTED				0xc
> > +#define CHECK_CONDITION_ABORT_CMD		0xd
> > +#define CHECK_CONDITION_UNIT_ATTENTION		0xe
> > +#define CHECK_CONDITION_NOT_READY		0xf
> 
> So these #defines have nothing to do with SAM values. And looking at
> include/scsi/iscsi_proto.h, it also has an incremental list, but it has a 
> prefix.
> 

Yes, these are used by TCM to signal which ASC/ASCQ payload should be
built in transport_send_check_condition_and_sense().

To prevent namespace pollution I am happy to add a prefix to these..

> Should these #defines have a prefix? Say TARGET_
> > +
> > +struct se_obj {
> > +	atomic_t obj_access_count;
> > +} ____cacheline_aligned;
> > +
> > +typedef enum {
> > +	SPC_ALUA_PASSTHROUGH,
> > +	SPC2_ALUA_DISABLED,
> > +	SPC3_ALUA_EMULATED
> > +} t10_alua_index_t;
> 
> Could you reference which spec has this defined?
> > 
> > +typedef enum {
> > +	SAM_TASK_ATTR_PASSTHROUGH,
> > +	SAM_TASK_ATTR_UNTAGGED,
> > +	SAM_TASK_ATTR_EMULATED
> > +} t10_task_attr_index_t;
> > +
> Ditto.
> 

Both of these are internal states for ALUA and Task Attr emulation in
order to known if the:

	*) emulation is disabled for TCM/pSCSI passthrough (eg: the underlying
firmware will support it)
	*) Emulation is diabled all together for non passthrough ops
	*) Emulation is enabled

I think keeping the Task Attr emulation here makes sense, but I will go
ahead and move the ALUA ones to target_core_alua.h

> > +struct se_cmd;
> > +
> > +struct t10_alua {
> > +	t10_alua_index_t alua_type;
> > +	/* ALUA Target Port Group ID */
> > +	u16	alua_tg_pt_gps_counter;
> > +	u32	alua_tg_pt_gps_count;
> 
> So the ALUA tpg id is the alua_tg_pt_gps_counter or the alua_tg_pt_gps_count? 
> Or both? That is a mouthfull. Can you just call them 'counter' and 'count' 
> respectively?

I agree that these are a bit long, but is really does make it nice for
grepping having these (erm, unique) prefixes.

> 
> > +	spinlock_t tg_pt_gps_lock;
> > +	struct se_subsystem_dev *t10_sub_dev;
> 
> Just call it "sub_dev" ?

Ditto on the prefix bit.. :-)

> > +	/* Used for default ALUA Target Port Group */
> > +	struct t10_alua_tg_pt_gp *default_tg_pt_gp;
> 
> You don't seem to define the "struct t10_alua_tg_pt_gp" - won't the compiler 
> throw a fit here? Can you call it 'default'?
> 
> > +	/* Used for default ALUA Target Port Group ConfigFS group */
> > +	struct config_group alua_tg_pt_gps_group;
> 
> And here 'group' instead of 'alua_tg_pt_gps_group'?
> > +	int (*alua_state_check)(struct se_cmd *, unsigned char *, u8 *);
> 
> > +	struct list_head tg_pt_gps_list;
> 
> how about just 'list' ?
> > +} ____cacheline_aligned;
> 
> > +
> > +struct t10_alua_lu_gp {
> > +	u16	lu_gp_id;
> > +	int	lu_gp_valid_id;
> > +	u32	lu_gp_members;
> > +	atomic_t lu_gp_shutdown;
> > +	atomic_t lu_gp_ref_cnt;
> > +	spinlock_t lu_gp_lock;
> > +	struct config_group lu_gp_group;
> > +	struct list_head lu_gp_list;
> > +	struct list_head lu_gp_mem_list;
> 
> How about getting rid of the 'lu_gp' prefix for all members?
> > +} ____cacheline_aligned;
> > +
> > +struct t10_alua_lu_gp_member {
> > +	int lu_gp_assoc;
> > +	atomic_t lu_gp_mem_ref_cnt;
> > +	spinlock_t lu_gp_mem_lock;
> > +	struct t10_alua_lu_gp *lu_gp;
> > +	struct se_device *lu_gp_mem_dev;
> > +	struct list_head lu_gp_mem_list;
> 
> Ditto
> > +} ____cacheline_aligned;
> > +
> > +struct t10_alua_tg_pt_gp {
> > +	u16	tg_pt_gp_id;
> > +	int	tg_pt_gp_valid_id;
> > +	int	tg_pt_gp_alua_access_status;
> > +	int	tg_pt_gp_alua_access_type;
> > +	int	tg_pt_gp_nonop_delay_msecs;
> > +	int	tg_pt_gp_trans_delay_msecs;
> > +	int	tg_pt_gp_pref;
> > +	int	tg_pt_gp_write_metadata;
> > +	u32	tg_pt_gp_md_buf_len;
> > +	u32	tg_pt_gp_members;
> > +	atomic_t tg_pt_gp_alua_access_state;
> > +	atomic_t tg_pt_gp_ref_cnt;
> > +	spinlock_t tg_pt_gp_lock;
> > +	struct mutex tg_pt_gp_md_mutex;
> > +	struct se_subsystem_dev *tg_pt_gp_su_dev;
> > +	struct config_group tg_pt_gp_group;
> > +	struct list_head tg_pt_gp_list;
> > +	struct list_head tg_pt_gp_mem_list;
> 
> Ditto
> > +} ____cacheline_aligned;
> > +
> > +struct t10_alua_tg_pt_gp_member {
> > +	int tg_pt_gp_assoc;
> > +	atomic_t tg_pt_gp_mem_ref_cnt;
> > +	spinlock_t tg_pt_gp_mem_lock;
> > +	struct t10_alua_tg_pt_gp *tg_pt_gp;
> > +	struct se_port *tg_pt;
> > +	struct list_head tg_pt_gp_mem_list;
> 
> Ditto

So yeah, I would like to keep the prefixes for grokability here.. 8-)

> > +} ____cacheline_aligned;
> > +
> > +struct t10_vpd {
> > +	unsigned char device_identifier[INQUIRY_VPD_DEVICE_IDENTIFIER_LEN];
> > +	int protocol_identifier_set;
> > +	u32 protocol_identifier;
> > +	u32 device_identifier_code_set;
> > +	u32 association;
> > +	u32 device_identifier_type;
> > +	struct list_head vpd_list;
> > +} ____cacheline_aligned;
> 
> The 't10' makes this sound official. If so, can you provide in the comment 
> section the appropriate spec number?

Sure.

> > +
> > +struct t10_wwn {
> > +	unsigned char vendor[8];
> > +	unsigned char model[16];
> > +	unsigned char revision[4];
> > +	unsigned char unit_serial[INQUIRY_VPD_SERIAL_LEN];
> > +	spinlock_t t10_vpd_lock;
> 
> vpd_lock or wwn_lock? Why do you need a spinlock for this structure?

This lock protects the list of Device identifier (0x83) VPDs that are
scanned during struct se_device registration.

> BTW, if you run checkpatch I think it throws a fit if you don't document the 
> spinlock usage. You did use checkpatch.pl ,right?

Yes, at this point there are a few TAB, whitespace and some 80 character
columns warning.  Things are clean other than these nits, and a few
false positives on some of the target_core_configfs.c CPP macros.

> 
> > +	struct se_subsystem_dev *t10_sub_dev;
> 
> How about 'sub_dev' instead?
> > +	struct config_group t10_wwn_group;
> 
> Take out the 't10_wwn'
> > +	struct list_head t10_vpd_list;
> And make this 'list' by itself.
> 
> > +} ____cacheline_aligned;
> 
> 
> > +
> > +typedef enum {
> > +	SPC_PASSTHROUGH,
> > +	SPC2_RESERVATIONS,
> > +	SPC3_PERSISTENT_RESERVATIONS
> > +} t10_reservations_index_t;
> 
> Are those official values ? Spec number, page?

These are also internal emulation states for PR emulation.  I will move
these to target_core_pr.h..

> > +
> > +struct t10_pr_registration {
> > +	/* Used for fabrics that contain WWN+ISID */
> > +	char pr_reg_isid[PR_REG_ISID_LEN];
> > +	/* Used during APTPL metadata reading */
> > +	unsigned char pr_iport[PR_APTPL_MAX_IPORT_LEN];
> > +	/* Used during APTPL metadata reading */
> > +	unsigned char pr_tport[PR_APTPL_MAX_TPORT_LEN];
> > +	/* For writing out live meta data */
> > +	unsigned char *pr_aptpl_buf;
> > +	u16 pr_aptpl_rpti;
> > +	u16 pr_reg_tpgt;
> > +	/* Reservation effects all target ports */
> > +	int pr_reg_all_tg_pt;
> > +	/* Activate Persistence across Target Power Loss */
> > +	int pr_reg_aptpl;
> > +	int pr_res_holder;
> > +	int pr_res_type;
> > +	int pr_res_scope;
> > +	u32 pr_reg_flags;
> > +	u32 pr_res_mapped_lun;
> > +	u32 pr_aptpl_target_lun;
> > +	u32 pr_res_generation;
> > +	u64 pr_reg_bin_isid;
> > +	u64 pr_res_key;
> > +	atomic_t pr_res_holders;
> > +	struct se_node_acl *pr_reg_nacl;
> > +	struct se_dev_entry *pr_reg_deve;
> > +	struct se_lun *pr_reg_tg_pt_lun;
> > +	struct list_head pr_reg_list;
> > +	struct list_head pr_reg_abort_list;
> > +	struct list_head pr_reg_aptpl_list;
> > +	struct list_head pr_reg_atp_list;
> > +	struct list_head pr_reg_atp_mem_list;
> > +} ____cacheline_aligned;
> > +
> What is 'pr' ? Pringles? You could call that structure 'persist_reg' and
> remove all of those 'pr_reg_' prefixes.

This is intended to be shorthand for 'Persistent Reservations
Registration'..

> 
> > +struct t10_reservation_template {
> > +	/* Reservation effects all target ports */
> > +	int pr_all_tg_pt;
> 
> Um, not sure what the comment has to do with 'pr_all_tg_pt' ? What 
> does 'pr_all_tg_tg' do?

This has to do with PR logic effecting the other LUNs of the same device
server that the same Initiator Port is connecting to over the same or
different target port.

> > +	/* Activate Persistence across Target Power Loss enabled
> > +	 * for SCSI device */
> > +	int pr_aptpl_active;
> 
> Perhaps 'is_active' ?
> 
> > +	u32 pr_aptpl_buf_len;
> 
> Where is the buf? 

This is used to allocate a buffer for each registration at struct
t10_pr_registration->pr_aptpl_buf in drivers/target/target_core_pr.c:
__core_scsi3_do_alloc_registration()

> 
> > +	u32 pr_generation;
> > +	t10_reservations_index_t res_type;
> > +	spinlock_t registration_lock;
> > +	spinlock_t aptpl_reg_lock;
> > +	/* Reservation holder when pr_all_tg_pt=1 */
> 
> Ah, and when it is another value?

Actually, I think this is wrong..  Will fix this comment..

> 
> > +	struct se_node_acl *pr_res_holder;
> > +	struct list_head registration_list;
> > +	struct list_head aptpl_reg_list;
> > +	int (*t10_reservation_check)(struct se_cmd *, u32 *);
> > +	int (*t10_seq_non_holder)(struct se_cmd *, unsigned char *, u32);
> > +	int (*t10_pr_register)(struct se_cmd *);
> > +	int (*t10_pr_clear)(struct se_cmd *);
> 
> You can get rid of the t10_ prefix.
> > +} ____cacheline_aligned;
> > +
> 
> This is the second structure I see where you mix data with functions.
> 
> I don't know what the actual StyleGuide is for this, but would it be prudent 
> to split the data (spinlocks, list, values) from the behavior (functions?) 
> This could be done by embedding another struct - which only has function 
> pointers?

Sure, I don't have a problem with doing that here..

> 
> > +struct se_queue_req {
> > +	int			state;
> > +	void			*cmd;
> > +	struct list_head	qr_list;
> > +} ____cacheline_aligned;
> > +
> > +struct se_queue_obj {
> > +	atomic_t		queue_cnt;
> > +	spinlock_t		cmd_queue_lock;
> > +	struct list_head	qobj_list;
> > +	wait_queue_head_t	thread_wq;
> > +	struct completion	thread_create_comp;
> > +	struct completion	thread_done_comp;
> > +} ____cacheline_aligned;
> > +
> > +struct se_transport_task {
> > +	unsigned char		t_task_cdb[SCSI_CDB_SIZE];
> > +	unsigned long long	t_task_lba;
> > +	int			t_tasks_failed;
> > +	int			t_task_fua;
> > +	u32			t_task_cdbs;
> > +	u32			t_task_check;
> > +	u32			t_task_no;
> > +	u32			t_task_sectors;
> > +	u32			t_task_se_num;
> > +	atomic_t		t_fe_count;
> > +	atomic_t		t_se_count;
> > +	atomic_t		t_task_cdbs_left;
> > +	atomic_t		t_task_cdbs_ex_left;
> > +	atomic_t		t_task_cdbs_timeout_left;
> > +	atomic_t		t_task_cdbs_sent;
> > +	atomic_t		t_transport_aborted;
> > +	atomic_t		t_transport_active;
> > +	atomic_t		t_transport_complete;
> > +	atomic_t		t_transport_queue_active;
> > +	atomic_t		t_transport_sent;
> > +	atomic_t		t_transport_stop;
> > +	atomic_t		t_transport_timeout;
> > +	atomic_t		transport_dev_active;
> > +	atomic_t		transport_lun_active;
> > +	atomic_t		transport_lun_fe_stop;
> > +	atomic_t		transport_lun_stop;
> > +	spinlock_t		t_state_lock;
> > +	struct completion	t_transport_stop_comp;
> > +	struct completion	t_transport_passthrough_comp;
> > +	struct completion	t_transport_passthrough_wcomp;
> > +	struct completion	transport_lun_fe_stop_comp;
> > +	struct completion	transport_lun_stop_comp;
> > +	void			*t_task_buf;
> > +	void			*t_task_pt_buf;
> > +	struct list_head	t_task_list;
> > +	struct list_head	*t_mem_list;
> > +} ____cacheline_aligned;
> 
> I think you can remove the "t_" prefix.  I am though a bit confused, the 
> struct is called 'transport_task' and there are values that are 
> transport_task related, but then there are also some that are clearly related 
> to a task:t_task_no, t_task_check? Should they just be plural?
> 

Indeed, I think that make t_task into t_tasks makes sense above..

> > +
> > +struct se_task {
> > +	unsigned char	task_sense;
> > +	struct scatterlist *task_sg;
> > +	void		*transport_req;
> > +	u8		task_scsi_status;
> > +	u8		task_flags;
> > +	int		task_error_status;
> > +	int		task_state_flags;
> > +	unsigned long long	task_lba;
> > +	u32		task_no;
> > +	u32		task_sectors;
> > +	u32		task_size;
> > +	u32		task_sg_num;
> > +	u32		task_sg_offset;
> > +	struct se_cmd *task_se_cmd;
> > +	struct se_device	*se_dev;
> > +	struct completion	task_stop_comp;
> > +	atomic_t	task_active;
> > +	atomic_t	task_execute_queue;
> > +	atomic_t	task_timeout;
> > +	atomic_t	task_sent;
> > +	atomic_t	task_stop;
> > +	atomic_t	task_state_active;
> > +	struct timer_list	task_timer;
> > +	int (*transport_map_task)(struct se_task *, u32);
> > +	struct se_device *se_obj_ptr;
> > +	struct list_head t_list;
> > +	struct list_head t_execute_list;
> > +	struct list_head t_state_list;
> > +} ____cacheline_aligned;
> > +
> > +#define TASK_CMD(task)	((struct se_cmd *)task->task_se_cmd)
> > +#define TASK_DEV(task)	((struct se_device *)task->se_dev)
> > +
> 
> I stopped the review here. I think that using the ideas of how ehci.h mixes 
> the struct variables with #defines will make this much easier to read?

Ok, I will go ahead and make the changes per the above comments and will
drop you a line when the push is ready.  I do agree that mixing the
struct variables with #defines in target_core_base.h does make things a
bit more readable.  Let me consider this a bit.

Any comments from the kernel folks on this particular item..?

Thanks for your comments Konrad!

Best,

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux