Hello,

Eric W. Biederman wrote:
>> Having enumed tag types limits that a sb can have map to only one tag
>> but it doesn't really prevent multiple possibly visible entries which is
>> the real unnecessary degrees of freedom.  That said, I don't really
>> think it's an issue.
> 
> Having a single tag type per directory and thus a single tag visible per
> directory does prevent multiple possible visible entries.
> 
> That is we can check when we add the sd if there will be a conflict in
> the directory.

Yeap, that we can do.

>> I still would prefer something which is more generic.  The abstraction
>> is clearer too.  A sb shows untagged and a set of tags.  A sd can either
>> be untagged or tagged (a single tag).
> 
> That is the abstraction now.
> 
> The only difference is how we represent the set of tags.
> I use and array of the valid tags.
> You use a bitmap.
> 
> And array allows the lookup of the tag I am looking for before
> I search for the sd.  An bitmap requires me to compare each entry.

How so?  sysfs_sb->bitmap which contains enough bits for all the defined
tags and determining whether a sd should be shown or not is as simple as
single test_bit.

> For me that is a deal breaker.  Currently in certain pathological
> cases we have scaling issues with sysctl and sysfs that we can
> have enormous directories that start running slowly.  To fix
> lookup performance requires that we know the full name before
> we do the directory search which is the name string and the
> tag.
> 
> So I having a type of tag as being of fundamental importance in
> the interface now so we don't need to refactor all of the users
> later.  In addition to the fact that we need the type to know
> how to set the tags when mounting a superblock and when
> given a new kobject to create an sd for.
> 
> We could make the types dynamic rather then a static enumeration but
> that seems needless complexity for now.

What I'm feeling unease about is the extra level of abstraction added by
tag types.  A sd is given a tag.  A sb shows a set of tags.  The most
straight forward to implement that is to give sd a tag and test the tag
against sb's set of tags.  The type is added because pointer tag
requires sequential matching which is usually best to avoid.  It's
nothing fundamental.  It's an extra baggage.

>> Using ida (or idr if a pointer for private data is necessary) is really
>> easy.  It'll probably take a few tens of lines of code.  That said, I
>> don't think I have enough rationale to nack what you described.  So, as
>> long as the tags are made static, I won't object.
> 
> Sounds good.  The only justification I can think of for ida tags is that
> they are smaller, and so can keep the sysfs_dirents smaller.    Which
> occasionally is a significant concern.  Still that should be an optimization
> that we can apply later, as it is not a structural difference in the code.
> 
> Just to confirm.  Do you the two operations:
>   mount_tag - called only when the sb is mounted 
>   kobject_tag - called when we create new sd or rename an sd
> 
> Cause you to view an the tags as dynamic?

The thing is that I don't really see why there's tagged_dir_ops at all.
 What's needed is tagged sd's and sb's which can show subset of those
tags, so adding callback ops for tags just doesn't make much sense to
me.  The interface should ideally be...

1. alloc/release tag
2. set / change / remove tag on sd
3. enable / disable tag on a sb

This has been my opinion from the beginning.  Unless the tags need to be
changed dynamically on demand (which I hope is not the case), there just
is plainly no reason to have callbacks for tags.

Thanks.

-- 
tejun
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Tejun Heo <htejun@gmail.com> writes:

> Hello,
>
> Eric W. Biederman wrote:
>>> Having enumed tag types limits that a sb can have map to only one tag
>>> but it doesn't really prevent multiple possibly visible entries which is
>>> the real unnecessary degrees of freedom.  That said, I don't really
>>> think it's an issue.
>> 
>> Having a single tag type per directory and thus a single tag visible per
>> directory does prevent multiple possible visible entries.
>> 
>> That is we can check when we add the sd if there will be a conflict in
>> the directory.
>
> Yeap, that we can do.

What we are implementing is not, a sb with a set of tags that are displayed,
but directories with a single tag that is displayed.  The sb just happens
to hold the state for the directories.

A directory displaying only a single tag is an necessary constraint for
a large number of reasons.

>> And array allows the lookup of the tag I am looking for before
>> I search for the sd.  An bitmap requires me to compare each entry.
>
> How so?  sysfs_sb->bitmap which contains enough bits for all the defined
> tags and determining whether a sd should be shown or not is as simple as
> single test_bit.

Yes. The compare happens to be test_bit. 

With a bitmap you must visit each dirent with a given name and see if
it has a tag that is displayed.

With an array you can lookup the tag aprori and can potentially do a
hash table lookup or a tree lookup and are not required to visit each
entry.

> What I'm feeling unease about is the extra level of abstraction added by
> tag types.  A sd is given a tag.  A sb shows a set of tags.  The most
> straight forward to implement that is to give sd a tag and test the tag
> against sb's set of tags.  The type is added because pointer tag
> requires sequential matching which is usually best to avoid.  It's
> nothing fundamental.  It's an extra baggage.

That is just one important aspect of it.   We need a way to describe
which tag a sb,directory pair displays.  It is a fundamental concept.

>>> Using ida (or idr if a pointer for private data is necessary) is really
>>> easy.  It'll probably take a few tens of lines of code.  That said, I
>>> don't think I have enough rationale to nack what you described.  So, as
>>> long as the tags are made static, I won't object.
>> 
>> Sounds good.  The only justification I can think of for ida tags is that
>> they are smaller, and so can keep the sysfs_dirents smaller.    Which
>> occasionally is a significant concern.  Still that should be an optimization
>> that we can apply later, as it is not a structural difference in the code.
>> 
>> Just to confirm.  Do you the two operations:
>>   mount_tag - called only when the sb is mounted 
>>   kobject_tag - called when we create new sd or rename an sd
>> 
>> Cause you to view an the tags as dynamic?
>
> The thing is that I don't really see why there's tagged_dir_ops at all.

