All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHSET #master] sysfs: make sysfs disconnect immediately on deletion, take 2
@ 2007-04-09  4:18 Tejun Heo
  2007-04-09  4:18 ` [PATCH 01/14] sysfs: fix i_ino handling in sysfs Tejun Heo
                   ` (15 more replies)
  0 siblings, 16 replies; 36+ messages in thread
From: Tejun Heo @ 2007-04-09  4:18 UTC (permalink / raw)
  To: gregkh, maneesh, dmitry.torokhov, cornelia.huck, oneukum,
	rpurdie, James.Bottomley, stern, linux-kernel, htejun

Hello, all.

This is the second take of sysfs-immediate-disconnct patchset.

In the last take, rwsem was added to s_elem.dir to protect kobj only.
This wasn't enough because attr and bin_attr need to hold onto not
only the kobject of their parents but also the module backing
themselves and ops too, so the first set still needed separate and
duplicate attribute file orphaning mechanism.

In this take, the rwsem is generalized to become active reference
count.  Now each sysfs_dirent has two reference counts - s_count and
s_active.  s_count is a regular reference count which guarantees that
the containing sysfs_dirent is accessible.  As long as s_count
reference is held, all sysfs internal fields in sysfs_dirent are
accessible including s_parent and s_name.

The newly added s_active is active reference count.  This is acquired
by invoking sysfs_get_active() and it's the caller's responsibility to
ensure sysfs_dirent itself is accessible (should be holding s_count
one way or the other).  Dereferencing sysfs_dirent to access objects
out of sysfs proper requires active reference.  This includes access
to the associated kobjects, attributes and ops.

Because attr/bin_attr ops access both the node itself and its parent
for kobject, they need to hold active references to both.
sysfs_get/put_active_two() helpers are provided to help grabbing both
references.  Parent's is acquired first and released last.

Basically, s_count provides the reference counted objects to the upper
layer while s_active guards low level access such that low level
objects can just go away when they want to, and the same mechanism is
applied to all types of sysfs nodes.  I think it's conceptually
cleaner and thus easier to understand this way.

With all the patches applied, the same test used in the last take ran
9+hrs without any problem.

Change from the last take are...

* Patch 3 now doesn't move sysfs_get_kobject() as the it's replaced by
  active references later.

* Patch 12 updated such that sdir->rwsem is generalized into active
  reference count.

Please read the original lifetime rules discussion[1] and description
of the last take[2] for more info.

Thanks.

--
tejun

[1] http://thread.gmane.org/gmane.linux.kernel/510293
[2] http://thread.gmane.org/gmane.linux.kernel/513334



