Re: [PATCH 8/19]: SCST SYSFS interface implementation

[Vladislav Bolkhovitin <vst@vlnb.net>]

Dmitry Torokhov, on 11/12/2010 04:23 AM wrote:
>>> This put might not be the last put on the object, IOs in flight
>>> and/or open files might have extra reference on the object.
>>> We release our initial ref, and below wait for all operations
>>> to complete. (Is there a matter of timeout like files not closing?)
>>
>> This is the last internal put. All other references are from outsiders.
>> So, we are waiting for all them to put before we go on.
>>
> 
> The question is why do you need to wait here? I presume it is module
> unloading path, but then it is quite bad - you can easily wedge your
> subsystem if you make something to take a reference to your kobject
> while module is ytying to be unloaded. Back when sysfs attributes tied
> kobjects the easiest thing was to do:
> 
> 	rmmod <module> < / sys/devices/..../attribute
> 
> If you are done with the kobject - just proceed with what you were doing
> and let it die its own peaceful death some time later. You just need to
> make sure release code sticks around to free it and your subsystem core
> can be tasked with this. Use module counter to prevent unloading of the
> subsystem core until all kobjects belonging to the subsystem are
> destroyed.

This is a very good question. During implementation I spent a lot of
time working on it.

In fact, the first implementation was asynchronous similarly as you
proposing, i.e. it just proceed with what you were doing and let it die
its own peaceful death some time later. But soon the implementation was
becoming so complicated, so it started getting out of control. For
instance, some of the tasks to solve with this approach were:

1. What to do if another SCST object is being created with the same name
as supposed to be deleted, but not completely dead yet?

2. What to do if a dieing object is found on some list and reference for
is supposed to be taken? If the object deleted from the list before it
marked dieing, i.e. the latest internal put() done, it made additional
problems during deleting it after the latest external put done.

...

So, I decided to reimplement it to be completely synchronous. SYSFS
authors did really great job and thanks to the excellent internal SYSFS
design and implementation it is absolutely safe. See:

[root@tgt ~]# modprobe scst
[root@tgt ~]# cd /sys/kernel/scst_tgt/
[root@tgt scst_tgt]# ls -l
total 0
drwxr-xr-x 4 root root    0 Nov 13 21:31 devices
drwxr-xr-x 2 root root    0 Nov 13 21:31 handlers
-r--r--r-- 1 root root 4096 Nov 13 21:30 last_sysfs_mgmt_res
-rw-r--r-- 1 root root 4096 Nov 13 21:30 setup_id
drwxr-xr-x 5 root root    0 Nov 13 21:31 sgv
drwxr-xr-x 2 root root    0 Nov 13 21:31 targets
-rw-r--r-- 1 root root 4096 Nov 13 21:30 threads
-rw-r--r-- 1 root root 4096 Nov 13 21:30 trace_level
-r--r--r-- 1 root root 4096 Nov 13 21:30 version
[root@tgt scst_tgt]# cat version
2.1.0-pre1
EXTRACHECKS
DEBUG
[root@tgt scst_tgt]# rmmod scst </sys/kernel/scst_tgt/version
[root@tgt scst_tgt]# ls -l
total 0
[root@tgt scst_tgt]# pwd
/sys/kernel/scst_tgt
[root@tgt scst_tgt]# lsmod
Module                  Size  Used by
scsi_debug             65188  0
w83627hf               22424  0
hwmon_vid               2207  1 w83627hf
adm1021                 6189  0
binfmt_misc             6229  1
xfs                   673142  1
exportfs                3143  1 xfs
dm_mirror              12069  0
dm_region_hash          8703  1 dm_mirror
dm_log                  8345  2 dm_mirror,dm_region_hash
dm_mod                 63511  2 dm_mirror,dm_log
pci_slot                3378  0
hed                     1758  0
floppy                 52718  0
uhci_hcd               21459  0
sg                     25181  0
e1000                 128475  0
i2c_i801                8756  0
pcspkr                  1442  0
i2c_core               22319  2 adm1021,i2c_i801
e7xxx_edac              3463  0
parport_pc             25439  0
parport                29682  1 parport_pc

Everything works fine.

This is because SYSFS doesn't hold references for the corresponding
kobjects for every open file handle. It holds references only when
show() and store() functions called. So, everything is under control and
a malicious user can do nothing to hold a reference forever.

Vlad