| Message ID | 20240118104144.465158-1-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ovl: require xwhiteout feature flag on layer roots | expand |
On Thu, Jan 18, 2024 at 12:41 PM Miklos Szeredi <mszeredi@redhat.com> wrote: > > Add a check on each 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. Does it really have a significant cost or do you just not like the unneeded check? IIRC, we anyway check for ORIGIN xattr and IMPURE xattr on readdir. > > Fixes: bc8df7a3dc03 ("ovl: Add an alternative type of whiteout") > Cc: <stable@vger.kernel.org> # v6.7 > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > > Hi Alex, > > Can you please test this in your environment? > > I xwhiteout test in xfstests needs this tweak: > > --- a/tests/overlay/084 > +++ b/tests/overlay/084 > @@ -115,6 +115,7 @@ do_test_xwhiteout() > > mkdir -p $basedir/lower $basedir/upper $basedir/work > touch $basedir/lower/regular $basedir/lower/hidden $basedir/upper/hidden > + setfattr -n $prefix.overlay.feature_xwhiteout -v "y" $basedir/upper > setfattr -n $prefix.overlay.whiteouts -v "y" $basedir/upper > setfattr -n $prefix.overlay.whiteout -v "y" $basedir/upper/hidden > > > Thanks, > Miklos > > fs/overlayfs/namei.c | 10 +++++++--- > fs/overlayfs/overlayfs.h | 8 ++++++-- > fs/overlayfs/ovl_entry.h | 2 ++ > fs/overlayfs/readdir.c | 11 ++++++++--- > fs/overlayfs/super.c | 13 ++++++++++++- > fs/overlayfs/util.c | 9 ++++++++- > 6 files changed, 43 insertions(+), 10 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..991eb5d5c66c 100644 > --- a/fs/overlayfs/overlayfs.h > +++ b/fs/overlayfs/overlayfs.h > @@ -51,6 +51,7 @@ enum ovl_xattr { > OVL_XATTR_PROTATTR, > OVL_XATTR_XWHITEOUT, > OVL_XATTR_XWHITEOUTS, > + OVL_XATTR_FEATURE_XWHITEOUT, Can we not add a new OVL_XATTR_FEATURE_XWHITEOUT xattr. Setting OVL_XATTR_XWHITEOUTS on directories with xwhiteouts is anyway the responsibility of the layer composer. Let's just require the layer composer to set OVL_XATTR_XWHITEOUTS on the layer root even if it does not have any immediate xwhiteout children as "layer may have xwhiteouts" indication. ok? > }; > > enum ovl_inode_flag { > @@ -492,7 +493,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 +677,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..51729e614f5a 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 feature_xwhiteout; > }; > > 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..4e507ab780f3 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -1291,7 +1291,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc) > struct ovl_entry *oe; > struct ovl_layer *layers; > struct cred *cred; > - int err; > + int err, i; > > err = -EIO; > if (WARN_ON(fc->user_ns != current_user_ns())) > @@ -1414,6 +1414,17 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc) > if (err) > goto out_free_oe; > > + for (i = 0; i < ofs->numlayer; i++) { > + struct path path = { .mnt = layers[i].mnt }; > + > + if (path.mnt) { > + path.dentry = path.mnt->mnt_root; > + err = ovl_path_getxattr(ofs, &path, OVL_XATTR_FEATURE_XWHITEOUT, NULL, 0); > + if (err >= 0) > + layers[i].feature_xwhiteout = true; Any reason not to do this in ovl_get_layers() when adding the layer? Thanks, Amir.
On Thu, 18 Jan 2024 at 12:22, Amir Goldstein <amir73il@gmail.com> wrote: > > On Thu, Jan 18, 2024 at 12:41 PM Miklos Szeredi <mszeredi@redhat.com> wrote: > > > > Add a check on each 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. > > Does it really have a significant cost or do you just not like the > unneeded check? It's probably insignificant. But I don't know and it would be hard to prove. > IIRC, we anyway check for ORIGIN xattr and IMPURE xattr on > readdir. We check those on lookup, not at readdir. Might make sense to check XWHITEOUTS at lookup regardless of this patch, just for consistency. > > --- a/fs/overlayfs/overlayfs.h > > +++ b/fs/overlayfs/overlayfs.h > > @@ -51,6 +51,7 @@ enum ovl_xattr { > > OVL_XATTR_PROTATTR, > > OVL_XATTR_XWHITEOUT, > > OVL_XATTR_XWHITEOUTS, > > + OVL_XATTR_FEATURE_XWHITEOUT, > > Can we not add a new OVL_XATTR_FEATURE_XWHITEOUT xattr. > > Setting OVL_XATTR_XWHITEOUTS on directories with xwhiteouts is > anyway the responsibility of the layer composer. > > Let's just require the layer composer to set OVL_XATTR_XWHITEOUTS > on the layer root even if it does not have any immediate xwhiteout > children as "layer may have xwhiteouts" indication. ok? Okay. > > @@ -1414,6 +1414,17 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc) > > if (err) > > goto out_free_oe; > > > > + for (i = 0; i < ofs->numlayer; i++) { > > + struct path path = { .mnt = layers[i].mnt }; > > + > > + if (path.mnt) { > > + path.dentry = path.mnt->mnt_root; > > + err = ovl_path_getxattr(ofs, &path, OVL_XATTR_FEATURE_XWHITEOUT, NULL, 0); > > + if (err >= 0) > > + layers[i].feature_xwhiteout = true; > > > Any reason not to do this in ovl_get_layers() when adding the layer? Well, ovl_get_layers() is called form ovl_get_lowerstack() implying that it's part of the lower layer setup. Otherwise I don't see why it could not be in ovl_get_layers(). Maybe some renaming can help. Thanks, Miklos
On Thu, 2024-01-18 at 11:41 +0100, Miklos Szeredi wrote: > Add a check on each 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. > > Fixes: bc8df7a3dc03 ("ovl: Add an alternative type of whiteout") > Cc: <stable@vger.kernel.org> # v6.7 > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > > Hi Alex, > > Can you please test this in your environment? Interesting, I was just finishing a patch for this, and it is mostly identical. It works in my testing. Reviewed-by: Alexander Larsson <alexl@redhat.com> > > I xwhiteout test in xfstests needs this tweak: > > --- a/tests/overlay/084 > +++ b/tests/overlay/084 > @@ -115,6 +115,7 @@ do_test_xwhiteout() > > mkdir -p $basedir/lower $basedir/upper $basedir/work > touch $basedir/lower/regular $basedir/lower/hidden > $basedir/upper/hidden > + setfattr -n $prefix.overlay.feature_xwhiteout -v "y" > $basedir/upper > setfattr -n $prefix.overlay.whiteouts -v "y" $basedir/upper > setfattr -n $prefix.overlay.whiteout -v "y" > $basedir/upper/hidden > Also, I will need to update mkcomposefs to set this xattr and bump the image version. > > Thanks, > Miklos > > fs/overlayfs/namei.c | 10 +++++++--- > fs/overlayfs/overlayfs.h | 8 ++++++-- > fs/overlayfs/ovl_entry.h | 2 ++ > fs/overlayfs/readdir.c | 11 ++++++++--- > fs/overlayfs/super.c | 13 ++++++++++++- > fs/overlayfs/util.c | 9 ++++++++- > 6 files changed, 43 insertions(+), 10 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..991eb5d5c66c 100644 > --- a/fs/overlayfs/overlayfs.h > +++ b/fs/overlayfs/overlayfs.h > @@ -51,6 +51,7 @@ enum ovl_xattr { > OVL_XATTR_PROTATTR, > OVL_XATTR_XWHITEOUT, > OVL_XATTR_XWHITEOUTS, > + OVL_XATTR_FEATURE_XWHITEOUT, > }; > > enum ovl_inode_flag { > @@ -492,7 +493,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 +677,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..51729e614f5a 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 feature_xwhiteout; > }; > > 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; > I'm not sure exactly how impure dirs work, but I don't think this is needed. When we hit the read_impure() codepath, we never then actually look at the p->is_whiteout information. > err = ovl_dir_read(&realpath, &rdd); > if (err) > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index a0967bb25003..4e507ab780f3 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -1291,7 +1291,7 @@ int ovl_fill_super(struct super_block *sb, > struct fs_context *fc) > struct ovl_entry *oe; > struct ovl_layer *layers; > struct cred *cred; > - int err; > + int err, i; > > err = -EIO; > if (WARN_ON(fc->user_ns != current_user_ns())) > @@ -1414,6 +1414,17 @@ int ovl_fill_super(struct super_block *sb, > struct fs_context *fc) > if (err) > goto out_free_oe; > > + for (i = 0; i < ofs->numlayer; i++) { > + struct path path = { .mnt = layers[i].mnt }; > + > + if (path.mnt) { > + path.dentry = path.mnt->mnt_root; > + err = ovl_path_getxattr(ofs, &path, > OVL_XATTR_FEATURE_XWHITEOUT, NULL, 0); > + if (err >= 0) > + layers[i].feature_xwhiteout = true; > + } > + } > + > /* Show index=off in /proc/mounts for forced r/o mount */ > if (!ofs->indexdir) { > ofs->config.index = false; > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c > index c3f020ca13a8..cae8219618e3 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->feature_xwhiteout) > + return false; > + > /* xattr.whiteouts must be a directory */ > if (!d_is_dir(dentry)) > return false; > @@ -838,6 +843,7 @@ bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, > const struct path *path, > #define OVL_XATTR_PROTATTR_POSTFIX "protattr" > #define OVL_XATTR_XWHITEOUT_POSTFIX "whiteout" > #define OVL_XATTR_XWHITEOUTS_POSTFIX "whiteouts" > +#define OVL_XATTR_FEATURE_XWHITEOUT_POSTFIX "feature_xwhiteout" > > #define OVL_XATTR_TAB_ENTRY(x) \ > [x] = { [false] = OVL_XATTR_TRUSTED_PREFIX x ## _POSTFIX, \ > @@ -855,6 +861,7 @@ const char *const ovl_xattr_table[][2] = { > OVL_XATTR_TAB_ENTRY(OVL_XATTR_PROTATTR), > OVL_XATTR_TAB_ENTRY(OVL_XATTR_XWHITEOUT), > OVL_XATTR_TAB_ENTRY(OVL_XATTR_XWHITEOUTS), > + OVL_XATTR_TAB_ENTRY(OVL_XATTR_FEATURE_XWHITEOUT), > }; > > int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry > *upperdentry,
Resending with plan text. On Thu, 2024-01-18 at 12:39 +0100, Miklos Szeredi wrote: > On Thu, 18 Jan 2024 at 12:22, Amir Goldstein <amir73il@gmail.com> > wrote: > > > > On Thu, Jan 18, 2024 at 12:41 PM Miklos Szeredi > > <mszeredi@redhat.com> wrote: > > > > > > Add a check on each 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. > > > > Does it really have a significant cost or do you just not like the > > unneeded check? > > It's probably insignificant. But I don't know and it would be hard > to prove. > > > IIRC, we anyway check for ORIGIN xattr and IMPURE xattr on > > readdir. > > We check those on lookup, not at readdir. Might make sense to check > XWHITEOUTS at lookup regardless of this patch, just for consistency. > > > > --- a/fs/overlayfs/overlayfs.h > > > +++ b/fs/overlayfs/overlayfs.h > > > @@ -51,6 +51,7 @@ enum ovl_xattr { > > > OVL_XATTR_PROTATTR, > > > OVL_XATTR_XWHITEOUT, > > > OVL_XATTR_XWHITEOUTS, > > > + OVL_XATTR_FEATURE_XWHITEOUT, > > > > Can we not add a new OVL_XATTR_FEATURE_XWHITEOUT xattr. > > > > Setting OVL_XATTR_XWHITEOUTS on directories with xwhiteouts is > > anyway the responsibility of the layer composer. > > > > Let's just require the layer composer to set OVL_XATTR_XWHITEOUTS > > on the layer root even if it does not have any immediate xwhiteout > > children as "layer may have xwhiteouts" indication. ok? > > Okay. > > This will cause readdir() on the root dir to always look for whiteouts even though there are none, but that is probably fine. It does mean we don't have to change xfstests, but I still have to change mkcomposefs. > > > @@ -1414,6 +1414,17 @@ int ovl_fill_super(struct super_block *sb, > > > struct fs_context *fc) > > > if (err) > > > goto out_free_oe; > > > > > > + for (i = 0; i < ofs->numlayer; i++) { > > > + struct path path = { .mnt = layers[i].mnt }; > > > + > > > + if (path.mnt) { > > > + path.dentry = path.mnt->mnt_root; > > > + err = ovl_path_getxattr(ofs, &path, > > > OVL_XATTR_FEATURE_XWHITEOUT, NULL, 0); > > > + if (err >= 0) > > > + layers[i].feature_xwhiteout = > > > true; > > > > > > Any reason not to do this in ovl_get_layers() when adding the > > layer? > > Well, ovl_get_layers() is called form ovl_get_lowerstack() implying > that it's part of the lower layer setup. > > Otherwise I don't see why it could not be in ovl_get_layers(). > Maybe > some renaming can help. > In the version I was preparing (https://github.com/alexlarsson/linux/tree/ovl-xattr-whiteouts-feature) it does the setup in ovl_get_layers(). The one difference this makes is that it doesn't apply feature_xwhiteout on the upperdir layer. I don't think we want to do xwhiteouts on the upperdir, but if we do it needs to be initialized in ovl_get_upper() too.
On Thu, Jan 18, 2024 at 2:08 PM Alexander Larsson <alexl@redhat.com> wrote: > > Resending with plan text. > > On Thu, 2024-01-18 at 12:39 +0100, Miklos Szeredi wrote: > > On Thu, 18 Jan 2024 at 12:22, Amir Goldstein <amir73il@gmail.com> > > wrote: > > > > > > On Thu, Jan 18, 2024 at 12:41 PM Miklos Szeredi > > > <mszeredi@redhat.com> wrote: > > > > > > > > Add a check on each 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. > > > > > > Does it really have a significant cost or do you just not like the > > > unneeded check? > > > > It's probably insignificant. But I don't know and it would be hard > > to prove. > > > > > IIRC, we anyway check for ORIGIN xattr and IMPURE xattr on > > > readdir. > > > > We check those on lookup, not at readdir. Might make sense to check > > XWHITEOUTS at lookup regardless of this patch, just for consistency. > > > > > > --- a/fs/overlayfs/overlayfs.h > > > > +++ b/fs/overlayfs/overlayfs.h > > > > @@ -51,6 +51,7 @@ enum ovl_xattr { > > > > OVL_XATTR_PROTATTR, > > > > OVL_XATTR_XWHITEOUT, > > > > OVL_XATTR_XWHITEOUTS, > > > > + OVL_XATTR_FEATURE_XWHITEOUT, > > > > > > Can we not add a new OVL_XATTR_FEATURE_XWHITEOUT xattr. > > > > > > Setting OVL_XATTR_XWHITEOUTS on directories with xwhiteouts is > > > anyway the responsibility of the layer composer. > > > > > > Let's just require the layer composer to set OVL_XATTR_XWHITEOUTS > > > on the layer root even if it does not have any immediate xwhiteout > > > children as "layer may have xwhiteouts" indication. ok? > > > > Okay. > > > > > This will cause readdir() on the root dir to always look for whiteouts > even though there are none, but that is probably fine. > > It does mean we don't have to change xfstests, but I still have to > change mkcomposefs. > > > > > > @@ -1414,6 +1414,17 @@ int ovl_fill_super(struct super_block *sb, > > > > struct fs_context *fc) > > > > if (err) > > > > goto out_free_oe; > > > > > > > > + for (i = 0; i < ofs->numlayer; i++) { > > > > + struct path path = { .mnt = layers[i].mnt }; > > > > + > > > > + if (path.mnt) { > > > > + path.dentry = path.mnt->mnt_root; > > > > + err = ovl_path_getxattr(ofs, &path, > > > > OVL_XATTR_FEATURE_XWHITEOUT, NULL, 0); > > > > + if (err >= 0) > > > > + layers[i].feature_xwhiteout = > > > > true; > > > > > > > > > Any reason not to do this in ovl_get_layers() when adding the > > > layer? > > > > Well, ovl_get_layers() is called form ovl_get_lowerstack() implying > > that it's part of the lower layer setup. > > > > Otherwise I don't see why it could not be in ovl_get_layers(). > > Maybe > > some renaming can help. > > > > In the version I was preparing > (https://github.com/alexlarsson/linux/tree/ovl-xattr-whiteouts-feature) > it does the setup in ovl_get_layers(). The one difference this makes is > that it doesn't apply feature_xwhiteout on the upperdir layer. I don't > think we want to do xwhiteouts on the upperdir, but if we do it needs > to be initialized in ovl_get_upper() too. > If there is no use case for xwhiteouts support in upper then maybe we should not support it? Anyway, I am fine with Miklos' version or with checking xwhiteouts in ovl_get_upper() and ovl_get_layers() or with not supporting xwhiteouts on upper. Thanks, Amir.
--- a/tests/overlay/084 +++ b/tests/overlay/084 @@ -115,6 +115,7 @@ do_test_xwhiteout() mkdir -p $basedir/lower $basedir/upper $basedir/work touch $basedir/lower/regular $basedir/lower/hidden $basedir/upper/hidden + setfattr -n $prefix.overlay.feature_xwhiteout -v "y" $basedir/upper setfattr -n $prefix.overlay.whiteouts -v "y" $basedir/upper setfattr -n $prefix.overlay.whiteout -v "y" $basedir/upper/hidden Thanks, Miklos fs/overlayfs/namei.c | 10 +++++++--- fs/overlayfs/overlayfs.h | 8 ++++++-- fs/overlayfs/ovl_entry.h | 2 ++ fs/overlayfs/readdir.c | 11 ++++++++--- fs/overlayfs/super.c | 13 ++++++++++++- fs/overlayfs/util.c | 9 ++++++++- 6 files changed, 43 insertions(+), 10 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..991eb5d5c66c 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -51,6 +51,7 @@ enum ovl_xattr { OVL_XATTR_PROTATTR, OVL_XATTR_XWHITEOUT, OVL_XATTR_XWHITEOUTS, + OVL_XATTR_FEATURE_XWHITEOUT, }; enum ovl_inode_flag { @@ -492,7 +493,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 +677,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..51729e614f5a 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 feature_xwhiteout; }; 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..4e507ab780f3 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -1291,7 +1291,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc) struct ovl_entry *oe; struct ovl_layer *layers; struct cred *cred; - int err; + int err, i; err = -EIO; if (WARN_ON(fc->user_ns != current_user_ns())) @@ -1414,6 +1414,17 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc) if (err) goto out_free_oe; + for (i = 0; i < ofs->numlayer; i++) { + struct path path = { .mnt = layers[i].mnt }; + + if (path.mnt) { + path.dentry = path.mnt->mnt_root; + err = ovl_path_getxattr(ofs, &path, OVL_XATTR_FEATURE_XWHITEOUT, NULL, 0); + if (err >= 0) + layers[i].feature_xwhiteout = true; + } + } + /* Show index=off in /proc/mounts for forced r/o mount */ if (!ofs->indexdir) { ofs->config.index = false; diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index c3f020ca13a8..cae8219618e3 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->feature_xwhiteout) + return false; + /* xattr.whiteouts must be a directory */ if (!d_is_dir(dentry)) return false; @@ -838,6 +843,7 @@ bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, const struct path *path, #define OVL_XATTR_PROTATTR_POSTFIX "protattr" #define OVL_XATTR_XWHITEOUT_POSTFIX "whiteout" #define OVL_XATTR_XWHITEOUTS_POSTFIX "whiteouts" +#define OVL_XATTR_FEATURE_XWHITEOUT_POSTFIX "feature_xwhiteout" #define OVL_XATTR_TAB_ENTRY(x) \ [x] = { [false] = OVL_XATTR_TRUSTED_PREFIX x ## _POSTFIX, \ @@ -855,6 +861,7 @@ const char *const ovl_xattr_table[][2] = { OVL_XATTR_TAB_ENTRY(OVL_XATTR_PROTATTR), OVL_XATTR_TAB_ENTRY(OVL_XATTR_XWHITEOUT), OVL_XATTR_TAB_ENTRY(OVL_XATTR_XWHITEOUTS), + OVL_XATTR_TAB_ENTRY(OVL_XATTR_FEATURE_XWHITEOUT), }; int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry,
Add a check on each 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. Fixes: bc8df7a3dc03 ("ovl: Add an alternative type of whiteout") Cc: <stable@vger.kernel.org> # v6.7 Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- Hi Alex, Can you please test this in your environment? I xwhiteout test in xfstests needs this tweak: