linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Liu Bo <bo.liu@linux.alibaba.com>
To: Gao Xiang <hsiangkao@linux.alibaba.com>
Cc: linux-erofs@lists.ozlabs.org, Chao Yu <chao@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Peng Tao <tao.peng@linux.alibaba.com>,
	Yan Song <imeoer@linux.alibaba.com>,
	Changwei Ge <chge@linux.alibaba.com>,
	Liu Jiang <gerry@linux.alibaba.com>,
	Joseph Qi <joseph.qi@linux.alibaba.com>
Subject: Re: [PATCH v4 1/2] erofs: decouple basic mount options from fs_context
Date: Fri, 8 Oct 2021 01:47:55 +0800	[thread overview]
Message-ID: <20211007174755.GA59186@rsjd01523.et2sqa> (raw)
In-Reply-To: <20211007070224.12833-1-hsiangkao@linux.alibaba.com>

On Thu, Oct 07, 2021 at 03:02:23PM +0800, Gao Xiang wrote:
> Previously, EROFS mount options are all in the basic types, so
> erofs_fs_context can be directly copied with assignment. However,
> when the multiple device feature is introduced, it's hard to handle
> multiple device information like the other basic mount options.
> 
> Let's separate basic mount option usage from fs_context, thus
> multiple device information can be handled gracefully then.
> 
> No logic changes.
>

Looks good.
Reviewed-by: Liu Bo <bo.liu@linux.alibaba.com>

thanks,
liubo

