Re: [PATCH] target: drop unnecessary get_fabric_name() accessor from fabric_ops
On Thu, Nov 22, 2018 at 03:16:23PM +0100, David Disseldorp wrote: > All fabrics return a const string. In all cases *except* iSCSI the > get_fabric_name() string matches fabric_ops.name. > > Both fabric_ops.get_fabric_name() and fabric_ops.name are user facing, > with the former being used for PR/ALUA state and the latter for configFS > (config/target/$name), so we unfortunately need to keep both strings > around for now. Would it make sense to just use .name unless .fabric_name is set to mostly avoid the duplication?
[PATCH v4] scsi: tcmu: avoid cmd/qfull timers updated whenever a new cmd comes
From: Xiubo Li
Currently there has one cmd timeout timer and one qfull timer for
each udev, and whenever there has any new coming cmd it will update
the cmd timer or qfull timer. And for some corner case the timers
are always working only for the ringbuffer's and full queue's newest
cmd. That's to say the timer won't be fired even there has one cmd
stuck for a very long time and the deadline is reached.
This fix will keep the cmd/qfull timers to be pended for the oldest
cmd in ringbuffer and full queue, and will update them with the next
cmd's deadline only when the old cmd's deadline is reached or removed
from the ringbuffer and full queue.
Signed-off-by: Xiubo Li
---
Changed in V4:
- Addressed all the new comments.
Changed in V3:
- Add inflight queue support, thanks Mike.
Changed in V2:
- Add qfull timer
- Addressed all the comments from Mike, thanks.
drivers/target/target_core_user.c | 88 +++
1 file changed, 61 insertions(+), 27 deletions(-)
diff --git a/drivers/target/target_core_user.c
b/drivers/target/target_core_user.c
index 9cd404a..ac76201 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -148,7 +148,7 @@ struct tcmu_dev {
size_t ring_size;
struct mutex cmdr_lock;
- struct list_head cmdr_queue;
+ struct list_head qfull_queue;
uint32_t dbi_max;
uint32_t dbi_thresh;
@@ -159,6 +159,7 @@ struct tcmu_dev {
struct timer_list cmd_timer;
unsigned int cmd_time_out;
+ struct list_head inflight_queue;
struct timer_list qfull_timer;
int qfull_time_out;
@@ -179,7 +180,7 @@ struct tcmu_dev {
struct tcmu_cmd {
struct se_cmd *se_cmd;
struct tcmu_dev *tcmu_dev;
- struct list_head cmdr_queue_entry;
+ struct list_head queue_entry;
uint16_t cmd_id;
@@ -192,6 +193,7 @@ struct tcmu_cmd {
unsigned long deadline;
#define TCMU_CMD_BIT_EXPIRED 0
+#define TCMU_CMD_BIT_INFLIGHT 1
unsigned long flags;
};
/*
@@ -586,7 +588,7 @@ static struct tcmu_cmd *tcmu_alloc_cmd(struct se_cmd
*se_cmd)
if (!tcmu_cmd)
return NULL;
- INIT_LIST_HEAD(_cmd->cmdr_queue_entry);
+ INIT_LIST_HEAD(_cmd->queue_entry);
tcmu_cmd->se_cmd = se_cmd;
tcmu_cmd->tcmu_dev = udev;
@@ -915,11 +917,13 @@ static int tcmu_setup_cmd_timer(struct tcmu_cmd
*tcmu_cmd, unsigned int tmo,
return 0;
tcmu_cmd->deadline = round_jiffies_up(jiffies + msecs_to_jiffies(tmo));
- mod_timer(timer, tcmu_cmd->deadline);
+ if (!timer_pending(timer))
+ mod_timer(timer, tcmu_cmd->deadline);
+
return 0;
}
-static int add_to_cmdr_queue(struct tcmu_cmd *tcmu_cmd)
+static int add_to_qfull_queue(struct tcmu_cmd *tcmu_cmd)
{
struct tcmu_dev *udev = tcmu_cmd->tcmu_dev;
unsigned int tmo;
@@ -942,7 +946,7 @@ static int add_to_cmdr_queue(struct tcmu_cmd *tcmu_cmd)
if (ret)
return ret;
- list_add_tail(_cmd->cmdr_queue_entry, >cmdr_queue);
+ list_add_tail(_cmd->queue_entry, >qfull_queue);
pr_debug("adding cmd %u on dev %s to ring space wait queue\n",
tcmu_cmd->cmd_id, udev->name);
return 0;
@@ -999,7 +1003,7 @@ static sense_reason_t queue_cmd_ring(struct tcmu_cmd
*tcmu_cmd, int *scsi_err)
base_command_size = tcmu_cmd_get_base_cmd_size(tcmu_cmd->dbi_cnt);
command_size = tcmu_cmd_get_cmd_size(tcmu_cmd, base_command_size);
- if (!list_empty(>cmdr_queue))
+ if (!list_empty(>qfull_queue))
goto queue;
mb = udev->mb_addr;
@@ -1096,13 +1100,16 @@ static sense_reason_t queue_cmd_ring(struct tcmu_cmd
*tcmu_cmd, int *scsi_err)
UPDATE_HEAD(mb->cmd_head, command_size, udev->cmdr_size);
tcmu_flush_dcache_range(mb, sizeof(*mb));
+ list_add_tail(_cmd->queue_entry, >inflight_queue);
+ set_bit(TCMU_CMD_BIT_INFLIGHT, _cmd->flags);
+
/* TODO: only if FLUSH and FUA? */
uio_event_notify(>uio_info);
return 0;
queue:
- if (add_to_cmdr_queue(tcmu_cmd)) {
+ if (add_to_qfull_queue(tcmu_cmd)) {
*scsi_err = TCM_OUT_OF_RESOURCES;
return -1;
}
@@ -1145,6 +1152,8 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd,
struct tcmu_cmd_entry *
if (test_bit(TCMU_CMD_BIT_EXPIRED, >flags))
goto out;
+ list_del_init(>queue_entry);
+
tcmu_cmd_reset_dbi_cur(cmd);
if (entry->hdr.uflags & TCMU_UFLAG_UNKNOWN_OP) {
@@ -1194,9 +1203,29 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd,
struct tcmu_cmd_entry *
tcmu_free_cmd(cmd);
}
+static void tcmu_set_next_deadline(struct list_head *queue,
+ struct timer_list *timer)
+{
+ struct tcmu_cmd *tcmu_cmd, *tmp_cmd;
+ unsigned long deadline = 0;
+
+
Re: [PATCH v7] scsi: Add hwmon support for SMART temperature sensors
[-cc] Hi Linus, On 11/22/18 5:49 AM, Linus Walleij wrote: [ ... ] (It's also worth noting that HDD temperature sensors are notoriously unreliable). I am sorry if you think that D-Link does bad engineering, what I am trying to achieve is upstream support for this device, without any out-of-tree patches. The D-Link DIR-685 uses the harddisk sensor for this, whether we like it or not. Following the above argument (not yours, the one about accuracy), I guess we should drop support for all CPU temperature sensors from the Linux kernel. After all, they are known to be much more inaccurate than a HDD temperature sensor could ever be. Seriously, any argument about (lack of) sensor accuracy should be silently ignored. I may be overly pessimistic nowadays, but whenever I see such an argument, I read it as "we'll never accept your patch, so better stop wasting your time and forget about it". I hope I am wrong, but it seems to me that this is where things are going. Can you possibly extract this as pure hwmon driver outside scsi control ? I'll be happy to accept it as standalone hwmon driver. Thanks, Guenter
[PATCH] scsi: ufs: Remove redundant sense size definition
By spec, the ufs sense data is 18 bytes long.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufs.h| 4 ++--
drivers/scsi/ufs/ufshcd.c | 17 +++--
2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 8e4e526..dd65fea 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -46,7 +46,7 @@
#define QUERY_DESC_HDR_SIZE 2
#define QUERY_OSF_SIZE(GENERAL_UPIU_REQUEST_SIZE - \
(sizeof(struct utp_upiu_header)))
-#define RESPONSE_UPIU_SENSE_DATA_LENGTH18
+#define UFS_SENSE_SIZE 18
#define UPIU_HEADER_DWORD(byte3, byte2, byte1, byte0)\
cpu_to_be32((byte3 << 24) | (byte2 << 16) |\
@@ -458,7 +458,7 @@ struct utp_cmd_rsp {
__be32 residual_transfer_count;
__be32 reserved[4];
__be16 sense_data_len;
- u8 sense_data[RESPONSE_UPIU_SENSE_DATA_LENGTH];
+ u8 sense_data[UFS_SENSE_SIZE];
};
/**
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 003d489..303cefd 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -51,8 +51,6 @@
#define CREATE_TRACE_POINTS
#include
-#define UFSHCD_REQ_SENSE_SIZE 18
-
#define UFSHCD_ENABLE_INTRS(UTP_TRANSFER_REQ_COMPL |\
UTP_TASK_REQ_COMPL |\
UFSHCD_ERROR_MASK)
@@ -1890,11 +1888,10 @@ static inline void ufshcd_copy_sense_data(struct
ufshcd_lrb *lrbp)
int len_to_copy;
len = be16_to_cpu(lrbp->ucd_rsp_ptr->sr.sense_data_len);
- len_to_copy = min_t(int, RESPONSE_UPIU_SENSE_DATA_LENGTH, len);
+ len_to_copy = min_t(int, UFS_SENSE_SIZE, len);
- memcpy(lrbp->sense_buffer,
- lrbp->ucd_rsp_ptr->sr.sense_data,
- min_t(int, len_to_copy, UFSHCD_REQ_SENSE_SIZE));
+ memcpy(lrbp->sense_buffer, lrbp->ucd_rsp_ptr->sr.sense_data,
+ len_to_copy);
}
}
@@ -2456,7 +2453,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host,
struct scsi_cmnd *cmd)
WARN_ON(lrbp->cmd);
lrbp->cmd = cmd;
- lrbp->sense_bufflen = UFSHCD_REQ_SENSE_SIZE;
+ lrbp->sense_bufflen = UFS_SENSE_SIZE;
lrbp->sense_buffer = cmd->sense_buffer;
lrbp->task_tag = tag;
lrbp->lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun);
@@ -7461,19 +7458,19 @@ static void ufshcd_hba_exit(struct ufs_hba *hba)
0,
0,
0,
- UFSHCD_REQ_SENSE_SIZE,
+ UFS_SENSE_SIZE,
0};
char *buffer;
int ret;
- buffer = kzalloc(UFSHCD_REQ_SENSE_SIZE, GFP_KERNEL);
+ buffer = kzalloc(UFS_SENSE_SIZE, GFP_KERNEL);
if (!buffer) {
ret = -ENOMEM;
goto out;
}
ret = scsi_execute(sdp, cmd, DMA_FROM_DEVICE, buffer,
- UFSHCD_REQ_SENSE_SIZE, NULL, NULL,
+ UFS_SENSE_SIZE, NULL, NULL,
msecs_to_jiffies(1000), 3, 0, RQF_PM, NULL);
if (ret)
pr_err("%s: failed with err %d\n", __func__, ret);
--
1.9.1
[PATCH] target: drop unnecessary get_fabric_name() accessor from fabric_ops
All fabrics return a const string. In all cases *except* iSCSI the
get_fabric_name() string matches fabric_ops.name.
Both fabric_ops.get_fabric_name() and fabric_ops.name are user facing,
with the former being used for PR/ALUA state and the latter for configFS
(config/target/$name), so we unfortunately need to keep both strings
around for now.
Replace the useless .get_fabric_name() accessor function with a const
string fabric_name member variable.
Signed-off-by: David Disseldorp
---
Note: This conflicts with:
[RFC PATCH] target: sanitize ALUA and PR state file paths before use
I'll resolve this once we decide whether or not the RFC change should
go in as-is.
drivers/infiniband/ulp/srpt/ib_srpt.c| 7 +--
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 7 +--
drivers/scsi/qla2xxx/tcm_qla2xxx.c | 14 +
drivers/target/iscsi/iscsi_target_configfs.c | 7 +--
drivers/target/loopback/tcm_loop.c | 7 +--
drivers/target/sbp/sbp_target.c | 7 +--
drivers/target/target_core_alua.c| 6 +-
drivers/target/target_core_configfs.c| 18 +++---
drivers/target/target_core_device.c | 26
drivers/target/target_core_fabric_configfs.c | 2 +-
drivers/target/target_core_pr.c | 88 ++--
drivers/target/target_core_stat.c| 4 +-
drivers/target/target_core_tmr.c | 4 +-
drivers/target/target_core_tpg.c | 22 +++
drivers/target/target_core_transport.c | 10 ++--
drivers/target/target_core_ua.c | 4 +-
drivers/target/target_core_xcopy.c | 7 +--
drivers/target/tcm_fc/tfc_conf.c | 7 +--
drivers/usb/gadget/function/f_tcm.c | 7 +--
drivers/vhost/scsi.c | 7 +--
drivers/xen/xen-scsiback.c | 7 +--
include/target/target_core_fabric.h | 6 +-
22 files changed, 109 insertions(+), 165 deletions(-)
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 2357aa727dcf..657d728da40c 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -3147,11 +3147,6 @@ static int srpt_check_false(struct se_portal_group
*se_tpg)
return 0;
}
-static char *srpt_get_fabric_name(void)
-{
- return "srpt";
-}
-
static struct srpt_port *srpt_tpg_to_sport(struct se_portal_group *tpg)
{
return tpg->se_tpg_wwn->priv;
@@ -3679,7 +3674,7 @@ static struct configfs_attribute *srpt_wwn_attrs[] = {
static const struct target_core_fabric_ops srpt_template = {
.module = THIS_MODULE,
.name = "srpt",
- .get_fabric_name= srpt_get_fabric_name,
+ .fabric_name= "srpt",
.tpg_get_wwn= srpt_get_fabric_wwn,
.tpg_get_tag= srpt_get_tag,
.tpg_check_demo_mode= srpt_check_false,
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index e63aadd10dfd..6e1c3e65f37b 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -3695,11 +3695,6 @@ static int ibmvscsis_get_system_info(void)
return 0;
}
-static char *ibmvscsis_get_fabric_name(void)
-{
- return "ibmvscsis";
-}
-
static char *ibmvscsis_get_fabric_wwn(struct se_portal_group *se_tpg)
{
struct ibmvscsis_tport *tport =
@@ -4045,8 +4040,8 @@ static struct configfs_attribute *ibmvscsis_tpg_attrs[] =
{
static const struct target_core_fabric_ops ibmvscsis_ops = {
.module = THIS_MODULE,
.name = "ibmvscsis",
+ .fabric_name= "ibmvscsis",
.max_data_sg_nents = MAX_TXU / PAGE_SIZE,
- .get_fabric_name= ibmvscsis_get_fabric_name,
.tpg_get_wwn= ibmvscsis_get_fabric_wwn,
.tpg_get_tag= ibmvscsis_get_tag,
.tpg_get_default_depth = ibmvscsis_get_default_depth,
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 65053c066680..ff8735effe28 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -108,11 +108,6 @@ static ssize_t tcm_qla2xxx_format_wwn(char *buf, size_t
len, u64 wwn)
b[0], b[1], b[2], b[3], b[4], b[5], b[6], b[7]);
}
-static char *tcm_qla2xxx_get_fabric_name(void)
-{
- return "qla2xxx";
-}
-
/*
* From drivers/scsi/scsi_transport_fc.c:fc_parse_wwn
*/
@@ -178,11 +173,6 @@ static int tcm_qla2xxx_npiv_parse_wwn(
return 0;
}
-static char *tcm_qla2xxx_npiv_get_fabric_name(void)
-{
- return "qla2xxx_npiv";
-}
-
static char *tcm_qla2xxx_get_fabric_wwn(struct se_portal_group *se_tpg)
{
struct tcm_qla2xxx_tpg *tpg =
[RFC PATCH] target: sanitize ALUA and PR state file paths before use
Block ALUA and PR state storage if any of the dynamic subdirectory
components include a path separator.
Fixes: c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6")
Signed-off-by: David Disseldorp
Signed-off-by: Lee Duncan
---
Note:
Submitted as an RFC, as I've not properly tested the alua code path.
I'm also not sure whether it's reasonable to break existing setups
with a '/' in the configured unit_serial. Where "break" means fail
APTPL PR requests; ALUA state-save failures are ignored internally.
drivers/target/target_core_alua.c | 27 ---
drivers/target/target_core_pr.c | 5 +
2 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/target/target_core_alua.c
b/drivers/target/target_core_alua.c
index 4f134b0c3e29..517945f881e0 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -918,9 +918,16 @@ static int core_alua_update_tpg_primary_metadata(
{
unsigned char *md_buf;
struct t10_wwn *wwn = _pt_gp->tg_pt_gp_dev->t10_wwn;
+ const char *tpgs_name;
char *path;
int len, rc;
+ tpgs_name = config_item_name(_pt_gp->tg_pt_gp_group.cg_item);
+ if (strchr(wwn->unit_serial, '/') || strchr(tpgs_name, '/')) {
+ pr_err("Unable to construct valid ALUA metadata path\n");
+ return -EINVAL;
+ }
+
md_buf = kzalloc(ALUA_MD_BUF_LEN, GFP_KERNEL);
if (!md_buf) {
pr_err("Unable to allocate buf for ALUA metadata\n");
@@ -937,8 +944,7 @@ static int core_alua_update_tpg_primary_metadata(
rc = -ENOMEM;
path = kasprintf(GFP_KERNEL, "%s/alua/tpgs_%s/%s", db_root,
- >unit_serial[0],
- config_item_name(_pt_gp->tg_pt_gp_group.cg_item));
+ >unit_serial[0], tpgs_name);
if (path) {
rc = core_alua_write_tpg_metadata(path, md_buf, len);
kfree(path);
@@ -1210,6 +1216,8 @@ static int core_alua_update_tpg_secondary_metadata(struct
se_lun *lun)
{
struct se_portal_group *se_tpg = lun->lun_tpg;
unsigned char *md_buf;
+ const char *fabric_name;
+ const char *wwn;
char *path;
int len, rc;
@@ -1227,17 +1235,22 @@ static int
core_alua_update_tpg_secondary_metadata(struct se_lun *lun)
atomic_read(>lun_tg_pt_secondary_offline),
lun->lun_tg_pt_secondary_stat);
+ fabric_name = se_tpg->se_tpg_tfo->get_fabric_name();
+ wwn = se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg);
+ if (strchr(fabric_name, '/') || strchr(wwn, '/')) {
+ pr_err("Unable to construct valid ALUA metadata path\n");
+ rc = -EINVAL;
+ goto out_free;
+ }
+
if (se_tpg->se_tpg_tfo->tpg_get_tag != NULL) {
path = kasprintf(GFP_KERNEL, "%s/alua/%s/%s+%hu/lun_%llu",
- db_root, se_tpg->se_tpg_tfo->get_fabric_name(),
- se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg),
+ db_root, fabric_name, wwn,
se_tpg->se_tpg_tfo->tpg_get_tag(se_tpg),
lun->unpacked_lun);
} else {
path = kasprintf(GFP_KERNEL, "%s/alua/%s/%s/lun_%llu",
- db_root, se_tpg->se_tpg_tfo->get_fabric_name(),
- se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg),
- lun->unpacked_lun);
+ db_root, fabric_name, wwn, lun->unpacked_lun);
}
if (!path) {
rc = -ENOMEM;
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 10db5656fd5d..48397cf919d4 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1980,6 +1980,11 @@ static int __core_scsi3_write_aptpl_to_file(
int ret;
loff_t pos = 0;
+ if (strchr(wwn->unit_serial, '/')) {
+ pr_err("Unable to construct valid APTPL metadata path\n");
+ return -EINVAL;
+ }
+
path = kasprintf(GFP_KERNEL, "%s/pr/aptpl_%s", db_root,
>unit_serial[0]);
if (!path)
--
2.13.7
Re: [PATCH 22/23] zfcp: drop default switch case which might paper over missing case
On 11/16/2018 12:22 PM, Hannes Reinecke wrote:
On 11/8/18 3:44 PM, Steffen Maier wrote:
would now
suppress helpful -Wswitch compiler warnings when building with W=1
But then again, only with W=1 we would notice unhandled enum cases.
that's the only caveat
Without the default cases and a missed unhandled enum case, the code
might perform unforeseen things we might not want...
this would be a bug that needs fixing
As of today, we never run through the removed default case,
so removing it is no functional change.
In the future, we never should run through a default case but
introduce the necessary specific case(s) to handle new functionality.
that's the fix
diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c
index b2845c5b8106..9345fed3bb37 100644
--- a/drivers/s390/scsi/zfcp_erp.c
+++ b/drivers/s390/scsi/zfcp_erp.c
@@ -151,9 +151,6 @@ static enum zfcp_erp_act_type zfcp_erp_handle_failed(
adapter, ZFCP_STATUS_COMMON_ERP_FAILED);
}
break;
- default:
- need = 0;
- break;
}
return need;
If you 'should' not run through this code path, doesn't it warrant a
WARN_ON() or something?
#include
enum foo { A, B };
int main(int argc, char *argv[]) {
enum foo f = argc - 1;
switch (f) {
case A: printf("A\n"); break;
case B: printf("B\n"); break;
default: printf("default\n"); break;
}
return 0;
}
$ gcc -Wswitch -Wall -g -o Wswitch Wswitch.c
Removing case B (while keeping default:) does not warn on build.
Removing case B and default: nicely warns on build.
Hopefully I haven't missed anything. From above, I conclude:
A runtime check would require the introduction of a default case.
Adding a default case would trade a static build warning for a runtime
WARN_ON(_ONCE) which only appears if one manages to get the code run
into the default case that should not happen.
I find the static build warning more helpful for future extensions
adding more value(s) to the enum. Without a default case, we always
getting a build warning for missing switch cases.
--
Mit freundlichen Gruessen / Kind regards
Steffen Maier
Linux on IBM Z Development
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH 01/23] zfcp: make DIX experimental, disabled, and independent of DIF
Hi Martin, On 11/21/2018 07:13 PM, Martin K. Petersen wrote: Sorry about the delay. Travel got in the way. No problem. BDI_CAP_STABLE_WRITES should take care of this. What's the configuration that fails? Apologies, if the commit description sounds unfair. I did not mean to blame anyone. It's just the collection of issues we saw in distros over the years. Some of the old issues might be fixed with above zfcp patch or common code changes. Unfortunately, I could not handle the DIX things we saw. I think, DIF by itself provides a lot of the protection benefit and was not affected by the encountered issues. We would like to give users an easy way to operate in such setup. I don't have a problem with zfcp having a parameter that affects the host protection mask, the other drivers do that too. However, these knobs exist exclusively for debugging and testing purposes. They are not something regular users should twiddle to switch features on or off. So DIF and DIX should always be enabled in the driver. And there is no point in ever operating without DIF enabled if the hardware is capable. Our long term plan is to make the new zfcp.dif (for DIF only) default to enabled once we got enough experience about zfcp stability in this mode. If there is a desire to disable DIX protection for whatever reason (legacy code doing bad things), do so using the block layer sysfs knobs. That's where the policy of whether to generate and verify protection information resides, not in the HBA driver. Yes, we came up with udev rules to set read_verify and write_generate to 0 in order to get DIF without DIX. However, this seems complicated for users, especially since we always have at least dm-multipath and maybe other dm targets such as LVM on top. The setting that matters is on the top level block device of some dm (or maybe mdraid) virtual block device stack. Getting this right, gets more complicated if there are also disks not attached through zfcp, and which may need different settings, so the udev rules would need somewhat involved matching. The new zfcp.dif parameter makes it simpler because the SCSI disk comes up with the desired limits and anything on top automatically inherits these block queue limits. There's one more important thing that has performance impact: We need to pack payload and protection data into the same queue of limited length. So for the worst case with DIX, we have to use half the size for sg_tablesize to get the other half for sg_prot_tablesize. This limits the maximum I/O request size and thus throughput. Using read_verify and write_generate does not change the tablesizes, as zfcp would still announce support for DIF and DIX. With the new zfcp.dif=1 and zfcp.dix=0, we can use the full sg_tablesize for payload data and sg_prot_tablesize=0. (The DIF "overhead" on the fibre still exists of course.) Are there other ways for accomplishing this which I'm not aware of? And if there are unaddressed issues in the I/O stack that prevents you from having integrity enabled, I'd prefer to know about them so they can be fixed rather than circumventing them through driver module parameter. Sure. -- Mit freundlichen Gruessen / Kind regards Steffen Maier Linux on IBM Z Development IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschaeftsfuehrung: Dirk Wittkopp Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
