| Message ID | 508924AB.4060902@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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. >
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. -- 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
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.
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
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;
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 --- return error; -- 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