Re: [PATCH 7/7] target: core: check RTPI uniquity for enabled TPG

[Dmitry Bogdanov <d.bogdanov@yadro.com>]

On Sat, Oct 01, 2022 at 11:19:45AM -0500, michael.christie@oracle.com wrote:
> 
> On 9/29/22 7:02 PM, Mike Christie wrote:
> > On 9/6/22 10:45 AM, Dmitry Bogdanov wrote:
> >> Garantee uniquity of RTPI only for enabled target port groups.
> >> Allow any RPTI for disabled tpg until it is enabled.
> >>
> >> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
> >> ---
> >>  drivers/target/target_core_fabric_configfs.c | 29 +++++++++++++-
> >>  drivers/target/target_core_internal.h        |  2 +
> >>  drivers/target/target_core_tpg.c             | 40 +++++++++++++-------
> >>  3 files changed, 56 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
> >> index a34b5db4eec5..fc1b8f54fb54 100644
> >> --- a/drivers/target/target_core_fabric_configfs.c
> >> +++ b/drivers/target/target_core_fabric_configfs.c
> >> @@ -857,6 +857,7 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
> >>                                                 size_t count)
> >>  {
> >>      struct se_portal_group *se_tpg = to_tpg(item);
> >> +    struct se_portal_group *tpg;
> >>      int ret;
> >>      bool op;
> >>
> >> @@ -867,11 +868,37 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
> >>      if (se_tpg->enabled == op)
> >>              return count;
> >>
> >> +    spin_lock(&g_tpg_lock);
> >> +
> >> +    if (op) {
> >> +            tpg = core_get_tpg_by_rtpi(se_tpg->tpg_rtpi);
> >> +            if (tpg) {
> >> +                    spin_unlock(&g_tpg_lock);
> >> +
> >> +                    pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
> >> +                           se_tpg->se_tpg_tfo->fabric_name,
> >> +                           se_tpg->se_tpg_tfo->tpg_get_tag(tpg),
> >> +                           se_tpg->tpg_rtpi,
> >> +                           tpg->se_tpg_tfo->fabric_name,
> >> +                           tpg->se_tpg_tfo->tpg_get_tag(tpg));
> >> +                    return -EINVAL;
> >> +            }
> >> +    }
> >> +
> >> +    se_tpg->enabled |= 0x10; /* transient state */
> >
> > Just use a mutex and hold it the entire time if
> I was looking at the configfs code and am now not sure what the transient state
> is for. It looks like when doing a read or write configfs holds the buffer->mutex,
> so I don't think userspace would ever see the transient state.
> 
> Can you just drop it?

sysfs lock is per file, so 'enable' and 'rtpi' files can be written in
parallel. But after rewriting of the patchset, this part will be
dropped.