Add a check on each lower layer for the xwhiteout feature. This prevents unnecessary checking the overlay.whiteouts xattr when reading a directory if this feature is not enabled, i.e. most of the time.
Share the same xattr for the per-directory and the per-layer flag, which has the effect that if this is enabled for a layer, then the optimization to bypass checking of individual entries does not work on the root of the layer. This was deemed better, than having a separate xattr for the layer and the directory.
Fixes: bc8df7a3dc03 ("ovl: Add an alternative type of whiteout") Cc: stable@vger.kernel.org # v6.7 Signed-off-by: Miklos Szeredi mszeredi@redhat.com --- v2: - use overlay.whiteouts instead of overlay.feature_xwhiteout - move initialization to ovl_get_layers() - xwhiteouts can only be enabled on lower layer
fs/overlayfs/namei.c | 10 +++++++--- fs/overlayfs/overlayfs.h | 7 +++++-- fs/overlayfs/ovl_entry.h | 2 ++ fs/overlayfs/readdir.c | 11 ++++++++--- fs/overlayfs/super.c | 13 +++++++++++++ fs/overlayfs/util.c | 7 ++++++- 6 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index 03bc8d5dfa31..583cf56df66e 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -863,7 +863,8 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper, * Returns next layer in stack starting from top. * Returns -1 if this is the last layer. */ -int ovl_path_next(int idx, struct dentry *dentry, struct path *path) +int ovl_path_next(int idx, struct dentry *dentry, struct path *path, + const struct ovl_layer **layer) { struct ovl_entry *oe = OVL_E(dentry); struct ovl_path *lowerstack = ovl_lowerstack(oe); @@ -871,13 +872,16 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path) BUG_ON(idx < 0); if (idx == 0) { ovl_path_upper(dentry, path); - if (path->dentry) + if (path->dentry) { + *layer = &OVL_FS(dentry->d_sb)->layers[0]; return ovl_numlower(oe) ? 1 : -1; + } idx++; } BUG_ON(idx > ovl_numlower(oe)); path->dentry = lowerstack[idx - 1].dentry; - path->mnt = lowerstack[idx - 1].layer->mnt; + *layer = lowerstack[idx - 1].layer; + path->mnt = (*layer)->mnt;
return (idx < ovl_numlower(oe)) ? idx + 1 : -1; } diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index 05c3dd597fa8..6359cf5c66ff 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -492,7 +492,9 @@ bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, const struct path *path, enum ovl_xattr ox); bool ovl_path_check_origin_xattr(struct ovl_fs *ofs, const struct path *path); bool ovl_path_check_xwhiteout_xattr(struct ovl_fs *ofs, const struct path *path); -bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs, const struct path *path); +bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs, + const struct ovl_layer *layer, + const struct path *path); bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs, const struct path *upperpath);
@@ -674,7 +676,8 @@ int ovl_get_index_name(struct ovl_fs *ofs, struct dentry *origin, struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh); struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper, struct dentry *origin, bool verify); -int ovl_path_next(int idx, struct dentry *dentry, struct path *path); +int ovl_path_next(int idx, struct dentry *dentry, struct path *path, + const struct ovl_layer **layer); int ovl_verify_lowerdata(struct dentry *dentry); struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags); diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h index d82d2a043da2..33fcd3d3af30 100644 --- a/fs/overlayfs/ovl_entry.h +++ b/fs/overlayfs/ovl_entry.h @@ -40,6 +40,8 @@ struct ovl_layer { int idx; /* One fsid per unique underlying sb (upper fsid == 0) */ int fsid; + /* xwhiteouts are enabled on this layer*/ + bool xwhiteouts; };
struct ovl_path { diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c index a490fc47c3e7..c2597075e3f8 100644 --- a/fs/overlayfs/readdir.c +++ b/fs/overlayfs/readdir.c @@ -305,8 +305,6 @@ static inline int ovl_dir_read(const struct path *realpath, if (IS_ERR(realfile)) return PTR_ERR(realfile);
- rdd->in_xwhiteouts_dir = rdd->dentry && - ovl_path_check_xwhiteouts_xattr(OVL_FS(rdd->dentry->d_sb), realpath); rdd->first_maybe_whiteout = NULL; rdd->ctx.pos = 0; do { @@ -359,10 +357,14 @@ static int ovl_dir_read_merged(struct dentry *dentry, struct list_head *list, .is_lowest = false, }; int idx, next; + struct ovl_fs *ofs = OVL_FS(dentry->d_sb); + const struct ovl_layer *layer;
for (idx = 0; idx != -1; idx = next) { - next = ovl_path_next(idx, dentry, &realpath); + next = ovl_path_next(idx, dentry, &realpath, &layer); rdd.is_upper = ovl_dentry_upper(dentry) == realpath.dentry; + if (ovl_path_check_xwhiteouts_xattr(ofs, layer, &realpath)) + rdd.in_xwhiteouts_dir = true;
if (next != -1) { err = ovl_dir_read(&realpath, &rdd); @@ -568,6 +570,7 @@ static int ovl_dir_read_impure(const struct path *path, struct list_head *list, int err; struct path realpath; struct ovl_cache_entry *p, *n; + struct ovl_fs *ofs = OVL_FS(path->dentry->d_sb); struct ovl_readdir_data rdd = { .ctx.actor = ovl_fill_plain, .list = list, @@ -577,6 +580,8 @@ static int ovl_dir_read_impure(const struct path *path, struct list_head *list, INIT_LIST_HEAD(list); *root = RB_ROOT; ovl_path_upper(path->dentry, &realpath); + if (ovl_path_check_xwhiteouts_xattr(ofs, &ofs->layers[0], &realpath)) + rdd.in_xwhiteouts_dir = true;
err = ovl_dir_read(&realpath, &rdd); if (err) diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index a0967bb25003..04588721eb2a 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -1027,6 +1027,7 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs, struct ovl_fs_context_layer *l = &ctx->lower[i]; struct vfsmount *mnt; struct inode *trap; + struct path root; int fsid;
if (i < nr_merged_lower) @@ -1069,6 +1070,16 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs, */ mnt->mnt_flags |= MNT_READONLY | MNT_NOATIME;
+ /* + * Check if xwhiteout (xattr whiteout) support is enabled on + * this layer. + */ + root.mnt = mnt; + root.dentry = mnt->mnt_root; + err = ovl_path_getxattr(ofs, &root, OVL_XATTR_XWHITEOUTS, NULL, 0); + if (err >= 0) + layers[ofs->numlayer].xwhiteouts = true; + layers[ofs->numlayer].trap = trap; layers[ofs->numlayer].mnt = mnt; layers[ofs->numlayer].idx = ofs->numlayer; @@ -1079,6 +1090,8 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs, l->name = NULL; ofs->numlayer++; ofs->fs[fsid].is_lower = true; + + }
/* diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index c3f020ca13a8..6c6e6f5893ea 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -739,11 +739,16 @@ bool ovl_path_check_xwhiteout_xattr(struct ovl_fs *ofs, const struct path *path) return res >= 0; }
-bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs, const struct path *path) +bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs, + const struct ovl_layer *layer, + const struct path *path) { struct dentry *dentry = path->dentry; int res;
+ if (!layer->xwhiteouts) + return false; + /* xattr.whiteouts must be a directory */ if (!d_is_dir(dentry)) return false;
On Fri, Jan 19, 2024 at 12:14 PM Miklos Szeredi mszeredi@redhat.com wrote:
Add a check on each lower layer for the xwhiteout feature. This prevents unnecessary checking the overlay.whiteouts xattr when reading a directory if this feature is not enabled, i.e. most of the time.
Share the same xattr for the per-directory and the per-layer flag, which has the effect that if this is enabled for a layer, then the optimization to bypass checking of individual entries does not work on the root of the layer. This was deemed better, than having a separate xattr for the layer and the directory.
Fixes: bc8df7a3dc03 ("ovl: Add an alternative type of whiteout") Cc: stable@vger.kernel.org # v6.7 Signed-off-by: Miklos Szeredi mszeredi@redhat.com
v2:
- use overlay.whiteouts instead of overlay.feature_xwhiteout
- move initialization to ovl_get_layers()
- xwhiteouts can only be enabled on lower layer
fs/overlayfs/namei.c | 10 +++++++--- fs/overlayfs/overlayfs.h | 7 +++++-- fs/overlayfs/ovl_entry.h | 2 ++ fs/overlayfs/readdir.c | 11 ++++++++--- fs/overlayfs/super.c | 13 +++++++++++++ fs/overlayfs/util.c | 7 ++++++- 6 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index 03bc8d5dfa31..583cf56df66e 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -863,7 +863,8 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
- Returns next layer in stack starting from top.
- Returns -1 if this is the last layer.
*/ -int ovl_path_next(int idx, struct dentry *dentry, struct path *path) +int ovl_path_next(int idx, struct dentry *dentry, struct path *path,
const struct ovl_layer **layer){ struct ovl_entry *oe = OVL_E(dentry); struct ovl_path *lowerstack = ovl_lowerstack(oe); @@ -871,13 +872,16 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path) BUG_ON(idx < 0); if (idx == 0) { ovl_path_upper(dentry, path);
if (path->dentry)
if (path->dentry) {*layer = &OVL_FS(dentry->d_sb)->layers[0]; return ovl_numlower(oe) ? 1 : -1;} idx++; } BUG_ON(idx > ovl_numlower(oe)); path->dentry = lowerstack[idx - 1].dentry;
path->mnt = lowerstack[idx - 1].layer->mnt;
*layer = lowerstack[idx - 1].layer;path->mnt = (*layer)->mnt; return (idx < ovl_numlower(oe)) ? idx + 1 : -1;} diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index 05c3dd597fa8..6359cf5c66ff 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -492,7 +492,9 @@ bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, const struct path *path, enum ovl_xattr ox); bool ovl_path_check_origin_xattr(struct ovl_fs *ofs, const struct path *path); bool ovl_path_check_xwhiteout_xattr(struct ovl_fs *ofs, const struct path *path); -bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs, const struct path *path); +bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs,
const struct ovl_layer *layer,const struct path *path);bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs, const struct path *upperpath);
@@ -674,7 +676,8 @@ int ovl_get_index_name(struct ovl_fs *ofs, struct dentry *origin, struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh); struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper, struct dentry *origin, bool verify); -int ovl_path_next(int idx, struct dentry *dentry, struct path *path); +int ovl_path_next(int idx, struct dentry *dentry, struct path *path,
const struct ovl_layer **layer);int ovl_verify_lowerdata(struct dentry *dentry); struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags); diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h index d82d2a043da2..33fcd3d3af30 100644 --- a/fs/overlayfs/ovl_entry.h +++ b/fs/overlayfs/ovl_entry.h @@ -40,6 +40,8 @@ struct ovl_layer { int idx; /* One fsid per unique underlying sb (upper fsid == 0) */ int fsid;
/* xwhiteouts are enabled on this layer*/bool xwhiteouts;};
struct ovl_path { diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c index a490fc47c3e7..c2597075e3f8 100644 --- a/fs/overlayfs/readdir.c +++ b/fs/overlayfs/readdir.c @@ -305,8 +305,6 @@ static inline int ovl_dir_read(const struct path *realpath, if (IS_ERR(realfile)) return PTR_ERR(realfile);
rdd->in_xwhiteouts_dir = rdd->dentry &&ovl_path_check_xwhiteouts_xattr(OVL_FS(rdd->dentry->d_sb), realpath); rdd->first_maybe_whiteout = NULL; rdd->ctx.pos = 0; do {@@ -359,10 +357,14 @@ static int ovl_dir_read_merged(struct dentry *dentry, struct list_head *list, .is_lowest = false, }; int idx, next;
struct ovl_fs *ofs = OVL_FS(dentry->d_sb);const struct ovl_layer *layer; for (idx = 0; idx != -1; idx = next) {
next = ovl_path_next(idx, dentry, &realpath);
next = ovl_path_next(idx, dentry, &realpath, &layer); rdd.is_upper = ovl_dentry_upper(dentry) == realpath.dentry;if (ovl_path_check_xwhiteouts_xattr(ofs, layer, &realpath))rdd.in_xwhiteouts_dir = true; if (next != -1) { err = ovl_dir_read(&realpath, &rdd);@@ -568,6 +570,7 @@ static int ovl_dir_read_impure(const struct path *path, struct list_head *list, int err; struct path realpath; struct ovl_cache_entry *p, *n;
struct ovl_fs *ofs = OVL_FS(path->dentry->d_sb); struct ovl_readdir_data rdd = { .ctx.actor = ovl_fill_plain, .list = list,@@ -577,6 +580,8 @@ static int ovl_dir_read_impure(const struct path *path, struct list_head *list, INIT_LIST_HEAD(list); *root = RB_ROOT; ovl_path_upper(path->dentry, &realpath);
if (ovl_path_check_xwhiteouts_xattr(ofs, &ofs->layers[0], &realpath))rdd.in_xwhiteouts_dir = true;
Not needed since we do not support xwhiteouts on upper.
err = ovl_dir_read(&realpath, &rdd); if (err)diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index a0967bb25003..04588721eb2a 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -1027,6 +1027,7 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs, struct ovl_fs_context_layer *l = &ctx->lower[i]; struct vfsmount *mnt; struct inode *trap;
struct path root; int fsid; if (i < nr_merged_lower)@@ -1069,6 +1070,16 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs, */ mnt->mnt_flags |= MNT_READONLY | MNT_NOATIME;
/** Check if xwhiteout (xattr whiteout) support is enabled on* this layer.*/root.mnt = mnt;root.dentry = mnt->mnt_root;err = ovl_path_getxattr(ofs, &root, OVL_XATTR_XWHITEOUTS, NULL, 0);if (err >= 0)layers[ofs->numlayer].xwhiteouts = true;layers[ofs->numlayer].trap = trap; layers[ofs->numlayer].mnt = mnt; layers[ofs->numlayer].idx = ofs->numlayer;@@ -1079,6 +1090,8 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs, l->name = NULL; ofs->numlayer++; ofs->fs[fsid].is_lower = true;
extra spaces.
} /*diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index c3f020ca13a8..6c6e6f5893ea 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -739,11 +739,16 @@ bool ovl_path_check_xwhiteout_xattr(struct ovl_fs *ofs, const struct path *path) return res >= 0; }
-bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs, const struct path *path) +bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs,
const struct ovl_layer *layer,const struct path *path){ struct dentry *dentry = path->dentry; int res;
if (!layer->xwhiteouts)return false;/* xattr.whiteouts must be a directory */ if (!d_is_dir(dentry)) return false;-- 2.43.0
Do you want me to fix/test and send this to Linus?
Alex, can we add your RVB to v2?
Thanks, Amir.
On Fri, 19 Jan 2024 at 12:08, Amir Goldstein amir73il@gmail.com wrote:
@@ -577,6 +580,8 @@ static int ovl_dir_read_impure(const struct path *path, struct list_head *list, INIT_LIST_HEAD(list); *root = RB_ROOT; ovl_path_upper(path->dentry, &realpath);
if (ovl_path_check_xwhiteouts_xattr(ofs, &ofs->layers[0], &realpath))rdd.in_xwhiteouts_dir = true;Not needed since we do not support xwhiteouts on upper.
Right.
@@ -1079,6 +1090,8 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs, l->name = NULL; ofs->numlayer++; ofs->fs[fsid].is_lower = true;
extra spaces.
Sorry, missing self review...
Do you want me to fix/test and send this to Linus?
Yes please, if it's not a problem four you.
Thanks, Miklos
On Fri, Jan 19, 2024 at 1:20 PM Miklos Szeredi miklos@szeredi.hu wrote:
On Fri, 19 Jan 2024 at 12:08, Amir Goldstein amir73il@gmail.com wrote:
@@ -577,6 +580,8 @@ static int ovl_dir_read_impure(const struct path *path, struct list_head *list, INIT_LIST_HEAD(list); *root = RB_ROOT; ovl_path_upper(path->dentry, &realpath);
if (ovl_path_check_xwhiteouts_xattr(ofs, &ofs->layers[0], &realpath))rdd.in_xwhiteouts_dir = true;Not needed since we do not support xwhiteouts on upper.
Right.
@@ -1079,6 +1090,8 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs, l->name = NULL; ofs->numlayer++; ofs->fs[fsid].is_lower = true;
extra spaces.
Sorry, missing self review...
Do you want me to fix/test and send this to Linus?
Yes please, if it's not a problem four you.
ok. queued.
Thanks, Amir.
On Fri, 2024-01-19 at 13:08 +0200, Amir Goldstein wrote:
On Fri, Jan 19, 2024 at 12:14 PM Miklos Szeredi mszeredi@redhat.com wrote:
Add a check on each lower layer for the xwhiteout feature. This prevents unnecessary checking the overlay.whiteouts xattr when reading a directory if this feature is not enabled, i.e. most of the time.
Share the same xattr for the per-directory and the per-layer flag, which has the effect that if this is enabled for a layer, then the optimization to bypass checking of individual entries does not work on the root of the layer. This was deemed better, than having a separate xattr for the layer and the directory.
Fixes: bc8df7a3dc03 ("ovl: Add an alternative type of whiteout") Cc: stable@vger.kernel.org # v6.7 Signed-off-by: Miklos Szeredi mszeredi@redhat.com
v2: - use overlay.whiteouts instead of overlay.feature_xwhiteout - move initialization to ovl_get_layers() - xwhiteouts can only be enabled on lower layer
fs/overlayfs/namei.c | 10 +++++++--- fs/overlayfs/overlayfs.h | 7 +++++-- fs/overlayfs/ovl_entry.h | 2 ++ fs/overlayfs/readdir.c | 11 ++++++++--- fs/overlayfs/super.c | 13 +++++++++++++ fs/overlayfs/util.c | 7 ++++++- 6 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index 03bc8d5dfa31..583cf56df66e 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -863,7 +863,8 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper, * Returns next layer in stack starting from top. * Returns -1 if this is the last layer. */ -int ovl_path_next(int idx, struct dentry *dentry, struct path *path) +int ovl_path_next(int idx, struct dentry *dentry, struct path *path, + const struct ovl_layer **layer) { struct ovl_entry *oe = OVL_E(dentry); struct ovl_path *lowerstack = ovl_lowerstack(oe); @@ -871,13 +872,16 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path) BUG_ON(idx < 0); if (idx == 0) { ovl_path_upper(dentry, path); - if (path->dentry) + if (path->dentry) { + *layer = &OVL_FS(dentry->d_sb)->layers[0]; return ovl_numlower(oe) ? 1 : -1; + } idx++; } BUG_ON(idx > ovl_numlower(oe)); path->dentry = lowerstack[idx - 1].dentry; - path->mnt = lowerstack[idx - 1].layer->mnt; + *layer = lowerstack[idx - 1].layer; + path->mnt = (*layer)->mnt;
return (idx < ovl_numlower(oe)) ? idx + 1 : -1; } diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index 05c3dd597fa8..6359cf5c66ff 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -492,7 +492,9 @@ bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, const struct path *path, enum ovl_xattr ox); bool ovl_path_check_origin_xattr(struct ovl_fs *ofs, const struct path *path); bool ovl_path_check_xwhiteout_xattr(struct ovl_fs *ofs, const struct path *path); -bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs, const struct path *path); +bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs, + const struct ovl_layer *layer, + const struct path *path); bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs, const struct path *upperpath);
@@ -674,7 +676,8 @@ int ovl_get_index_name(struct ovl_fs *ofs, struct dentry *origin, struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh); struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper, struct dentry *origin, bool verify); -int ovl_path_next(int idx, struct dentry *dentry, struct path *path); +int ovl_path_next(int idx, struct dentry *dentry, struct path *path, + const struct ovl_layer **layer); int ovl_verify_lowerdata(struct dentry *dentry); struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags); diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h index d82d2a043da2..33fcd3d3af30 100644 --- a/fs/overlayfs/ovl_entry.h +++ b/fs/overlayfs/ovl_entry.h @@ -40,6 +40,8 @@ struct ovl_layer { int idx; /* One fsid per unique underlying sb (upper fsid == 0) */ int fsid; + /* xwhiteouts are enabled on this layer*/ + bool xwhiteouts; };
struct ovl_path { diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c index a490fc47c3e7..c2597075e3f8 100644 --- a/fs/overlayfs/readdir.c +++ b/fs/overlayfs/readdir.c @@ -305,8 +305,6 @@ static inline int ovl_dir_read(const struct path *realpath, if (IS_ERR(realfile)) return PTR_ERR(realfile);
- rdd->in_xwhiteouts_dir = rdd->dentry && - ovl_path_check_xwhiteouts_xattr(OVL_FS(rdd->dentry-
d_sb), realpath);
rdd->first_maybe_whiteout = NULL; rdd->ctx.pos = 0; do { @@ -359,10 +357,14 @@ static int ovl_dir_read_merged(struct dentry *dentry, struct list_head *list, .is_lowest = false, }; int idx, next; + struct ovl_fs *ofs = OVL_FS(dentry->d_sb); + const struct ovl_layer *layer;
for (idx = 0; idx != -1; idx = next) { - next = ovl_path_next(idx, dentry, &realpath); + next = ovl_path_next(idx, dentry, &realpath, &layer); rdd.is_upper = ovl_dentry_upper(dentry) == realpath.dentry; + if (ovl_path_check_xwhiteouts_xattr(ofs, layer, &realpath)) + rdd.in_xwhiteouts_dir = true;
if (next != -1) { err = ovl_dir_read(&realpath, &rdd); @@ -568,6 +570,7 @@ static int ovl_dir_read_impure(const struct path *path, struct list_head *list, int err; struct path realpath; struct ovl_cache_entry *p, *n; + struct ovl_fs *ofs = OVL_FS(path->dentry->d_sb); struct ovl_readdir_data rdd = { .ctx.actor = ovl_fill_plain, .list = list, @@ -577,6 +580,8 @@ static int ovl_dir_read_impure(const struct path *path, struct list_head *list, INIT_LIST_HEAD(list); *root = RB_ROOT; ovl_path_upper(path->dentry, &realpath); + if (ovl_path_check_xwhiteouts_xattr(ofs, &ofs->layers[0], &realpath)) + rdd.in_xwhiteouts_dir = true;
Not needed since we do not support xwhiteouts on upper.
err = ovl_dir_read(&realpath, &rdd); if (err) diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index a0967bb25003..04588721eb2a 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -1027,6 +1027,7 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs, struct ovl_fs_context_layer *l = &ctx->lower[i]; struct vfsmount *mnt; struct inode *trap; + struct path root; int fsid;
if (i < nr_merged_lower) @@ -1069,6 +1070,16 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs, */ mnt->mnt_flags |= MNT_READONLY | MNT_NOATIME;
+ /* + * Check if xwhiteout (xattr whiteout) support is enabled on + * this layer. + */ + root.mnt = mnt; + root.dentry = mnt->mnt_root; + err = ovl_path_getxattr(ofs, &root, OVL_XATTR_XWHITEOUTS, NULL, 0); + if (err >= 0) + layers[ofs->numlayer].xwhiteouts = true;
layers[ofs->numlayer].trap = trap; layers[ofs->numlayer].mnt = mnt; layers[ofs->numlayer].idx = ofs->numlayer; @@ -1079,6 +1090,8 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs, l->name = NULL; ofs->numlayer++; ofs->fs[fsid].is_lower = true;
extra spaces.
}
/* diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index c3f020ca13a8..6c6e6f5893ea 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -739,11 +739,16 @@ bool ovl_path_check_xwhiteout_xattr(struct ovl_fs *ofs, const struct path *path) return res >= 0; }
-bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs, const struct path *path) +bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs, + const struct ovl_layer *layer, + const struct path *path) { struct dentry *dentry = path->dentry; int res;
+ if (!layer->xwhiteouts) + return false;
/* xattr.whiteouts must be a directory */ if (!d_is_dir(dentry)) return false; -- 2.43.0
Do you want me to fix/test and send this to Linus?
Alex, can we add your RVB to v2?
Yeah, other that your comments this looks good to me (and I tested it here too).
Reviewed-By: Alexander Larsson alex@redhat.com
I'll have a look at doing the required changes in composefs.
Alex, can we add your RVB to v2?
Yeah, other that your comments this looks good to me (and I tested it here too).
Reviewed-By: Alexander Larsson alex@redhat.com
Added.
Thanks, Amir.
On Fri, 2024-01-19 at 13:08 +0200, Amir Goldstein wrote:
On Fri, Jan 19, 2024 at 12:14 PM Miklos Szeredi mszeredi@redhat.com wrote:
Do you want me to fix/test and send this to Linus?
Alex, can we add your RVB to v2?
I ran into an issue converting composefs to use this.
Suppose we have a chroot of files containing some upper dirs and we want to make a composefs of this. For example, say /foo/lower/dir/whiteout is a traditional whiteout.
Previously, what happened is that I marked the whiteout file with trusted.overlay.overlay.whiteout, and the /foo/lower/dir with trusted.overlay.overlay.whiteouts.
Them when I mounted then entire chroot with overlayfs these xattrs would get unescaped and I would get a $mnt/foo/lower/dir/whiteout with a trusted.overlay.whiteout xattr, and a $mnt/foo/lower/dir with a trusted.overlay.whiteout. When I then mounted another overlayfs with a lowerdir of $mnt/foo/lower it would treat the whiteout as a xwhiteout.
However, now I need the lowerdir toplevel dir to also have a trusted.overlay.whiteouts xattr. But when I'm converting the entire chroot I do not know which of the directories is going to be used as the toplevel lower dir, so I don't know where to put this marker.
The only solution I see is to put it on *all* parent directories. Is there a better approach here?
On Fri, Jan 19, 2024 at 6:35 PM Alexander Larsson alexl@redhat.com wrote:
On Fri, 2024-01-19 at 13:08 +0200, Amir Goldstein wrote:
On Fri, Jan 19, 2024 at 12:14 PM Miklos Szeredi mszeredi@redhat.com wrote:
Do you want me to fix/test and send this to Linus?
Alex, can we add your RVB to v2?
I ran into an issue converting composefs to use this.
Suppose we have a chroot of files containing some upper dirs and we want to make a composefs of this. For example, say /foo/lower/dir/whiteout is a traditional whiteout.
Previously, what happened is that I marked the whiteout file with trusted.overlay.overlay.whiteout, and the /foo/lower/dir with trusted.overlay.overlay.whiteouts.
Them when I mounted then entire chroot with overlayfs these xattrs would get unescaped and I would get a $mnt/foo/lower/dir/whiteout with a trusted.overlay.whiteout xattr, and a $mnt/foo/lower/dir with a trusted.overlay.whiteout. When I then mounted another overlayfs with a lowerdir of $mnt/foo/lower it would treat the whiteout as a xwhiteout.
However, now I need the lowerdir toplevel dir to also have a trusted.overlay.whiteouts xattr. But when I'm converting the entire chroot I do not know which of the directories is going to be used as the toplevel lower dir, so I don't know where to put this marker.
The only solution I see is to put it on *all* parent directories. Is there a better approach here?
How about checking xwhiteouts xattrs along with impure and origin xattrs in ovl_get_inode()?
Then there will be no overhead in readdir and no need for marking the layer root?
Miklos, would that be acceptable?
Thanks, Amir.
On Fri, 19 Jan 2024 at 20:06, Amir Goldstein amir73il@gmail.com wrote:
How about checking xwhiteouts xattrs along with impure and origin xattrs in ovl_get_inode()?
Then there will be no overhead in readdir and no need for marking the layer root?
Miklos, would that be acceptable?
It's certainly a good idea, but doesn't really address my worry. The minor performance impact is not what bothers me most. It's the fact that in the common case the result of these calls are discarded. That's just plain ugly, IMO.
My preferred alternative would be a mount option. Amir, Alex, would you both be okay with that?
Thanks, Miklos
On Fri, Jan 19, 2024 at 10:30 PM Miklos Szeredi miklos@szeredi.hu wrote:
On Fri, 19 Jan 2024 at 20:06, Amir Goldstein amir73il@gmail.com wrote:
How about checking xwhiteouts xattrs along with impure and origin xattrs in ovl_get_inode()?
That was not a very good suggestion on my part. ovl_get_inode() only checks impure xattr on upper dir and origin xattr on non-merge non-multi lower dir.
If we change the location of xwhiteout xattr check, it should be in ovl_lookup_single() next to checking opaque xattr, which makes me think - hey, why don't we overload opaque xattr, just like we did with metacopy xattr?
An overlay.opaque xattr with empty string means "may have xwhiteouts" and that is backward compatible, because ovl_is_opaquedir() checks for xattr of length 1.
The only extra getxattr() needed would be for the d->last case...
Then there will be no overhead in readdir and no need for marking the layer root?
Miklos, would that be acceptable?
It's certainly a good idea, but doesn't really address my worry. The minor performance impact is not what bothers me most. It's the fact that in the common case the result of these calls are discarded. That's just plain ugly, IMO.
...so the question boils down to, whether you find it too ugly to always getxattr(opaque) on lookup of the last lower layer and whether you find the overloading of opaque xattr too hacky?
As a precedent, we *always* check metacopy xattr in last lower layer to check for error conditions, even if user did not opt-in to metacopy at all, while we could just as easily have ignored it.
My preferred alternative would be a mount option. Amir, Alex, would you both be okay with that?
I think I had suggested that escaped private xattrs would also require an opt-in mount option, but Alex explained that the users mounting the overlay are not always aware of the fact that the layers were composed this way, but I admit that I do not remember all the exact details.
Alex, do I remember correctly that the overlay instance where xwhiteouts needs to be checked does NOT necessarily have a lowerdata layers? The composefs instance with lowerdata layers is the one exposing the (escaped) xwhiteout entries as xwhiteouts. Is that correct?
Is there even a use case for xwhiteouts NOT inside another lower overlayfs?
If we limit the check for xwhiteouts only to nested overlayfs, then maybe Miklos will care less about an extra getxattr on lookup?
Attached patch implements both xwhiteout and opaque checks during lookup - we can later choose only one of them to keep.
Note that is changes the optimization to per-dentry, not per-layer, so in the common case (no layers have xwhiteouts) xwhiteouts will not be checked, but if xwhiteouts exist in any lower layer in the stack, then xwhiteouts will be checked in all the layers of the merged dir (*).
(*) still need to optimize away lookup of xwhiteouts in upperdir.
Let me know what you think.
Thanks, Amir.
On Sat, 2024-01-20 at 12:32 +0200, Amir Goldstein wrote:
On Fri, Jan 19, 2024 at 10:30 PM Miklos Szeredi miklos@szeredi.hu wrote:
On Fri, 19 Jan 2024 at 20:06, Amir Goldstein amir73il@gmail.com wrote:
How about checking xwhiteouts xattrs along with impure and origin xattrs in ovl_get_inode()?
That was not a very good suggestion on my part. ovl_get_inode() only checks impure xattr on upper dir and origin xattr on non-merge non-multi lower dir.
If we change the location of xwhiteout xattr check, it should be in ovl_lookup_single() next to checking opaque xattr, which makes me think - hey, why don't we overload opaque xattr, just like we did with metacopy xattr?
An overlay.opaque xattr with empty string means "may have xwhiteouts" and that is backward compatible, because ovl_is_opaquedir() checks for xattr of length 1.
The only extra getxattr() needed would be for the d->last case...
Then there will be no overhead in readdir and no need for marking the layer root?
Miklos, would that be acceptable?
It's certainly a good idea, but doesn't really address my worry. The minor performance impact is not what bothers me most. It's the fact that in the common case the result of these calls are discarded. That's just plain ugly, IMO.
...so the question boils down to, whether you find it too ugly to always getxattr(opaque) on lookup of the last lower layer and whether you find the overloading of opaque xattr too hacky?
As a precedent, we *always* check metacopy xattr in last lower layer to check for error conditions, even if user did not opt-in to metacopy at all, while we could just as easily have ignored it.
My preferred alternative would be a mount option. Amir, Alex, would you both be okay with that?
I think I had suggested that escaped private xattrs would also require an opt-in mount option, but Alex explained that the users mounting the overlay are not always aware of the fact that the layers were composed this way, but I admit that I do not remember all the exact details.
Alex, do I remember correctly that the overlay instance where xwhiteouts needs to be checked does NOT necessarily have a lowerdata layers? The composefs instance with lowerdata layers is the one exposing the (escaped) xwhiteout entries as xwhiteouts. Is that correct?
Is there even a use case for xwhiteouts NOT inside another lower overlayfs?
No. Strictly speaking regular whiteouts are always preferable to xwhiteouts (as they work for both user and system ovl mounts and are supported by older kernels and existing software, etc). The only place where xwhiteouts are useful is when we need to escape them to put inside an overlayfs mount.
The best way to think about the composefs usecase is:
Suppose you had a pre-existing system image that someone installed a multi layer container image in. This means that somewhere inside this image is a set of lowerdirs, and one of them may have a traditional whiteout. Now we want to create an overlayfs mount that when used, works just like the above image would work, including when you mount the sub-overlayfs mounts from the container image lowerdirs.
Fallout of this:
The composefs overlay lowerdir will need to contain escaped xwhiteouts, so that the mount will have unescaped xwhiteouts. These escaped whiteouts can be anywhere, even in a "single layer" overlayfs. But the unescaped xwhiteouts will never be in a lowermost lowerdir.
In the composefs case we will be using lowerdata for the outer overlayfs, but the actual unescaped xwhiteouts in the container image lowerdirs don't need to have a lowerdata involved at all.
The container mount will be done by pre-existing software (say docker) that isn't aware that we converted the regular whiteouts to xwhiteouts, so having to use a mount option is not ideal (would require docker changes).
If we limit the check for xwhiteouts only to nested overlayfs, then maybe Miklos will care less about an extra getxattr on lookup?
Attached patch implements both xwhiteout and opaque checks during lookup - we can later choose only one of them to keep.
Seems like you attached the wrong patch, but I will comment on the other patch you sent to the list.
Note that is changes the optimization to per-dentry, not per-layer, so in the common case (no layers have xwhiteouts) xwhiteouts will not be checked, but if xwhiteouts exist in any lower layer in the stack, then xwhiteouts will be checked in all the layers of the merged dir (*).
(*) still need to optimize away lookup of xwhiteouts in upperdir.
Let me know what you think.
Thanks, Amir.
On Fri, Jan 19, 2024 at 6:35 PM Alexander Larsson alexl@redhat.com wrote:
On Fri, 2024-01-19 at 13:08 +0200, Amir Goldstein wrote:
On Fri, Jan 19, 2024 at 12:14 PM Miklos Szeredi mszeredi@redhat.com wrote:
Do you want me to fix/test and send this to Linus?
Alex, can we add your RVB to v2?
I ran into an issue converting composefs to use this.
Suppose we have a chroot of files containing some upper dirs and we want to make a composefs of this. For example, say /foo/lower/dir/whiteout is a traditional whiteout.
Previously, what happened is that I marked the whiteout file with trusted.overlay.overlay.whiteout, and the /foo/lower/dir with trusted.overlay.overlay.whiteouts.
Them when I mounted then entire chroot with overlayfs these xattrs would get unescaped and I would get a $mnt/foo/lower/dir/whiteout with a trusted.overlay.whiteout xattr, and a $mnt/foo/lower/dir with a trusted.overlay.whiteout. When I then mounted another overlayfs with a lowerdir of $mnt/foo/lower it would treat the whiteout as a xwhiteout.
However, now I need the lowerdir toplevel dir to also have a trusted.overlay.whiteouts xattr. But when I'm converting the entire chroot I do not know which of the directories is going to be used as the toplevel lower dir, so I don't know where to put this marker.
The only solution I see is to put it on *all* parent directories. Is there a better approach here?
Alex,
As you can see, I posted v3 with an alternative approach that would not require marking all possible lower layer roots.
However, I cannot help wondering if it wouldn't be better practice, when composing layers, to always be explicit, per-directory about whether the composed directory is a "base" or a "diff" layer.
Isn't this information always known at composing time?
In legacy overlayfs, there is an explicit mark for "this is a base dir" - namely, the opaque xattr, but there is no such explicit mark on directories without an entry with the same name in layers below them.
The lack of explicit mark "merge" vs. "opaque" in all directories in all the layers had led to problems in the past, for example, this is the reason that this fix was needed:
b79e05aaa166 ovl: no direct iteration for dir with origin xattr
In conclusion, since composefs is the first tool, that I know of, to compose "non-legacy" overlayfs layers (i.e. with overlay xattrs), I think the correct design decision would mark every directory in every layer explicitly as at exactly one of "merge"/"opaque".
Note that non-dir are always marked explicitly as "metacopy", so there is no ambiguity with non-dirs and we also error out if a non-dir stack does not end with an "opaque" entry.
Additionally, when composing layers, since all the children of a directory should be explicitly marked as "merge" vs. "opaque" then the parent's "impure" (meaning contains "merge" children) can also be set at composing time.
Failing to set "impure" correctly when composing layers could result in wrong readdir d_ino results.
My proposition in v3 for an explicit mark was to "reinterpret the opaque xattr from boolean to enum". My proposal included only the states 'y' (opaque) and 'x' (contains xwhiteouts), but for composefs, I would extend this to also mark a merge dir explicitly with opaque='n' and explicitly mark all the directories in a "base layer" with opaque='y'.
Implementation-wise, composefs could start by marking each directory with either 'y'/'n' state based on the lowerstack, and if xwhiteout entries are added, 'n' state could be changed to 'x' state.
What do you think?
Does it make sense from composefs POV? Am I correct to assume that at composing time, every directory state is known (base 'y' vs. diff 'n')?
Thanks, Amir.
On Mon, 2024-01-22 at 10:38 +0200, Amir Goldstein wrote:
On Fri, Jan 19, 2024 at 6:35 PM Alexander Larsson alexl@redhat.com wrote:
On Fri, 2024-01-19 at 13:08 +0200, Amir Goldstein wrote:
On Fri, Jan 19, 2024 at 12:14 PM Miklos Szeredi mszeredi@redhat.com wrote:
Do you want me to fix/test and send this to Linus?
Alex, can we add your RVB to v2?
I ran into an issue converting composefs to use this.
Suppose we have a chroot of files containing some upper dirs and we want to make a composefs of this. For example, say /foo/lower/dir/whiteout is a traditional whiteout.
Previously, what happened is that I marked the whiteout file with trusted.overlay.overlay.whiteout, and the /foo/lower/dir with trusted.overlay.overlay.whiteouts.
Them when I mounted then entire chroot with overlayfs these xattrs would get unescaped and I would get a $mnt/foo/lower/dir/whiteout with a trusted.overlay.whiteout xattr, and a $mnt/foo/lower/dir with a trusted.overlay.whiteout. When I then mounted another overlayfs with a lowerdir of $mnt/foo/lower it would treat the whiteout as a xwhiteout.
However, now I need the lowerdir toplevel dir to also have a trusted.overlay.whiteouts xattr. But when I'm converting the entire chroot I do not know which of the directories is going to be used as the toplevel lower dir, so I don't know where to put this marker.
The only solution I see is to put it on *all* parent directories. Is there a better approach here?
Alex,
As you can see, I posted v3 with an alternative approach that would not require marking all possible lower layer roots.
However, I cannot help wondering if it wouldn't be better practice, when composing layers, to always be explicit, per-directory about whether the composed directory is a "base" or a "diff" layer.
Isn't this information always known at composing time?
Currently, composefs images are not layered as such. They normally only have one or more lowerdata layers, and then the actual image as a single lowerdir, and on top of that an optional upper if you want some kind of writability.
But, when composing the composefs the content of the image is opaque to us. We're just given a directory with some files in it for the image. It might contain some other lowerdirs, but the details are not know to us at compose time.
That said, I can see that in some cases it may make sense to use multiple lower dirs, for example when using composefs for multi-layer container images. When generating such layers we typically have the lower levels available, so we could probably extract the base/dir status for each directory.
In legacy overlayfs, there is an explicit mark for "this is a base dir" - namely, the opaque xattr, but there is no such explicit mark on directories without an entry with the same name in layers below them.
The lack of explicit mark "merge" vs. "opaque" in all directories in all the layers had led to problems in the past, for example, this is the reason that this fix was needed:
b79e05aaa166 ovl: no direct iteration for dir with origin xattr
In conclusion, since composefs is the first tool, that I know of, to compose "non-legacy" overlayfs layers (i.e. with overlay xattrs), I think the correct design decision would mark every directory in every layer explicitly as at exactly one of "merge"/"opaque".
Note that non-dir are always marked explicitly as "metacopy", so there is no ambiguity with non-dirs and we also error out if a non-dir stack does not end with an "opaque" entry.
Additionally, when composing layers, since all the children of a directory should be explicitly marked as "merge" vs. "opaque" then the parent's "impure" (meaning contains "merge" children) can also be set at composing time.
Failing to set "impure" correctly when composing layers could result in wrong readdir d_ino results.
My proposition in v3 for an explicit mark was to "reinterpret the opaque xattr from boolean to enum". My proposal included only the states 'y' (opaque) and 'x' (contains xwhiteouts), but for composefs, I would extend this to also mark a merge dir explicitly with opaque='n' and explicitly mark all the directories in a "base layer" with opaque='y'.
Implementation-wise, composefs could start by marking each directory with either 'y'/'n' state based on the lowerstack, and if xwhiteout entries are added, 'n' state could be changed to 'x' state.
We never add xwhiteout entries to the composefs, all directories in the base layer would be overlay=n or y, and any directory containing an escaped xwhiteout would have an escaped opaque xattr with 'x'.
What do you think?
I feel it may be overkill, as most composefs images would be a one- layer thing, and adding opaque=n to every directory in the lowermost layer would just waste space and makes little sense (being in the lowest layer means we already know if the directory is merged or not). However, I think it may make sense to be able to mark non-lowest-layer directories with either n or y.
Does it make sense from composefs POV? Am I correct to assume that at composing time, every directory state is known (base 'y' vs. diff 'n')?
Thanks, Amir.
Alex,
As you can see, I posted v3 with an alternative approach that would not require marking all possible lower layer roots.
However, I cannot help wondering if it wouldn't be better practice, when composing layers, to always be explicit, per-directory about whether the composed directory is a "base" or a "diff" layer.
Isn't this information always known at composing time?
Currently, composefs images are not layered as such. They normally only have one or more lowerdata layers, and then the actual image as a single lowerdir, and on top of that an optional upper if you want some kind of writability.
But, when composing the composefs the content of the image is opaque to us. We're just given a directory with some files in it for the image. It might contain some other lowerdirs, but the details are not know to us at compose time.
Got it, though I may need you to explain this again to me next time ;)
If we were to change the tools that pack/extract overlayfs images to mark directories more explicitly, we would need to change other tools, not composefs.
composefs has no knowledge of the fact that it is packing an overlayfs image, until it is asked to pack a whiteout or a file/dir that happens to have an overlay.* xattr, but lower layers do not typically have to contain files/dir with overlay.* xattrs.
However, I think it may make sense to be able to mark non-lowest-layer directories with either n or y.
There is nothing stopping you from setting opaque=y on lowest layer dirs or setting opaque=n on merge dirs when packing composefs. Old kernels will not be bothered by these marks.
However, I forgot about the consideration of xattr lookup performance that was the drive for erofs xattr bloom filter support.
I guess erofs will pack much smaller without those explicit annotations and getxattr(opaque) will have better performance for no xattr case then with opaque=n.
Thanks, Amir.
linux-stable-mirror@lists.linaro.org
-
Alexander Larsson -
Amir Goldstein -
Miklos Szeredi -
Miklos Szeredi