Hi nab, I'm getting back to looking at this patchset, but wanted to just
discuss and understand this one first because all the kref ones are
similar. see below.
On 12/16/2013 12:52 PM, Nicholas A. Bellinger wrote:
> On Fri, 2013-12-13 at 15:58 -0800, Andy Grover wrote:
>> Use kref to handle reference counting
>>
>> Signed-off-by: Andy Grover <agrover@xxxxxxxxxx>
>> ---
>> drivers/target/target_core_alua.c | 37 ++++++++++++++++++++-----------------
>> include/target/target_core_base.h | 2 +-
>> 2 files changed, 21 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
>> index 2ac2f11..8c01ade 100644
>> --- a/drivers/target/target_core_alua.c
>> +++ b/drivers/target/target_core_alua.c
>> @@ -54,6 +54,16 @@ static LIST_HEAD(lu_gps_list);
>>
>> struct t10_alua_lu_gp *default_lu_gp;
>>
>> +static void release_alua_lu_gp(struct kref *ref)
>> +{
>> + struct t10_alua_lu_gp *lu_gp = container_of(ref, struct t10_alua_lu_gp, refcount);
>> +
>> + kmem_cache_free(t10_alua_lu_gp_cache, lu_gp);
>> +}
>> +
>> +#define get_alua_lu_gp(x) kref_get(&x->refcount)
>> +#define put_alua_lu_gp(x) kref_put(&x->refcount, release_alua_lu_gp)
>> +
>> /*
>> * REPORT_TARGET_PORT_GROUPS
>> *
>> @@ -898,8 +908,7 @@ int core_alua_do_port_transition(
>> local_lu_gp_mem = l_dev->dev_alua_lu_gp_mem;
>> spin_lock(&local_lu_gp_mem->lu_gp_mem_lock);
>> lu_gp = local_lu_gp_mem->lu_gp;
>> - atomic_inc(&lu_gp->lu_gp_ref_cnt);
>> - smp_mb__after_atomic_inc();
>> + get_alua_lu_gp(lu_gp);
>> spin_unlock(&local_lu_gp_mem->lu_gp_mem_lock);
>> /*
>> * For storage objects that are members of the 'default_lu_gp',
>> @@ -913,8 +922,8 @@ int core_alua_do_port_transition(
>> */
>> core_alua_do_transition_tg_pt(l_tg_pt_gp, l_port, l_nacl,
>> md_buf, new_state, explicit);
>> - atomic_dec(&lu_gp->lu_gp_ref_cnt);
>> - smp_mb__after_atomic_dec();
>> +
>> + put_alua_lu_gp(lu_gp);
>> kfree(md_buf);
>> return 0;
>> }
>> @@ -985,8 +994,7 @@ int core_alua_do_port_transition(
>> l_tg_pt_gp->tg_pt_gp_id, (explicit) ? "explicit" : "implicit",
>> core_alua_dump_state(new_state));
>>
>> - atomic_dec(&lu_gp->lu_gp_ref_cnt);
>> - smp_mb__after_atomic_dec();
>> + put_alua_lu_gp(lu_gp);
>> kfree(md_buf);
>> return 0;
>> }
>> @@ -1107,7 +1115,8 @@ core_alua_allocate_lu_gp(const char *name, int def_group)
>> INIT_LIST_HEAD(&lu_gp->lu_gp_node);
>> INIT_LIST_HEAD(&lu_gp->lu_gp_mem_list);
>> spin_lock_init(&lu_gp->lu_gp_lock);
>> - atomic_set(&lu_gp->lu_gp_ref_cnt, 0);
>> +
>> + kref_init(&lu_gp->refcount);
>>
>> if (def_group) {
>> lu_gp->lu_gp_id = alua_lu_gps_counter++;
>> @@ -1200,13 +1209,7 @@ void core_alua_free_lu_gp(struct t10_alua_lu_gp *lu_gp)
>> list_del(&lu_gp->lu_gp_node);
>> alua_lu_gps_count--;
>> spin_unlock(&lu_gps_lock);
>> - /*
>> - * Allow struct t10_alua_lu_gp * referenced by core_alua_get_lu_gp_by_name()
>> - * in target_core_configfs.c:target_core_store_alua_lu_gp() to be
>> - * released with core_alua_put_lu_gp_from_name()
>> - */
>> - while (atomic_read(&lu_gp->lu_gp_ref_cnt))
>> - cpu_relax();
>> +
>> /*
>> * Release reference to struct t10_alua_lu_gp * from all associated
>> * struct se_device.
>> @@ -1241,7 +1244,7 @@ void core_alua_free_lu_gp(struct t10_alua_lu_gp *lu_gp)
>> }
>> spin_unlock(&lu_gp->lu_gp_lock);
>>
>> - kmem_cache_free(t10_alua_lu_gp_cache, lu_gp);
>> + put_alua_lu_gp(lu_gp);
>> }
>>
> The assumption that it's safe to 'Release reference to struct
> t10_alua_lu_gp * from all associated struct device' below the original
> cpu_relax(), while there are still other process contexts doing their
> respective put_alua_lu_gp() is totally wrong.
The only other spot is core_alua_do_port_transition, afaics. I think if
it races with free_lu_gp, lu_gp will either be the old lu_gp (which no
longer will have anything on lu_gp_mem_list) or will be default_lu_gp.
If it's the old lu_gp then it iterates over an empty list, and then the
lu_gp gets finally freed by the put() at the bottom.
> Furthermore, allowing a configfs_group_ops->drop_item() to return while
> there are still active references from other process contexts means that
> the parent struct config_group is no longer referenced counted (eg:
> configfs child is removed), and introduces a whole host of potential
> bugs.
>
> So that said, NAK on this patch.
I think some of the other patches used drop_item() and thus were bad,
but in this one the existing code is already calling
core_alua_free_lu_gp() from release().
Thoughts?
Thanks in advance -- Regards -- Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
