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

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 <[email protected]>

Index: scst/include/scst.h
===================================================================

include/scst.h +4 -1

@@ -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
===================================================================

src/scst_main.c +17 -2

@@ -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
===================================================================

src/scst_priv.h +6 -0

@@ -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
===================================================================

src/scst_sysfs.c +46 -5

@@ -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;
+	}

Hi Daniel,

Quoted 3 lines · 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/.

Quoted 4 lines

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.

Quoted 2 lines

Signed-off-by Daniel Debonzi <[email protected]>

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.

Thanks,
Vlad

Hi Vlad,

Quoted 11 lines · Vladislav Bolkhovitin

Hi Daniel,

Quoted 7 lines · 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

Quoted 2 lines

Quoted 1 line

Signed-off-by Daniel Debonzi <[email protected]>

You forgot diffstat here.

Actually I don't know how to generate them.

Quoted 6 lines

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.

Quoted 7 lines

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?

Quoted 1 line

See also below.

[snip]

Quoted 3 lines

Thanks,
Vlad

Regards,
Daniel Debonzi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Daniel Debonzi, on 05/20/2009 08:57 PM wrote:

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

Signed-off-by Daniel Debonzi <[email protected]>

You forgot diffstat here.

Actually I don't know how to generate them.

$ diffstat patch

Quoted 34 lines

Quoted 6 lines

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.

Quoted 7 lines

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

Quoted 5 lines

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.