[PATCH] efs: convert efs to use the new mount api

[PATCH] efs: convert efs to use the new mount api

[Bill O'Donnell]

Convert the efs filesystem to use the new mount API.

Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
---
 fs/efs/super.c | 114 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 84 insertions(+), 30 deletions(-)

diff --git a/fs/efs/super.c b/fs/efs/super.c
index f17fdac76b2e..c837ac89b384 100644
--- a/fs/efs/super.c
+++ b/fs/efs/super.c
@@ -14,19 +14,14 @@
 #include <linux/buffer_head.h>
 #include <linux/vfs.h>
 #include <linux/blkdev.h>
-
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
 #include "efs.h"
 #include <linux/efs_vh.h>
 #include <linux/efs_fs_sb.h>
 
 static int efs_statfs(struct dentry *dentry, struct kstatfs *buf);
-static int efs_fill_super(struct super_block *s, void *d, int silent);
-
-static struct dentry *efs_mount(struct file_system_type *fs_type,
-	int flags, const char *dev_name, void *data)
-{
-	return mount_bdev(fs_type, flags, dev_name, data, efs_fill_super);
-}
+static int efs_init_fs_context(struct fs_context *fc);
 
 static void efs_kill_sb(struct super_block *s)
 {
@@ -35,15 +30,6 @@ static void efs_kill_sb(struct super_block *s)
 	kfree(sbi);
 }
 
-static struct file_system_type efs_fs_type = {
-	.owner		= THIS_MODULE,
-	.name		= "efs",
-	.mount		= efs_mount,
-	.kill_sb	= efs_kill_sb,
-	.fs_flags	= FS_REQUIRES_DEV,
-};
-MODULE_ALIAS_FS("efs");
-
 static struct pt_types sgi_pt_types[] = {
 	{0x00,		"SGI vh"},
 	{0x01,		"SGI trkrepl"},
@@ -63,6 +49,27 @@ static struct pt_types sgi_pt_types[] = {
 	{0,		NULL}
 };
 
+enum {
+	Opt_explicit_open,
+};
+
+static const struct fs_parameter_spec efs_param_spec[] = {
+	fsparam_flag    ("explicit-open",       Opt_explicit_open),
+	{}
+};
+
+/*
+ * File system definition and registration.
+ */
+static struct file_system_type efs_fs_type = {
+	.owner			= THIS_MODULE,
+	.name			= "efs",
+	.kill_sb		= efs_kill_sb,
+	.fs_flags		= FS_REQUIRES_DEV,
+	.init_fs_context	= efs_init_fs_context,
+	.parameters		= efs_param_spec,
+};
+MODULE_ALIAS_FS("efs");
 
 static struct kmem_cache * efs_inode_cachep;
 
@@ -108,18 +115,10 @@ static void destroy_inodecache(void)
 	kmem_cache_destroy(efs_inode_cachep);
 }
 
-static int efs_remount(struct super_block *sb, int *flags, char *data)
-{
-	sync_filesystem(sb);
-	*flags |= SB_RDONLY;
-	return 0;
-}
-
 static const struct super_operations efs_superblock_operations = {
 	.alloc_inode	= efs_alloc_inode,
 	.free_inode	= efs_free_inode,
 	.statfs		= efs_statfs,
-	.remount_fs	= efs_remount,
 };
 
 static const struct export_operations efs_export_ops = {
@@ -249,26 +248,26 @@ static int efs_validate_super(struct efs_sb_info *sb, struct efs_super *super) {
 	return 0;    
 }
 
-static int efs_fill_super(struct super_block *s, void *d, int silent)
+static int efs_fill_super(struct super_block *s, struct fs_context *fc)
 {
 	struct efs_sb_info *sb;
 	struct buffer_head *bh;
 	struct inode *root;
 
- 	sb = kzalloc(sizeof(struct efs_sb_info), GFP_KERNEL);
+	sb = kzalloc(sizeof(struct efs_sb_info), GFP_KERNEL);
 	if (!sb)
 		return -ENOMEM;
 	s->s_fs_info = sb;
 	s->s_time_min = 0;
 	s->s_time_max = U32_MAX;
- 
+
 	s->s_magic		= EFS_SUPER_MAGIC;
 	if (!sb_set_blocksize(s, EFS_BLOCKSIZE)) {
 		pr_err("device does not support %d byte blocks\n",
 			EFS_BLOCKSIZE);
 		return -EINVAL;
 	}
-  
+
 	/* read the vh (volume header) block */
 	bh = sb_bread(s, 0);
 
@@ -294,7 +293,7 @@ static int efs_fill_super(struct super_block *s, void *d, int silent)
 		pr_err("cannot read superblock\n");
 		return -EIO;
 	}
-		
+
 	if (efs_validate_super(sb, (struct efs_super *) bh->b_data)) {
 #ifdef DEBUG
 		pr_warn("invalid superblock at block %u\n",
@@ -328,6 +327,61 @@ static int efs_fill_super(struct super_block *s, void *d, int silent)
 	return 0;
 }
 
+static void efs_free_fc(struct fs_context *fc)
+{
+	kfree(fc->fs_private);
+}
+
+static int efs_get_tree(struct fs_context *fc)
+{
+	return get_tree_bdev(fc, efs_fill_super);
+}
+
+static int efs_parse_param(struct fs_context *fc, struct fs_parameter *param)
+{
+	int token;
+	struct fs_parse_result result;
+
+	token = fs_parse(fc, efs_param_spec, param, &result);
+	if (token < 0)
+		return token;
+	return 0;
+}
+
+static int efs_reconfigure(struct fs_context *fc)
+{
+	sync_filesystem(fc->root->d_sb);
+
+	return 0;
+}
+
+struct efs_context {
+	unsigned long s_mount_opts;
+};
+
+static const struct fs_context_operations efs_context_opts = {
+	.parse_param	= efs_parse_param,
+	.get_tree	= efs_get_tree,
+	.reconfigure	= efs_reconfigure,
+	.free		= efs_free_fc,
+};
+
+/*
+ * Set up the filesystem mount context.
+ */
+static int efs_init_fs_context(struct fs_context *fc)
+{
+	struct efs_context *ctx;
+
+	ctx = kzalloc(sizeof(struct efs_context), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	fc->fs_private = ctx;
+	fc->ops = &efs_context_opts;
+
+	return 0;
+}
+
 static int efs_statfs(struct dentry *dentry, struct kstatfs *buf) {
 	struct super_block *sb = dentry->d_sb;
 	struct efs_sb_info *sbi = SUPER_INFO(sb);
-- 
2.43.2

Re: [PATCH] efs: convert efs to use the new mount api

[Matthew Wilcox]

On Mon, Feb 19, 2024 at 06:33:18PM -0600, Bill O'Donnell wrote:
> Convert the efs filesystem to use the new mount API.

Hey Bill, what testing were you able to do for this?  I found some EFS
images, but they didn't have any symlinks in them, so I wasn't able to
test my rewrite of the symlink handling code.  Do you have a source of
EFS images?

Re: [PATCH] efs: convert efs to use the new mount api

[Bill O'Donnell]

On Tue, Feb 20, 2024 at 12:46:35AM +0000, Matthew Wilcox wrote:
> On Mon, Feb 19, 2024 at 06:33:18PM -0600, Bill O'Donnell wrote:
> > Convert the efs filesystem to use the new mount API.
> 
> Hey Bill, what testing were you able to do for this?  I found some EFS
> images, but they didn't have any symlinks in them, so I wasn't able to
> test my rewrite of the symlink handling code.  Do you have a source of
> EFS images?
> 
https://archive.org/details/indigo-irix-4-0-5-datapartition.-7z

Re: [PATCH] efs: convert efs to use the new mount api

[Bill O'Donnell]

On Tue, Feb 20, 2024 at 12:46:35AM +0000, Matthew Wilcox wrote:
> On Mon, Feb 19, 2024 at 06:33:18PM -0600, Bill O'Donnell wrote:
> > Convert the efs filesystem to use the new mount API.
> 
> Hey Bill, what testing were you able to do for this?  I found some EFS
> images, but they didn't have any symlinks in them, so I wasn't able to
> test my rewrite of the symlink handling code.  Do you have a source of
> EFS images?
> 
Hi Matthew-
I only checked if it mounts and remounts. Since no mount options, my testing
was minimal.
Thanks-
Bill

Re: [PATCH] efs: convert efs to use the new mount api

[Christian Brauner]

On Mon, Feb 19, 2024 at 06:33:18PM -0600, Bill O'Donnell wrote:
> Convert the efs filesystem to use the new mount API.
> 
> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> ---

Thanks for doing this. One question below.

>  fs/efs/super.c | 114 ++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 84 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/efs/super.c b/fs/efs/super.c
> index f17fdac76b2e..c837ac89b384 100644
> --- a/fs/efs/super.c
> +++ b/fs/efs/super.c
> @@ -14,19 +14,14 @@
>  #include <linux/buffer_head.h>
>  #include <linux/vfs.h>
>  #include <linux/blkdev.h>
> -
> +#include <linux/fs_context.h>
> +#include <linux/fs_parser.h>
>  #include "efs.h"
>  #include <linux/efs_vh.h>
>  #include <linux/efs_fs_sb.h>
>  
>  static int efs_statfs(struct dentry *dentry, struct kstatfs *buf);
> -static int efs_fill_super(struct super_block *s, void *d, int silent);
> -
> -static struct dentry *efs_mount(struct file_system_type *fs_type,
> -	int flags, const char *dev_name, void *data)
> -{
> -	return mount_bdev(fs_type, flags, dev_name, data, efs_fill_super);
> -}
> +static int efs_init_fs_context(struct fs_context *fc);
>  
>  static void efs_kill_sb(struct super_block *s)
>  {
> @@ -35,15 +30,6 @@ static void efs_kill_sb(struct super_block *s)
>  	kfree(sbi);
>  }
>  
> -static struct file_system_type efs_fs_type = {
> -	.owner		= THIS_MODULE,
> -	.name		= "efs",
> -	.mount		= efs_mount,
> -	.kill_sb	= efs_kill_sb,
> -	.fs_flags	= FS_REQUIRES_DEV,
> -};
> -MODULE_ALIAS_FS("efs");
> -
>  static struct pt_types sgi_pt_types[] = {
>  	{0x00,		"SGI vh"},
>  	{0x01,		"SGI trkrepl"},
> @@ -63,6 +49,27 @@ static struct pt_types sgi_pt_types[] = {
>  	{0,		NULL}
>  };
>  
> +enum {
> +	Opt_explicit_open,
> +};
> +
> +static const struct fs_parameter_spec efs_param_spec[] = {
> +	fsparam_flag    ("explicit-open",       Opt_explicit_open),
> +	{}
> +};

That looks like it is copy-pasted from zonefs?

> +
> +/*
> + * File system definition and registration.
> + */
> +static struct file_system_type efs_fs_type = {
> +	.owner			= THIS_MODULE,
> +	.name			= "efs",
> +	.kill_sb		= efs_kill_sb,
> +	.fs_flags		= FS_REQUIRES_DEV,
> +	.init_fs_context	= efs_init_fs_context,
> +	.parameters		= efs_param_spec,
> +};
> +MODULE_ALIAS_FS("efs");
>  
>  static struct kmem_cache * efs_inode_cachep;
>  
> @@ -108,18 +115,10 @@ static void destroy_inodecache(void)
>  	kmem_cache_destroy(efs_inode_cachep);
>  }
>  
> -static int efs_remount(struct super_block *sb, int *flags, char *data)
> -{
> -	sync_filesystem(sb);
> -	*flags |= SB_RDONLY;
> -	return 0;
> -}
> -
>  static const struct super_operations efs_superblock_operations = {
>  	.alloc_inode	= efs_alloc_inode,
>  	.free_inode	= efs_free_inode,
>  	.statfs		= efs_statfs,
> -	.remount_fs	= efs_remount,
>  };
>  
>  static const struct export_operations efs_export_ops = {
> @@ -249,26 +248,26 @@ static int efs_validate_super(struct efs_sb_info *sb, struct efs_super *super) {
>  	return 0;    
>  }
>  
> -static int efs_fill_super(struct super_block *s, void *d, int silent)
> +static int efs_fill_super(struct super_block *s, struct fs_context *fc)
>  {
>  	struct efs_sb_info *sb;
>  	struct buffer_head *bh;
>  	struct inode *root;
>  
> - 	sb = kzalloc(sizeof(struct efs_sb_info), GFP_KERNEL);
> +	sb = kzalloc(sizeof(struct efs_sb_info), GFP_KERNEL);
>  	if (!sb)
>  		return -ENOMEM;
>  	s->s_fs_info = sb;
>  	s->s_time_min = 0;
>  	s->s_time_max = U32_MAX;
> - 
> +
>  	s->s_magic		= EFS_SUPER_MAGIC;
>  	if (!sb_set_blocksize(s, EFS_BLOCKSIZE)) {
>  		pr_err("device does not support %d byte blocks\n",
>  			EFS_BLOCKSIZE);
>  		return -EINVAL;
>  	}
> -  
> +
>  	/* read the vh (volume header) block */
>  	bh = sb_bread(s, 0);
>  
> @@ -294,7 +293,7 @@ static int efs_fill_super(struct super_block *s, void *d, int silent)
>  		pr_err("cannot read superblock\n");
>  		return -EIO;
>  	}
> -		
> +
>  	if (efs_validate_super(sb, (struct efs_super *) bh->b_data)) {
>  #ifdef DEBUG
>  		pr_warn("invalid superblock at block %u\n",
> @@ -328,6 +327,61 @@ static int efs_fill_super(struct super_block *s, void *d, int silent)
>  	return 0;
>  }
>  
> +static void efs_free_fc(struct fs_context *fc)
> +{
> +	kfree(fc->fs_private);
> +}
> +
> +static int efs_get_tree(struct fs_context *fc)
> +{
> +	return get_tree_bdev(fc, efs_fill_super);
> +}
> +
> +static int efs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> +{
> +	int token;
> +	struct fs_parse_result result;
> +
> +	token = fs_parse(fc, efs_param_spec, param, &result);
> +	if (token < 0)
> +		return token;

Any mount option here is completely ignored, no? Why even have any mount
options then? It's not required to implement ->parse_param.

> +	return 0;
> +}
> +
> +static int efs_reconfigure(struct fs_context *fc)
> +{
> +	sync_filesystem(fc->root->d_sb);
> +
> +	return 0;
> +}
> +
> +struct efs_context {
> +	unsigned long s_mount_opts;
> +};
> +
> +static const struct fs_context_operations efs_context_opts = {
> +	.parse_param	= efs_parse_param,
> +	.get_tree	= efs_get_tree,
> +	.reconfigure	= efs_reconfigure,
> +	.free		= efs_free_fc,
> +};
> +
> +/*
> + * Set up the filesystem mount context.
> + */
> +static int efs_init_fs_context(struct fs_context *fc)
> +{
> +	struct efs_context *ctx;
> +
> +	ctx = kzalloc(sizeof(struct efs_context), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +	fc->fs_private = ctx;
> +	fc->ops = &efs_context_opts;
> +
> +	return 0;
> +}
> +
>  static int efs_statfs(struct dentry *dentry, struct kstatfs *buf) {
>  	struct super_block *sb = dentry->d_sb;
>  	struct efs_sb_info *sbi = SUPER_INFO(sb);
> -- 
> 2.43.2
> 

Re: [PATCH] efs: convert efs to use the new mount api

[Bill O'Donnell]

On Tue, Feb 20, 2024 at 09:42:35AM +0100, Christian Brauner wrote:
> On Mon, Feb 19, 2024 at 06:33:18PM -0600, Bill O'Donnell wrote:
> > Convert the efs filesystem to use the new mount API.
> > 
> > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > ---
> 
> Thanks for doing this. One question below.
> 
> >  fs/efs/super.c | 114 ++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 84 insertions(+), 30 deletions(-)
> > 
> > diff --git a/fs/efs/super.c b/fs/efs/super.c
> > index f17fdac76b2e..c837ac89b384 100644
> > --- a/fs/efs/super.c
> > +++ b/fs/efs/super.c
> > @@ -14,19 +14,14 @@
> >  #include <linux/buffer_head.h>
> >  #include <linux/vfs.h>
> >  #include <linux/blkdev.h>
> > -
> > +#include <linux/fs_context.h>
> > +#include <linux/fs_parser.h>
> >  #include "efs.h"
> >  #include <linux/efs_vh.h>
> >  #include <linux/efs_fs_sb.h>
> >  
> >  static int efs_statfs(struct dentry *dentry, struct kstatfs *buf);
> > -static int efs_fill_super(struct super_block *s, void *d, int silent);
> > -
> > -static struct dentry *efs_mount(struct file_system_type *fs_type,
> > -	int flags, const char *dev_name, void *data)
> > -{
> > -	return mount_bdev(fs_type, flags, dev_name, data, efs_fill_super);
> > -}
> > +static int efs_init_fs_context(struct fs_context *fc);
> >  
> >  static void efs_kill_sb(struct super_block *s)
> >  {
> > @@ -35,15 +30,6 @@ static void efs_kill_sb(struct super_block *s)
> >  	kfree(sbi);
> >  }
> >  
> > -static struct file_system_type efs_fs_type = {
> > -	.owner		= THIS_MODULE,
> > -	.name		= "efs",
> > -	.mount		= efs_mount,
> > -	.kill_sb	= efs_kill_sb,
> > -	.fs_flags	= FS_REQUIRES_DEV,
> > -};
> > -MODULE_ALIAS_FS("efs");
> > -
> >  static struct pt_types sgi_pt_types[] = {
> >  	{0x00,		"SGI vh"},
> >  	{0x01,		"SGI trkrepl"},
> > @@ -63,6 +49,27 @@ static struct pt_types sgi_pt_types[] = {
> >  	{0,		NULL}
> >  };
> >  
> > +enum {
> > +	Opt_explicit_open,
> > +};
> > +
> > +static const struct fs_parameter_spec efs_param_spec[] = {
> > +	fsparam_flag    ("explicit-open",       Opt_explicit_open),
> > +	{}
> > +};
> 
> That looks like it is copy-pasted from zonefs?

Yes. efs_param_spec will be removed in v2.

> 
> > +
> > +/*
> > + * File system definition and registration.
> > + */
> > +static struct file_system_type efs_fs_type = {
> > +	.owner			= THIS_MODULE,
> > +	.name			= "efs",
> > +	.kill_sb		= efs_kill_sb,
> > +	.fs_flags		= FS_REQUIRES_DEV,
> > +	.init_fs_context	= efs_init_fs_context,
> > +	.parameters		= efs_param_spec,
> > +};
> > +MODULE_ALIAS_FS("efs");
> >  
> >  static struct kmem_cache * efs_inode_cachep;
> >  
> > @@ -108,18 +115,10 @@ static void destroy_inodecache(void)
> >  	kmem_cache_destroy(efs_inode_cachep);
> >  }
> >  
> > -static int efs_remount(struct super_block *sb, int *flags, char *data)
> > -{
> > -	sync_filesystem(sb);
> > -	*flags |= SB_RDONLY;
> > -	return 0;
> > -}
> > -
> >  static const struct super_operations efs_superblock_operations = {
> >  	.alloc_inode	= efs_alloc_inode,
> >  	.free_inode	= efs_free_inode,
> >  	.statfs		= efs_statfs,
> > -	.remount_fs	= efs_remount,
> >  };
> >  
> >  static const struct export_operations efs_export_ops = {
> > @@ -249,26 +248,26 @@ static int efs_validate_super(struct efs_sb_info *sb, struct efs_super *super) {
> >  	return 0;    
> >  }
> >  
> > -static int efs_fill_super(struct super_block *s, void *d, int silent)
> > +static int efs_fill_super(struct super_block *s, struct fs_context *fc)
> >  {
> >  	struct efs_sb_info *sb;
> >  	struct buffer_head *bh;
> >  	struct inode *root;
> >  
> > - 	sb = kzalloc(sizeof(struct efs_sb_info), GFP_KERNEL);
> > +	sb = kzalloc(sizeof(struct efs_sb_info), GFP_KERNEL);
> >  	if (!sb)
> >  		return -ENOMEM;
> >  	s->s_fs_info = sb;
> >  	s->s_time_min = 0;
> >  	s->s_time_max = U32_MAX;
> > - 
> > +
> >  	s->s_magic		= EFS_SUPER_MAGIC;
> >  	if (!sb_set_blocksize(s, EFS_BLOCKSIZE)) {
> >  		pr_err("device does not support %d byte blocks\n",
> >  			EFS_BLOCKSIZE);
> >  		return -EINVAL;
> >  	}
> > -  
> > +
> >  	/* read the vh (volume header) block */
> >  	bh = sb_bread(s, 0);
> >  
> > @@ -294,7 +293,7 @@ static int efs_fill_super(struct super_block *s, void *d, int silent)
> >  		pr_err("cannot read superblock\n");
> >  		return -EIO;
> >  	}
> > -		
> > +
> >  	if (efs_validate_super(sb, (struct efs_super *) bh->b_data)) {
> >  #ifdef DEBUG
> >  		pr_warn("invalid superblock at block %u\n",
> > @@ -328,6 +327,61 @@ static int efs_fill_super(struct super_block *s, void *d, int silent)
> >  	return 0;
> >  }
> >  
> > +static void efs_free_fc(struct fs_context *fc)
> > +{
> > +	kfree(fc->fs_private);
> > +}
> > +
> > +static int efs_get_tree(struct fs_context *fc)
> > +{
> > +	return get_tree_bdev(fc, efs_fill_super);
> > +}
> > +
> > +static int efs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> > +{
> > +	int token;
> > +	struct fs_parse_result result;
> > +
> > +	token = fs_parse(fc, efs_param_spec, param, &result);
> > +	if (token < 0)
> > +		return token;
> 
> Any mount option here is completely ignored, no? Why even have any mount
> options then? It's not required to implement ->parse_param.

Correct. My wrong-thinking was that parse_param needed to be there for vfs,
regardless of the lack of mount options. I'll remove it.

Thanks for the review. I'll submit a v2.
Bill

> 
> > +	return 0;
> > +}
> > +
> > +static int efs_reconfigure(struct fs_context *fc)
> > +{
> > +	sync_filesystem(fc->root->d_sb);
> > +
> > +	return 0;
> > +}
> > +
> > +struct efs_context {
> > +	unsigned long s_mount_opts;
> > +};
> > +
> > +static const struct fs_context_operations efs_context_opts = {
> > +	.parse_param	= efs_parse_param,
> > +	.get_tree	= efs_get_tree,
> > +	.reconfigure	= efs_reconfigure,
> > +	.free		= efs_free_fc,
> > +};
> > +
> > +/*
> > + * Set up the filesystem mount context.
> > + */
> > +static int efs_init_fs_context(struct fs_context *fc)
> > +{
> > +	struct efs_context *ctx;
> > +
> > +	ctx = kzalloc(sizeof(struct efs_context), GFP_KERNEL);
> > +	if (!ctx)
> > +		return -ENOMEM;
> > +	fc->fs_private = ctx;
> > +	fc->ops = &efs_context_opts;
> > +
> > +	return 0;
> > +}
> > +
> >  static int efs_statfs(struct dentry *dentry, struct kstatfs *buf) {
> >  	struct super_block *sb = dentry->d_sb;
> >  	struct efs_sb_info *sbi = SUPER_INFO(sb);
> > -- 
> > 2.43.2
> > 
>