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