> Cc: Liu Bo <bo.liu@linux.alibaba.com>
> Cc: Chao Yu <chao@kernel.org>
> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> ---
> v3: https://lore.kernel.org/r/20211006194453.130447-1-hsiangkao@linux.alibaba.com
> changes since v3:
>  - refine commit message pointed out by Bo.
> 
>  fs/erofs/inode.c    |  2 +-
>  fs/erofs/internal.h | 16 ++++++++-----
>  fs/erofs/super.c    | 58 ++++++++++++++++++++++-----------------------
>  fs/erofs/xattr.c    |  4 ++--
>  fs/erofs/zdata.c    |  8 +++----
>  5 files changed, 45 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> index a552399e211d..2345f1de438e 100644
> --- a/fs/erofs/inode.c
> +++ b/fs/erofs/inode.c
> @@ -192,7 +192,7 @@ static struct page *erofs_read_inode(struct inode *inode,
>  	inode->i_atime.tv_nsec = inode->i_ctime.tv_nsec;
>  
>  	inode->i_flags &= ~S_DAX;
> -	if (test_opt(&sbi->ctx, DAX_ALWAYS) && S_ISREG(inode->i_mode) &&
> +	if (test_opt(&sbi->opt, DAX_ALWAYS) && S_ISREG(inode->i_mode) &&
>  	    vi->datalayout == EROFS_INODE_FLAT_PLAIN)
>  		inode->i_flags |= S_DAX;
>  	if (!nblks)
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 9524e155b38f..b1b9d1b5cb66 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -47,7 +47,7 @@ typedef u64 erofs_off_t;
>  /* data type for filesystem-wide blocks number */
>  typedef u32 erofs_blk_t;
>  
> -struct erofs_fs_context {
> +struct erofs_mount_opts {
>  #ifdef CONFIG_EROFS_FS_ZIP
>  	/* current strategy of how to use managed cache */
>  	unsigned char cache_strategy;
> @@ -60,6 +60,10 @@ struct erofs_fs_context {
>  	unsigned int mount_opt;
>  };
>  
> +struct erofs_fs_context {
> +	struct erofs_mount_opts opt;
> +};
> +
>  /* all filesystem-wide lz4 configurations */
>  struct erofs_sb_lz4_info {
>  	/* # of pages needed for EROFS lz4 rolling decompression */
> @@ -69,6 +73,8 @@ struct erofs_sb_lz4_info {
>  };
>  
>  struct erofs_sb_info {
> +	struct erofs_mount_opts opt;	/* options */
> +
>  #ifdef CONFIG_EROFS_FS_ZIP
>  	/* list for all registered superblocks, mainly for shrinker */
>  	struct list_head list;
> @@ -108,8 +114,6 @@ struct erofs_sb_info {
>  	u8 volume_name[16];             /* volume name */
>  	u32 feature_compat;
>  	u32 feature_incompat;
> -
> -	struct erofs_fs_context ctx;	/* options */
>  };
>  
>  #define EROFS_SB(sb) ((struct erofs_sb_info *)(sb)->s_fs_info)
> @@ -121,9 +125,9 @@ struct erofs_sb_info {
>  #define EROFS_MOUNT_DAX_ALWAYS		0x00000040
>  #define EROFS_MOUNT_DAX_NEVER		0x00000080
>  
> -#define clear_opt(ctx, option)	((ctx)->mount_opt &= ~EROFS_MOUNT_##option)
> -#define set_opt(ctx, option)	((ctx)->mount_opt |= EROFS_MOUNT_##option)
> -#define test_opt(ctx, option)	((ctx)->mount_opt & EROFS_MOUNT_##option)
> +#define clear_opt(opt, option)	((opt)->mount_opt &= ~EROFS_MOUNT_##option)
> +#define set_opt(opt, option)	((opt)->mount_opt |= EROFS_MOUNT_##option)
> +#define test_opt(opt, option)	((opt)->mount_opt & EROFS_MOUNT_##option)
>  
>  enum {
>  	EROFS_ZIP_CACHE_DISABLED,
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 11b88559f8bf..25f6b8b37f28 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -340,15 +340,15 @@ static int erofs_read_superblock(struct super_block *sb)
>  static void erofs_default_options(struct erofs_fs_context *ctx)
>  {
>  #ifdef CONFIG_EROFS_FS_ZIP
> -	ctx->cache_strategy = EROFS_ZIP_CACHE_READAROUND;
> -	ctx->max_sync_decompress_pages = 3;
> -	ctx->readahead_sync_decompress = false;
> +	ctx->opt.cache_strategy = EROFS_ZIP_CACHE_READAROUND;
> +	ctx->opt.max_sync_decompress_pages = 3;
> +	ctx->opt.readahead_sync_decompress = false;
>  #endif
>  #ifdef CONFIG_EROFS_FS_XATTR
> -	set_opt(ctx, XATTR_USER);
> +	set_opt(&ctx->opt, XATTR_USER);
>  #endif
>  #ifdef CONFIG_EROFS_FS_POSIX_ACL
> -	set_opt(ctx, POSIX_ACL);
> +	set_opt(&ctx->opt, POSIX_ACL);
>  #endif
>  }
>  
> @@ -392,12 +392,12 @@ static bool erofs_fc_set_dax_mode(struct fs_context *fc, unsigned int mode)
>  	switch (mode) {
>  	case EROFS_MOUNT_DAX_ALWAYS:
>  		warnfc(fc, "DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
> -		set_opt(ctx, DAX_ALWAYS);
> -		clear_opt(ctx, DAX_NEVER);
> +		set_opt(&ctx->opt, DAX_ALWAYS);
> +		clear_opt(&ctx->opt, DAX_NEVER);
>  		return true;
>  	case EROFS_MOUNT_DAX_NEVER:
> -		set_opt(ctx, DAX_NEVER);
> -		clear_opt(ctx, DAX_ALWAYS);
> +		set_opt(&ctx->opt, DAX_NEVER);
> +		clear_opt(&ctx->opt, DAX_ALWAYS);
>  		return true;
>  	default:
>  		DBG_BUGON(1);
> @@ -424,9 +424,9 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>  	case Opt_user_xattr:
>  #ifdef CONFIG_EROFS_FS_XATTR
>  		if (result.boolean)
> -			set_opt(ctx, XATTR_USER);
> +			set_opt(&ctx->opt, XATTR_USER);
>  		else
> -			clear_opt(ctx, XATTR_USER);
> +			clear_opt(&ctx->opt, XATTR_USER);
>  #else
>  		errorfc(fc, "{,no}user_xattr options not supported");
>  #endif
> @@ -434,16 +434,16 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>  	case Opt_acl:
>  #ifdef CONFIG_EROFS_FS_POSIX_ACL
>  		if (result.boolean)
> -			set_opt(ctx, POSIX_ACL);
> +			set_opt(&ctx->opt, POSIX_ACL);
>  		else
> -			clear_opt(ctx, POSIX_ACL);
> +			clear_opt(&ctx->opt, POSIX_ACL);
>  #else
>  		errorfc(fc, "{,no}acl options not supported");
>  #endif
>  		break;
>  	case Opt_cache_strategy:
>  #ifdef CONFIG_EROFS_FS_ZIP
> -		ctx->cache_strategy = result.uint_32;
> +		ctx->opt.cache_strategy = result.uint_32;
>  #else
>  		errorfc(fc, "compression not supported, cache_strategy ignored");
>  #endif
> @@ -540,15 +540,16 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
>  		return -ENOMEM;
>  
>  	sb->s_fs_info = sbi;
> +	sbi->opt = ctx->opt;
>  	sbi->dax_dev = fs_dax_get_by_bdev(sb->s_bdev);
>  	err = erofs_read_superblock(sb);
>  	if (err)
>  		return err;
>  
> -	if (test_opt(ctx, DAX_ALWAYS) &&
> +	if (test_opt(&sbi->opt, DAX_ALWAYS) &&
>  	    !dax_supported(sbi->dax_dev, sb->s_bdev, EROFS_BLKSIZ, 0, bdev_nr_sectors(sb->s_bdev))) {
>  		errorfc(fc, "DAX unsupported by block device. Turning off DAX.");
> -		clear_opt(ctx, DAX_ALWAYS);
> +		clear_opt(&sbi->opt, DAX_ALWAYS);
>  	}
>  	sb->s_flags |= SB_RDONLY | SB_NOATIME;
>  	sb->s_maxbytes = MAX_LFS_FILESIZE;
> @@ -557,13 +558,11 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
>  	sb->s_op = &erofs_sops;
>  	sb->s_xattr = erofs_xattr_handlers;
>  
> -	if (test_opt(ctx, POSIX_ACL))
> +	if (test_opt(&sbi->opt, POSIX_ACL))
>  		sb->s_flags |= SB_POSIXACL;
>  	else
>  		sb->s_flags &= ~SB_POSIXACL;
>  
> -	sbi->ctx = *ctx;
> -
>  #ifdef CONFIG_EROFS_FS_ZIP
>  	xa_init(&sbi->managed_pslots);
>  #endif
> @@ -607,12 +606,12 @@ static int erofs_fc_reconfigure(struct fs_context *fc)
>  
>  	DBG_BUGON(!sb_rdonly(sb));
>  
> -	if (test_opt(ctx, POSIX_ACL))
> +	if (test_opt(&ctx->opt, POSIX_ACL))
>  		fc->sb_flags |= SB_POSIXACL;
>  	else
>  		fc->sb_flags &= ~SB_POSIXACL;
>  
> -	sbi->ctx = *ctx;
> +	sbi->opt = ctx->opt;
>  
>  	fc->sb_flags |= SB_RDONLY;
>  	return 0;
> @@ -640,7 +639,6 @@ static int erofs_init_fs_context(struct fs_context *fc)
>  	erofs_default_options(fc->fs_private);
>  
>  	fc->ops = &erofs_context_ops;
> -
>  	return 0;
>  }
>  
> @@ -763,31 +761,31 @@ static int erofs_statfs(struct dentry *dentry, struct kstatfs *buf)
>  static int erofs_show_options(struct seq_file *seq, struct dentry *root)
>  {
>  	struct erofs_sb_info *sbi = EROFS_SB(root->d_sb);
> -	struct erofs_fs_context *ctx = &sbi->ctx;
> +	struct erofs_mount_opts *opt = &sbi->opt;
>  
>  #ifdef CONFIG_EROFS_FS_XATTR
> -	if (test_opt(ctx, XATTR_USER))
> +	if (test_opt(opt, XATTR_USER))
>  		seq_puts(seq, ",user_xattr");
>  	else
>  		seq_puts(seq, ",nouser_xattr");
>  #endif
>  #ifdef CONFIG_EROFS_FS_POSIX_ACL
> -	if (test_opt(ctx, POSIX_ACL))
> +	if (test_opt(opt, POSIX_ACL))
>  		seq_puts(seq, ",acl");
>  	else
>  		seq_puts(seq, ",noacl");
>  #endif
>  #ifdef CONFIG_EROFS_FS_ZIP
> -	if (ctx->cache_strategy == EROFS_ZIP_CACHE_DISABLED)
> +	if (opt->cache_strategy == EROFS_ZIP_CACHE_DISABLED)
>  		seq_puts(seq, ",cache_strategy=disabled");
> -	else if (ctx->cache_strategy == EROFS_ZIP_CACHE_READAHEAD)
> +	else if (opt->cache_strategy == EROFS_ZIP_CACHE_READAHEAD)
>  		seq_puts(seq, ",cache_strategy=readahead");
> -	else if (ctx->cache_strategy == EROFS_ZIP_CACHE_READAROUND)
> +	else if (opt->cache_strategy == EROFS_ZIP_CACHE_READAROUND)
>  		seq_puts(seq, ",cache_strategy=readaround");
>  #endif
> -	if (test_opt(ctx, DAX_ALWAYS))
> +	if (test_opt(opt, DAX_ALWAYS))
>  		seq_puts(seq, ",dax=always");
> -	if (test_opt(ctx, DAX_NEVER))
> +	if (test_opt(opt, DAX_NEVER))
>  		seq_puts(seq, ",dax=never");
>  	return 0;
>  }
> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> index 778f2c52295d..01c581e93c5f 100644
> --- a/fs/erofs/xattr.c
> +++ b/fs/erofs/xattr.c
> @@ -429,7 +429,7 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
>  
>  static bool erofs_xattr_user_list(struct dentry *dentry)
>  {
> -	return test_opt(&EROFS_SB(dentry->d_sb)->ctx, XATTR_USER);
> +	return test_opt(&EROFS_SB(dentry->d_sb)->opt, XATTR_USER);
>  }
>  
>  static bool erofs_xattr_trusted_list(struct dentry *dentry)
> @@ -476,7 +476,7 @@ static int erofs_xattr_generic_get(const struct xattr_handler *handler,
>  
>  	switch (handler->flags) {
>  	case EROFS_XATTR_INDEX_USER:
> -		if (!test_opt(&sbi->ctx, XATTR_USER))
> +		if (!test_opt(&sbi->opt, XATTR_USER))
>  			return -EOPNOTSUPP;
>  		break;
>  	case EROFS_XATTR_INDEX_TRUSTED:
> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> index 11c7a1aaebad..e59e22852c78 100644
> --- a/fs/erofs/zdata.c
> +++ b/fs/erofs/zdata.c
> @@ -695,7 +695,7 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
>  		goto err_out;
>  
>  	/* preload all compressed pages (maybe downgrade role if necessary) */
> -	if (should_alloc_managed_pages(fe, sbi->ctx.cache_strategy, map->m_la))
> +	if (should_alloc_managed_pages(fe, sbi->opt.cache_strategy, map->m_la))
>  		cache_strategy = TRYALLOC;
>  	else
>  		cache_strategy = DONTALLOC;
> @@ -796,7 +796,7 @@ static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
>  	/* Use workqueue and sync decompression for atomic contexts only */
>  	if (in_atomic() || irqs_disabled()) {
>  		queue_work(z_erofs_workqueue, &io->u.work);
> -		sbi->ctx.readahead_sync_decompress = true;
> +		sbi->opt.readahead_sync_decompress = true;
>  		return;
>  	}
>  	z_erofs_decompressqueue_work(&io->u.work);
> @@ -1411,8 +1411,8 @@ static void z_erofs_readahead(struct readahead_control *rac)
>  	struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
>  
>  	unsigned int nr_pages = readahead_count(rac);
> -	bool sync = (sbi->ctx.readahead_sync_decompress &&
> -			nr_pages <= sbi->ctx.max_sync_decompress_pages);
> +	bool sync = (sbi->opt.readahead_sync_decompress &&
> +			nr_pages <= sbi->opt.max_sync_decompress_pages);
>  	struct z_erofs_decompress_frontend f = DECOMPRESS_FRONTEND_INIT(inode);
>  	struct page *page, *head = NULL;
>  	LIST_HEAD(pagepool);
> -- 
> 2.24.4

  parent reply	other threads:[~2021-10-07 17:48 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-07  7:02 [PATCH v4 1/2] erofs: decouple basic mount options from fs_context Gao Xiang
2021-10-07  7:02 ` [PATCH v4 2/2] erofs: add multiple device support Gao Xiang
2021-10-10  6:33   ` [PATCH v5 " Gao Xiang
2021-10-14  8:10     ` [PATCH v6 " Gao Xiang
2021-10-17  2:10       ` Chao Yu
2021-10-17  4:15         ` Gao Xiang
2021-10-17 15:00           ` Chao Yu
2021-10-07 17:47 ` Liu Bo [this message]
2021-10-17  1:18 ` [PATCH v4 1/2] erofs: decouple basic mount options from fs_context Chao Yu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211007174755.GA59186@rsjd01523.et2sqa \
    --to=bo.liu@linux.alibaba.com \
    --cc=chao@kernel.org \
    --cc=chge@linux.alibaba.com \
    --cc=gerry@linux.alibaba.com \
    --cc=hsiangkao@linux.alibaba.com \
    --cc=imeoer@linux.alibaba.com \
    --cc=joseph.qi@linux.alibaba.com \
    --cc=linux-erofs@lists.ozlabs.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tao.peng@linux.alibaba.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).