| Message ID | 20180404194703.31888-1-lduncan@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Apr 04, 2018 at 12:47:03PM -0700, Lee Duncan wrote: > The dbroot (target PR database root directory) is > configurable but default to /var/target, a historic > value. But the reason for adding configurability > was to move the target directory out of /var. This > is because the File Hierarchy Standard v3.0 mandates > that this "target" directory not be in /var. See > https://refspecs.linuxfoundation.org/FHS_3.0/fhs-3.0.pdf > > This change moves the default from /var/target to > /etc/target, but this value is still configurable, > so those wishing to continue to use /var/target > can still do so. This will break upgrades of all existing setups, so NAK.
On 04/05/2018 12:17 AM, Christoph Hellwig wrote: > On Wed, Apr 04, 2018 at 12:47:03PM -0700, Lee Duncan wrote: >> The dbroot (target PR database root directory) is >> configurable but default to /var/target, a historic >> value. But the reason for adding configurability >> was to move the target directory out of /var. This >> is because the File Hierarchy Standard v3.0 mandates >> that this "target" directory not be in /var. See >> https://refspecs.linuxfoundation.org/FHS_3.0/fhs-3.0.pdf >> >> This change moves the default from /var/target to >> /etc/target, but this value is still configurable, >> so those wishing to continue to use /var/target >> can still do so. > > This will break upgrades of all existing setups, so NAK. > Hi Christoph: I half expected this response, but here's the problem with the current approach ... The "dbroot" attribute is managed by target_core_mod, but this module is generally never loaded directly. Usually it is loaded because some other module (like ib_isert) is loaded, and it depends on target_core_mod. So if a user starts up rdma services, for example, they end up with a target_core_mod that will not allow dbroot to be changed. This is because the dbroot change rules will not allow a change when there are any children modules. [This problem will continue to grow as we get other target-mode drivers, such as nvme?] So setting the dbroot for all target driver requires some entity to load target_core_mod and set dbroot before any other module that uses target_core_mod loads. This means either we need a special service, at boot time, to load the core driver and set dbroot, so that all target drivers are treated equally, or we put the burden on every target driver to do this setup, which seems like an unreasonable duplication of effort. Bottom line, we need a better way to set dbroot. This is true no matter what the default value is for this attribute. I understand your reluctance to change dbroot, but I also see this leading to /var/target being the default location from now on unless we change it some time. I'm open to suggestions on how we could update this value and not break existing systems. A simple shell script could move things from /var/target to /etc/target, but bare kernels have no way to invoke "upgrade" scripts that I know of. Also, I think it's a bit unlikely that anyone will still be using /var/target, since targetcli-fb has been setting the target root to /etc/target for a while now, and the old targetcli has been deprecated. (It's the only app I know that hard-codes the old location.) For now, I believe a workable solution may be to add "dbroot" as a module parameter? If dbroot was a module param for target_core_mod, then dbroot=/etc/target could be passed in by default (via modprobe.d) for distros that wish to change the location. I will work on such a patch, but if you dislike this idea please feel free to save me some work, and let me know.
On Thu, Apr 05, 2018 at 10:06:35AM -0700, Lee Duncan wrote: > Also, I think it's a bit unlikely that anyone will still be using > /var/target, since targetcli-fb has been setting the target root to > /etc/target for a while now, and the old targetcli has been deprecated. > (It's the only app I know that hard-codes the old location.) That is a a good point. How about having a search path in the kernel? Try the configs attribute first if one is set, then /etc/target, then /var/target?
On 04/05/2018 10:57 AM, Christoph Hellwig wrote: > On Thu, Apr 05, 2018 at 10:06:35AM -0700, Lee Duncan wrote: >> Also, I think it's a bit unlikely that anyone will still be using >> /var/target, since targetcli-fb has been setting the target root to >> /etc/target for a while now, and the old targetcli has been deprecated. >> (It's the only app I know that hard-codes the old location.) > > That is a a good point. How about having a search path in the kernel? > Try the configs attribute first if one is set, then /etc/target, then > /var/target? > Good suggestion. But the configfs path is only passed in optionally, and possibly much after initialization, so there's no way the code can check the configfs path at initialization time, when this decision is first made. How about if the code looks for /etc/target, then uses /var/target if /etc/target is not present? Then users can use the current sysfs logic to set the path to any other directory, if neither of these paths are to their liking? And users can always find out the current dbroot with the sysfs attribute.
On Thu, Apr 05, 2018 at 01:54:25PM -0700, Lee Duncan wrote: > How about if the code looks for /etc/target, then uses /var/target if > /etc/target is not present? Then users can use the current sysfs logic > to set the path to any other directory, if neither of these paths are to > their liking? > > And users can always find out the current dbroot with the sysfs attribute. Sounds fine to me.
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h index 1d5afc3ae017..34eccef975b7 100644 --- a/drivers/target/target_core_internal.h +++ b/drivers/target/target_core_internal.h @@ -165,7 +165,7 @@ extern struct se_portal_group xcopy_pt_tpg; /* target_core_configfs.c */ #define DB_ROOT_LEN 4096 -#define DB_ROOT_DEFAULT "/var/target" +#define DB_ROOT_DEFAULT "/etc/target" extern char db_root[];
The dbroot (target PR database root directory) is configurable but default to /var/target, a historic value. But the reason for adding configurability was to move the target directory out of /var. This is because the File Hierarchy Standard v3.0 mandates that this "target" directory not be in /var. See https://refspecs.linuxfoundation.org/FHS_3.0/fhs-3.0.pdf This change moves the default from /var/target to /etc/target, but this value is still configurable, so those wishing to continue to use /var/target can still do so. Signed-off-by: Lee Duncan <lduncan@suse.com> --- drivers/target/target_core_internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)