We need callbacks for interfacing with the kobject layer, and for
selecting our set of tags at mount time.  Not tagged_dir_ops so much
as tagged_type_ops.

>  What's needed is tagged sd's and sb's which can show subset of those
> tags, so adding callback ops for tags just doesn't make much sense to
> me.  The interface should ideally be...

> 1. alloc/release tag
     Agreed.

> 2. set / change / remove tag on sd
     Essentially agreed.

     Create an sd with a tag, change the tag on a sd.
     Having an untagged sd in a directory that requires tags should
     not be allowed.

> 3. enable / disable tag on a sb
     Disagree that is too flexible.  Tags on a sb need to be
     unchanging or else we get vfs layer issues.

    Further the abstraction is logically exactly one tag on a
    (sb,directory) pair.

    The operations needed are.
    - Select the set of tags on a sb (at mount time)
      This requires we call a set of callbacks.  [ My mount_sb callback ]

    - release a tag (which implies removing all tagged entries and
      removing the sb reference)

4. Interface with the kobject layer.
   kobject_add calls sysfs_create_dir
   kboject_rename calls sysfs_rename_dir 
   kobject_del calls sysfs_remove_dir

   For the first two operations we need a helper function to go from a
   kobject to a tag.

   For the second two operations we need to go from a kobject to a sd.

> This has been my opinion from the beginning.  Unless the tags need to be
> changed dynamically on demand (which I hope is not the case), there just
> is plainly no reason to have callbacks for tags.

We don't need callbacks to poll to see if the tags on a sd have
changed.

We need helper functions for interfacing with the rest of the kernel. 

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Hello,

Eric W. Biederman wrote:
> What we are implementing is not, a sb with a set of tags that are displayed,
> but directories with a single tag that is displayed.  The sb just happens
> to hold the state for the directories.
> 
> A directory displaying only a single tag is an necessary constraint for
> a large number of reasons.

Okay, that isn't exactly the impression I get but... well.  Let's see.

>>> And array allows the lookup of the tag I am looking for before
>>> I search for the sd.  An bitmap requires me to compare each entry.
>> How so?  sysfs_sb->bitmap which contains enough bits for all the defined
>> tags and determining whether a sd should be shown or not is as simple as
>> single test_bit.
> 
> Yes. The compare happens to be test_bit. 
> 
> With a bitmap you must visit each dirent with a given name and see if
> it has a tag that is displayed.
> 
> With an array you can lookup the tag aprori and can potentially do a
> hash table lookup or a tree lookup and are not required to visit each
> entry.

A few things...

1. The lookup is currently done linearly and is fast enough for now.
Also, most lookup ops are cached by vfs layer.  I'm not sure how
probable it is that we're gonna need hash or tree based sd lookup.

2. I don't think it's gonna be too difficult to speed up bitmap based
lookup.  It would require a bit more intelligence but there's no
fundamental restriction.  Just organizing the tree by tag first would
give us the same order of magnitude lookup given that the tags are used
the same way.

>> What I'm feeling unease about is the extra level of abstraction added by
>> tag types.  A sd is given a tag.  A sb shows a set of tags.  The most
>> straight forward to implement that is to give sd a tag and test the tag
>> against sb's set of tags.  The type is added because pointer tag
>> requires sequential matching which is usually best to avoid.  It's
>> nothing fundamental.  It's an extra baggage.
> 
> That is just one important aspect of it.   We need a way to describe
> which tag a sb,directory pair displays.  It is a fundamental concept.

For netns, yes.  I just think it would be better if the sysfs mechanism
to support that concept is more generic especially because it doesn't
seem too difficult to make it that way.

>>> Cause you to view an the tags as dynamic?
>> The thing is that I don't really see why there's tagged_dir_ops at all.
> 
> We need callbacks for interfacing with the kobject layer, and for
> selecting our set of tags at mount time.  Not tagged_dir_ops so much
> as tagged_type_ops.

The kobject op seems a bit strange way to interface to me.  For mount,
yeah, we'll need a hook somewhere or pass it via mount option maybe.

>>  What's needed is tagged sd's and sb's which can show subset of those
>> tags, so adding callback ops for tags just doesn't make much sense to
>> me.  The interface should ideally be...
> 
>> 1. alloc/release tag
>      Agreed.
> 
>> 2. set / change / remove tag on sd
>      Essentially agreed.
> 
>      Create an sd with a tag, change the tag on a sd.
>      Having an untagged sd in a directory that requires tags should
>      not be allowed.
> 
>> 3. enable / disable tag on a sb
>      Disagree that is too flexible.  Tags on a sb need to be
>      unchanging or else we get vfs layer issues.

Yeah, this really should be something which can't change once it's mounted.

>     Further the abstraction is logically exactly one tag on a
>     (sb,directory) pair.

I'm not so sure here.  As a policy, maybe but I don't really see a
fundamental reason that the mechanism should enforce this.

