Btrfs devel - Oct 2012 - [RESEND PATCH] fs/super.c set_anon_super calling optimization

Hi,
set_anon_super is called by many filesystems. Some call directly and 
some call through the wrapper. Many of them in the wrapper''s call to 
this function are passing the second argument to this function which is 
not used anywhere.

This patch replaces the second variable with NULL.

Thanks,
Abhijit Pawar

Signed-off-by: Abhijit Pawar <abhi.c.pawar@gmail.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: linux-kernel@vger.kernel.org
CC: linux-btrfs@vger.kernel.org

---

diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
index 137d503..132db90 100644
--- a/fs/9p/vfs_super.c
+++ b/fs/9p/vfs_super.c
@@ -61,7 +61,7 @@ static const struct super_operations v9fs_super_ops,
v9fs_super_ops_dotl;
static int v9fs_set_super(struct super_block *s, void *data)
{
         s->s_fs_info = data;
-       return set_anon_super(s, data);
+       return set_anon_super(s, NULL);
}

/**
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 915ac14..c9994a3 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -954,7 +954,7 @@ static int btrfs_test_super(struct super_block *s, void
*data)

static int btrfs_set_super(struct super_block *s, void *data)
{
-       int err = set_anon_super(s, data);
+       int err = set_anon_super(s, NULL);
         if (!err)
                 s->s_fs_info = data;
         return err;
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index e831bce..486bf7a 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2260,7 +2260,7 @@ static int nfs_set_super(struct super_block *s, void
*data)
         s->s_flags = sb_mntdata->mntflags;
         s->s_fs_info = server;
         s->s_d_op = server->nfs_client->rpc_ops->dentry_ops;
-       ret = set_anon_super(s, server);
+       ret = set_anon_super(s, NULL);
         if (ret == 0)
                 server->s_dev = s->s_dev;
         return ret;
diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
index 71eb7e2..56d0059 100644
--- a/fs/sysfs/mount.c
+++ b/fs/sysfs/mount.c
@@ -89,7 +89,7 @@ static int sysfs_test_super(struct super_block *sb, void
*data)
static int sysfs_set_super(struct super_block *sb, void *data)
{
         int error;
-       error = set_anon_super(sb, data);
+       error = set_anon_super(sb, NULL);
         if (!error)
                 sb->s_fs_info = data;
         return error;

Hi,

On Thu, Oct 25, 2012 at 05:08:19PM +0530, Abhijit Pawar
wrote:
> Hi,
> set_anon_super is called by many filesystems. Some call directly and
> some call through the wrapper. Many of them in the wrapper''s call
to
> this function are passing the second argument to this function which
> is not used anywhere.
> 
> This patch replaces the second variable with NULL.
> 
If the variable isn''t used anymore, why don''t just get rid of
it, instead of
call the function passing a NULL pointer on it?

-- 
--Carlos

On 10/26/2012 06:44 PM, Carlos Maiolino wrote:
> Hi,
>
> On Thu, Oct 25, 2012 at 05:08:19PM +0530, Abhijit Pawar wrote:
>> Hi,
>> set_anon_super is called by many filesystems. Some call directly and
>> some call through the wrapper. Many of them in the wrapper''s
call to
>> this function are passing the second argument to this function which
>> is not used anywhere.
>>
>> This patch replaces the second variable with NULL.
>>
>
> If the variable isn''t used anymore, why don''t just get
rid of it, instead of
> call the function passing a NULL pointer on it?
At the moment its a callback function with two params so that 
filesystems are free to override it while mounting. This is to support 
filesystem specific information at mount time.

nfs uses it to get its server specific information. btrfs uses it to 
populate its filesystem information. So the signature can not be changed 
without affecting these filesystems.

sysfs, ceph, 9p will also be affected if we are to change the signature.
>

-- 
-
Abhijit

On Fri, Oct 26, 2012 at 11:14:41AM -0200, Carlos Maiolino
wrote:
> Hi,
> 
> On Thu, Oct 25, 2012 at 05:08:19PM +0530, Abhijit Pawar wrote:
> > Hi,
> > set_anon_super is called by many filesystems. Some call directly and
> > some call through the wrapper. Many of them in the wrapper''s
call to
> > this function are passing the second argument to this function which
> > is not used anywhere.
> > 
> > This patch replaces the second variable with NULL.
> > 
> 
> If the variable isn''t used anymore, why don''t just get
rid of it, instead of
> call the function passing a NULL pointer on it?
	Because we want it to be a valid sget() callback.  I doubt that this
optimization is worth doing, though - might even micro-pessimize the things
on architectures where all arguments are passed in registers.

On 11/30/2012 09:35 AM, Al Viro wrote:
> On Fri, Oct 26, 2012 at 11:14:41AM -0200, Carlos Maiolino wrote:
>> Hi,
>>
>> On Thu, Oct 25, 2012 at 05:08:19PM +0530, Abhijit Pawar wrote:
>>> Hi,
>>> set_anon_super is called by many filesystems. Some call directly
and
>>> some call through the wrapper. Many of them in the
wrapper''s call to
>>> this function are passing the second argument to this function
which
>>> is not used anywhere.
>>>
>>> This patch replaces the second variable with NULL.
>>>
>>
>> If the variable isn''t used anymore, why don''t just
get rid of it, instead of
>> call the function passing a NULL pointer on it?
> 
> 	Because we want it to be a valid sget() callback.  I doubt that this
> optimization is worth doing, though - might even micro-pessimize the things
> on architectures where all arguments are passed in registers.
> 
Al,
Yes. it will be helpful in registers case.

-- 
-
Abhijit

On Fri, Nov 30, 2012 at 11:10:02AM +0530, Abhijit Pawar wrote:
> > 	Because we want it to be a valid sget() callback.  I doubt that this
> > optimization is worth doing, though - might even micro-pessimize the
things
> > on architectures where all arguments are passed in registers.
> > 
> Al,
> Yes. it will be helpful in registers case.
How so?  Consider something like
static int btrfs_set_super(struct super_block *s, void *data)
{
        int err = set_anon_super(s, data);
        if (!err)
                s->s_fs_info = data;
        return err;
}
Compile it e.g. for alpha.  Or powerpc.  Or amd64, for that matter.
With and without your change.  And compare the resulting assembler.

Hell, if the arguments are passed in register, without your patch
we have the args for set_anon_super() all set just as we enter
btrfs_set_super().  With your patch the second one needs to be zeroed
out...

In any case, that''s microoptimization in the best case and on quite a
few
architectures it''s a pessimization (granted, an equally minor one).
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html