2022-09-15 12:57:15

by Jia Zhu

[permalink] [raw]
Subject: [PATCH V4 0/6] Introduce erofs shared domain

Changes since V3:
1. Avoid race condition.
1.1. Relinquish the volume before removing the domain from list.
1.2. Hold erofs_domain_list_lock before dec the refcnt of domain.
2. Relinquish previously registered erofs_fscache in
erofs_fscache_domain_init_cookie()'s error handling path.
3. Some code cleanups without logic change.

[Kernel Patchset]
===============
Git tree:
https://github.com/userzj/linux.git zhujia/shared-domain-v4
Git web:
https://github.com/userzj/linux/tree/zhujia/shared-domain-v4

[User Daemon for Quick Test]
============================
Git web:
https://github.com/userzj/demand-read-cachefilesd/tree/shared-domain
More test cases will be added to:
https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/log/?h=experimental-tests-fscache

[E2E Container Demo for Quick Test]
===================================
[Issue]
https://github.com/containerd/nydus-snapshotter/issues/161
[PR]
https://github.com/containerd/nydus-snapshotter/pull/162

[Background]
============
In ondemand read mode, we use individual volume to present an erofs
mountpoint, cookies to present bootstrap and data blobs.

In which case, since cookies can't be shared between fscache volumes,
even if the data blobs between different mountpoints are exactly same,
they can't be shared.

[Introduction]
==============
Here we introduce erofs shared domain to resolve above mentioned case.
Several erofs filesystems can belong to one domain, and data blobs can
be shared among these erofs filesystems of same domain.

[Usage]
Users could specify 'domain_id' mount option to create or join into a
domain which reuses the same cookies(blobs).

[Design]
========
1. Use pseudo mnt to manage domain's lifecycle.
2. Use a linked list to maintain & traverse domains.
3. Use pseudo sb to create anonymous inode for recording cookie's info
and manage cookies lifecycle.

[Flow Path]
===========
1. User specify a new 'domain_id' in mount option.
1.1 Traverse domain list, compare domain_id with existing domain.[Miss]
1.2 Create a new domain(volume), add it to domain list.
1.3 Traverse pseudo sb's inode list, compare cookie name with
existing cookies.[Miss]
1.4 Alloc new anonymous inodes and cookies.

2. User specify an existing 'domain_id' in mount option and the data
blob is existed in domain.
2.1 Traverse domain list, compare domain_id with existing domain.[Hit]
2.2 Reuse the domain and increase its refcnt.
2.3 Traverse pseudo sb's inode list, compare cookie name with
existing cookies.[Hit]
2.4 Reuse the cookie and increase its refcnt.

RFC: https://lore.kernel.org/all/YxAlO%[email protected]/
V1: https://lore.kernel.org/all/[email protected]/
V2: https://lore.kernel.org/all/[email protected]/
V3: https://lore.kernel.org/all/[email protected]/

Jia Zhu (6):
erofs: use kill_anon_super() to kill super in fscache mode
erofs: code clean up for fscache
erofs: introduce fscache-based domain
erofs: introduce a pseudo mnt to manage shared cookies
erofs: Support sharing cookies in the same domain
erofs: introduce 'domain_id' mount option

fs/erofs/fscache.c | 253 ++++++++++++++++++++++++++++++++++++++------
fs/erofs/internal.h | 30 ++++--
fs/erofs/super.c | 72 ++++++++++---
fs/erofs/sysfs.c | 19 +++-
4 files changed, 317 insertions(+), 57 deletions(-)

--
2.20.1


2022-09-15 12:57:20

by Jia Zhu

[permalink] [raw]
Subject: [PATCH V4 5/6] erofs: Support sharing cookies in the same domain

Several erofs filesystems can belong to one domain, and data blobs can
be shared among these erofs filesystems of same domain.

Users could specify domain_id mount option to create or join into a
domain.

