Re: [PATCH v2 3/6] fs/ntfs3: Use new api for mounting

Kari Argillander
2021-08-19T21:53:20Z

On Thu, Aug 19, 2021 at 03:26:30AM +0300, Kari Argillander wrote:
> We have now new mount api as described in Documentation/filesystems. We
> should use it as it gives us some benefits which are desribed here
> lore.kernel.org/linux-fsdevel/159646178122.1784947.11705396571718464082.stgit@warthog.procyon.org.uk/
>
> Nls loading is changed a to load with string. This did make code also
> little cleaner.
>
> Also try to use fsparam_flag_no as much as possible. This is just nice
> little touch and is not mandatory but it should not make any harm. It
> is just convenient that we can use example acl/noacl mount options.
>
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>

I will send new patches when Komarov has take a look of these. I have
found some bugs and how to improve things. Please take a look below my
second comment.

> fs/ntfs3/super.c | 392 +++++++++++++++++++++++----------------------

> +static void ntfs_fs_free(struct fs_context *fc)
> +{
> + struct ntfs_sb_info *sbi = fc->s_fs_info;
> +
> + if (sbi)
> + put_ntfs(sbi);
> +}
> +
> +static const struct fs_context_operations ntfs_context_ops = {
> + .parse_param = ntfs_fs_parse_param,
> + .get_tree = ntfs_fs_get_tree,
> + .reconfigure = ntfs_fs_reconfigure,
> + .free = ntfs_fs_free,
> +};
> +
> +static int ntfs_init_fs_context(struct fs_context *fc)
> {
> - return mount_bdev(fs_type, flags, dev_name, data, ntfs_fill_super);
> + struct ntfs_sb_info *sbi;
> +
> + sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
> + if (!sbi)
> + return -ENOMEM;
> +
> + /* Default options */
> + sbi->options.fs_uid = current_uid();
> + sbi->options.fs_gid = current_gid();
> + sbi->options.fs_fmask_inv = ~current_umask();
> + sbi->options.fs_dmask_inv = ~current_umask();
> +
> + fc->s_fs_info = sbi;
> + fc->ops = &ntfs_context_ops;
> +
> + return 0;
> }

In this code I did not like that we make whole new sbi everytime.
Especially because we do zalloc. So when we regonfigure then new sbi
is allocated just for options. I notice that example xfs does allocate
their "sbi" everytime. Then I notice that example squashfs allocate
just mount options.

I have impression that we should allocate sbi if it first time so I
did "between code". I would like to do things like they are intended
with api so can Christoph comment that is this "right" thing to do
and is there any draw backs which I should know.

static void ntfs_fs_free(struct fs_context *fc)
{
struct ntfs_mount_options *opts = fc->fs_private;
struct ntfs_sb_info *sbi = fc->s_fs_info;

if (sbi)
put_ntfs(sbi);

if (opts)
clear_mount_options(opts);
}

static int ntfs_init_fs_context(struct fs_context *fc)
{
struct ntfs_mount_options *opts;
struct ntfs_sb_info *sbi = NULL;

fc->ops = &ntfs_context_ops;

opts = ntfs_zalloc(sizeof(struct ntfs_mount_options));
if (!opts)
return -ENOMEM;

/* Default options. */
opts->fs_uid = current_uid();
opts->fs_gid = current_gid();
opts->fs_fmask_inv = ~current_umask();
opts->fs_dmask_inv = ~current_umask();

fc->fs_private = opts;

/* No need to initialize sbi if we just reconf. */
if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE)
return 0;

sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
if (!sbi) {
ntfs_free(opts);
return -ENOMEM;
}

mutex_init(&sbi->compress.mtx_lznt);
#ifdef CONFIG_NTFS3_LZX_XPRESS
mutex_init(&sbi->compress.mtx_xpress);
mutex_init(&sbi->compress.mtx_lzx);
#endif

sbi->options = opts;
fc->s_fs_info = sbi;

return 0;
}