>     The operations needed are.
>     - Select the set of tags on a sb (at mount time)
>       This requires we call a set of callbacks.  [ My mount_sb callback ]
> 
>     - release a tag (which implies removing all tagged entries and
>       removing the sb reference)
> 
> 4. Interface with the kobject layer.
>    kobject_add calls sysfs_create_dir
>    kboject_rename calls sysfs_rename_dir 
>    kobject_del calls sysfs_remove_dir
> 
>    For the first two operations we need a helper function to go from a
>    kobject to a tag.

Why not just add a parameter to sysfs_create_dir()?  It's just twisted.

>    For the second two operations we need to go from a kobject to a sd.
> 
>> This has been my opinion from the beginning.  Unless the tags need to be
>> changed dynamically on demand (which I hope is not the case), there just
>> is plainly no reason to have callbacks for tags.
> 
> We don't need callbacks to poll to see if the tags on a sd have
> changed.
> 
> We need helper functions for interfacing with the rest of the kernel. 

Yes, that's why I view it as strange.  These can be done in forward way
(by passing in mount options and/or arguments) but it's done by first
going into the sysfs and then calling back out to outer layer.

Thanks.

-- 
tejun
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Tejun Heo <htejun@gmail.com> writes:

> Hello,
>
> Eric W. Biederman wrote:
>> What we are implementing is not, a sb with a set of tags that are displayed,
>> but directories with a single tag that is displayed.  The sb just happens
>> to hold the state for the directories.
>> 
>> A directory displaying only a single tag is an necessary constraint for
>> a large number of reasons.
>
> Okay, that isn't exactly the impression I get but... well.  Let's see.

Well one of those reasons is not having duplicate entries in your directory listing.
That is much harder otherwise.

> A few things...
>
> 1. The lookup is currently done linearly and is fast enough for now.
> Also, most lookup ops are cached by vfs layer.  I'm not sure how
> probable it is that we're gonna need hash or tree based sd lookup.

I don't know how bad sysfs is.  On the sysctl side I have people complaining
because I am doing a lookup during insert and that lookup is linear.  Sysfs
appears to have the same complexity as sysctl but just smaller constants.

>> That is just one important aspect of it.   We need a way to describe
>> which tag a sb,directory pair displays.  It is a fundamental concept.
>
> For netns, yes.  I just think it would be better if the sysfs mechanism
> to support that concept is more generic especially because it doesn't
> seem too difficult to make it that way.

Well the envisioned use is for other namespaces and they all are similar
to the network namespace in that way.

>>>> Cause you to view an the tags as dynamic?
>>> The thing is that I don't really see why there's tagged_dir_ops at all.
>> 
>> We need callbacks for interfacing with the kobject layer, and for
>> selecting our set of tags at mount time.  Not tagged_dir_ops so much
>> as tagged_type_ops.
>
> The kobject op seems a bit strange way to interface to me.  For mount,
> yeah, we'll need a hook somewhere or pass it via mount option maybe.

I will look how if there is a place in the kobject layer to put it.  With
a second but noticeably different user I can compare and see how hard that will be.

>>> 3. enable / disable tag on a sb
>>      Disagree that is too flexible.  Tags on a sb need to be
>>      unchanging or else we get vfs layer issues.
>
> Yeah, this really should be something which can't change once it's mounted.
The VFS chokes otherwise because it can't cache things properly.

>>     Further the abstraction is logically exactly one tag on a
>>     (sb,directory) pair.
>
> I'm not so sure here.  As a policy, maybe but I don't really see a
> fundamental reason that the mechanism should enforce this.

Well in the first implementation.

>> 4. Interface with the kobject layer.
>>    kobject_add calls sysfs_create_dir
>>    kboject_rename calls sysfs_rename_dir 
>>    kobject_del calls sysfs_remove_dir
>> 
>>    For the first two operations we need a helper function to go from a
>>    kobject to a tag.
>
> Why not just add a parameter to sysfs_create_dir()?  It's just twisted.

I added it where it was easiest.  Adding a parameter to sysfs_create_dir
simply means I have to add the function to the kobject layer.  It is certainly
worth a second look though.

>> We need helper functions for interfacing with the rest of the kernel. 
>
> Yes, that's why I view it as strange.  These can be done in forward way
> (by passing in mount options and/or arguments) but it's done by first
> going into the sysfs and then calling back out to outer layer.

Well in the case of mount the default parameter at least is current, and
there are good reasons for that.

On the other side I can't pass a tag through from the device layer to
the kobject layer.  It isn't a concept the kobject layer supports.

At least though the conversation is in relative agreement.  I will refresh
the patches shortly and see where we are at.

Eric

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Hello,

Eric W. Biederman wrote:
>>> A directory displaying only a single tag is an necessary constraint for
>>> a large number of reasons.
>> Okay, that isn't exactly the impression I get but... well.  Let's see.
> 
> Well one of those reasons is not having duplicate entries in your directory listing.
> That is much harder otherwise.

Agreed.

>> For netns, yes.  I just think it would be better if the sysfs mechanism
>> to support that concept is more generic especially because it doesn't
>> seem too difficult to make it that way.
> 
> Well the envisioned use is for other namespaces and they all are similar
> to the network namespace in that way.

Something I've been curious about is a directory which contains both the
untagged entries and tagged ones.  I can definitely imagine something
like that to be useful for block device namespace.

>>>>> Cause you to view an the tags as dynamic?
>>>> The thing is that I don't really see why there's tagged_dir_ops at all.
>>> We need callbacks for interfacing with the kobject layer, and for
>>> selecting our set of tags at mount time.  Not tagged_dir_ops so much
>>> as tagged_type_ops.
>> The kobject op seems a bit strange way to interface to me.  For mount,
>> yeah, we'll need a hook somewhere or pass it via mount option maybe.
> 
> I will look how if there is a place in the kobject layer to put it.  With
> a second but noticeably different user I can compare and see how hard that will be.

