[PATCH 0/2] target: make location of /var/target configurable

[PATCH 0/2] target: make location of /var/target configurable

[Lee Duncan]

These patches make the location of "/var/target" configurable,
though it still defauls to "/var/target".

This configuration is accomplished via the configfs
top-level target attribute "dbroot", i.e. dumping
out "/sys/kernel/config/target/dbroot" will normally
return "/var/target". Writing to this attribute
changes the loation where the kernel looks for the
target database.

** NOTE/QUESTION: no sanity checks are done on the path passed in,
   but it seems like *some* should be done. At least checking that
   it's an abosolute path (i.e. starts with '/')? Opinions?

Lee Duncan (2):
  Make target db location /var/targets configurable.
  Use the new "dbroot" configfs target attribute.

 drivers/target/target_core_alua.c     |  6 +++---
 drivers/target/target_core_configfs.c | 31 +++++++++++++++++++++++++++++++
 drivers/target/target_core_internal.h |  6 ++++++
 drivers/target/target_core_pr.c       |  2 +-
 4 files changed, 41 insertions(+), 4 deletions(-)

-- 
2.1.4

[PATCH 1/2] target: Make target db location configurable

[Lee Duncan]

This commit adds the read-write attribute "dbroot", in
the top-level CONFIGFS (core) target directory,
normally /sys/kernel/config/target. This attribute
defaults to "/var/target" but can be changed by
writing a new pathname string to it.

