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

[Bill O'Donnell <bodonnel@redhat.com>]

On Tue, Feb 20, 2024 at 03:05:38PM -0600, Eric Sandeen wrote:
> On 2/20/24 8:45 AM, Bill O'Donnell wrote:
> > Convert the efs filesystem to use the new mount API.
> > 
> > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > ---
> > 
> > Changelog:
> > v2: Remove efs_param_spec and efs_parse_param, since no mount options.
> 
> A few more items below
> 
> > ---
> >  fs/efs/super.c | 91 +++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 61 insertions(+), 30 deletions(-)
> > 
> > diff --git a/fs/efs/super.c b/fs/efs/super.c
> > index f17fdac76b2e..d86c84e9e497 100644
> > --- a/fs/efs/super.c
> > +++ b/fs/efs/super.c
> > @@ -14,19 +14,13 @@
> >  #include <linux/buffer_head.h>
> >  #include <linux/vfs.h>
> >  #include <linux/blkdev.h>
> > -
> > +#include <linux/fs_context.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 +29,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 +48,17 @@ static struct pt_types sgi_pt_types[] = {
> >  	{0,		NULL}
> >  };
> >  
> > +/*
> > + * 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,
> > +};
> > +MODULE_ALIAS_FS("efs");
> >  
> >  static struct kmem_cache * efs_inode_cachep;
> >  
> > @@ -108,18 +104,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 +237,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);
> 
> Ok, I guess this and elsewhere is fixing up whitespace oddities,
> not adding them. :)

Yeah, I fixed some tabs to spaces whitespace, when I was in that area of code.

> 
> >  	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;
> 
> I think this can (should?) be converted to:
> 
> 		return invalf(fc,
> 			"device does not support %d byte blocks",
> 			EFS_BLOCKSIZE);
> 
> and similarly for other error printing failures along the fill_super path,
> with appropriate variants of invalf()/errorf()/warnf()/etc
> 
> (dhowells - am I right about this?)

I'm still looking at this.

> 
> >  	}
> > -  
> > +
> >  	/* read the vh (volume header) block */
> >  	bh = sb_bread(s, 0);
> >  
> > @@ -294,7 +282,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 +316,49 @@ 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);
> > +}
> 
> unneeded; see below

Agreed.

> 
> > +static int efs_get_tree(struct fs_context *fc)
> > +{
> > +	return get_tree_bdev(fc, efs_fill_super);
> > +}
> > +
> > +static int efs_reconfigure(struct fs_context *fc)
> > +{
> > +	sync_filesystem(fc->root->d_sb);
> 
> I think you need:
> 
> 	fc->sb_flags |= SB_RDONLY;
> 
> here to preserve the original behavior in efs_remount()

Good catch. I had noticed that /proc/mounts changed the permission to rw,
but not behaving as rw.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +struct efs_context {
> > +	unsigned long s_mount_opts;
> > +};
> 
> This looks unused, and probably also copied from zonefs, which used it
> to store mount options - something efs doesn't have.

Agreed.

> 
> > +
> > +static const struct fs_context_operations efs_context_opts = {
> > +	.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;
> 
> so there's no reason to allocate and assign it here.
> which means efs_free_fc() doesn't need to exist either.

Agreed.

> 
> > +	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);
>