[PATCH][SCST]: Implementation of subdirectories sessions, luns, ini_group inside targets/<target_name>/target/.

[PATCH][SCST]: Implementation of subdirectories sessions, luns, ini_group inside targets/<target_name>/target/.

[Daniel Debonzi]

This patch implements the creation of the subdirectories sessions, luns, ini_group
inside targets/<target_name>/target/.

Also it makes some changes on scst_unregister since the scst_tgt can't just be
kfreed as long as it has a kobject embedded on it. Resuming, the scst_unregister
call kobject_put and on the kobject release functions there is a call to scst_release
(which is the same as scst_unregister was).

Signed-off-by: Daniel Debonzi <debonzi@linux.vnet.ibm.com>
Index: scst/include/scst.h
===================================================================
--- scst/include/scst.h	(revision 851)
+++ scst/include/scst.h	(working copy)
@@ -961,7 +961,10 @@ struct scst_tgt {
  	/* Name on the default security group ("Default_target_name") */
  	char *default_group_name;

-	struct kobject *tgt_kobj; /* kobject for this struct. */
+	struct kobject tgt_kobj; /* main kobject for this struct. */
+	struct kobject *tgt_sess_kobj;
+	struct kobject *tgt_luns_kobj;
+	struct kobject *tgt_ini_grp_kobj;
  };

  /* Hash size and hash fn for hash based lun translation */
