[RFC] fs/super.c: Why alloc_super use a static variable default_op?

[RFC] fs/super.c: Why alloc_super use a static variable default_op?

[rae l]

Why alloc_super use a static variable default_op?
the static struct super_operations default_op is just all zeros, and
just referenced as the initial value of a new allocated super_block,
what does it for?

the filesystem dependent code such as ext2_fill_super would fill this
field eventually,
and after carefully checked, it seems no one filesystem would need a
all zero default_op,

as the command output in the kernel source tree:
$ grep -RInw s_op fs/
You could check all the use of s_op.

/**
 *	alloc_super	-	create new superblock
 *	@type:	filesystem type superblock should belong to
 *
 *	Allocates and initializes a new &struct super_block.  alloc_super()
 *	returns a pointer new superblock or %NULL if allocation had failed.
 */
static struct super_block *alloc_super(struct file_system_type *type)
{
	struct super_block *s = kzalloc(sizeof(struct super_block),  GFP_USER);

	static struct super_operations default_op;

	if (s) {
		...
		s->s_op = &default_op;
		s->s_time_gran = 1000000000;
	}
out:
	return s;
}


-- 
Denis Cheng
Linux Application Developer

"One of my most productive days was throwing away 1000 lines of code."
 - Ken Thompson.

Re: [RFC] fs/super.c: Why alloc_super use a static variable default_op?

[Al Viro]

On Wed, Jul 25, 2007 at 11:48:35AM +0800, rae l wrote:
> Why alloc_super use a static variable default_op?
> the static struct super_operations default_op is just all zeros, and
> just referenced as the initial value of a new allocated super_block,
> what does it for?

So that we would not have to care about ->s_op *ever* being NULL.

Re: [RFC] fs/super.c: Why alloc_super use a static variable default_op?

[rae l]

On 7/25/07, Al Viro <viro@ftp.linux.org.uk> wrote:
> On Wed, Jul 25, 2007 at 11:48:35AM +0800, rae l wrote:
> > Why alloc_super use a static variable default_op?
> > the static struct super_operations default_op is just all zeros, and
> > just referenced as the initial value of a new allocated super_block,
> > what does it for?
>
> So that we would not have to care about ->s_op *ever* being NULL.
But is it valuable? Compared to a waste of sizeof(struct super_block)
bytes memory.

When some code want to refer fs_type->s_op, it almost always want to
refer some function pointer in s_op with fs_type->s_op->***, but all
pointers in default_op are all NULLs, what about this scenario?

and if you do grep s_op in the source code, you will found nowhere
will want to test s_op or dependent on s_op not NULL.

So my opinion is to remove default_ops, just keep new allocated s_op NULL.

>


-- 
Denis Cheng
Linux Application Developer

"One of my most productive days was throwing away 1000 lines of code."
 - Ken Thompson.

Re: [RFC] fs/super.c: Why alloc_super use a static variable default_op?

[Al Viro]

On Wed, Jul 25, 2007 at 12:29:17PM +0800, rae l wrote:
> But is it valuable? Compared to a waste of sizeof(struct super_block)
> bytes memory.

It's less that struct super_block, actually.

> When some code want to refer fs_type->s_op, it almost always want to
> refer some function pointer in s_op with fs_type->s_op->***, but all
> pointers in default_op are all NULLs, what about this scenario?

Yes, and?  You still need one test instead of two.  Which gets you
more than 21 words used by that sucker, only in .text instead of .bss.
 
> and if you do grep s_op in the source code, you will found nowhere
> will want to test s_op or dependent on s_op not NULL.

What?  fs/inode.c:
        if (sb->s_op->alloc_inode)
                inode = sb->s_op->alloc_inode(sb);
        else
                inode = (struct inode *) kmem_cache_alloc(inode_cachep, GFP_KERNEL);
and the same goes everywhere else.  Of course we don't check for
sb->s_op not being NULL - that's exactly why we are safe skipping such
tests.

Re: [RFC] fs/super.c: Why alloc_super use a static variable default_op?

[rae l]

On 7/25/07, Al Viro <viro@ftp.linux.org.uk> wrote:
> On Wed, Jul 25, 2007 at 12:29:17PM +0800, rae l wrote:
> > But is it valuable? Compared to a waste of sizeof(struct super_block)
> > bytes memory.
>
> It's less that struct super_block, actually.
>
> > When some code want to refer fs_type->s_op, it almost always want to
> > refer some function pointer in s_op with fs_type->s_op->***, but all
> > pointers in default_op are all NULLs, what about this scenario?
>
> Yes, and?  You still need one test instead of two.  Which gets you
> more than 21 words used by that sucker, only in .text instead of .bss.
>
> > and if you do grep s_op in the source code, you will found nowhere
> > will want to test s_op or dependent on s_op not NULL.
>
> What?  fs/inode.c:
>         if (sb->s_op->alloc_inode)
>                 inode = sb->s_op->alloc_inode(sb);
>         else
>                 inode = (struct inode *) kmem_cache_alloc(inode_cachep, GFP_KERNEL);
> and the same goes everywhere else.  Of course we don't check for
> sb->s_op not being NULL - that's exactly why we are safe skipping such
> tests.
Oh, Thank you.

But there are also many other subsystems will do
fs/dcache.c:
void dput(struct dentry *dentry)
	if (dentry->d_op && dentry->d_op->d_delete) {
Do you think it's worth optimizing it with a static d_op filled?

we can add a static variable to d_alloc and set its initial d_op to
this static variable?
struct dentry *d_alloc(struct dentry * parent, const struct qstr *name)

>

-- 
Denis Cheng
Linux Application Developer

"One of my most productive days was throwing away 1000 lines of code."
 - Ken Thompson.

Re: [RFC] fs/super.c: Why alloc_super use a static variable default_op?

[Al Viro]

On Wed, Jul 25, 2007 at 01:21:19PM +0800, rae l wrote:
 
> But there are also many other subsystems will do
> fs/dcache.c:
> void dput(struct dentry *dentry)
> 	if (dentry->d_op && dentry->d_op->d_delete) {
> Do you think it's worth optimizing it with a static d_op filled?
> 
> we can add a static variable to d_alloc and set its initial d_op to
> this static variable?
> struct dentry *d_alloc(struct dentry * parent, const struct qstr *name)

Try and compare...  It really depends - I suspect that for dentries the
situation differs since the case of ->d_op == NULL is *common*.  So
these checks actually might be a win - we are not unlikely to bail out
on the first one, without hitting the contents of *dentry->d_op.

For superblocks and inodes that is different - if we go looking for a method,
we *are* going to hit the method table anyway; it's not going to be NULL in
anything resembling a normal case.  So having the pointer to table initialized
that way is an obvious win - we don't really lose on space (savings in .text
at least balance the loss in .bss), we win on simpler logics and we actually
win by skipping the useless test.

How well would that pay for dentries...  Hell knows.  Space-wise it's the
same story, but we might end up doing extra work at accesses.  Try it - it's
not a hard patch to write, after all.

Re: [RFC] fs/super.c: Why alloc_super use a static variable default_op?

[Al Viro]

On Wed, Jul 25, 2007 at 07:39:53AM +0100, Al Viro wrote:

> For superblocks and inodes that is different - if we go looking for a method,
> we *are* going to hit the method table anyway; it's not going to be NULL in
> anything resembling a normal case.  So having the pointer to table initialized

"it" == "pointer to method table", of course.  Pointers _in_ method table
might very well be NULL...