Al thank you for taking a fresh look at sysfs.
For the rest of the readers. Most of the bugs and what seem to
me to be the most significant bugs that Al is seeing existing
without my sysfs tagged directory patchset.
The concept of a single directory tree in the basic data structure
where only some parts show up depending upon the namespace you are in
conceptually the same, and that is what I was referring to.
I'm going to attempt to step back a bit and see if I can convert this into
a constructive exchange. Picking on the nasty details without discussing
the reasons for those details won't get us anywhere.
From the point of view of the VFS sysfs is a distributed filesystem.
As a distributed filesystem we don't always go through the VFS sanity
checks when a filesystem change occurs, and only update the VFS caches
to the state of the change.
As a distributed filesystem sysfs internally needs to provide all of
the locking necessary to ensure the sysfs data structures are consistent.
Currently there are two basic paths into sysfs:
- Directly to the kernel internal state to the internal sysfs state.
This is where all mutation happens.
- Read-only through the VFS.
In the case of directory structure mutation, the mutation has happened
and the VFS simply must be told about it.
Which means that in a sane and simple design of a global filesystem
the locking would work was follows:
o Obtain filesystem lock
o Modify filesystem internal state.
o For each mounted instance
o Obtain VFS locks
o Update the mounted instance.
o Release VFS locks
o Release filesystem lock.
The above is simple and allows the same locking ordering for all accesses.
Unfortunately the VFS does not give us the opportunity to takes the locks
in the proper order. And it insists on grabbing a VFS lock before we can
grab a global lock.
Which in my case leads to weirdness like:
I can't currently use separate inodes because of the current locking of sysfs,
because by the current VFS limitations. In particular:
The currently locking order is:
mutex_lock(&inode->i_mutex);
mutex_lock(&sysfs_mutex);
If I use separate inodes that locking order can not be maintained. Frankly I would
love to change this.
Quiet frankly if the VFS requires the dcache to be out of sync
with the filesystem that is a VFS bug.
If the filesystem deletes the directory then it should be unhashed
so it can not be looked up.
Currently I know unhashing a busy directory potentially causes
mount leaks. Are you thinking of anything else?
Nah. In this case just a silly oversight. It is a trivial fix.
Good point about UP deadlocks.
It shouldn't be hard to do the equivalent of lock_rename() and unlock_rename().
Hardly. sb->s_vfs_rename_mutex only serializes against renames in the VFS.
As such it isn't useful to protect the sysfs data structures. That is
the job of sysfs_rename_mutex. At which point sb->s_vfs_rename_mutex
is redundant.
The only places I can see where s_vfs_rename_mutex is taken is in
lock_rename(), unlock_rename() and d_materialise_unique().
d_materialise_unique() only gets called by NFS. lock_rename() and
unlock_rename() only get called from renameat which always fails in the
case of sysfs and we aren't doing anything silly like hard links to
directories so I don't see a way to support that path.
Ouch! Good point. That is simple enough to solve by reordering the
operations.
Cute. Sounds like we need to put a reference to the sysfs_dirent into the inode.
sd->s_tag can only be changed by sysfs_mv_dir so it should share the same locking
as anything else that can be renamed.
I haven't touched that one, and I am tired tonight. Are you thinking
of something in addition to an early sysfs_put and a
I don't see where we come anywhere close to sysfs for running ifconfig up.
Certainly ifconfig up works fine with sysfs compiled out and I haven't
spotted the path from the networking code to sysfs in that instance.
sysfs really only shows up in adding and removing network interfaces
in my experience.
I haven't looked at devfs so I wouldn't know. I just know it feels
futile a lot of time to try and improve sysfs.
Eric
--
