Date:	Sat, 30 May 2009 14:20:35 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	James Bottomley <James.Bottomley@...senPartnership.com>
Cc:	Tejun Heo <tj@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	linux-kernel@...r.kernel.org,
	Cornelia Huck <cornelia.huck@...ibm.com>,
	linux-fsdevel@...r.kernel.org, Kay Sievers <kay.sievers@...y.org>,
	Greg KH <greg@...ah.com>,
	"Eric W. Biederman" <ebiederm@...stanetworks.com>
Subject: Re: [PATCH 04/24] sysfs: Normalize removing sysfs directories.

James Bottomley <James.Bottomley@...senPartnership.com> writes:

>> >> My take is simply that a correct user has to wait until no one else
>> >> can find the kobject before calling kobject_del.  At which point
>> >> races are impossible, and it doesn't matter if sysfs_mutex is held
>> >> across the entire operation.
>> >
>> > I'm afraid this one isn't a valid assumption.  If you look in SCSI,
>> > you'll see we do get objects after they've been removed from visibility.
>> > We use it as part of the state model for how our objects work (objects
>> > removed from visibility are dying, but we still need them to be findable
>> > (and gettable).
>> 
>> I was not precise enough.  It appears I overlooked the fact that
>> kobject_del is not always called from kobject_put by way of
>> kobject_release.
>
> OK ... just so you understand, I'm thinking about the device model
> rather than kobjects.  device_del() can't be called from release methods
> because they're often called from interrupt context and the mutex
> requirements in device_del() mean it needs user context.

Makes sense.

>> Strictly the requirement is that after kobject_del we don't add,
>> remove or otherwise manipulate sysfs attributes.  That is we don't
>> call any of:
>> 
>> sysfs_add_file
>> sysfs_create_file
>> sysfs_create_bin_file
>> sysfs_remove_file
>> sysfs_remove_bin_file
>> sysfs_create_link
>> sysfs_remove_link
>> sysfs_create_group
>> sysfs_remove_group
>> sysfs_create_subdir
>> sysfs_remove_subdir
>> 
>> 
>> Those all either oops or BUG today if you try it.  So I can't see how
>> a subsystem could depend on those working.
>
> It doesn't; you've altered your requirement.  We can fully buy into this
> new relaxed one.

My apologies for misstating it earlier.  Sometimes translating what
is happening in sysfs up to the device model can be a bit of a challenge.

At the sysfs layer the requirement is all the same.  Don't mess with a
directory as or after you have deleted it.


To recap, my change that Tejun has a problem with is simply that I have
refactored sysfs_remove_dir so that if there are directory entries
present.  A very fast observer in the kernel or in user space can see
each directory entry being deleted individually.  Before I delete the
directory itself.

This is because I now drop and reacquire the sysfs_mutex in between
each delete.

As the upper layers must already avoid messing with the attributes
of a sysfs directory from the time we call kobject_del I don't
see that this makes any difference to them.

>> Also there is sysfs_remove_dir (on a subdirectory) aka kobject_del on
>> a child object after kobject_del on the parent object.
>> 
>> As best I can tell that only works by fluke today.
>
> Yes, that's an artifact of the fact that the reference counted lifecycle
> is on release ... del just happens at a certain point in it.  We don't
> hold any counters that tell us what the visibility of our children are,
> so it's possible to make a parent invisible by calling del simply
> because you don't know.

Strictly speaking my changes don't affect this part either except to
issue a warning that something unexpected is going on.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists