Messages in this thread | | | Subject | Re: [PATCH 1/3] sysfs: simplify handling for s_active refcount | From | (Eric W. Biederman) | Date | Thu, 25 Mar 2010 21:24:43 -0700 |
| |
NeilBrown <neilb@suse.de> writes:
> s_active counts the number of active references to a 'sysfs_direct'. > When we wish to deactivate a sysfs_direct, we subtract a large > number for the refcount so it will always appear negative. When > it is negative, new references will not be taken. > After that subtraction, we wait for all the active references to > drain away. > > The subtraction of the large number contains exactly the same > information as the setting of the flag SYSFS_FLAG_REMOVED. > (We know this as we already assert that SYSFS_FLAG_REMOVED is set > before adding the large-negative-bias). > So doing both is pointless. > > By starting s_active with a value of 1, not 0 (as is typical of > reference counts) and using atomic_inc_not_zero, we can significantly > simplify the code while keeping exactly the same functionality.
Overall your logic appears correct but in detail this patch scares me.
sd->s_flags is protected by the sysfs_mutex, and you aren't taking it when you read it. So in general I don't see the new check if (sd->s_flags & SYSFS_FLAG_REMOVED) == 0 providing any guarantee of progress whatsoever with user space applications repeated reading from a sysfs file when that sysfs file is being removed. They could easily have the sd->s_flags value cached and never see the new value, given a crazy enough cache architecture.
So as attractive as this patch is I don't think it is correct.
Eric
| |