Am Dienstag, 28. November 2006 04:38 schrieb Alan Stern:
> 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.
I have to modify the statement.
In fs/namei.c:do_path_lookup() a dget() is done for every open() syscall.
The corresponding dput appears to be in fs/file_table:__fput().
In contrast sysfs pins/calls dget() only in fs/sysfs/inode.c:sysfs_create()
for directories.
Therefore, opened files have a valid dentry in memory. This should be enough.
Only opened files can have buffers attached. As for inodes see below.
It seems to me that the Documentation for sysfs is outdated and says that
sysfs works like ramfs. This seems still true only for directories.
> > 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
__iget()/igrab() vs. iput()
This hasn't changed since 2.0 or so.
> include any obvious refcount field, except possibly i_count. For that
i_count is the refcount field. Its use is subject to flags.
> 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?
I don't know. The existing code assumes it might be 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?
Documented core VFS code? The notion is indecent verging on blasphemous ;-)
fs/inode.c:igrab() acquires the spinlocks and makes some checks.
It then calls __get().
> > 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?
This _is_ documented. A dentry without associated inode is called a
negative dentry and means that open() must fail. I don't see how
sysfs can get there, as you cannot delete files, but in theory such
dentries exist.
IMHO this routine is written to be called several times without harm.
Considering that, need I call __iget directly?
/*
* Unhashes the dentry corresponding to given sysfs_dirent
* Called with parent inode's i_mutex held.
*/
void sysfs_drop_dentry(struct sysfs_dirent * sd, struct dentry * parent)
{
struct dentry * dentry = sd->s_dentry;
struct inode *node;
if (dentry) {
spin_lock(&dcache_lock);
spin_lock(&dentry->d_lock);
if (!(d_unhashed(dentry) && (node = dentry->d_inode))) {
spin_lock(&node->inode_lock);
__iget(node);
spin_unlock(&node->inode_lock);
dget_locked(dentry);
__d_drop(dentry);
spin_unlock(&dentry->d_lock);
spin_unlock(&dcache_lock);
simple_unlink(parent->d_inode, dentry);
orphan_all_buffers(node);
iput(node);
} else {
spin_unlock(&dentry->d_lock);
spin_unlock(&dcache_lock);
}
}
}
> > 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.
This is VFS code. I am not sure I answered them.
Regards
Oliver
-------------------------------------------------------------------------
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