Target modules that care about the target database
root directory will be modified to use this
attribute in a future commit.
---
 drivers/target/target_core_configfs.c | 31 +++++++++++++++++++++++++++++++
 drivers/target/target_core_internal.h |  6 ++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 713c63d9681b..bfc5a8bb5778 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -99,6 +99,36 @@ static ssize_t target_core_item_version_show(struct config_item *item,
 
 CONFIGFS_ATTR_RO(target_core_item_, version);
 
+char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
+
+static ssize_t target_core_item_dbroot_show(struct config_item *item,
+					    char *page)
+{
+	return sprintf(page, "%s\n", db_root);
+}
+
+static ssize_t target_core_item_dbroot_store(struct config_item *item,
+					const char *page, size_t count)
+{
+	ssize_t read_bytes;
+
+	if (count > (DB_ROOT_LEN - 1)) {
+		pr_err("db_root count: %d exceeds DB_ROOT_LEN-1: %u\n",
+		       (int)count, DB_ROOT_LEN - 1);
+		return -EINVAL;
+	}
+
+	read_bytes = snprintf(db_root, DB_ROOT_LEN, "%s", page);
+	if (!read_bytes)
+		return -EINVAL;
+	if (db_root[read_bytes - 1] == '\n')
+		db_root[read_bytes - 1] = '\0';
+
+	return read_bytes;
+}
+
+CONFIGFS_ATTR(target_core_item_, dbroot);
+
 static struct target_fabric_configfs *target_core_get_fabric(
 	const char *name)
 {
@@ -249,6 +279,7 @@ static struct configfs_group_operations target_core_fabric_group_ops = {
  */
 static struct configfs_attribute *target_core_fabric_item_attrs[] = {
 	&target_core_item_attr_version,
+	&target_core_item_attr_dbroot,
 	NULL,
 };
 
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 040cf5202e54..c2a18b960c5d 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -156,4 +156,10 @@ void	target_stat_setup_mappedlun_default_groups(struct se_lun_acl *);
 /* target_core_xcopy.c */
 extern struct se_portal_group xcopy_pt_tpg;
 
+/* target_core_configfs.c */
+#define DB_ROOT_LEN		4096
+#define	DB_ROOT_DEFAULT		"/var/target"
+
+extern char db_root[];
+
 #endif /* TARGET_CORE_INTERNAL_H */
-- 
2.1.4

Re: [PATCH 1/2] target: Make target db location configurable

[Johannes Thumshirn]

On 2016-03-31 20:05, Lee Duncan wrote:
> This commit adds the read-write attribute "dbroot", in
> the top-level CONFIGFS (core) target directory,
> normally /sys/kernel/config/target. This attribute
> defaults to "/var/target" but can be changed by
> writing a new pathname string to it.
> 
> Target modules that care about the target database
> root directory will be modified to use this
> attribute in a future commit.

You forgot to add your Signed-off-by

Otherwise

Reviewed-by: Johannnes Thumshirn <jthumshirn@suse.de>

[PATCH 2/2] target: use new "dbroot" target attribute

[Lee Duncan]

This commit updates the target core ALUA and PR
modules to use the new "dbroot" attribute instead
of assuming the target database is in "/var/target".
---
 drivers/target/target_core_alua.c | 6 +++---
 drivers/target/target_core_pr.c   | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index 49aba4a31747..22a6a9b18a86 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -932,7 +932,7 @@ static int core_alua_update_tpg_primary_metadata(
 			tg_pt_gp->tg_pt_gp_alua_access_status);
 
 	snprintf(path, ALUA_METADATA_PATH_LEN,
-		"/var/target/alua/tpgs_%s/%s", &wwn->unit_serial[0],
+		"%s/alua/tpgs_%s/%s", db_root, &wwn->unit_serial[0],
 		config_item_name(&tg_pt_gp->tg_pt_gp_group.cg_item));
 
 	rc = core_alua_write_tpg_metadata(path, md_buf, len);
@@ -1275,8 +1275,8 @@ static int core_alua_update_tpg_secondary_metadata(struct se_lun *lun)
 			atomic_read(&lun->lun_tg_pt_secondary_offline),
 			lun->lun_tg_pt_secondary_stat);
 
-	snprintf(path, ALUA_METADATA_PATH_LEN, "/var/target/alua/%s/%s/lun_%llu",
-			se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
+	snprintf(path, ALUA_METADATA_PATH_LEN, "%s/alua/%s/%s/lun_%llu",
+		 	db_root, se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
 			lun->unpacked_lun);
 
 	rc = core_alua_write_tpg_metadata(path, md_buf, len);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index b1795735eafc..47463c99c318 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1985,7 +1985,7 @@ static int __core_scsi3_write_aptpl_to_file(
 		return -EMSGSIZE;
 	}
 
-	snprintf(path, 512, "/var/target/pr/aptpl_%s", &wwn->unit_serial[0]);
+	snprintf(path, 512, "%s/pr/aptpl_%s", db_root, &wwn->unit_serial[0]);
 	file = filp_open(path, flags, 0600);
 	if (IS_ERR(file)) {
 		pr_err("filp_open(%s) for APTPL metadata"
-- 
2.1.4

Re: [PATCH 2/2] target: use new "dbroot" target attribute

[Johannes Thumshirn]

On 2016-03-31 20:05, Lee Duncan wrote:
> This commit updates the target core ALUA and PR
> modules to use the new "dbroot" attribute instead
> of assuming the target database is in "/var/target".

Same goes for this one,

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
as soon as it has a Signed-off-by line

Re: [PATCH 2/2] target: use new "dbroot" target attribute

[Lee Duncan]

On 04/01/2016 12:58 AM, Johannes Thumshirn wrote:
> On 2016-03-31 20:05, Lee Duncan wrote:
>> This commit updates the target core ALUA and PR
>> modules to use the new "dbroot" attribute instead
>> of assuming the target database is in "/var/target".
> 
> Same goes for this one,
> 
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> as soon as it has a Signed-off-by line

Thanks Johannes.

I will wait to see if there are any other comments, then resubmit v2.
-- 
Lee Duncan
SUSE Labs

Re: [PATCH 2/2] target: use new "dbroot" target attribute

[Andy Grover]

On 04/01/2016 11:01 AM, Lee Duncan wrote:
> On 04/01/2016 12:58 AM, Johannes Thumshirn wrote:
>> On 2016-03-31 20:05, Lee Duncan wrote:
>>> This commit updates the target core ALUA and PR
>>> modules to use the new "dbroot" attribute instead
>>> of assuming the target database is in "/var/target".
>>
>> Same goes for this one,
>>
>> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
>> as soon as it has a Signed-off-by line
>
> Thanks Johannes.
>
> I will wait to see if there are any other comments, then resubmit v2.

Seems fine to me, too.

So, if not /var/target, where do you recommend we be pointing this to?

Re: [PATCH 2/2] target: use new "dbroot" target attribute

[Lee Duncan]

On 04/01/2016 11:18 AM, Andy Grover wrote:
> On 04/01/2016 11:01 AM, Lee Duncan wrote:
>> On 04/01/2016 12:58 AM, Johannes Thumshirn wrote:
>>> On 2016-03-31 20:05, Lee Duncan wrote:
>>>> This commit updates the target core ALUA and PR
>>>> modules to use the new "dbroot" attribute instead
>>>> of assuming the target database is in "/var/target".
>>>
>>> Same goes for this one,
>>>
>>> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
>>> as soon as it has a Signed-off-by line
>>
>> Thanks Johannes.
>>
>> I will wait to see if there are any other comments, then resubmit v2.
> 
> Seems fine to me, too.

Thank you for your review.

> 
> So, if not /var/target, where do you recommend we be pointing this to?
> 

Good question!

For testing, I put it in /etc/target/target_db. But /etc is supposed to
be for configuration data.

Part of my problem in picking a place was that there seems to be two
different kinds of data there: policy, and state.

Since it was not my intention to sort that out, I just picked the /etc
location mentioned and verified it could work.

Re: [PATCH 0/2] target: make location of /var/target configurable

[Nicholas A. Bellinger]

On Thu, 2016-03-31 at 11:05 -0700, Lee Duncan wrote:
> These patches make the location of "/var/target" configurable,
> though it still defauls to "/var/target".
> 
> This configuration is accomplished via the configfs
> top-level target attribute "dbroot", i.e. dumping
> out "/sys/kernel/config/target/dbroot" will normally
> return "/var/target". Writing to this attribute
> changes the loation where the kernel looks for the
> target database.
> 
> ** NOTE/QUESTION: no sanity checks are done on the path passed in,
>    but it seems like *some* should be done. At least checking that
>    it's an abosolute path (i.e. starts with '/')? Opinions?
> 

Wrt to sanity checking db_root at configfs attribute store time, how
about doing a filp_open() + S_DIR(f_inode->imode) + filp_close() of the
requested path to verify it's really a directory..?

Also, it would probably be a good idea to limit when db_root can be
changed.  Eg, only allow db_root to be changed when no active target
fabric drivers have been registered (list_empty(g_tf_list)), and require
userspace to set a different db_root after modprobe target_core_mod
completes, but before any fabric drivers are loaded.

Re: [PATCH 0/2] target: make location of /var/target configurable

[Lee Duncan]

On 04/02/2016 08:36 PM, Nicholas A. Bellinger wrote:
> On Thu, 2016-03-31 at 11:05 -0700, Lee Duncan wrote:
>> These patches make the location of "/var/target" configurable,
>> though it still defauls to "/var/target".
>>
>> This configuration is accomplished via the configfs
>> top-level target attribute "dbroot", i.e. dumping
>> out "/sys/kernel/config/target/dbroot" will normally
>> return "/var/target". Writing to this attribute
>> changes the loation where the kernel looks for the
>> target database.
>>
>> ** NOTE/QUESTION: no sanity checks are done on the path passed in,
>>    but it seems like *some* should be done. At least checking that
>>    it's an abosolute path (i.e. starts with '/')? Opinions?
>>
> 
> Wrt to sanity checking db_root at configfs attribute store time, how
> about doing a filp_open() + S_DIR(f_inode->imode) + filp_close() of the
> requested path to verify it's really a directory..?

That seems reasonable. I will try that out and add it assuming it works. :)

> 
> Also, it would probably be a good idea to limit when db_root can be
> changed.  Eg, only allow db_root to be changed when no active target
> fabric drivers have been registered (list_empty(g_tf_list)), and require
> userspace to set a different db_root after modprobe target_core_mod
> completes, but before any fabric drivers are loaded.
> 

I will also try that.

Would it be worthwhile to have a module parameter for target_core_mod to
set the root, so it could be accomplished at the right time? (As much as
I like playing with configfs.)

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

-- 
Lee Duncan
SUSE Labs

Re: [PATCH 0/2] target: make location of /var/target configurable

[Nicholas A. Bellinger]

On Mon, 2016-04-04 at 18:15 -0700, Lee Duncan wrote:
> On 04/02/2016 08:36 PM, Nicholas A. Bellinger wrote:
> > On Thu, 2016-03-31 at 11:05 -0700, Lee Duncan wrote:
> >> These patches make the location of "/var/target" configurable,
> >> though it still defauls to "/var/target".
> >>
> >> This configuration is accomplished via the configfs
> >> top-level target attribute "dbroot", i.e. dumping
> >> out "/sys/kernel/config/target/dbroot" will normally
> >> return "/var/target". Writing to this attribute
> >> changes the loation where the kernel looks for the
> >> target database.
> >>
> >> ** NOTE/QUESTION: no sanity checks are done on the path passed in,
> >>    but it seems like *some* should be done. At least checking that
> >>    it's an abosolute path (i.e. starts with '/')? Opinions?
> >>
> > 
> > Wrt to sanity checking db_root at configfs attribute store time, how
> > about doing a filp_open() + S_DIR(f_inode->imode) + filp_close() of the
> > requested path to verify it's really a directory..?
> 
> That seems reasonable. I will try that out and add it assuming it works. :)
> 
> > 
> > Also, it would probably be a good idea to limit when db_root can be
> > changed.  Eg, only allow db_root to be changed when no active target
> > fabric drivers have been registered (list_empty(g_tf_list)), and require
> > userspace to set a different db_root after modprobe target_core_mod
> > completes, but before any fabric drivers are loaded.
> > 
> 
> I will also try that.
> 
> Would it be worthwhile to have a module parameter for target_core_mod to
> set the root, so it could be accomplished at the right time? (As much as
> I like playing with configfs.)
> 

target_core_mod + related drivers have been able to avoid mod params
thus far, so unless there's a reason why it can be done with a configfs
attribute (eg: it breaks existing userspace somehow), I'd prefer to
avoid that unless it's really necessary.

[PATCHv2 0/2] target: make location of /var/targets configurable

[Lee Duncan]

These patches make the location of "/var/target" configurable,
though it still defauls to "/var/target".

This "target database directory" can only be changed
after the target_core_mod loads but before any
fabric drivers are loaded, and must be the pathname
of an existing directory.

This configuration is accomplished via the configfs
top-level target attribute "dbroot", i.e. dumping
out "/sys/kernel/config/target/dbroot" will normally
return "/var/target". Writing to this attribute
changes the loation where the kernel looks for the
target database.

The first patch creates this configurable value for
the "dbroot", and the second patch modifies users
of this directory to use this new attribute.

Changes from v1:
 * Only allow changing target DB root before it
   can be used by others
 * Validate that new DB root is a valid directory

Lee Duncan (2):
  target: make target db location configurable
  target: use new "dbroot" target attribute

 drivers/target/target_core_alua.c     |  6 ++---
 drivers/target/target_core_configfs.c | 51 +++++++++++++++++++++++++++++++++++
 drivers/target/target_core_internal.h |  6 +++++
 drivers/target/target_core_pr.c       |  2 +-
 4 files changed, 61 insertions(+), 4 deletions(-)

-- 
2.1.4

[PATCH 2/2] target: use new "dbroot" target attribute

[Lee Duncan]

This commit updates the target core ALUA and PR
modules to use the new "dbroot" attribute instead
of assuming the target database is in "/var/target".

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/target/target_core_alua.c | 6 +++---
 drivers/target/target_core_pr.c   | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index 49aba4a31747..4c82bbe19003 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -932,7 +932,7 @@ static int core_alua_update_tpg_primary_metadata(
 			tg_pt_gp->tg_pt_gp_alua_access_status);
 
 	snprintf(path, ALUA_METADATA_PATH_LEN,
-		"/var/target/alua/tpgs_%s/%s", &wwn->unit_serial[0],
+		"%s/alua/tpgs_%s/%s", db_root, &wwn->unit_serial[0],
 		config_item_name(&tg_pt_gp->tg_pt_gp_group.cg_item));
 
 	rc = core_alua_write_tpg_metadata(path, md_buf, len);
@@ -1275,8 +1275,8 @@ static int core_alua_update_tpg_secondary_metadata(struct se_lun *lun)
 			atomic_read(&lun->lun_tg_pt_secondary_offline),
 			lun->lun_tg_pt_secondary_stat);
 
-	snprintf(path, ALUA_METADATA_PATH_LEN, "/var/target/alua/%s/%s/lun_%llu",
-			se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
+	snprintf(path, ALUA_METADATA_PATH_LEN, "%s/alua/%s/%s/lun_%llu",
+			db_root, se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
 			lun->unpacked_lun);
 
 	rc = core_alua_write_tpg_metadata(path, md_buf, len);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index b1795735eafc..47463c99c318 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1985,7 +1985,7 @@ static int __core_scsi3_write_aptpl_to_file(
 		return -EMSGSIZE;
 	}
 
-	snprintf(path, 512, "/var/target/pr/aptpl_%s", &wwn->unit_serial[0]);
+	snprintf(path, 512, "%s/pr/aptpl_%s", db_root, &wwn->unit_serial[0]);
 	file = filp_open(path, flags, 0600);
 	if (IS_ERR(file)) {
 		pr_err("filp_open(%s) for APTPL metadata"
-- 
2.1.4

[PATCH 2/2] target: use new "dbroot" target attribute

[Lee Duncan]

This commit updates the target core ALUA and PR
modules to use the new "dbroot" attribute instead
of assuming the target database is in "/var/target".

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/target/target_core_alua.c | 6 +++---
 drivers/target/target_core_pr.c   | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index 49aba4a31747..4c82bbe19003 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -932,7 +932,7 @@ static int core_alua_update_tpg_primary_metadata(
 			tg_pt_gp->tg_pt_gp_alua_access_status);
 
 	snprintf(path, ALUA_METADATA_PATH_LEN,
-		"/var/target/alua/tpgs_%s/%s", &wwn->unit_serial[0],
+		"%s/alua/tpgs_%s/%s", db_root, &wwn->unit_serial[0],
 		config_item_name(&tg_pt_gp->tg_pt_gp_group.cg_item));
 
 	rc = core_alua_write_tpg_metadata(path, md_buf, len);
@@ -1275,8 +1275,8 @@ static int core_alua_update_tpg_secondary_metadata(struct se_lun *lun)
 			atomic_read(&lun->lun_tg_pt_secondary_offline),
 			lun->lun_tg_pt_secondary_stat);
 
-	snprintf(path, ALUA_METADATA_PATH_LEN, "/var/target/alua/%s/%s/lun_%llu",
-			se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
+	snprintf(path, ALUA_METADATA_PATH_LEN, "%s/alua/%s/%s/lun_%llu",
+			db_root, se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
 			lun->unpacked_lun);
 
 	rc = core_alua_write_tpg_metadata(path, md_buf, len);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index b1795735eafc..47463c99c318 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1985,7 +1985,7 @@ static int __core_scsi3_write_aptpl_to_file(
 		return -EMSGSIZE;
 	}
 
-	snprintf(path, 512, "/var/target/pr/aptpl_%s", &wwn->unit_serial[0]);
+	snprintf(path, 512, "%s/pr/aptpl_%s", db_root, &wwn->unit_serial[0]);
 	file = filp_open(path, flags, 0600);
 	if (IS_ERR(file)) {
 		pr_err("filp_open(%s) for APTPL metadata"
-- 
2.1.4

Re: [PATCH 2/2] target: use new "dbroot" target attribute

[Hannes Reinecke]

On 04/13/2016 10:25 PM, Lee Duncan wrote:
> This commit updates the target core ALUA and PR
> modules to use the new "dbroot" attribute instead
> of assuming the target database is in "/var/target".
> 
> Signed-off-by: Lee Duncan <lduncan@suse.com>
> ---
>  drivers/target/target_core_alua.c | 6 +++---
>  drivers/target/target_core_pr.c   | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)