Great, thanks.

>>>     Further the abstraction is logically exactly one tag on a
>>>     (sb,directory) pair.
>> I'm not so sure here.  As a policy, maybe but I don't really see a
>> fundamental reason that the mechanism should enforce this.
> 
> Well in the first implementation.

This pretty much defines the interface and is likely to force future
users to fit themselves into it.

>>> 4. Interface with the kobject layer.
>>>    kobject_add calls sysfs_create_dir
>>>    kboject_rename calls sysfs_rename_dir 
>>>    kobject_del calls sysfs_remove_dir
>>>
>>>    For the first two operations we need a helper function to go from a
>>>    kobject to a tag.
>> Why not just add a parameter to sysfs_create_dir()?  It's just twisted.
> 
> I added it where it was easiest.  Adding a parameter to sysfs_create_dir
> simply means I have to add the function to the kobject layer.  It is certainly
> worth a second look though.

Is it difficult to just export it via kobject and device layer?  If
changing the default function is too much of a hassle (and I'm sure it
would be), just add an extended version which takes @tag.  The current
implementation feels like it tried too hard to not add intermediate
interfaces and ended up shooting outside from the innermost layer.

>>> We need helper functions for interfacing with the rest of the kernel. 
>> Yes, that's why I view it as strange.  These can be done in forward way
>> (by passing in mount options and/or arguments) but it's done by first
>> going into the sysfs and then calling back out to outer layer.
> 
> Well in the case of mount the default parameter at least is current, and
> there are good reasons for that.

I was imagining something like...

	mount -t sysfs -o ns=0,4,5 /my/sys

And let the userland control which ns's are visible in the particular
mount.  I'm not sure how useful that will be tho.

> On the other side I can't pass a tag through from the device layer to
> the kobject layer.  It isn't a concept the kobject layer supports.

I think it's best to make kobject layer support it.

> At least though the conversation is in relative agreement.  I will refresh
> the patches shortly and see where we are at.

Thanks a lot for the patience.  :-)

-- 
tejun
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Tejun Heo <htejun@gmail.com> writes:

> Is it difficult to just export it via kobject and device layer?

Well gregkh thought it wasn't a good idea last time I tried exploring
that.

> If
> changing the default function is too much of a hassle (and I'm sure it
> would be), just add an extended version which takes @tag.  The current
> implementation feels like it tried too hard to not add intermediate
> interfaces and ended up shooting outside from the innermost layer.

It tried for something that was simple to use and that worked.

Also the way things work.  I have to use all of the intermediate layers
and their calls to various functions.  So just passing a parameter through
doesn't work to well.

It looks to me like the clean solution is move kobject_tag into
kobj_type, and have it call some higher level function.

We also need to remove the maintenance disaster that is
kobject_set_name from sysfs_rename_dir.  And push it into
kobject_rename instead.  The error handling is harder in
that case but otherwise we should be in good shape.

>> On the other side I can't pass a tag through from the device layer to
>> the kobject layer.  It isn't a concept the kobject layer supports.
>
> I think it's best to make kobject layer support it.

Assuming Greg will accept it when he sees reasonable patches.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Wed, Jul 02, 2008 at 09:49:33AM -0700, Eric W. Biederman wrote:
> Assuming Greg will accept it when he sees reasonable patches.

I always accept "reasonable patches" :)

thanks,

greg k-h
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Hello, Eric.

Eric W. Biederman wrote:
>> If
>> changing the default function is too much of a hassle (and I'm sure it
>> would be), just add an extended version which takes @tag.  The current
>> implementation feels like it tried too hard to not add intermediate
>> interfaces and ended up shooting outside from the innermost layer.
> 
> It tried for something that was simple to use and that worked.
> 
> Also the way things work.  I have to use all of the intermediate layers
> and their calls to various functions.  So just passing a parameter through
> doesn't work to well.

There is rather large possibility that I'm just being dumb here
especially because I haven't reviewed the users of this facility, so all
the comments I'm making are from the POV of interfaces of sysfs and the
related layers.  I think I've made my concerns clear by now.  If you
still think the callbacks are the best way to go, please try to
enlighten me.  I really don't wanna be stopping something which is
better from ignorance.  Just give me some concrete examples or point me
to codes which show how and why the current interface is the best for
the users and switching isn't a good idea.

> It looks to me like the clean solution is move kobject_tag into
> kobj_type, and have it call some higher level function.
> 
> We also need to remove the maintenance disaster that is
> kobject_set_name from sysfs_rename_dir.  And push it into
> kobject_rename instead.  The error handling is harder in
> that case but otherwise we should be in good shape.

Heh... I personally think kobject layer as a whole should just be hidden
under the cabinet of device driver model but I'm having difficult time
convincing other people of it.  Anyways, fully agree the interaction
between kobject and sysfs is ugly at a lot of places.

>>> On the other side I can't pass a tag through from the device layer to
>>> the kobject layer.  It isn't a concept the kobject layer supports.
>> I think it's best to make kobject layer support it.
> 
> Assuming Greg will accept it when he sees reasonable patches.

Greg says he would.  :-)

Thanks a lot for your patience.

-- 
tejun
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Tejun Heo <htejun@gmail.com> writes:

