Since sysfs_mount is static and used only in sysfs_init function, it could be just an automatic variable.

Since sysfs_mount is static and used only in sysfs_init function, it could be just an automatic variable.

[rae l]

---
and I still have questions about this code:
1. Why here kern_mount is needed?
	Or the first time user space `mount -t sysfs` won't do that?
2. If root executes many mounts to mount sysfs on /sys and many other places,
	are there many instances of struct vfsmount those have only
mnt_mountpoint different?

	For most common case, mount a virtual filesystem(proc, sysfs, ...) on
multiple mounting point,
	how to handle it more efficiently?

and where is a detailed explaination on kern_mount? could someone give
some comments or documentation pointers on this?

diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
index 7416826..add0dda 100644
--- a/fs/sysfs/mount.c
+++ b/fs/sysfs/mount.c
@@ -22,7 +22,6 @@
 /* Random magic number */
 #define SYSFS_MAGIC 0x62656572

-static struct vfsmount *sysfs_mount;
 struct super_block * sysfs_sb = NULL;
 struct kmem_cache *sysfs_dir_cachep;

@@ -98,11 +97,10 @@ int __init sysfs_init(void)

 	err = register_filesystem(&sysfs_fs_type);
 	if (!err) {
-		sysfs_mount = kern_mount(&sysfs_fs_type);
+		struct vfsmount *sysfs_mount = kern_mount(&sysfs_fs_type);
 		if (IS_ERR(sysfs_mount)) {
 			printk(KERN_ERR "sysfs: could not mount!\n");
 			err = PTR_ERR(sysfs_mount);
-			sysfs_mount = NULL;
 			unregister_filesystem(&sysfs_fs_type);
 			goto out_err;
 		}

-- 
Denis Cheng

Re: Since sysfs_mount is static and used only in sysfs_init function, it could be just an automatic variable.

[Greg KH]

On Sun, Dec 02, 2007 at 04:31:09AM +0800, rae l wrote:
> ---
> and I still have questions about this code:
> 1. Why here kern_mount is needed?
> 	Or the first time user space `mount -t sysfs` won't do that?
> 2. If root executes many mounts to mount sysfs on /sys and many other places,
> 	are there many instances of struct vfsmount those have only
> mnt_mountpoint different?
> 
> 	For most common case, mount a virtual filesystem(proc, sysfs, ...) on
> multiple mounting point,
> 	how to handle it more efficiently?
> 
> and where is a detailed explaination on kern_mount? could someone give
> some comments or documentation pointers on this?

See the patches that Eric Biederman just posted to lkml for why this
structure is a static pointer this way right now, it's in preparation
for future patches.

Hope this helps,

greg k-h

Re: Since sysfs_mount is static and used only in sysfs_init function, it could be just an automatic variable.

[rae l]

On Dec 2, 2007 12:48 PM, Greg KH <gregkh@suse.de> wrote:
...
> > and where is a detailed explaination on kern_mount? could someone give
> > some comments or documentation pointers on this?
>
> See the patches that Eric Biederman just posted to lkml for why this
> structure is a static pointer this way right now, it's in preparation
> for future patches.
I have checked commit 7d0c7d676cc066413e1583b5af9fba8011972d41 by Eric
W. Biederman,
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d0c7d676cc066413e1583b5af9fba8011972d41

which just make sysfs_mount from externally visible to static that
could be only used in one c file,

but I mean that the static variable is still on kernel bss section,
this consumes a pointer (4 or 8 bytes) memory,

through a grep from fs/sysfs/, it appears that the variable
sysfs_mount is only used in the sysfs_init function,

$ grep -RsInw sysfs_mount fs/sysfs/
fs/sysfs/mount.c:25:static struct vfsmount *sysfs_mount;
fs/sysfs/mount.c:101:           sysfs_mount = kern_mount(&sysfs_fs_type);
fs/sysfs/mount.c:102:           if (IS_ERR(sysfs_mount)) {
fs/sysfs/mount.c:104:                   err = PTR_ERR(sysfs_mount);
fs/sysfs/mount.c:105:                   sysfs_mount = NULL;

we could mark this variable an automatic one, which scope is just in
this function, thus created and destroyed with the stack,
this approach does not consume a pointer on kernel bss section,

Why not do this?

-- 
Denis Cheng

Re: Since sysfs_mount is static and used only in sysfs_init function, it could be just an automatic variable.

[Greg KH]

On Sun, Dec 02, 2007 at 02:52:17PM +0800, rae l wrote:
> On Dec 2, 2007 12:48 PM, Greg KH <gregkh@suse.de> wrote:
> ...
> > > and where is a detailed explaination on kern_mount? could someone give
> > > some comments or documentation pointers on this?
> >
> > See the patches that Eric Biederman just posted to lkml for why this
> > structure is a static pointer this way right now, it's in preparation
> > for future patches.
> I have checked commit 7d0c7d676cc066413e1583b5af9fba8011972d41 by Eric
> W. Biederman,
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d0c7d676cc066413e1583b5af9fba8011972d41
> 
> which just make sysfs_mount from externally visible to static that
> could be only used in one c file,
> 
> but I mean that the static variable is still on kernel bss section,
> this consumes a pointer (4 or 8 bytes) memory,
> 
> through a grep from fs/sysfs/, it appears that the variable
> sysfs_mount is only used in the sysfs_init function,
> 
> $ grep -RsInw sysfs_mount fs/sysfs/
> fs/sysfs/mount.c:25:static struct vfsmount *sysfs_mount;
> fs/sysfs/mount.c:101:           sysfs_mount = kern_mount(&sysfs_fs_type);
> fs/sysfs/mount.c:102:           if (IS_ERR(sysfs_mount)) {
> fs/sysfs/mount.c:104:                   err = PTR_ERR(sysfs_mount);
> fs/sysfs/mount.c:105:                   sysfs_mount = NULL;
> 
> we could mark this variable an automatic one, which scope is just in
> this function, thus created and destroyed with the stack,
> this approach does not consume a pointer on kernel bss section,
> 
> Why not do this?

Again, see the patches he _just_ posted to lkml, the specific message
you are looking for is:
	Message-ID: <m11wa693t0.fsf@ebiederm.dsl.xmission.com>
	Subject: [PATCH 01/10] sysfs: Make sysfs_mount static again.

Also see the whole long thread for more details.

If you have further questions about this, please ask Eric.

thanks,

greg k-h

Re: Since sysfs_mount is static and used only in sysfs_init function, it could be just an automatic variable.

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> On Sun, Dec 02, 2007 at 02:52:17PM +0800, rae l wrote:
>> On Dec 2, 2007 12:48 PM, Greg KH <gregkh@suse.de> wrote:
>> ...
>> > > and where is a detailed explaination on kern_mount? could someone give
>> > > some comments or documentation pointers on this?
>> >
>> > See the patches that Eric Biederman just posted to lkml for why this
>> > structure is a static pointer this way right now, it's in preparation
>> > for future patches.
>> I have checked commit 7d0c7d676cc066413e1583b5af9fba8011972d41 by Eric
>> W. Biederman,
>>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d0c7d676cc066413e1583b5af9fba8011972d41
>> 
>> which just make sysfs_mount from externally visible to static that
>> could be only used in one c file,
>> 
>> but I mean that the static variable is still on kernel bss section,
>> this consumes a pointer (4 or 8 bytes) memory,
>> 
>> through a grep from fs/sysfs/, it appears that the variable
>> sysfs_mount is only used in the sysfs_init function,
>> 
>> $ grep -RsInw sysfs_mount fs/sysfs/
>> fs/sysfs/mount.c:25:static struct vfsmount *sysfs_mount;
>> fs/sysfs/mount.c:101:           sysfs_mount = kern_mount(&sysfs_fs_type);
>> fs/sysfs/mount.c:102:           if (IS_ERR(sysfs_mount)) {
>> fs/sysfs/mount.c:104:                   err = PTR_ERR(sysfs_mount);
>> fs/sysfs/mount.c:105:                   sysfs_mount = NULL;
>> 
>> we could mark this variable an automatic one, which scope is just in
>> this function, thus created and destroyed with the stack,
>> this approach does not consume a pointer on kernel bss section,
>> 
>> Why not do this?
>
> Again, see the patches he _just_ posted to lkml, the specific message
> you are looking for is:
> 	Message-ID: <m11wa693t0.fsf@ebiederm.dsl.xmission.com>
> 	Subject: [PATCH 01/10] sysfs: Make sysfs_mount static again.
>
> Also see the whole long thread for more details.

As long as you aren't talking about the subthread that spun off
about GPL exports, and just the rest of my patches and showing
where I am going that sounds reasonable.

> If you have further questions about this, please ask Eric.

Honestly I think there is a reasonable chance we could kill
sysfs_mount and the kern_mount entirely.  We used to need  the
internal kernel mount because of the coupling between sysfs_dirent
and the directory dentries but that is gone now.

So basically the variable sysfs_mount is static because sysfs_mount
used to be simply global I changed it to be static, and I haven't
looked at optimization opportunities beyond that.

So as long as we are using sysfs_mount less and don't get in
the way of multiple superblocks for sysfs I'm happy.

Eric

Re: Since sysfs_mount is static and used only in sysfs_init function, it could be just an automatic variable.

[Greg KH]

On Sun, Dec 02, 2007 at 03:22:46PM -0700, Eric W. Biederman wrote:
> Greg KH <gregkh@suse.de> writes:
> 
> > On Sun, Dec 02, 2007 at 02:52:17PM +0800, rae l wrote:
> >> On Dec 2, 2007 12:48 PM, Greg KH <gregkh@suse.de> wrote:
> >> ...
> >> > > and where is a detailed explaination on kern_mount? could someone give
> >> > > some comments or documentation pointers on this?
> >> >
> >> > See the patches that Eric Biederman just posted to lkml for why this
> >> > structure is a static pointer this way right now, it's in preparation
> >> > for future patches.
> >> I have checked commit 7d0c7d676cc066413e1583b5af9fba8011972d41 by Eric
> >> W. Biederman,
> >>
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d0c7d676cc066413e1583b5af9fba8011972d41
> >> 
> >> which just make sysfs_mount from externally visible to static that
> >> could be only used in one c file,
> >> 
> >> but I mean that the static variable is still on kernel bss section,
> >> this consumes a pointer (4 or 8 bytes) memory,
> >> 
> >> through a grep from fs/sysfs/, it appears that the variable
> >> sysfs_mount is only used in the sysfs_init function,
> >> 
> >> $ grep -RsInw sysfs_mount fs/sysfs/
> >> fs/sysfs/mount.c:25:static struct vfsmount *sysfs_mount;
> >> fs/sysfs/mount.c:101:           sysfs_mount = kern_mount(&sysfs_fs_type);
> >> fs/sysfs/mount.c:102:           if (IS_ERR(sysfs_mount)) {
> >> fs/sysfs/mount.c:104:                   err = PTR_ERR(sysfs_mount);
> >> fs/sysfs/mount.c:105:                   sysfs_mount = NULL;
> >> 
> >> we could mark this variable an automatic one, which scope is just in
> >> this function, thus created and destroyed with the stack,
> >> this approach does not consume a pointer on kernel bss section,
> >> 
> >> Why not do this?
> >
> > Again, see the patches he _just_ posted to lkml, the specific message
> > you are looking for is:
> > 	Message-ID: <m11wa693t0.fsf@ebiederm.dsl.xmission.com>
> > 	Subject: [PATCH 01/10] sysfs: Make sysfs_mount static again.
> >
> > Also see the whole long thread for more details.
> 
> As long as you aren't talking about the subthread that spun off
> about GPL exports, and just the rest of my patches and showing
> where I am going that sounds reasonable.

Yes, that subthread really had nothing to do with your patch series :)

> > If you have further questions about this, please ask Eric.
> 
> Honestly I think there is a reasonable chance we could kill
> sysfs_mount and the kern_mount entirely.  We used to need  the
> internal kernel mount because of the coupling between sysfs_dirent
> and the directory dentries but that is gone now.

Hm, this does take into account the fact that we internally mount sysfs
as part of the boot process to determine the boot disk major:minor,
right?  As long as we don't break that, I'm happy.

thanks,

greg k-h

Re: Since sysfs_mount is static and used only in sysfs_init function, it could be just an automatic variable.

[Kay Sievers]

On Dec 2, 2007 11:53 PM, Greg KH <gregkh@suse.de> wrote:
>
> On Sun, Dec 02, 2007 at 03:22:46PM -0700, Eric W. Biederman wrote:
> > Greg KH <gregkh@suse.de> writes:
> >
> > > On Sun, Dec 02, 2007 at 02:52:17PM +0800, rae l wrote:
> > >> On Dec 2, 2007 12:48 PM, Greg KH <gregkh@suse.de> wrote:
> > >> ...
> > >> > > and where is a detailed explaination on kern_mount? could someone give
> > >> > > some comments or documentation pointers on this?
> > >> >
> > >> > See the patches that Eric Biederman just posted to lkml for why this
> > >> > structure is a static pointer this way right now, it's in preparation
> > >> > for future patches.
> > >> I have checked commit 7d0c7d676cc066413e1583b5af9fba8011972d41 by Eric
> > >> W. Biederman,
> > >>
> > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d0c7d676cc066413e1583b5af9fba8011972d41
> > >>
> > >> which just make sysfs_mount from externally visible to static that
> > >> could be only used in one c file,
> > >>
> > >> but I mean that the static variable is still on kernel bss section,
> > >> this consumes a pointer (4 or 8 bytes) memory,
> > >>
> > >> through a grep from fs/sysfs/, it appears that the variable
> > >> sysfs_mount is only used in the sysfs_init function,
> > >>
> > >> $ grep -RsInw sysfs_mount fs/sysfs/
> > >> fs/sysfs/mount.c:25:static struct vfsmount *sysfs_mount;
> > >> fs/sysfs/mount.c:101:           sysfs_mount = kern_mount(&sysfs_fs_type);
> > >> fs/sysfs/mount.c:102:           if (IS_ERR(sysfs_mount)) {
> > >> fs/sysfs/mount.c:104:                   err = PTR_ERR(sysfs_mount);
> > >> fs/sysfs/mount.c:105:                   sysfs_mount = NULL;
> > >>
> > >> we could mark this variable an automatic one, which scope is just in
> > >> this function, thus created and destroyed with the stack,
> > >> this approach does not consume a pointer on kernel bss section,
> > >>
> > >> Why not do this?
> > >
> > > Again, see the patches he _just_ posted to lkml, the specific message
> > > you are looking for is:
> > >     Message-ID: <m11wa693t0.fsf@ebiederm.dsl.xmission.com>
> > >     Subject: [PATCH 01/10] sysfs: Make sysfs_mount static again.
> > >
> > > Also see the whole long thread for more details.
> >
> > As long as you aren't talking about the subthread that spun off
> > about GPL exports, and just the rest of my patches and showing
> > where I am going that sounds reasonable.
>
> Yes, that subthread really had nothing to do with your patch series :)
>
> > > If you have further questions about this, please ask Eric.
> >
> > Honestly I think there is a reasonable chance we could kill
> > sysfs_mount and the kern_mount entirely.  We used to need  the
> > internal kernel mount because of the coupling between sysfs_dirent
> > and the directory dentries but that is gone now.
>
> Hm, this does take into account the fact that we internally mount sysfs
> as part of the boot process to determine the boot disk major:minor,
> right?  As long as we don't break that, I'm happy.

That cruft is going away with the block patch in your tree.

Kay

Re: Since sysfs_mount is static and used only in sysfs_init function, it could be just an automatic variable.

[Eric W. Biederman]

"Kay Sievers" <kay.sievers@vrfy.org> writes:

> That cruft is going away with the block patch in your tree.

And it doesn't matter because it is a separate mount anyway.
Although I have to admit I hadn't taken that mount into
account earlier.

Eric