* Nicholas A. Bellinger | 2011-11-28 02:50:45 [-0800]:
>Hi again Sebastian,
>
>I like this cleanup using se_device->se_dev_udev_path instead of
>internal struct member usage for iblock, but this will currently break
>userspace during iblock device setup when returning -EINVAL for existing
>iblock_set_configfs_dev_params() usage.
>
>So I'd like to avoid this for the moment as we still expect all backend
>devices to use some form of target/core/$HBA/$DEV/control input, and
>while making a special case for IBLOCK in userspace may be a future
>option, it's not worth the pain to userspace breakage w/ this patch atm.
Okay, I wasn't aware that I'm breaking something.
>The other option here would be to return non exception status when using
>target/core/$HBA/$DEV/control for backend drivers (like IBLOCK) that
>would no longer provide se_subsystem_api->set_configfs_dev_params() from
>target_core_store_dev_control() usage. This would still expect to fail
>during target_core_store_dev_enable() -> check_configfs_dev_params() if
>udev_path has not been set via se_device->se_dev_udev_path, so returning
>non exception status here from legacy control attribute input should be
>enough to make existing userspace happy with iblock backend.
Yeah but not returning an error + doing nothing and exploding later
duing enable is kinda wrong I think. What about this:
>From 48b67d6cc07c2397736c06dba9b28bd959022e96 Mon Sep 17 00:00:00 2001
From: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
Date: Thu, 24 Nov 2011 14:01:26 +0100
Subject: [PATCH] target/iblock: use se_dev_udev_path instead of ibd_udev_path
This is probably a leftover from a rework. According to the wiki
| echo iblock_major=254,iblock_minor=2 > $TARGET/iblock_0/lvm_test0/control
should be issued. However the code matches for udev_path and force.
Since we have already udev_path handling in target_core_configfs lets
use it. I remove the major/minor thingy since there is no evidence that
this is used.
This also provides compatibility for old users of control interface.
Once they are gone and no control user comes along, then this field can
be removed.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
---
drivers/target/target_core_configfs.c | 3 +-
drivers/target/target_core_iblock.c | 45 ++++++++++++--------------------
drivers/target/target_core_iblock.h | 3 --
drivers/target/target_core_internal.h | 3 ++
4 files changed, 22 insertions(+), 32 deletions(-)
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 3f75f1b..d7cce7b 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1740,7 +1740,7 @@ static ssize_t target_core_show_dev_udev_path(void *p, char *page)
return snprintf(page, PAGE_SIZE, "%s\n", se_dev->se_dev_udev_path);
}
-static ssize_t target_core_store_dev_udev_path(
+ssize_t target_core_store_dev_udev_path(
void *p,
const char *page,
size_t count)
@@ -1770,6 +1770,7 @@ static ssize_t target_core_store_dev_udev_path(
return read_bytes;
}
+EXPORT_SYMBOL_GPL(target_core_store_dev_udev_path);
static struct target_core_configfs_attribute target_core_attr_dev_udev_path = {
.attr = { .ca_owner = THIS_MODULE,
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index a71cf88..cef6337 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -44,6 +44,7 @@
#include <target/target_core_backend.h>
#include "target_core_iblock.h"
+#include "target_core_internal.h"
static struct se_subsystem_api iblock_template;
@@ -135,13 +136,14 @@ static struct se_device *iblock_create_virtdevice(
}
pr_debug("IBLOCK: Created bio_set()\n");
/*
- * iblock_check_configfs_dev_params() ensures that ib_dev->ibd_udev_path
- * must already have been set in order for echo 1 > $HBA/$DEV/enable to run.
+ * iblock_check_configfs_dev_params() ensures that
+ * se_dev->se_dev_udev_path must already have been set in order for
+ * echo 1 > $HBA/$DEV/enable to run.
*/
pr_debug( "IBLOCK: Claiming struct block_device: %s\n",
- ib_dev->ibd_udev_path);
+ se_dev->se_dev_udev_path);
- bd = blkdev_get_by_path(ib_dev->ibd_udev_path,
+ bd = blkdev_get_by_path(se_dev->se_dev_udev_path,
FMODE_WRITE|FMODE_READ|FMODE_EXCL, ib_dev);
if (IS_ERR(bd)) {
ret = PTR_ERR(bd);
@@ -365,12 +367,11 @@ static void iblock_free_task(struct se_task *task)
}
enum {
- Opt_udev_path, Opt_force, Opt_err
+ Opt_udev_path, Opt_err
};
static match_table_t tokens = {
{Opt_udev_path, "udev_path=%s"},
- {Opt_force, "force=%d"},
{Opt_err, NULL}
};
@@ -378,11 +379,14 @@ static ssize_t iblock_set_configfs_dev_params(struct se_hba *hba,
struct se_subsystem_dev *se_dev,
const char *page, ssize_t count)
{
- struct iblock_dev *ib_dev = se_dev->se_dev_su_ptr;
char *orig, *ptr, *arg_p, *opts;
substring_t args[MAX_OPT_ARGS];
int ret = 0, token;
+ pr_err("%s/%s: deprecated 'control' interface for iblock\n",
+ config_item_name(&hba->hba_group.cg_item),
+ config_item_name(&se_dev->se_dev_group.cg_item));
+
opts = kstrdup(page, GFP_KERNEL);
if (!opts)
return -ENOMEM;
@@ -396,32 +400,20 @@ static ssize_t iblock_set_configfs_dev_params(struct se_hba *hba,
token = match_token(ptr, tokens, args);
switch (token) {
case Opt_udev_path:
- if (ib_dev->ibd_bd) {
- pr_err("Unable to set udev_path= while"
- " ib_dev->ibd_bd exists\n");
- ret = -EEXIST;
- goto out;
- }
arg_p = match_strdup(&args[0]);
if (!arg_p) {
ret = -ENOMEM;
break;
}
- snprintf(ib_dev->ibd_udev_path, SE_UDEV_PATH_LEN,
- "%s", arg_p);
+ ret = target_core_store_dev_udev_path(se_dev,
+ arg_p, strlen(arg_p));
kfree(arg_p);
- pr_debug("IBLOCK: Referencing UDEV path: %s\n",
- ib_dev->ibd_udev_path);
- ib_dev->ibd_flags |= IBDF_HAS_UDEV_PATH;
- break;
- case Opt_force:
break;
default:
break;
}
}
-out:
kfree(orig);
return (!ret) ? count : ret;
}
@@ -430,13 +422,10 @@ static ssize_t iblock_check_configfs_dev_params(
struct se_hba *hba,
struct se_subsystem_dev *se_dev)
{
- struct iblock_dev *ibd = se_dev->se_dev_su_ptr;
-
- if (!(ibd->ibd_flags & IBDF_HAS_UDEV_PATH)) {
- pr_err("Missing udev_path= parameters for IBLOCK\n");
+ if (!(se_dev->su_dev_flags & SDF_USING_UDEV_PATH)) {
+ pr_err("udev_path attribute has not been set before ENABLE\n");
return -EINVAL;
}
-
return 0;
}
@@ -453,9 +442,9 @@ static ssize_t iblock_show_configfs_dev_params(
if (bd)
bl += sprintf(b + bl, "iBlock device: %s",
bdevname(bd, buf));
- if (ibd->ibd_flags & IBDF_HAS_UDEV_PATH) {
+ if (se_dev->su_dev_flags & SDF_USING_UDEV_PATH) {
bl += sprintf(b + bl, " UDEV PATH: %s\n",
- ibd->ibd_udev_path);
+ se_dev->se_dev_udev_path);
} else
bl += sprintf(b + bl, "\n");
diff --git a/drivers/target/target_core_iblock.h b/drivers/target/target_core_iblock.h
index 5cf1860..a385405 100644
--- a/drivers/target/target_core_iblock.h
+++ b/drivers/target/target_core_iblock.h
@@ -12,10 +12,7 @@ struct iblock_req {
atomic_t ib_bio_err_cnt;
} ____cacheline_aligned;
-#define IBDF_HAS_UDEV_PATH 0x01
-
struct iblock_dev {
- unsigned char ibd_udev_path[SE_UDEV_PATH_LEN];
u32 ibd_flags;
struct bio_set *ibd_bio_set;
struct block_device *ibd_bd;
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 26f135e..6ff6708 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -120,4 +120,7 @@ void target_stat_setup_dev_default_groups(struct se_subsystem_dev *);
void target_stat_setup_port_default_groups(struct se_lun *);
void target_stat_setup_mappedlun_default_groups(struct se_lun_acl *);
+/* temp target_core_configfs.c */
+ssize_t target_core_store_dev_udev_path(void *p, const char *page,
+ size_t count);
#endif /* TARGET_CORE_INTERNAL_H */
--
1.7.7.3
? Once userland is defined to user new API it can be removed.
>--nab
Sebastian
--
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