^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCHSET #master] sysfs: make sysfs disconnect immediately from kobject on deletion
@ 2007-04-07  8:23 Tejun Heo
  2007-04-07  8:23 ` [PATCH 14/14] sysfs: kill unnecessary attribute->owner Tejun Heo
  0 siblings, 1 reply; 36+ messages in thread
From: Tejun Heo @ 2007-04-07  8:23 UTC (permalink / raw)
  To: gregkh, maneesh, dmitry.torokhov, cornelia.huck, oneukum,
	rpurdie, James.Bottomley, linux-kernel, htejun

Hello, all.

This patchset is result of the following thread.

  http://thread.gmane.org/gmane.linux.kernel/510293

This patchset takes sysfs out of device driver lifetime equation which
not not only fixes several race conditions caused by sysfs not holding
onto the proper module when referencing kobject, but also helps fixing
and simplifying lifetime management in driver model.

sysfs is peculiar in how it's intertwined with driver model via
kobject and fs layer by using dentry to record some of its hierarchy.
This not only complicates lifetime management outside of sysfs but
also inside sysfs proper.  We end up with several different yet
inter-dependent lifetime rules.

For example, dentry depends on sysfs_dirent for file accesses as any
dentry would depend on inode and its backing fs private data to do so,
while sysfs_dirent depends on dentry to walk sysfs_dirent tree for
internal purpose (symlink walk) and the initial access to the dentry
happens by going through kobject pointer.  This interdependcy is okay
while all the objects are on memory but hell breaks loose when it's
time to kill those objects.  dentry and sysfs_dirent depend on each
other.  Unless they go away at the same time or use some way to safely
break the loop, one side ends up with dangled pointer to the other.

This patchset solves this by making sysfs_dirent behave more like fs
internal inodes in other filesystems which don't depend on dentry or
other external entity to manage itself.  Most information is already
there.  Only sd->s_parent and s_name are added.  These do increase the
size of sysfs_dirent a bit but makes the logic look designed more in
Earth instead of Mars and with further changes, dentry and inode for
kobject can be made reclaimable which can probably compensate the
added space overhead.

Sysfs lifetime rules are much simpler now.  sd denotes sysfs_dirent.

* sd has default reference of 1 on creation which is dropped on
  deletion.

* dentry holds reference to sd.  dentry->d_fsdata can be safely
  dereferenced while referecne to dentry is held.

* sd->s_parent points to the parent sd and each child holds a
  reference to its parent which is released when the child is released
  (reference reaches zero), so sd->s_parent can be dereferenced
  recursively if reference to the sd is held.

* sd->s_name can always be read while sd is valid.

* sd->s_elem.dir.kobj should only be accessed while
  sd->s_elem.dir.rwsem is read locked, which can be done by calling
  sysfs_get_dir_kobj() on the sd.

* sd->s_elem.[bin_]attr.[bin_attr] should only be accessed while its
  parent's sd->s_elem.dir.rwsem is read locked.  If
  sysfs_get_dir_kobj() returns NULL, attr pointer might point to
  released area.

* sysfs doesn't reference foreign objects for internal purpose.
  Foreign objets are accessed from the callbacks or interface
  functions where the caller is responsible for guaranteeing
  accessbility - symlink interface function is currently an exception.
  sysfs should export sysfs_dirent based interface and kobject code
  should do the locking.

* Directory dentries are still pinned as they are used in interface
  function - this should change in the future.

This patchset is consisted of the following 14 patches.

#01	: fix i_no handling bug and reduce dependency to inode
#02	: fix error handling in binattr write
#03-05	: prep for symlink reimplementation
#06-07	: add s_parent and s_name
#08	: make s_elem a union so that chaning what it points to doesn't
	  cause chaos and s_elem can contain more than one pointer
#09	: implement kobj_sysfs_assoc_lock to protect kobj->s_dentry
	  which will be used to keep symlink interface compatible
#10	: reimplement symlink using only sysfs_dirent tree
#11	: implement bin_buffer for immediate-kobject-disconnect
#12	: implement immediate-kobject-disconnect
#13-14	: kill now obsolete stuff

The first 11 are some fixes and preparation for
immediate-kobject-disconnect.  Depencies to external objects are
gradually removed such that only accesses during file ops remain which
are converted by patch #12.  The last two patches remove now
unnecessary attribute orphaning and attribute->owner.

I've run the following test on a UP machine for several hours without
any oops or memory leak and the test is currently running on a dual
processor (hyperthreading) machine for about half an hour.  I'll keep
it running for at least 5 hours.

 # (cd kernel-build-dir; while true; do echo [Loading...]; insmod drivers/scsi/scsi_mod.ko; insmod drivers/scsi/sd_mod.ko; insmod drivers/ata/libata.ko; insmod drivers/ata/ahci.ko; sleep 1; echo [Unloading...]; while lsmod | grep -q sd_mod; do rmmod ahci; rmmod libata; rmmod sd_mod; rmmod scsi_mod; sleep .1; done; done) &
 # (cd /sys; while true; do ls -liR > /dev/null; done) &
 # (cd /sys; while true; do find . | xargs cat > /dev/null 2>&1; done) &
 # (cd /sys; while true; do find . | sort | xargs cat > /dev/null 2>&1; done) &

Three harddisks and a dvd writer are attached to on-board ahci
controller, so there are plenty of sysfs activities going on involving
symlinks and all.

Thanks.

--
tejun

 drivers/base/class.c                        |    2 -
 drivers/base/core.c                         |    4 -
 drivers/base/firmware_class.c               |    2 +-
 drivers/block/pktcdvd.c                     |    3 +-
 drivers/char/ipmi/ipmi_msghandler.c         |   10 -
 drivers/cpufreq/cpufreq_stats.c             |    3 +-
 drivers/cpufreq/cpufreq_userspace.c         |    2 +-
 drivers/cpufreq/freq_table.c                |    1 -
 drivers/firmware/dcdbas.h                   |    3 +-
 drivers/firmware/dell_rbu.c                 |    6 +-
 drivers/firmware/edd.c                      |    2 +-
 drivers/firmware/efivars.c                  |    6 +-
 drivers/i2c/chips/eeprom.c                  |    1 -
 drivers/i2c/chips/max6875.c                 |    1 -
 drivers/infiniband/core/sysfs.c             |    1 -
 drivers/input/mouse/psmouse.h               |    1 -
 drivers/media/video/pvrusb2/pvrusb2-sysfs.c |   13 --
 drivers/misc/asus-laptop.c                  |    3 +-
 drivers/pci/hotplug/acpiphp_ibm.c           |    1 -
 drivers/pci/pci-sysfs.c                     |    4 -
 drivers/pcmcia/socket_sysfs.c               |    2 +-
 drivers/rtc/rtc-ds1553.c                    |    1 -
 drivers/rtc/rtc-ds1742.c                    |    1 -
 drivers/scsi/arcmsr/arcmsr_attr.c           |    3 -
 drivers/scsi/lpfc/lpfc_attr.c               |    2 -
 drivers/scsi/qla2xxx/qla_attr.c             |    6 -
 drivers/spi/at25.c                          |    1 -
 drivers/video/aty/radeon_base.c             |    2 -
 drivers/video/backlight/backlight.c         |    2 +-
 drivers/video/backlight/lcd.c               |    2 +-
 drivers/w1/slaves/w1_ds2433.c               |    1 -
 drivers/w1/slaves/w1_therm.c                |    1 -
 drivers/w1/w1.c                             |    2 -
 fs/ecryptfs/main.c                          |    2 -
 fs/ocfs2/cluster/masklog.c                  |    1 -
 fs/partitions/check.c                       |    1 -
 fs/sysfs/bin.c                              |  189 ++++++++++++-------
 fs/sysfs/dir.c                              |  270 ++++++++++++++++-----------
 fs/sysfs/file.c                             |  206 ++++++++++-----------
 fs/sysfs/inode.c                            |   61 +------
 fs/sysfs/mount.c                            |   10 +-
 fs/sysfs/symlink.c                          |  121 ++++++-------
 fs/sysfs/sysfs.h                            |  109 ++++-------
 include/linux/sysdev.h                      |    3 +-
 include/linux/sysfs.h                       |    8 +-
 kernel/module.c                             |    9 +-
 kernel/params.c                             |    1 -
 net/bridge/br_sysfs_br.c                    |    3 +-
 net/bridge/br_sysfs_if.c                    |    3 +-
 49 files changed, 496 insertions(+), 596 deletions(-)



^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2007-04-27 16:06 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-04-09  4:18 [PATCHSET #master] sysfs: make sysfs disconnect immediately on deletion, take 2 Tejun Heo
2007-04-09  4:18 ` [PATCH 01/14] sysfs: fix i_ino handling in sysfs Tejun Heo
2007-04-27 15:29   ` Eric Sandeen
2007-04-27 15:32     ` Greg KH
2007-04-27 16:04       ` Eric Sandeen
2007-04-09  4:18 ` [PATCH 02/14] sysfs: fix error handling in binattr write() Tejun Heo
2007-04-09  4:18 ` [PATCH 05/14] sysfs: consolidate sysfs_dirent creation functions Tejun Heo
2007-04-09  4:18 ` [PATCH 03/14] sysfs: move release_sysfs_dirent() to dir.c Tejun Heo
2007-04-09  4:18 ` [PATCH 04/14] sysfs: flatten cleanup paths in sysfs_add_link() and create_dir() Tejun Heo
2007-04-09  4:18 ` [PATCH 07/14] sysfs: add sysfs_dirent->s_name Tejun Heo
2007-04-09  4:18 ` [PATCH 08/14] sysfs: make sysfs_dirent->s_element a union Tejun Heo
2007-04-09  4:18 ` [PATCH 11/14] sysfs: implement bin_buffer Tejun Heo
2007-04-09  4:18 ` [PATCH 06/14] sysfs: add sysfs_dirent->s_parent Tejun Heo
2007-04-09  4:18 ` [PATCH 09/14] sysfs: implement kobj_sysfs_assoc_lock Tejun Heo
2007-04-09  4:18 ` [PATCH 10/14] sysfs: reimplement symlink using sysfs_dirent tree Tejun Heo
2007-04-09  4:18 ` [PATCH 14/14] sysfs: kill unnecessary attribute->owner Tejun Heo
2007-04-10 14:17   ` Cornelia Huck
2007-04-10 14:30     ` Cornelia Huck
2007-04-11  4:21       ` Tejun Heo
2007-04-11  4:25   ` [PATCH 14/14 UPDATED] " Tejun Heo
2007-04-09  4:18 ` [PATCH 13/14] sysfs: kill attribute file orphaning Tejun Heo
2007-04-09  4:18 ` [PATCH 12/14] sysfs: implement sysfs_dirent active reference and immediate disconnect Tejun Heo
2007-04-11  4:15   ` [PATCH 12/14 UPDATED] " Tejun Heo
2007-04-11  9:00     ` Cornelia Huck
2007-04-11  9:26       ` Tejun Heo
2007-04-11  9:32         ` Tejun Heo
2007-04-11 10:13         ` Tejun Heo
2007-04-11 10:26           ` Cornelia Huck
2007-04-12  7:18     ` Greg KH
2007-04-12  7:39       ` Greg KH
2007-04-10 14:44 ` [PATCHSET #master] sysfs: make sysfs disconnect immediately on deletion, take 2 Cornelia Huck
2007-04-11  4:18   ` Tejun Heo
2007-04-16  8:22 ` Maneesh Soni
2007-04-17  5:06   ` Tejun Heo
2007-04-17 12:42   ` Tejun Heo
  -- strict thread matches above, loose matches on Subject: below --
2007-04-07  8:23 [PATCHSET #master] sysfs: make sysfs disconnect immediately from kobject on deletion Tejun Heo
2007-04-07  8:23 ` [PATCH 14/14] sysfs: kill unnecessary attribute->owner Tejun Heo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.