Signed-off-by: Jia Zhu <[email protected]>
---
fs/erofs/fscache.c | 90 ++++++++++++++++++++++++++++++++++++++++++++-
fs/erofs/internal.h | 3 ++
2 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
index ff8382df493e..8873a093cf7b 100644
--- a/fs/erofs/fscache.c
+++ b/fs/erofs/fscache.c
@@ -7,6 +7,7 @@
#include "internal.h"

static DEFINE_MUTEX(erofs_domain_list_lock);
+static DEFINE_MUTEX(erofs_domain_cookies_lock);
static LIST_HEAD(erofs_domain_list);
static struct vfsmount *erofs_pseudo_mnt;

@@ -527,8 +528,8 @@ static int erofs_fscache_register_domain(struct super_block *sb)
return err;
}

-struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
- char *name, bool need_inode)
+struct erofs_fscache *erofs_fscache_acquire_cookie(struct super_block *sb,
+ char *name, bool need_inode)
{
struct fscache_volume *volume = EROFS_SB(sb)->volume;
struct erofs_fscache *ctx;
@@ -577,14 +578,99 @@ struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
return ERR_PTR(ret);
}

+static
+struct erofs_fscache *erofs_fscache_domain_init_cookie(struct super_block *sb,
+ char *name, bool need_inode)
+{
+ int err;
+ struct inode *inode;
+ struct erofs_fscache *ctx;
+ struct erofs_domain *domain = EROFS_SB(sb)->domain;
+
+ ctx = erofs_fscache_acquire_cookie(sb, name, need_inode);
+ if (IS_ERR(ctx))
+ return ctx;
+
+ ctx->name = kstrdup(name, GFP_KERNEL);
+ if (!ctx->name) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ inode = new_inode(erofs_pseudo_mnt->mnt_sb);
+ if (!inode) {
+ kfree(ctx->name);
+ err = -ENOMEM;
+ goto out;
+ }
+
+ ctx->domain = domain;
+ ctx->anon_inode = inode;
+ inode->i_private = ctx;
+ refcount_inc(&domain->ref);
+ return ctx;
+out:
+ fscache_unuse_cookie(ctx->cookie, NULL, NULL);
+ fscache_relinquish_cookie(ctx->cookie, false);
+ if (need_inode)
+ iput(ctx->inode);
+ kfree(ctx);
+ return ERR_PTR(err);
+}
+
+static
+struct erofs_fscache *erofs_domain_register_cookie(struct super_block *sb,
+ char *name, bool need_inode)
+{
+ struct inode *inode;
+ struct erofs_fscache *ctx;
+ struct erofs_domain *domain = EROFS_SB(sb)->domain;
+ struct super_block *psb = erofs_pseudo_mnt->mnt_sb;
+
+ mutex_lock(&erofs_domain_cookies_lock);
+ list_for_each_entry(inode, &psb->s_inodes, i_sb_list) {
+ ctx = inode->i_private;
+ if (!ctx || ctx->domain != domain || strcmp(ctx->name, name))
+ continue;
+ igrab(inode);
+ mutex_unlock(&erofs_domain_cookies_lock);
+ return ctx;
+ }
+ ctx = erofs_fscache_domain_init_cookie(sb, name, need_inode);
+ mutex_unlock(&erofs_domain_cookies_lock);
+ return ctx;
+}
+
+struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
+ char *name, bool need_inode)
+{
+ if (EROFS_SB(sb)->opt.domain_id)
+ return erofs_domain_register_cookie(sb, name, need_inode);
+ return erofs_fscache_acquire_cookie(sb, name, need_inode);
+}
+
void erofs_fscache_unregister_cookie(struct erofs_fscache *ctx)
{
+ bool drop;
+ struct erofs_domain *domain;
+
if (!ctx)
return;
+ domain = ctx->domain;
+ if (domain) {
+ mutex_lock(&erofs_domain_cookies_lock);
+ drop = atomic_read(&ctx->anon_inode->i_count) == 1;
+ iput(ctx->anon_inode);
+ mutex_unlock(&erofs_domain_cookies_lock);
+ if (!drop)
+ return;
+ }

fscache_unuse_cookie(ctx->cookie, NULL, NULL);
fscache_relinquish_cookie(ctx->cookie, false);
+ erofs_fscache_domain_put(domain);
iput(ctx->inode);
+ kfree(ctx->name);
kfree(ctx);
}

diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 88c1a46867b3..8a6f94b27a23 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -109,6 +109,9 @@ struct erofs_domain {
struct erofs_fscache {
struct fscache_cookie *cookie;
struct inode *inode;
+ struct inode *anon_inode;
+ struct erofs_domain *domain;
+ char *name;
};

struct erofs_sb_info {
--
2.20.1

2022-09-15 12:59:31

by Jia Zhu

[permalink] [raw]
Subject: [PATCH V4 2/6] erofs: code clean up for fscache

Some cleanups. No logic changes.

Suggested-by: Jingbo Xu <[email protected]>
Signed-off-by: Jia Zhu <[email protected]>
---
fs/erofs/fscache.c | 36 ++++++++++++++++++------------------
fs/erofs/internal.h | 17 ++++++++---------
fs/erofs/super.c | 22 +++++++++-------------
3 files changed, 35 insertions(+), 40 deletions(-)

diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
index 8e01d89c3319..d72e2a7ea6ab 100644
--- a/fs/erofs/fscache.c
+++ b/fs/erofs/fscache.c
@@ -417,9 +417,8 @@ const struct address_space_operations erofs_fscache_access_aops = {
.readahead = erofs_fscache_readahead,
};

-int erofs_fscache_register_cookie(struct super_block *sb,
- struct erofs_fscache **fscache,
- char *name, bool need_inode)
+struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
+ char *name, bool need_inode)
{
struct fscache_volume *volume = EROFS_SB(sb)->volume;
struct erofs_fscache *ctx;
@@ -428,7 +427,7 @@ int erofs_fscache_register_cookie(struct super_block *sb,

ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);

cookie = fscache_acquire_cookie(volume, FSCACHE_ADV_WANT_CACHE_SIZE,
name, strlen(name), NULL, 0, 0);
@@ -458,40 +457,32 @@ int erofs_fscache_register_cookie(struct super_block *sb,
ctx->inode = inode;
}

- *fscache = ctx;
- return 0;
+ return ctx;

err_cookie:
fscache_unuse_cookie(ctx->cookie, NULL, NULL);
fscache_relinquish_cookie(ctx->cookie, false);
- ctx->cookie = NULL;
err:
kfree(ctx);
- return ret;
+ return ERR_PTR(ret);
}

-void erofs_fscache_unregister_cookie(struct erofs_fscache **fscache)
+void erofs_fscache_unregister_cookie(struct erofs_fscache *ctx)
{
- struct erofs_fscache *ctx = *fscache;
-
if (!ctx)
return;

fscache_unuse_cookie(ctx->cookie, NULL, NULL);
fscache_relinquish_cookie(ctx->cookie, false);
- ctx->cookie = NULL;
-
iput(ctx->inode);
- ctx->inode = NULL;
-
kfree(ctx);
- *fscache = NULL;
}

int erofs_fscache_register_fs(struct super_block *sb)
{
struct erofs_sb_info *sbi = EROFS_SB(sb);
struct fscache_volume *volume;
+ struct erofs_fscache *fscache;
char *name;
int ret = 0;

@@ -502,12 +493,19 @@ int erofs_fscache_register_fs(struct super_block *sb)
volume = fscache_acquire_volume(name, NULL, NULL, 0);
if (IS_ERR_OR_NULL(volume)) {
erofs_err(sb, "failed to register volume for %s", name);
- ret = volume ? PTR_ERR(volume) : -EOPNOTSUPP;
- volume = NULL;
+ kfree(name);
+ return volume ? PTR_ERR(volume) : -EOPNOTSUPP;
}

sbi->volume = volume;
kfree(name);
+
+ fscache = erofs_fscache_register_cookie(sb, sbi->opt.fsid, true);
+ /* acquired volume will be relinquished in kill_sb() */
+ if (IS_ERR(fscache))
+ return PTR_ERR(fscache);
+
+ sbi->s_fscache = fscache;
return ret;
}

@@ -515,6 +513,8 @@ void erofs_fscache_unregister_fs(struct super_block *sb)
{
struct erofs_sb_info *sbi = EROFS_SB(sb);

+ erofs_fscache_unregister_cookie(sbi->s_fscache);
fscache_relinquish_volume(sbi->volume, NULL, false);
+ sbi->s_fscache = NULL;
sbi->volume = NULL;
}
diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index cfee49d33b95..aa71eb65e965 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -610,27 +610,26 @@ static inline int z_erofs_load_lzma_config(struct super_block *sb,
int erofs_fscache_register_fs(struct super_block *sb);
void erofs_fscache_unregister_fs(struct super_block *sb);

-int erofs_fscache_register_cookie(struct super_block *sb,
- struct erofs_fscache **fscache,
- char *name, bool need_inode);
-void erofs_fscache_unregister_cookie(struct erofs_fscache **fscache);
+struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
+ char *name, bool need_inode);
+void erofs_fscache_unregister_cookie(struct erofs_fscache *fscache);

extern const struct address_space_operations erofs_fscache_access_aops;
#else
static inline int erofs_fscache_register_fs(struct super_block *sb)
{
- return 0;
+ return -EOPNOTSUPP;
}
static inline void erofs_fscache_unregister_fs(struct super_block *sb) {}

-static inline int erofs_fscache_register_cookie(struct super_block *sb,
- struct erofs_fscache **fscache,
- char *name, bool need_inode)
+static inline
+struct erofs_fscache *erofs_fscache_register_cookie(struct super_block *sb,
+ char *name, bool need_inode)
{
return -EOPNOTSUPP;
}

-static inline void erofs_fscache_unregister_cookie(struct erofs_fscache **fscache)
+static inline void erofs_fscache_unregister_cookie(struct erofs_fscache *fscache)
{
}
#endif
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 9716d355a63e..79e871c04fe2 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -224,10 +224,10 @@ static int erofs_init_device(struct erofs_buf *buf, struct super_block *sb,
struct erofs_device_info *dif, erofs_off_t *pos)
{
struct erofs_sb_info *sbi = EROFS_SB(sb);
+ struct erofs_fscache *fscache;
struct erofs_deviceslot *dis;
struct block_device *bdev;
void *ptr;
- int ret;

ptr = erofs_read_metabuf(buf, sb, erofs_blknr(*pos), EROFS_KMAP);
if (IS_ERR(ptr))
@@ -245,10 +245,10 @@ static int erofs_init_device(struct erofs_buf *buf, struct super_block *sb,
}

if (erofs_is_fscache_mode(sb)) {
- ret = erofs_fscache_register_cookie(sb, &dif->fscache,
- dif->path, false);
- if (ret)
- return ret;
+ fscache = erofs_fscache_register_cookie(sb, dif->path, false);
+ if (IS_ERR(fscache))
+ return PTR_ERR(fscache);
+ dif->fscache = fscache;
} else {
bdev = blkdev_get_by_path(dif->path, FMODE_READ | FMODE_EXCL,
sb->s_type);
@@ -706,11 +706,6 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
if (err)
return err;

- err = erofs_fscache_register_cookie(sb, &sbi->s_fscache,
- sbi->opt.fsid, true);
- if (err)
- return err;
-
err = super_setup_bdi(sb);
if (err)
return err;
@@ -817,7 +812,8 @@ static int erofs_release_device_info(int id, void *ptr, void *data)
fs_put_dax(dif->dax_dev, NULL);
if (dif->bdev)
blkdev_put(dif->bdev, FMODE_READ | FMODE_EXCL);
- erofs_fscache_unregister_cookie(&dif->fscache);
+ erofs_fscache_unregister_cookie(dif->fscache);
+ dif->fscache = NULL;
kfree(dif->path);
kfree(dif);
return 0;
@@ -889,7 +885,6 @@ static void erofs_kill_sb(struct super_block *sb)

erofs_free_dev_context(sbi->devs);
fs_put_dax(sbi->dax_dev, NULL);
- erofs_fscache_unregister_cookie(&sbi->s_fscache);
erofs_fscache_unregister_fs(sb);
kfree(sbi->opt.fsid);
kfree(sbi);
@@ -909,7 +904,8 @@ static void erofs_put_super(struct super_block *sb)
iput(sbi->managed_cache);
sbi->managed_cache = NULL;
#endif
- erofs_fscache_unregister_cookie(&sbi->s_fscache);
+ erofs_fscache_unregister_fs(sb);
+ sbi->s_fscache = NULL;
}

static struct file_system_type erofs_fs_type = {
--
2.20.1

2022-09-15 13:01:24

by Jia Zhu

[permalink] [raw]
Subject: [PATCH V4 1/6] erofs: use kill_anon_super() to kill super in fscache mode

Use kill_anon_super() instead of generic_shutdown_super() since the
mount() in erofs fscache mode uses get_tree_nodev() and associated
anon bdev needs to be freed.

Suggested-by: Jingbo Xu <[email protected]>
Signed-off-by: Jia Zhu <[email protected]>
Reviewed-by: Jingbo Xu <[email protected]>
---
fs/erofs/super.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 3173debeaa5a..9716d355a63e 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -879,7 +879,7 @@ static void erofs_kill_sb(struct super_block *sb)
WARN_ON(sb->s_magic != EROFS_SUPER_MAGIC);

if (erofs_is_fscache_mode(sb))
- generic_shutdown_super(sb);
+ kill_anon_super(sb);
else
kill_block_super(sb);

--
2.20.1

2022-09-15 13:30:16

by Jia Zhu

[permalink] [raw]
Subject: [PATCH V4 6/6] erofs: introduce 'domain_id' mount option

Introduce 'domain_id' mount option to enable shared domain sementics.
In which case, the related cookie is shared if two mountpoints in the
same domain have the same data blob. Users could specify the name of
domain by this mount option.

Signed-off-by: Jia Zhu <[email protected]>
---
fs/erofs/super.c | 17 +++++++++++++++++
fs/erofs/sysfs.c | 19 +++++++++++++++++--
2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 24bac58285e8..5e55c4fe6220 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -440,6 +440,7 @@ enum {
Opt_dax_enum,
Opt_device,
Opt_fsid,
+ Opt_domain_id,
Opt_err
};

@@ -465,6 +466,7 @@ static const struct fs_parameter_spec erofs_fs_parameters[] = {
fsparam_enum("dax", Opt_dax_enum, erofs_dax_param_enums),
fsparam_string("device", Opt_device),
fsparam_string("fsid", Opt_fsid),
+ fsparam_string("domain_id", Opt_domain_id),
{}
};

@@ -568,6 +570,16 @@ static int erofs_fc_parse_param(struct fs_context *fc,
return -ENOMEM;
#else
errorfc(fc, "fsid option not supported");
+#endif
+ break;
+ case Opt_domain_id:
+#ifdef CONFIG_EROFS_FS_ONDEMAND
+ kfree(ctx->opt.domain_id);
+ ctx->opt.domain_id = kstrdup(param->string, GFP_KERNEL);
+ if (!ctx->opt.domain_id)
+ return -ENOMEM;
+#else
+ errorfc(fc, "domain_id option not supported");
#endif
break;
default:
@@ -702,6 +714,7 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
sb->s_fs_info = sbi;
sbi->opt = ctx->opt;
ctx->opt.fsid = NULL;
+ ctx->opt.domain_id = NULL;
sbi->devs = ctx->devs;
ctx->devs = NULL;

@@ -846,6 +859,7 @@ static void erofs_fc_free(struct fs_context *fc)

erofs_free_dev_context(ctx->devs);
kfree(ctx->opt.fsid);
+ kfree(ctx->opt.domain_id);
kfree(ctx);
}

@@ -914,6 +928,7 @@ static void erofs_kill_sb(struct super_block *sb)
fs_put_dax(sbi->dax_dev, NULL);
erofs_fscache_unregister_fs(sb);
kfree(sbi->opt.fsid);
+ kfree(sbi->opt.domain_id);
kfree(sbi);
sb->s_fs_info = NULL;
}
@@ -1067,6 +1082,8 @@ static int erofs_show_options(struct seq_file *seq, struct dentry *root)
#ifdef CONFIG_EROFS_FS_ONDEMAND
if (opt->fsid)
seq_printf(seq, ",fsid=%s", opt->fsid);
+ if (opt->domain_id)
+ seq_printf(seq, ",domain_id=%s", opt->domain_id);
#endif
return 0;
}
diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
index c1383e508bbe..341fb43ad587 100644
--- a/fs/erofs/sysfs.c
+++ b/fs/erofs/sysfs.c
@@ -201,12 +201,27 @@ static struct kobject erofs_feat = {
int erofs_register_sysfs(struct super_block *sb)
{
struct erofs_sb_info *sbi = EROFS_SB(sb);
+ char *name;
+ char *str = NULL;
int err;

+ if (erofs_is_fscache_mode(sb)) {
+ if (sbi->opt.domain_id) {
+ str = kasprintf(GFP_KERNEL, "%s,%s", sbi->opt.domain_id,
+ sbi->opt.fsid);
+ if (!str)
+ return -ENOMEM;
+ name = str;
+ } else {
+ name = sbi->opt.fsid;
+ }
+ } else {
+ name = sb->s_id;
+ }
sbi->s_kobj.kset = &erofs_root;
init_completion(&sbi->s_kobj_unregister);
- err = kobject_init_and_add(&sbi->s_kobj, &erofs_sb_ktype, NULL, "%s",
- erofs_is_fscache_mode(sb) ? sbi->opt.fsid : sb->s_id);
+ err = kobject_init_and_add(&sbi->s_kobj, &erofs_sb_ktype, NULL, "%s", name);
+ kfree(str);
if (err)
goto put_sb_kobj;
return 0;
--
2.20.1

2022-09-16 02:03:27

by Jingbo Xu

[permalink] [raw]
Subject: Re: [PATCH V4 1/6] erofs: use kill_anon_super() to kill super in fscache mode



On 9/15/22 8:42 PM, Jia Zhu wrote:
> Use kill_anon_super() instead of generic_shutdown_super() since the
> mount() in erofs fscache mode uses get_tree_nodev() and associated
> anon bdev needs to be freed.
>
> Suggested-by: Jingbo Xu <[email protected]>
> Signed-off-by: Jia Zhu <[email protected]>
> Reviewed-by: Jingbo Xu <[email protected]>

Could you please also add a "Fixes" tag?


--
Thanks,
Jingbo

2022-09-16 02:52:41

by Jingbo Xu

[permalink] [raw]
Subject: Re: [PATCH V4 2/6] erofs: code clean up for fscache



On 9/15/22 8:42 PM, Jia Zhu wrote:

> @@ -502,12 +493,19 @@ int erofs_fscache_register_fs(struct super_block *sb)
> volume = fscache_acquire_volume(name, NULL, NULL, 0);
> if (IS_ERR_OR_NULL(volume)) {
> erofs_err(sb, "failed to register volume for %s", name);
> - ret = volume ? PTR_ERR(volume) : -EOPNOTSUPP;
> - volume = NULL;
> + kfree(name);
> + return volume ? PTR_ERR(volume) : -EOPNOTSUPP;
> }
>
> sbi->volume = volume;
> kfree(name);
> +
> + fscache = erofs_fscache_register_cookie(sb, sbi->opt.fsid, true);
> + /* acquired volume will be relinquished in kill_sb() */
> + if (IS_ERR(fscache))
> + return PTR_ERR(fscache);
> +
> + sbi->s_fscache = fscache;
> return ret;

The local variable "ret" is not used in this case.


> @@ -889,7 +885,6 @@ static void erofs_kill_sb(struct super_block *sb)
>
> erofs_free_dev_context(sbi->devs);
> fs_put_dax(sbi->dax_dev, NULL);
> - erofs_fscache_unregister_cookie(&sbi->s_fscache);
> erofs_fscache_unregister_fs(sb);
> kfree(sbi->opt.fsid);
> kfree(sbi);
> @@ -909,7 +904,8 @@ static void erofs_put_super(struct super_block *sb)
> iput(sbi->managed_cache);
> sbi->managed_cache = NULL;
> #endif
> - erofs_fscache_unregister_cookie(&sbi->s_fscache);
> + erofs_fscache_unregister_fs(sb);

> + sbi->s_fscache = NULL;

This line is not needed since we already call
erofs_fscache_unregister_fs() here.


With these fixed:
Reviewed-by: Jingbo Xu <[email protected]>

--
Thanks,
Jingbo

2022-09-16 04:26:55

by Jingbo Xu

[permalink] [raw]
Subject: Re: [PATCH V4 5/6] erofs: Support sharing cookies in the same domain



On 9/15/22 8:42 PM, Jia Zhu wrote:

> +static
> +struct erofs_fscache *erofs_fscache_domain_init_cookie(struct super_block *sb,
> + char *name, bool need_inode)
> +{
> + int err;
> + struct inode *inode;
> + struct erofs_fscache *ctx;
> + struct erofs_domain *domain = EROFS_SB(sb)->domain;
> +
> + ctx = erofs_fscache_acquire_cookie(sb, name, need_inode);
> + if (IS_ERR(ctx))
> + return ctx;
> +
> + ctx->name = kstrdup(name, GFP_KERNEL);
> + if (!ctx->name) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + inode = new_inode(erofs_pseudo_mnt->mnt_sb);
> + if (!inode) {
> + kfree(ctx->name);
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + ctx->domain = domain;
> + ctx->anon_inode = inode;
> + inode->i_private = ctx;
> + refcount_inc(&domain->ref);
> + return ctx;
> +out:
> + fscache_unuse_cookie(ctx->cookie, NULL, NULL);
> + fscache_relinquish_cookie(ctx->cookie, false);
> + if (need_inode)
> + iput(ctx->inode);
> + kfree(ctx);
> + return ERR_PTR(err);

Could you please abstract the cleanup logic into one helper? like:

erofs_fscache_relinquish_cookie()
{
fscache_unuse_cookie(ctx->cookie, NULL, NULL);
fscache_relinquish_cookie(ctx->cookie, false);
iput(ctx->inode);
kfree(ctx->name);
kfree(ctx);
}

which could also be called in erofs_fscache_unregister_cookie().


> void erofs_fscache_unregister_cookie(struct erofs_fscache *ctx)
> {
> + bool drop;
> + struct erofs_domain *domain;
> +
> if (!ctx)
> return;
> + domain = ctx->domain;
> + if (domain) {
> + mutex_lock(&erofs_domain_cookies_lock);
> + drop = atomic_read(&ctx->anon_inode->i_count) == 1;
> + iput(ctx->anon_inode);
> + mutex_unlock(&erofs_domain_cookies_lock);
> + if (!drop)
> + return;
> + }
>
> fscache_unuse_cookie(ctx->cookie, NULL, NULL);
> fscache_relinquish_cookie(ctx->cookie, false);
> + erofs_fscache_domain_put(domain);
> iput(ctx->inode);
> + kfree(ctx->name);
> kfree(ctx);
> }


--
Thanks,
Jingbo

2022-09-16 06:04:36

by Jingbo Xu

[permalink] [raw]
Subject: Re: [PATCH V4 6/6] erofs: introduce 'domain_id' mount option



On 9/15/22 8:42 PM, Jia Zhu wrote:
> Introduce 'domain_id' mount option to enable shared domain sementics.
> In which case, the related cookie is shared if two mountpoints in the
> same domain have the same data blob. Users could specify the name of
> domain by this mount option.
>
> Signed-off-by: Jia Zhu <[email protected]>

LGTM.

Reviewed-by: Jingbo Xu <[email protected]>


> ---
> fs/erofs/super.c | 17 +++++++++++++++++
> fs/erofs/sysfs.c | 19 +++++++++++++++++--
> 2 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 24bac58285e8..5e55c4fe6220 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -440,6 +440,7 @@ enum {
> Opt_dax_enum,
> Opt_device,
> Opt_fsid,
> + Opt_domain_id,
> Opt_err
> };
>
> @@ -465,6 +466,7 @@ static const struct fs_parameter_spec erofs_fs_parameters[] = {
> fsparam_enum("dax", Opt_dax_enum, erofs_dax_param_enums),
> fsparam_string("device", Opt_device),
> fsparam_string("fsid", Opt_fsid),
> + fsparam_string("domain_id", Opt_domain_id),
> {}
> };
>
> @@ -568,6 +570,16 @@ static int erofs_fc_parse_param(struct fs_context *fc,
> return -ENOMEM;
> #else
> errorfc(fc, "fsid option not supported");
> +#endif
> + break;
> + case Opt_domain_id:
> +#ifdef CONFIG_EROFS_FS_ONDEMAND
> + kfree(ctx->opt.domain_id);
> + ctx->opt.domain_id = kstrdup(param->string, GFP_KERNEL);
> + if (!ctx->opt.domain_id)
> + return -ENOMEM;
> +#else
> + errorfc(fc, "domain_id option not supported");
> #endif
> break;
> default:
> @@ -702,6 +714,7 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
> sb->s_fs_info = sbi;
> sbi->opt = ctx->opt;
> ctx->opt.fsid = NULL;
> + ctx->opt.domain_id = NULL;
> sbi->devs = ctx->devs;
> ctx->devs = NULL;
>
> @@ -846,6 +859,7 @@ static void erofs_fc_free(struct fs_context *fc)
>
> erofs_free_dev_context(ctx->devs);
> kfree(ctx->opt.fsid);
> + kfree(ctx->opt.domain_id);
> kfree(ctx);
> }
>
> @@ -914,6 +928,7 @@ static void erofs_kill_sb(struct super_block *sb)
> fs_put_dax(sbi->dax_dev, NULL);
> erofs_fscache_unregister_fs(sb);
> kfree(sbi->opt.fsid);
> + kfree(sbi->opt.domain_id);
> kfree(sbi);
> sb->s_fs_info = NULL;
> }
> @@ -1067,6 +1082,8 @@ static int erofs_show_options(struct seq_file *seq, struct dentry *root)
> #ifdef CONFIG_EROFS_FS_ONDEMAND
> if (opt->fsid)
> seq_printf(seq, ",fsid=%s", opt->fsid);
> + if (opt->domain_id)
> + seq_printf(seq, ",domain_id=%s", opt->domain_id);
> #endif
> return 0;
> }
> diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
> index c1383e508bbe..341fb43ad587 100644
> --- a/fs/erofs/sysfs.c
> +++ b/fs/erofs/sysfs.c
> @@ -201,12 +201,27 @@ static struct kobject erofs_feat = {
> int erofs_register_sysfs(struct super_block *sb)
> {
> struct erofs_sb_info *sbi = EROFS_SB(sb);
> + char *name;
> + char *str = NULL;
> int err;
>
> + if (erofs_is_fscache_mode(sb)) {
> + if (sbi->opt.domain_id) {
> + str = kasprintf(GFP_KERNEL, "%s,%s", sbi->opt.domain_id,
> + sbi->opt.fsid);
> + if (!str)
> + return -ENOMEM;
> + name = str;
> + } else {
> + name = sbi->opt.fsid;
> + }
> + } else {
> + name = sb->s_id;
> + }
> sbi->s_kobj.kset = &erofs_root;
> init_completion(&sbi->s_kobj_unregister);
> - err = kobject_init_and_add(&sbi->s_kobj, &erofs_sb_ktype, NULL, "%s",
> - erofs_is_fscache_mode(sb) ? sbi->opt.fsid : sb->s_id);
> + err = kobject_init_and_add(&sbi->s_kobj, &erofs_sb_ktype, NULL, "%s", name);
> + kfree(str);
> if (err)
> goto put_sb_kobj;
> return 0;

--
Thanks,
Jingbo