> There is rather large possibility that I'm just being dumb here
> especially because I haven't reviewed the users of this facility, so all
> the comments I'm making are from the POV of interfaces of sysfs and the
> related layers.  I think I've made my concerns clear by now.  If you
> still think the callbacks are the best way to go, please try to
> enlighten me.  I really don't wanna be stopping something which is
> better from ignorance.  Just give me some concrete examples or point me
> to codes which show how and why the current interface is the best for
> the users and switching isn't a good idea.

Currently I think a callback on to get the tag from a kobject is the
best way to go.  That way we don't need to add a field to struct
kobject (and don't need the associated redundancy), and we can lookup
up the tag when we need it.

I have been playing with the code and just about have it ready
to go.  I just need to refactor all of my changes into clean
patches at this point, plus a bit of review and test.  Ben & Daniel
have given me a version of the previous patchset rebased unto the
latest -mm so that should help for the unchanged parts.

Introducing the sysfs_tag_type thing and pushing the functions to
the edges helps.  It especially cleans up the ugly mount/umount
situation allowing us to handle that with generic code.

Moving the kobject_tag into struct ktype works and looks roughly
as clean as what happens with attributes.  So I that seems reasonable,
and doesn't result in a significant change in the users.

The result of which means that I only have the helper function sysfs_creation_tag
left in sysfs/dir.c  Left in there are some of the nasties in dealing with symlinks.

At this point I believe I have achieved a nice degree of simplifying the sysfs
code in the current patches without really changing the users or
making it more complex for them.

I have not implemented ida tags, and I don't plan to.  That is just
unnecessary work right now.  The users are simple and the meat of the
logic would not change so it should be simple to add.

>> It looks to me like the clean solution is move kobject_tag into
>> kobj_type, and have it call some higher level function.
>> 
>> We also need to remove the maintenance disaster that is
>> kobject_set_name from sysfs_rename_dir.  And push it into
>> kobject_rename instead.  The error handling is harder in
>> that case but otherwise we should be in good shape.
>
> Heh... I personally think kobject layer as a whole should just be hidden
> under the cabinet of device driver model but I'm having difficult time
> convincing other people of it.  Anyways, fully agree the interaction
> between kobject and sysfs is ugly at a lot of places.

I would be happy if we could remove all nonsense kobject that are there just
for structural purposes but have no purpose otherwise.  Things like kobjects
for symlinks.  The kobject layer doesn't seem to have a clear identity
and purpose that I can see right now.

> Thanks a lot for your patience.

Welcome.  The code reached a point a while ago where it didn't make sense
to change it without review feedback.

Eric

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Wednesday 02 July 2008 06:37, Tejun Heo wrote:
> I was imagining something like...
>
> 	mount -t sysfs -o ns=0,4,5 /my/sys
>
> And let the userland control which ns's are visible in the particular
> mount.  I'm not sure how useful that will be tho.

Useful. However I didn't know that an indexnumber was associated with the 
network namespaces.

Especially if you want to use network namespaces for scaleability and not 
isolation purposes.

Regards,
-- 
Andreas Bach Aaen              System Developer, M. Sc. 
Tieto Enator A/S               tel: +45 89 38 51 00
Skanderborgvej 232             fax: +45 89 38 51 01
8260 Viby J      Denmark       andreas.aaen@tietoenator.com
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Eric W. Biederman wrote:
> Tejun Heo <htejun@gmail.com> writes:
> 
>> There is rather large possibility that I'm just being dumb here
>> especially because I haven't reviewed the users of this facility, so all
>> the comments I'm making are from the POV of interfaces of sysfs and the
>> related layers.  I think I've made my concerns clear by now.  If you
>> still think the callbacks are the best way to go, please try to
>> enlighten me.  I really don't wanna be stopping something which is
>> better from ignorance.  Just give me some concrete examples or point me
>> to codes which show how and why the current interface is the best for
>> the users and switching isn't a good idea.
> 
> Currently I think a callback on to get the tag from a kobject is the
> best way to go.  That way we don't need to add a field to struct
> kobject (and don't need the associated redundancy), and we can lookup
> up the tag when we need it.

The kobject events are sent through a netlink message which is not 
currently per network namespace. Shouldn't be useful to have a way to 
retrieve from the kobject the network namespace or the uevent socket 
associated with it ? IMHO having idr in the kobject + netns pointer 
associated may help to handle the sysfs isolation and makes the uevent 
per namespace trivial, no ?

> I have been playing with the code and just about have it ready
> to go.  I just need to refactor all of my changes into clean
> patches at this point, plus a bit of review and test.  Ben & Daniel
> have given me a version of the previous patchset rebased unto the
> latest -mm so that should help for the unchanged parts.
> 
> Introducing the sysfs_tag_type thing and pushing the functions to
> the edges helps.  It especially cleans up the ugly mount/umount
> situation allowing us to handle that with generic code.
> 
> Moving the kobject_tag into struct ktype works and looks roughly
> as clean as what happens with attributes.  So I that seems reasonable,
> and doesn't result in a significant change in the users.
> 
> The result of which means that I only have the helper function sysfs_creation_tag
> left in sysfs/dir.c  Left in there are some of the nasties in dealing with symlinks.
> 
> At this point I believe I have achieved a nice degree of simplifying the sysfs
> code in the current patches without really changing the users or
> making it more complex for them.
> 
> I have not implemented ida tags, and I don't plan to.  That is just
> unnecessary work right now.  The users are simple and the meat of the
> logic would not change so it should be simple to add.
> 
>>> It looks to me like the clean solution is move kobject_tag into
>>> kobj_type, and have it call some higher level function.
>>>
>>> We also need to remove the maintenance disaster that is
>>> kobject_set_name from sysfs_rename_dir.  And push it into
>>> kobject_rename instead.  The error handling is harder in
>>> that case but otherwise we should be in good shape.
>> Heh... I personally think kobject layer as a whole should just be hidden
>> under the cabinet of device driver model but I'm having difficult time
>> convincing other people of it.  Anyways, fully agree the interaction
>> between kobject and sysfs is ugly at a lot of places.
> 
> I would be happy if we could remove all nonsense kobject that are there just
> for structural purposes but have no purpose otherwise.  Things like kobjects
> for symlinks.  The kobject layer doesn't seem to have a clear identity
> and purpose that I can see right now.
> 
>> Thanks a lot for your patience.
> 
> Welcome.  The code reached a point a while ago where it didn't make sense
> to change it without review feedback.
> 
> Eric
> 
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/containers
> 


-- 






















































Sauf indication contraire ci-dessus:
Compagnie IBM France
Siège Social : Tour Descartes, 2, avenue Gambetta, La Défense 5, 92400
Courbevoie
RCS Nanterre 552 118 465
Forme Sociale : S.A.S.
Capital Social : 542.737.118 ?
SIREN/SIRET : 552 118 465 02430
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Daniel Lezcano <dlezcano@fr.ibm.com> writes:

> The kobject events are sent through a netlink message which is not currently per
> network namespace. Shouldn't be useful to have a way to retrieve from the
> kobject the network namespace or the uevent socket associated with it ? IMHO
> having idr in the kobject + netns pointer associated may help to handle the
> sysfs isolation and makes the uevent per namespace trivial, no ?

Grumble.  I have been conveniently been forgetting about that socket.
Similarly we have the user mode helpers to deal with.

For this conversation there is a simple answer.  All of that is in the
kobject layer, and works even when you compile sysfs out of your kernel.
Therefore it is a separate problem.  And sysfs idr tags have nothing
to do with it.

It is most definitely something we need to come back to.  I bet there
are some interesting interactions when you have multiple network devices
with the same name generating events.

Eric








_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Eric W. Biederman wrote:
> Daniel Lezcano <dlezcano@fr.ibm.com> writes:
> 
>> The kobject events are sent through a netlink message which is not currently per
>> network namespace. Shouldn't be useful to have a way to retrieve from the
>> kobject the network namespace or the uevent socket associated with it ? IMHO
>> having idr in the kobject + netns pointer associated may help to handle the
>> sysfs isolation and makes the uevent per namespace trivial, no ?
> 
> Grumble.  I have been conveniently been forgetting about that socket.
> Similarly we have the user mode helpers to deal with.
> 
> For this conversation there is a simple answer.  All of that is in the
> kobject layer, and works even when you compile sysfs out of your kernel.
> Therefore it is a separate problem.  And sysfs idr tags have nothing
> to do with it.

> It is most definitely something we need to come back to.  I bet there
> are some interesting interactions when you have multiple network devices
> with the same name generating events.

Indeed, we observed some fun things with one distro (which defines some
particular udev rules) when a device called eth0 in a namespace comes 
back to init net :)

Benjamin

> 
> Eric
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 


-- 
B e n j a m i n   T h e r y  - BULL/DT/Open Software R&D

    http://www.bull.com
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Eric W. Biederman wrote:
> Daniel Lezcano <dlezcano@fr.ibm.com> writes:
> 
>> The kobject events are sent through a netlink message which is not currently per
>> network namespace. Shouldn't be useful to have a way to retrieve from the
>> kobject the network namespace or the uevent socket associated with it ? IMHO
>> having idr in the kobject + netns pointer associated may help to handle the
>> sysfs isolation and makes the uevent per namespace trivial, no ?
> 
> Grumble.  I have been conveniently been forgetting about that socket.
> Similarly we have the user mode helpers to deal with.
> 
> For this conversation there is a simple answer.  All of that is in the
> kobject layer, and works even when you compile sysfs out of your kernel.
> Therefore it is a separate problem.  And sysfs idr tags have nothing
> to do with it.

Ah Ok, I am not really familiar with kobject/sysfs so I thought there 
was a proposition to store the id in the kobject instead of using the 
tag callbacks, so I figured, perhaps, the idr could have been used in 
the kobject layer and the sysfs being built upon that.

> It is most definitely something we need to come back to.  I bet there
> are some interesting interactions when you have multiple network devices
> with the same name generating events.

Yes as mentionned Benjamin, we have the eth0 in the init_net which is 
shut down when a network namespace with a netdev with the same name 
exits. There is a udev rule which ifdown eth0 :)
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Hello,