Index: scst/src/scst_main.c
===================================================================
--- scst/src/scst_main.c	(revision 851)
+++ scst/src/scst_main.c	(working copy)
@@ -375,6 +375,10 @@ struct scst_tgt *scst_register(struct sc
  	mutex_unlock(&scst_mutex);
  	scst_resume_activity();

+	rc = scst_create_tgt_child_kobjs(tgt);
+	if (rc < 0)
+		goto out_child_kobjs_err;
+
  	PRINT_INFO("Target %s (%p) for template %s registered successfully",
  		target_name, tgt, vtt->name);

@@ -382,6 +386,11 @@ out:
  	TRACE_EXIT();
  	return tgt;

+out_child_kobjs_err:
+	scst_destroy_tgt_kobj(tgt);
+	TRACE_EXIT();
+	return NULL;
+
  out_kobj_err:
  	scst_cleanup_proc_target_entries(tgt);

@@ -416,6 +425,13 @@ static inline int test_sess_list(struct

  void scst_unregister(struct scst_tgt *tgt)
  {
+	scst_destroy_tgt_child_kobjs(tgt);
+	scst_destroy_tgt_kobj(tgt);
+}
+EXPORT_SYMBOL(scst_unregister);
+
+void scst_release(struct scst_tgt *tgt)
+{
  	struct scst_session *sess;
  	struct scst_tgt_template *vtt = tgt->tgtt;

@@ -452,7 +468,6 @@ again:
  	list_del(&tgt->tgt_list_entry);

  	scst_cleanup_proc_target_entries(tgt);
-	scst_destroy_tgt_kobj(tgt);

  	kfree(tgt->default_group_name);

@@ -468,8 +483,8 @@ again:

  	TRACE_EXIT();
  	return;
+
  }
-EXPORT_SYMBOL(scst_unregister);

  static int scst_susp_wait(bool interruptible)
  {
Index: scst/src/scst_priv.h
===================================================================
--- scst/src/scst_priv.h	(revision 851)
+++ scst/src/scst_priv.h	(working copy)
@@ -390,6 +390,12 @@ int scst_create_tgtt_kobj(struct scst_tg
  void scst_destroy_tgtt_kobj(struct scst_tgt_template *vtt);
  int scst_create_tgt_kobj(struct scst_tgt *tgt);
  void scst_destroy_tgt_kobj(struct scst_tgt *tgt);
+int scst_create_tgt_child_kobjs(struct scst_tgt *tgt);
+void scst_destroy_tgt_child_kobjs(struct scst_tgt *tgt);
+
+/* kobjects release functions */
+/*releases the scst_tgt sent to scst_unregister */
+void scst_release(struct scst_tgt *tgt);

  int scst_get_cdb_len(const uint8_t *cdb);

Index: scst/src/scst_sysfs.c
===================================================================
--- scst/src/scst_sysfs.c	(revision 851)
+++ scst/src/scst_sysfs.c	(working copy)
@@ -43,16 +43,31 @@ void scst_destroy_tgtt_kobj(struct scst_
  	kobject_put(vtt->tgtt_kobj);
  }

+void scst_tgt_release(struct kobject *kobj)
+{
+	struct scst_tgt *tgt;
+
+	TRACE_ENTRY();
+
+	tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
+	scst_release(tgt);
+
+	TRACE_EXIT();
+}
+
+struct kobj_type tgt_ktype = {
+	.release = scst_tgt_release,
+};
+
  int scst_create_tgt_kobj(struct scst_tgt *tgt)
  {
  	int retval = 0;

  	TRACE_ENTRY();

-	tgt->tgt_kobj = kobject_create_and_add(tgt->default_group_name,
-					       tgt->tgtt->tgtt_kobj);
-	if (!tgt->tgt_kobj)
-		retval = -EINVAL;
+	retval = kobject_init_and_add(&tgt->tgt_kobj, &tgt_ktype,
+				      tgt->tgtt->tgtt_kobj,
+				      tgt->default_group_name);

  	TRACE_EXIT_RES(retval);
  	return retval;
@@ -60,7 +75,50 @@ int scst_create_tgt_kobj(struct scst_tgt

  void scst_destroy_tgt_kobj(struct scst_tgt *tgt)
  {
-	kobject_put(tgt->tgt_kobj);
+	kobject_put(&tgt->tgt_kobj);
+}
+
+int scst_create_tgt_child_kobjs(struct scst_tgt *tgt)
+{
+	int retval = 0;
+
+	TRACE_ENTRY();
+
+	tgt->tgt_sess_kobj = kobject_create_and_add("sessions", &tgt->tgt_kobj);
+	if (!tgt->tgt_sess_kobj) {
+		retval = -EINVAL;
+		goto sess_kobj_err;
+	}
+
+	tgt->tgt_luns_kobj = kobject_create_and_add("luns", &tgt->tgt_kobj);
+	if (!tgt->tgt_luns_kobj) {
+		retval = -EINVAL;
+		goto luns_kobj_err;
+	}
+
+	tgt->tgt_ini_grp_kobj = kobject_create_and_add("ini_group",
+						       &tgt->tgt_kobj);
+	if (!tgt->tgt_ini_grp_kobj) {
+		retval = -EINVAL;
+		goto ini_grp_kobj_err;
+	}
+
+out:
+	TRACE_EXIT_RES(retval);
+	return retval;
+ini_grp_kobj_err:
+	kobject_put(tgt->tgt_luns_kobj);
+luns_kobj_err:
+	kobject_put(tgt->tgt_sess_kobj);
+sess_kobj_err:
+	goto out;
+}
+
+void scst_destroy_tgt_child_kobjs(struct scst_tgt *tgt)
+{
+	kobject_put(tgt->tgt_sess_kobj);
+	kobject_put(tgt->tgt_luns_kobj);
+	kobject_put(tgt->tgt_ini_grp_kobj);
  }

  static ssize_t scst_threads_show(struct kobject *kobj, struct kobj_attribute *attr,

Re: [PATCH][SCST]: Implementation of subdirectories sessions, luns, ini_group inside targets/<target_name>/target/.

[Vladislav Bolkhovitin]

Hi Daniel,

Daniel Debonzi, on 05/16/2009 01:41 AM wrote:
> This patch implements the creation of the subdirectories sessions, luns, ini_group
> inside targets/<target_name>/target/.
> 
> Also it makes some changes on scst_unregister since the scst_tgt can't just be
> kfreed as long as it has a kobject embedded on it. Resuming, the scst_unregister
> call kobject_put and on the kobject release functions there is a call to scst_release
> (which is the same as scst_unregister was).

Unfortunately, your patch still contain a whitespace in the beginning of 
each line. Do you use mouse to paste the patch in the message? I 
personally use "Copy" and "Paste" through menu. Copy from KScope's editor.

> Signed-off-by: Daniel Debonzi <debonzi@linux.vnet.ibm.com>

You forgot diffstat here.

Looking at your patch I realized that, since we export our internal 
objects, we need to decide their lifetime rules as well as decide some 
unified agreement for the names of functions and error recover (your 
patch has problems in this area). I suggest the following. Your thought 
are welcome.

1. All kobjects without attributes will be dynamic kobjects. This is the 
same as you did. Example is tgtt_kobj.

2. All kobjects with attributes will have dedicated kobj_type.

3. For ease of error recovery all sysfs content for each object will be 
created in a single function with name scst_create_object_name_sysfs(), 
like scst_create_tgt_sysfs()

4. All sysfs content of an object will also be cleaned by a single 
function. For simple dynamic kobjects it will be called 
scst_cleanup_object_name_sysfs(), like scst_cleanup_tgtt_sysfs(). For 
other objects it will be called 
scst_cleanup_object_name_sysfs_put_object_name(), like 
scst_cleanup_tgt_sysfs_put_tgt(). It will also put reference to the 
corresponding kobject and, hence, can destroy the whole object. This is 
why I chose so big and inconvenient name: to emphasize that the object 
can be destroyed after this function returned.

5. In the unregister functions the object will be fully cleaned as much 
as possible for its sysfs attributes to work and not oops.

I committed your patch with the above changes. Hopefully, now there is a 
simple and straightforward path to implement other SCST sysfs files and 
directories.

See also below.

> Index: scst/include/scst.h
> ===================================================================
> --- scst/include/scst.h	(revision 851)
> +++ scst/include/scst.h	(working copy)
> @@ -961,7 +961,10 @@ struct scst_tgt {
>   	/* Name on the default security group ("Default_target_name") */
>   	char *default_group_name;
> 
> -	struct kobject *tgt_kobj; /* kobject for this struct. */
> +	struct kobject tgt_kobj; /* main kobject for this struct. */
> +	struct kobject *tgt_sess_kobj;
> +	struct kobject *tgt_luns_kobj;
> +	struct kobject *tgt_ini_grp_kobj;
>   };
> 
>   /* Hash size and hash fn for hash based lun translation */
> Index: scst/src/scst_main.c
> ===================================================================
> --- scst/src/scst_main.c	(revision 851)
> +++ scst/src/scst_main.c	(working copy)
> @@ -375,6 +375,10 @@ struct scst_tgt *scst_register(struct sc
>   	mutex_unlock(&scst_mutex);
>   	scst_resume_activity();
> 
> +	rc = scst_create_tgt_child_kobjs(tgt);
> +	if (rc < 0)
> +		goto out_child_kobjs_err;
> +
>   	PRINT_INFO("Target %s (%p) for template %s registered successfully",
>   		target_name, tgt, vtt->name);
> 
> @@ -382,6 +386,11 @@ out:
>   	TRACE_EXIT();
>   	return tgt;
> 
> +out_child_kobjs_err:
> +	scst_destroy_tgt_kobj(tgt);
> +	TRACE_EXIT();
> +	return NULL;
> +
>   out_kobj_err:
>   	scst_cleanup_proc_target_entries(tgt);
> 
> @@ -416,6 +425,13 @@ static inline int test_sess_list(struct
> 
>   void scst_unregister(struct scst_tgt *tgt)
>   {
> +	scst_destroy_tgt_child_kobjs(tgt);
> +	scst_destroy_tgt_kobj(tgt);
> +}
> +EXPORT_SYMBOL(scst_unregister);
> +
> +void scst_release(struct scst_tgt *tgt)
> +{
>   	struct scst_session *sess;
>   	struct scst_tgt_template *vtt = tgt->tgtt;
> 
> @@ -452,7 +468,6 @@ again:
>   	list_del(&tgt->tgt_list_entry);
> 
>   	scst_cleanup_proc_target_entries(tgt);
> -	scst_destroy_tgt_kobj(tgt);
> 
>   	kfree(tgt->default_group_name);
> 
> @@ -468,8 +483,8 @@ again:
> 
>   	TRACE_EXIT();
>   	return;
> +
>   }
> -EXPORT_SYMBOL(scst_unregister);
> 
>   static int scst_susp_wait(bool interruptible)
>   {
> Index: scst/src/scst_priv.h
> ===================================================================
> --- scst/src/scst_priv.h	(revision 851)
> +++ scst/src/scst_priv.h	(working copy)
> @@ -390,6 +390,12 @@ int scst_create_tgtt_kobj(struct scst_tg
>   void scst_destroy_tgtt_kobj(struct scst_tgt_template *vtt);
>   int scst_create_tgt_kobj(struct scst_tgt *tgt);
>   void scst_destroy_tgt_kobj(struct scst_tgt *tgt);
> +int scst_create_tgt_child_kobjs(struct scst_tgt *tgt);
> +void scst_destroy_tgt_child_kobjs(struct scst_tgt *tgt);
> +
> +/* kobjects release functions */
> +/*releases the scst_tgt sent to scst_unregister */
> +void scst_release(struct scst_tgt *tgt);
> 
>   int scst_get_cdb_len(const uint8_t *cdb);
> 
> Index: scst/src/scst_sysfs.c
> ===================================================================
> --- scst/src/scst_sysfs.c	(revision 851)
> +++ scst/src/scst_sysfs.c	(working copy)
> @@ -43,16 +43,31 @@ void scst_destroy_tgtt_kobj(struct scst_
>   	kobject_put(vtt->tgtt_kobj);
>   }
> 
> +void scst_tgt_release(struct kobject *kobj)
> +{
> +	struct scst_tgt *tgt;
> +
> +	TRACE_ENTRY();
> +
> +	tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
> +	scst_release(tgt);
> +
> +	TRACE_EXIT();
> +}
> +
> +struct kobj_type tgt_ktype = {
> +	.release = scst_tgt_release,
> +};
> +
>   int scst_create_tgt_kobj(struct scst_tgt *tgt)
>   {
>   	int retval = 0;
> 
>   	TRACE_ENTRY();
> 
> -	tgt->tgt_kobj = kobject_create_and_add(tgt->default_group_name,
> -					       tgt->tgtt->tgtt_kobj);
> -	if (!tgt->tgt_kobj)
> -		retval = -EINVAL;
> +	retval = kobject_init_and_add(&tgt->tgt_kobj, &tgt_ktype,
> +				      tgt->tgtt->tgtt_kobj,
> +				      tgt->default_group_name);
> 
>   	TRACE_EXIT_RES(retval);
>   	return retval;
> @@ -60,7 +75,50 @@ int scst_create_tgt_kobj(struct scst_tgt
> 
>   void scst_destroy_tgt_kobj(struct scst_tgt *tgt)
>   {
> -	kobject_put(tgt->tgt_kobj);
> +	kobject_put(&tgt->tgt_kobj);
> +}
> +
> +int scst_create_tgt_child_kobjs(struct scst_tgt *tgt)
> +{
> +	int retval = 0;
> +
> +	TRACE_ENTRY();
> +
> +	tgt->tgt_sess_kobj = kobject_create_and_add("sessions", &tgt->tgt_kobj);
> +	if (!tgt->tgt_sess_kobj) {
> +		retval = -EINVAL;
> +		goto sess_kobj_err;
> +	}
> +
> +	tgt->tgt_luns_kobj = kobject_create_and_add("luns", &tgt->tgt_kobj);
> +	if (!tgt->tgt_luns_kobj) {
> +		retval = -EINVAL;
> +		goto luns_kobj_err;
> +	}
> +
> +	tgt->tgt_ini_grp_kobj = kobject_create_and_add("ini_group",
> +						       &tgt->tgt_kobj);
> +	if (!tgt->tgt_ini_grp_kobj) {
> +		retval = -EINVAL;
> +		goto ini_grp_kobj_err;
> +	}
> +
> +out:
> +	TRACE_EXIT_RES(retval);
> +	return retval;
> +ini_grp_kobj_err:
> +	kobject_put(tgt->tgt_luns_kobj);
> +luns_kobj_err:
> +	kobject_put(tgt->tgt_sess_kobj);
> +sess_kobj_err:
> +	goto out;

Better have empty lines between labels.

> +}
> +
> +void scst_destroy_tgt_child_kobjs(struct scst_tgt *tgt)
> +{
> +	kobject_put(tgt->tgt_sess_kobj);
> +	kobject_put(tgt->tgt_luns_kobj);
> +	kobject_put(tgt->tgt_ini_grp_kobj);
>   }
> 
>   static ssize_t scst_threads_show(struct kobject *kobj, struct kobj_attribute *attr,

Thanks,
Vlad

Re: [PATCH][SCST]: Implementation of subdirectories sessions, luns, ini_group inside targets/<target_name>/target/.

[Daniel Debonzi]

Hi Vlad,

Vladislav Bolkhovitin wrote:
> Hi Daniel,
> 
> Daniel Debonzi, on 05/16/2009 01:41 AM wrote:
>> This patch implements the creation of the subdirectories sessions, 
>> luns, ini_group
>> inside targets/<target_name>/target/.
>>
>> Also it makes some changes on scst_unregister since the scst_tgt can't 
>> just be
>> kfreed as long as it has a kobject embedded on it. Resuming, the 
>> scst_unregister
>> call kobject_put and on the kobject release functions there is a call 
>> to scst_release
>> (which is the same as scst_unregister was).
> 
> Unfortunately, your patch still contain a whitespace in the beginning of 
> each line. Do you use mouse to paste the patch in the message? I 
> personally use "Copy" and "Paste" through menu. Copy from KScope's editor.

Really?
I will check it out again. Sorry for that inconvenience

> 
>> Signed-off-by: Daniel Debonzi <debonzi@linux.vnet.ibm.com>
> 
> You forgot diffstat here.

Actually I don't know how to generate them.

> 
> Looking at your patch I realized that, since we export our internal 
> objects, we need to decide their lifetime rules as well as decide some 
> unified agreement for the names of functions and error recover (your 
> patch has problems in this area). I suggest the following. Your thought 
> are welcome.
> 
> 1. All kobjects without attributes will be dynamic kobjects. This is the 
> same as you did. Example is tgtt_kobj.
> 
> 2. All kobjects with attributes will have dedicated kobj_type.
> 
> 3. For ease of error recovery all sysfs content for each object will be 
> created in a single function with name scst_create_object_name_sysfs(), 
> like scst_create_tgt_sysfs()
> 
> 4. All sysfs content of an object will also be cleaned by a single 
> function. For simple dynamic kobjects it will be called 
> scst_cleanup_object_name_sysfs(), like scst_cleanup_tgtt_sysfs(). For 
> other objects it will be called 
> scst_cleanup_object_name_sysfs_put_object_name(), like 
> scst_cleanup_tgt_sysfs_put_tgt(). It will also put reference to the 
> corresponding kobject and, hence, can destroy the whole object. This is 
> why I chose so big and inconvenient name: to emphasize that the object 
> can be destroyed after this function returned.
> 
> 5. In the unregister functions the object will be fully cleaned as much 
> as possible for its sysfs attributes to work and not oops.
> 
> I committed your patch with the above changes. Hopefully, now there is a 
> simple and straightforward path to implement other SCST sysfs files and 
> directories.

I think we may have a problem here.
I agree with the name definitions and conventions but, unless I am missing something, there
is a problem on your 3th sentence.

My first attempt to write the now called scst_create_tgt_sysfs function was using exactly
your approach but I stuck on the point that if any kobj creation (sessions, luns, ini_grp)
fail after a successful creation of tgt->tgt_kobj we  have to "destroy" tgt_kobj on the error
cover part, but I could not found a way do that if not with kobject_put(tgt_kobj).
That would call the release function (scst_tgt_free on scst_sysfs.c) that would kfree(tgt)
on a wrong place. I believe used kobject_del instead because of it but I afaik it does not
cleanup the kobj but only removes the directory from the sysfs.

Searching for it now on the doc I found:
"""
kobject_del() which will unregister the kobject from sysfs.  This makes the
kobject "invisible", but it is not cleaned up, and the reference count of
the object is still the same.  At a later time call kobject_put() to finish
the cleanup of the memory associated with the kobject.
"""

I feel that we only can kfree(tgt) on the error recover part if the error happened
before the tgt_kobj creation. After its creation the kfree only can be done by the
kobject release function.

What do you think?

> 
> See also below.
[snip]
> Thanks,
> Vlad
> -- 

Regards,
Daniel Debonzi

Re: [PATCH][SCST]: Implementation of subdirectories sessions, luns, ini_group inside targets/<target_name>/target/.

[Vladislav Bolkhovitin]

Daniel Debonzi, on 05/20/2009 08:57 PM wrote:
> Hi Vlad,
> 
> Vladislav Bolkhovitin wrote:
>> Hi Daniel,
>>
>> Daniel Debonzi, on 05/16/2009 01:41 AM wrote:
>>> This patch implements the creation of the subdirectories sessions, 
>>> luns, ini_group
>>> inside targets/<target_name>/target/.
>>>
>>> Also it makes some changes on scst_unregister since the scst_tgt can't 
>>> just be
>>> kfreed as long as it has a kobject embedded on it. Resuming, the 
>>> scst_unregister
>>> call kobject_put and on the kobject release functions there is a call 
>>> to scst_release
>>> (which is the same as scst_unregister was).
>> Unfortunately, your patch still contain a whitespace in the beginning of 
>> each line. Do you use mouse to paste the patch in the message? I 
>> personally use "Copy" and "Paste" through menu. Copy from KScope's editor.
> 
> Really?
> I will check it out again. Sorry for that inconvenience
> 
>>> Signed-off-by: Daniel Debonzi <debonzi@linux.vnet.ibm.com>
>> You forgot diffstat here.
> 
> Actually I don't know how to generate them.

$ diffstat patch

>> Looking at your patch I realized that, since we export our internal 
>> objects, we need to decide their lifetime rules as well as decide some 
>> unified agreement for the names of functions and error recover (your 
>> patch has problems in this area). I suggest the following. Your thought 
>> are welcome.
>>
>> 1. All kobjects without attributes will be dynamic kobjects. This is the 
>> same as you did. Example is tgtt_kobj.
>>
>> 2. All kobjects with attributes will have dedicated kobj_type.
>>
>> 3. For ease of error recovery all sysfs content for each object will be 
>> created in a single function with name scst_create_object_name_sysfs(), 
>> like scst_create_tgt_sysfs()
>>
>> 4. All sysfs content of an object will also be cleaned by a single 
>> function. For simple dynamic kobjects it will be called 
>> scst_cleanup_object_name_sysfs(), like scst_cleanup_tgtt_sysfs(). For 
>> other objects it will be called 
>> scst_cleanup_object_name_sysfs_put_object_name(), like 
>> scst_cleanup_tgt_sysfs_put_tgt(). It will also put reference to the 
>> corresponding kobject and, hence, can destroy the whole object. This is 
>> why I chose so big and inconvenient name: to emphasize that the object 
>> can be destroyed after this function returned.
>>
>> 5. In the unregister functions the object will be fully cleaned as much 
>> as possible for its sysfs attributes to work and not oops.
>>
>> I committed your patch with the above changes. Hopefully, now there is a 
>> simple and straightforward path to implement other SCST sysfs files and 
>> directories.
> 
> I think we may have a problem here.
> I agree with the name definitions and conventions but, unless I am missing something, there
> is a problem on your 3th sentence.
> 
> My first attempt to write the now called scst_create_tgt_sysfs function was using exactly
> your approach but I stuck on the point that if any kobj creation (sessions, luns, ini_grp)
> fail after a successful creation of tgt->tgt_kobj we  have to "destroy" tgt_kobj on the error
> cover part, but I could not found a way do that if not with kobject_put(tgt_kobj).
> That would call the release function (scst_tgt_free on scst_sysfs.c) that would kfree(tgt)
> on a wrong place. I believe used kobject_del instead because of it but I afaik it does not
> cleanup the kobj but only removes the directory from the sysfs.
> 
> Searching for it now on the doc I found:
> """
> kobject_del() which will unregister the kobject from sysfs.  This makes the
> kobject "invisible", but it is not cleaned up, and the reference count of
> the object is still the same.  At a later time call kobject_put() to finish
> the cleanup of the memory associated with the kobject.
> """
> 
> I feel that we only can kfree(tgt) on the error recover part if the error happened
> before the tgt_kobj creation. After its creation the kfree only can be done by the
> kobject release function.
> 
> What do you think?

That problem was the one on which I spent the most time. Originally I 
solved it by simply not calling kobject_put() on the error path, only 
kobject_del(). Since we have tgt_kobj embedded it worked on the SCST 
level pretty well. But I've just realized that it leaded to memory 
leak(s) on the kobject level. So, I reconsidered it and now the code 
looks like:

static void scst_tgt_free(struct kobject *kobj)
{
	struct scst_tgt *tgt;

	TRACE_ENTRY();

	tgt = container_of(kobj, struct scst_tgt, tgt_kobj);

	if (tgt->tgt_sysfs_initialized)
		kfree(tgt);

	TRACE_EXIT();
	return;
}

static struct kobj_type tgt_ktype = {
	.release = scst_tgt_free,
};

int scst_create_tgt_sysfs(struct scst_tgt *tgt)
{
	int retval;

	TRACE_ENTRY();

	retval = kobject_init_and_add(&tgt->tgt_kobj, &tgt_ktype,
			tgt->tgtt->tgtt_kobj, tgt->tgt_name);
	if (retval != 0) {
		PRINT_ERROR("Can't add tgt %s to sysfs", tgt->tgt_name);
		goto out;
	}

	tgt->tgt_sess_kobj = kobject_create_and_add("sessions",
				&tgt->tgt_kobj);
	if (!tgt->tgt_sess_kobj) {
		PRINT_ERROR("Can't create sess kobj for tgt %s",
				tgt->tgt_name);
		goto out_sess_obj_err;
	}

	tgt->tgt_luns_kobj = kobject_create_and_add("luns",
				&tgt->tgt_kobj);
	if (!tgt->tgt_luns_kobj) {
		PRINT_ERROR("Can't create luns kobj for tgt %s",
				tgt->tgt_name);
		goto luns_kobj_err;
	}

	tgt->tgt_ini_grp_kobj = kobject_create_and_add("ini_group",
						    &tgt->tgt_kobj);
	if (!tgt->tgt_ini_grp_kobj) {
		PRINT_ERROR("Can't create ini_grp kobj for tgt %s",
			tgt->tgt_name);
		goto ini_grp_kobj_err;
	}

	tgt->tgt_sysfs_initialized = 1;

out:
	TRACE_EXIT_RES(retval);
	return retval;

ini_grp_kobj_err:
	kobject_del(tgt->tgt_luns_kobj);
	kobject_put(tgt->tgt_luns_kobj);

luns_kobj_err:
	kobject_del(tgt->tgt_sess_kobj);
	kobject_put(tgt->tgt_sess_kobj);

out_sess_obj_err:
	kobject_del(&tgt->tgt_kobj);
	kobject_put(&tgt->tgt_kobj);
	retval = -ENOMEM;
	goto out;
}

I added tgt_sysfs_initialized flag to let scst_tgt_free() know if it 
called normally or on the error processing path.

Should it work now?

You can also notice that there is kobject_del() before each the latest 
on the SCST level kobject_put(). I did that to hide those objects ASAP. 
Otherwise seems it is possible that, if any of them has external 
reference, subsequent attempt to register object with the same name 
(session, for instance) can fail. Don't know if it's possible on 
practice, but better to be overinsured.

>> See also below.
> [snip]
>> Thanks,
>> Vlad
>> -- 
> 
> Regards,
> Daniel Debonzi
> --
> 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
> 

Re: [PATCH][SCST]: Implementation of subdirectories sessions, luns, ini_group inside targets/<target_name>/target/.

[Daniel Debonzi]

Vladislav Bolkhovitin wrote:
> Daniel Debonzi, on 05/20/2009 08:57 PM wrote:
[snip]

>>>> Signed-off-by: Daniel Debonzi <debonzi@linux.vnet.ibm.com>
>>> You forgot diffstat here.
>>
>> Actually I don't know how to generate them.
> 
> $ diffstat patch

Uhmm! for some reason I don't have it on my sistem. Will check it out.

>>> Looking at your patch I realized that, since we export our internal 
>>> objects, we need to decide their lifetime rules as well as decide 
>>> some unified agreement for the names of functions and error recover 
>>> (your patch has problems in this area). I suggest the following. Your 
>>> thought are welcome.
>>>
>>> 1. All kobjects without attributes will be dynamic kobjects. This is 
>>> the same as you did. Example is tgtt_kobj.
>>>
>>> 2. All kobjects with attributes will have dedicated kobj_type.
>>>
>>> 3. For ease of error recovery all sysfs content for each object will 
>>> be created in a single function with name 
>>> scst_create_object_name_sysfs(), like scst_create_tgt_sysfs()
>>>
>>> 4. All sysfs content of an object will also be cleaned by a single 
>>> function. For simple dynamic kobjects it will be called 
>>> scst_cleanup_object_name_sysfs(), like scst_cleanup_tgtt_sysfs(). For 
>>> other objects it will be called 
>>> scst_cleanup_object_name_sysfs_put_object_name(), like 
>>> scst_cleanup_tgt_sysfs_put_tgt(). It will also put reference to the 
>>> corresponding kobject and, hence, can destroy the whole object. This 
>>> is why I chose so big and inconvenient name: to emphasize that the 
>>> object can be destroyed after this function returned.
>>>
>>> 5. In the unregister functions the object will be fully cleaned as 
>>> much as possible for its sysfs attributes to work and not oops.
>>>
>>> I committed your patch with the above changes. Hopefully, now there 
>>> is a simple and straightforward path to implement other SCST sysfs 
>>> files and directories.
>>
>> I think we may have a problem here.
>> I agree with the name definitions and conventions but, unless I am 
>> missing something, there
>> is a problem on your 3th sentence.
>>
>> My first attempt to write the now called scst_create_tgt_sysfs 
>> function was using exactly
>> your approach but I stuck on the point that if any kobj creation 
>> (sessions, luns, ini_grp)
>> fail after a successful creation of tgt->tgt_kobj we  have to 
>> "destroy" tgt_kobj on the error
>> cover part, but I could not found a way do that if not with 
>> kobject_put(tgt_kobj).
>> That would call the release function (scst_tgt_free on scst_sysfs.c) 
>> that would kfree(tgt)
>> on a wrong place. I believe used kobject_del instead because of it but 
>> I afaik it does not
>> cleanup the kobj but only removes the directory from the sysfs.
>>
>> Searching for it now on the doc I found:
>> """
>> kobject_del() which will unregister the kobject from sysfs.  This 
>> makes the
>> kobject "invisible", but it is not cleaned up, and the reference count of
>> the object is still the same.  At a later time call kobject_put() to 
>> finish
>> the cleanup of the memory associated with the kobject.
>> """
>>
>> I feel that we only can kfree(tgt) on the error recover part if the 
>> error happened
>> before the tgt_kobj creation. After its creation the kfree only can be 
>> done by the
>> kobject release function.
>>
>> What do you think?
> 
> That problem was the one on which I spent the most time. Originally I 
> solved it by simply not calling kobject_put() on the error path, only 
> kobject_del(). Since we have tgt_kobj embedded it worked on the SCST 
> level pretty well. But I've just realized that it leaded to memory 
> leak(s) on the kobject level. So, I reconsidered it and now the code 
> looks like:
> 
> static void scst_tgt_free(struct kobject *kobj)
> {
>     struct scst_tgt *tgt;
> 
>     TRACE_ENTRY();
> 
>     tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
> 
>     if (tgt->tgt_sysfs_initialized)
>         kfree(tgt);
> 
>     TRACE_EXIT();
>     return;
> }
> 
> static struct kobj_type tgt_ktype = {
>     .release = scst_tgt_free,
> };
> 
> int scst_create_tgt_sysfs(struct scst_tgt *tgt)
> {
>     int retval;
> 
>     TRACE_ENTRY();
> 
>     retval = kobject_init_and_add(&tgt->tgt_kobj, &tgt_ktype,
>             tgt->tgtt->tgtt_kobj, tgt->tgt_name);
>     if (retval != 0) {
>         PRINT_ERROR("Can't add tgt %s to sysfs", tgt->tgt_name);
>         goto out;
>     }
> 
>     tgt->tgt_sess_kobj = kobject_create_and_add("sessions",
>                 &tgt->tgt_kobj);
>     if (!tgt->tgt_sess_kobj) {
>         PRINT_ERROR("Can't create sess kobj for tgt %s",
>                 tgt->tgt_name);
>         goto out_sess_obj_err;
>     }
> 
>     tgt->tgt_luns_kobj = kobject_create_and_add("luns",
>                 &tgt->tgt_kobj);
>     if (!tgt->tgt_luns_kobj) {
>         PRINT_ERROR("Can't create luns kobj for tgt %s",
>                 tgt->tgt_name);
>         goto luns_kobj_err;
>     }
> 
>     tgt->tgt_ini_grp_kobj = kobject_create_and_add("ini_group",
>                             &tgt->tgt_kobj);
>     if (!tgt->tgt_ini_grp_kobj) {
>         PRINT_ERROR("Can't create ini_grp kobj for tgt %s",
>             tgt->tgt_name);
>         goto ini_grp_kobj_err;
>     }
> 
>     tgt->tgt_sysfs_initialized = 1;
> 
> out:
>     TRACE_EXIT_RES(retval);
>     return retval;
> 
> ini_grp_kobj_err:
>     kobject_del(tgt->tgt_luns_kobj);
>     kobject_put(tgt->tgt_luns_kobj);
> 
> luns_kobj_err:
>     kobject_del(tgt->tgt_sess_kobj);
>     kobject_put(tgt->tgt_sess_kobj);
> 
> out_sess_obj_err:
>     kobject_del(&tgt->tgt_kobj);
>     kobject_put(&tgt->tgt_kobj);
>     retval = -ENOMEM;
>     goto out;
> }
> 
> I added tgt_sysfs_initialized flag to let scst_tgt_free() know if it 
> called normally or on the error processing path.
> 
> Should it work now?

I don't see any problems on that now. Very good solution. To be honest
I was looking for a way to know way the release was called (unregister
or error recover) for a long time and didn't think on that.

> You can also notice that there is kobject_del() before each the latest 
> on the SCST level kobject_put(). I did that to hide those objects ASAP. 
> Otherwise seems it is possible that, if any of them has external 
> reference, subsequent attempt to register object with the same name 
> (session, for instance) can fail. Don't know if it's possible on 
> practice, but better to be overinsured.

I don't know if it is really necessary and also I don't precisely know
what actions, events or whatever increment the kobj ref_count. Anyway
I think it can stay like that and if someone disagree and explain why
it is no necessary we can easily remote it.

Looking at this exactly moment on the kobject source code on kernel
to check what I wrote above I figured out that kobject_cleanup would
work where you were using kobject_del because it kobject_del and
cleanup all its resources.I could not found it on the kobject.txt so
it is not supposed to be used like this. Anyway I found worth to comment.

Regards,
Daniel Debonzi