On Mon, 27 Nov 2006, Oliver Neukum wrote:
> OK, let's make some basic observation. Sysfs exists in memory only,
> as long as the dentry exists the inode cannot be freed.
How do you know that?
For that matter, is there any written explanation at all of how the
dentry and inode lifecycles work? Trying to read the source code is
practically hopeless; it's a tangled mess.
> Therefore the
> window for the race is only between dropping the dentry and orphaning
> all buffers.
>
> sysfs_release() will always hold a reference for the inode, so the inode
> must be touched before the reference is dropped. How about:
How does one acquire and drop references to inodes? Struct inode doesn't
include any obvious refcount field, except possibly i_count. For that
matter, almost none of the fields in struct inode are explained in
include/linux/fs.h.
> static int sysfs_release(struct inode * inode, struct file * filp)
> {
> struct kobject * kobj = to_kobj(filp->f_dentry->d_parent);
> struct attribute * attr = to_attr(filp->f_dentry);
> struct module * owner = attr->owner;
> struct sysfs_buffer * buffer = filp->private_data;
>
> if (buffer)
> remove_from_collection(buffer, inode);
Why wouldn't buffer be non-NULL?
> if (kobj)
> kobject_put(kobj);
> /* After this point, attr should not be accessed. */
> module_put(owner);
>
> if (buffer) {
> if (buffer->page)
> free_page((unsigned long)buffer->page);
> kfree(buffer);
> }
> return 0;
> }
>
> orphan_all_buffers() must be called with an additional reference to
> the inode. So how about:
>
> void sysfs_drop_dentry(struct sysfs_dirent * sd, struct dentry * parent)
> {
> struct dentry * dentry = sd->s_dentry;
> struct inode * inode;
>
> if (dentry) {
> inode = dentry->d_inode;
> igrab(inode); /* must work as dentry is still valid */
Where is igrab() documented? How do you know it acquires a reference to
the inode? If it does, why isn't it called iget?
> spin_lock(&dcache_lock);
> spin_lock(&dentry->d_lock);
> if (!(d_unhashed(dentry) && dentry->d_inode)) {
Why is this test here if dentry->d_inode must be valid?
> dget_locked(dentry);
> __d_drop(dentry);
> spin_unlock(&dentry->d_lock);
> spin_unlock(&dcache_lock);
> simple_unlink(parent->d_inode, dentry);
> orphan_all_buffers(inode);
> } else {
> spin_unlock(&dentry->d_lock);
> spin_unlock(&dcache_lock);
> }
> iput(inode);
> }
> }
This looks reasonable, subject to the questions raised above.
Alan Stern
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