Eric W. Biederman wrote:
> Daniel Lezcano <dlezcano@fr.ibm.com> writes:
> 
>> The kobject events are sent through a netlink message which is not currently per
>> network namespace. Shouldn't be useful to have a way to retrieve from the
>> kobject the network namespace or the uevent socket associated with it ? IMHO
>> having idr in the kobject + netns pointer associated may help to handle the
>> sysfs isolation and makes the uevent per namespace trivial, no ?
> 
> Grumble.  I have been conveniently been forgetting about that socket.
> Similarly we have the user mode helpers to deal with.
> 
> For this conversation there is a simple answer.  All of that is in the
> kobject layer, and works even when you compile sysfs out of your kernel.
> Therefore it is a separate problem.  And sysfs idr tags have nothing
> to do with it.
> 
> It is most definitely something we need to come back to.  I bet there
> are some interesting interactions when you have multiple network devices
> with the same name generating events.

Related delta: I've been thinking that uevents should be part of sysfs
not kobject as that's what the userland is gonna associate the event
with.  Would that solve the problem you're thinking about?

Thanks.

-- 
tejun
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Tejun Heo wrote:
> Hello,
> 
> Eric W. Biederman wrote:
>> Daniel Lezcano <dlezcano@fr.ibm.com> writes:
>>
>>> The kobject events are sent through a netlink message which is not currently per
>>> network namespace. Shouldn't be useful to have a way to retrieve from the
>>> kobject the network namespace or the uevent socket associated with it ? IMHO
>>> having idr in the kobject + netns pointer associated may help to handle the
>>> sysfs isolation and makes the uevent per namespace trivial, no ?
>> Grumble.  I have been conveniently been forgetting about that socket.
>> Similarly we have the user mode helpers to deal with.
>>
>> For this conversation there is a simple answer.  All of that is in the
>> kobject layer, and works even when you compile sysfs out of your kernel.
>> Therefore it is a separate problem.  And sysfs idr tags have nothing
>> to do with it.
>>
>> It is most definitely something we need to come back to.  I bet there
>> are some interesting interactions when you have multiple network devices
>> with the same name generating events.
> 
> Related delta: I've been thinking that uevents should be part of sysfs
> not kobject as that's what the userland is gonna associate the event
> with.  Would that solve the problem you're thinking about?

