[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.