uevents can work with the network namespaces being compiled in and the 
sysfs compiled out. AFAICS, uevents will be unable to handle multiple 
network namespaces if it is tied with sysfs, no ?
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Benjamin Thery <benjamin.thery@bull.net> writes:

> Indeed, we observed some fun things with one distro (which defines some
> particular udev rules) when a device called eth0 in a namespace comes back to
> init net :)

Speaking of.  Don't let me forget but I have a patch I need to send out
that deletes pseudo devices instead of sending them back to eth0.   We can't
do that for real hardware obviously but for things like veth and macvlan
devices it greatly simplifies the cleanup.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Tejun Heo <htejun@gmail.com> writes:

> Related delta: I've been thinking that uevents should be part of sysfs
> not kobject as that's what the userland is gonna associate the event
> with.  Would that solve the problem you're thinking about?

The good news is that uevent_sock is currently restricted to just
the initial network namespace (so the functionality completely
disappears in the other namespaces), and that it is broadcast only.

So it should be possible to look at who the client is and by some
magic criterian decide if it should receive the broadcast message.

The call to the user mode helper is trickier.  How do we setup the
proper user space context.

None of this is fundamentally hard just different work, for a different
day.

Eric

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

When multiple namespaces are in use we can get multiple kernel objects
with the same name which is currently impossible to represent in sysfs.
In particular directories like /sys/class/net and /sys/kernel/uids have
significant problems.

Not wanting to change the user space interface and wanting to have a simple
implementation where all objects are in the kobject and sysfs trees.  The
decision has been made to tag objects with the namespace they live in,
and in a particular mount of sysfs only display objects with the tag
that corresponds to the namespaces in effect when sysfs was mounted.

After the last round of reviews the mount/umount logic is
significantly cleaned up and easier to maintain.  From a 10,000
foot view the code and the way it functions has remained the
same since we settled on tagged directories a year or so ago.   I
intend any future cleanups to be as incremental patches on top
of this existing set.

Lack of these patches are keeping the generally complete network
namespace work in 2.6.26 from being used and tested more heavily.
Can we please get the patches merged?

These patches are based off of 2.6.26-rc8 + the -gregkh tree from
last night.  Hopefully that means they apply -mm -gregkh and
-linux-next.

Eric


_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

It finally dawned on me what the clean fix to sysfs_rename_dir
calling kobject_set_name is.  Move the work into kobject_rename
where it belongs.  The callers serialize us anyway so this is
safe.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/sysfs/dir.c        |    6 +-----
 include/linux/sysfs.h |    4 +---
 lib/kobject.c         |   17 +++++++++++++++--
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 8c0e4b9..146b86a 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -799,16 +799,12 @@ int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 	if (!new_dentry)
 		goto out_unlock;
 
-	/* rename kobject and sysfs_dirent */
+	/* rename sysfs_dirent */
 	error = -ENOMEM;
 	new_name = dup_name = kstrdup(new_name, GFP_KERNEL);
 	if (!new_name)
 		goto out_unlock;
 
-	error = kobject_set_name(kobj, "%s", new_name);
-	if (error)
-		goto out_unlock;
-
 	dup_name = sd->s_name;
 	sd->s_name = new_name;
 
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 84d92bb..f7e43ed 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -20,8 +20,6 @@
 struct kobject;
 struct module;
 
-extern int kobject_set_name(struct kobject *kobj, const char *name, ...)
-			    __attribute__((format(printf, 2, 3)));
 /* FIXME
  * The *owner field is no longer used, but leave around
  * until the tree gets cleaned up fully.
@@ -140,7 +138,7 @@ static inline void sysfs_remove_dir(struct kobject *kobj)
 
 static inline int sysfs_rename_dir(struct kobject *kobj, const char *new_name)
 {
-	return kobject_set_name(kobj, "%s", new_name);
+	return 0;
 }
 
 static inline int sysfs_move_dir(struct kobject *kobj,
diff --git a/lib/kobject.c b/lib/kobject.c
index 829b839..49b3bc4 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -451,6 +451,7 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
 {
 	int error = 0;
 	const char *devpath = NULL;
+	const char *dup_name = NULL, *name;
 	char *devpath_string = NULL;
 	char *envp[2];
 
@@ -474,15 +475,27 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
 	envp[0] = devpath_string;
 	envp[1] = NULL;
 
+	name = dup_name = kstrdup(new_name, GFP_KERNEL);
+	if (!name) {
+		error = -ENOMEM;
+		goto out;
+	}
+
 	error = sysfs_rename_dir(kobj, new_name);
+	if (error)
+		goto out;
+
+	/* Install the new kobject name */
+	dup_name = kobj->name;
+	kobj->name = name;
 
 	/* This function is mostly/only used for network interface.
 	 * Some hotplug package track interfaces by their name and
 	 * therefore want to know when the name is changed by the user. */
-	if (!error)
-		kobject_uevent_env(kobj, KOBJ_MOVE, envp);
+	kobject_uevent_env(kobj, KOBJ_MOVE, envp);
 
 out:
+	kfree(dup_name);
 	kfree(devpath_string);
 	kfree(devpath);
 	kobject_put(kobj);
-- 
1.5.3.rc6.17.g1911

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

I should mention patches 1-12 are the core of the work.

Patches 13-15 are the users.  Included in complete form primarily to
aid review.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Eric W. Biederman wrote:
> It finally dawned on me what the clean fix to sysfs_rename_dir
> calling kobject_set_name is.  Move the work into kobject_rename
> where it belongs.  The callers serialize us anyway so this is
> safe.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

Nice clean up.  Acked-by: Tejun Heo <tj@kernel.org>

-- 
tejun
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

> These patches are based off of 2.6.26-rc8 + the -gregkh tree from
> last night.  Hopefully that means they apply -mm -gregkh and
> -linux-next.

A quick update.   My patchset conflicts with the recently added
  driver-core-suppress-sysfs-warnings-for-device_rename.patch

> driver core: Suppress sysfs warnings for device_rename().
> 
> Renaming network devices to an already existing name is not
> something we want sysfs to print a scary warning for, since the
> callers can deal with this correctly. So let's introduce
> sysfs_create_link_nowarn() which gets rid of the common warning.

This patch is unnecessary as that path is never exercised anymore.
as: dev_change_name returns early in the case of a noop rename.

In addition my introduction sysfs_rename_link handles this case
cleanly by first removing the old link and then creating the new
link.  Preventing false positives when the link names are the same.

So it should be safe to drop Cornelia patch without a reoccurance
of scary errors.

Eric


_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Sat, 05 Jul 2008 21:42:57 -0700,
ebiederm@xmission.com (Eric W. Biederman) wrote:

> 
> > These patches are based off of 2.6.26-rc8 + the -gregkh tree from
> > last night.  Hopefully that means they apply -mm -gregkh and
> > -linux-next.
> 
> A quick update.   My patchset conflicts with the recently added
>   driver-core-suppress-sysfs-warnings-for-device_rename.patch
> 
> > driver core: Suppress sysfs warnings for device_rename().
> > 
> > Renaming network devices to an already existing name is not
> > something we want sysfs to print a scary warning for, since the
> > callers can deal with this correctly. So let's introduce
> > sysfs_create_link_nowarn() which gets rid of the common warning.
> 
> This patch is unnecessary as that path is never exercised anymore.
> as: dev_change_name returns early in the case of a noop rename.

My impression was that the networking folks didn't want any warnings for
renaming failures, not just not for renaming a device to the same name.

> 
> In addition my introduction sysfs_rename_link handles this case
> cleanly by first removing the old link and then creating the new
> link.  Preventing false positives when the link names are the same.

sysfs_rename_link() looks cleaner, I agree.

> 
> So it should be safe to drop Cornelia patch without a reoccurance
> of scary errors.

Hm, the description looks badly worded - I unfortunately left the old
text unchanged when I respun the patch :( The patch re-introduces the
warning in sysfs_add_one() which had been removed in the meanwhile and
makes device_rename() use a non-warning version. I still think we want
a warning for the general case since this is usually caused be some
problems in the calling code (and the alternative would be to add
checks to all callers.)
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Cornelia Huck <cornelia.huck@de.ibm.com> writes:

> My impression was that the networking folks didn't want any warnings for
> renaming failures, not just not for renaming a device to the same name.

Which would be reasonable.  Because all of the checks have been done
before sysfs is called so if sysfs sees a problem it is a sysfs bug.

>> In addition my introduction sysfs_rename_link handles this case
>> cleanly by first removing the old link and then creating the new
>> link.  Preventing false positives when the link names are the same.
>
> sysfs_rename_link() looks cleaner, I agree.
>
>> 
>> So it should be safe to drop Cornelia patch without a reoccurance
>> of scary errors.
>
> Hm, the description looks badly worded - I unfortunately left the old
> text unchanged when I respun the patch :( The patch re-introduces the
> warning in sysfs_add_one() which had been removed in the meanwhile and
> makes device_rename() use a non-warning version. I still think we want
> a warning for the general case since this is usually caused be some
> problems in the calling code (and the alternative would be to add
> checks to all callers.)

Right.  We just need to get the sysfs paths clean enough that we don't
emit false positives.  I think I have accomplished that for rename.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers