We don't want to be settingc sb->s_uuid directly anymore, as there's a
length field that also has to be set, and this conversion was not
completely trivial.
Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: linux-unionfs@vger.kernel.org
---
fs/overlayfs/util.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 0217094c23ea..f1f0ee9a9dff 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -760,13 +760,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
const struct path *upperpath)
{
bool set = false;
+ uuid_t uuid;
int res;
/* Try to load existing persistent uuid */
- res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, sb->s_uuid.b,
+ res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, uuid.b,
UUID_SIZE);
if (res == UUID_SIZE)
- return true;
+ goto success;
if (res != -ENODATA)
goto fail;
@@ -794,14 +795,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
}
/* Generate overlay instance uuid */
- uuid_gen(&sb->s_uuid);
+ uuid_gen(&uuid);
/* Try to store persistent uuid */
set = true;
- res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, sb->s_uuid.b,
+ res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, uuid.b,
UUID_SIZE);
if (res == 0)
- return true;
+ goto success;
fail:
memset(sb->s_uuid.b, 0, UUID_SIZE);
@@ -809,6 +810,9 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
pr_warn("failed to %s uuid (%pd2, err=%i); falling back to uuid=null.\n",
set ? "set" : "get", upperpath->dentry, res);
return false;
+success:
+ super_set_uuid(sb, uuid.b, sizeof(uuid));
+ return true;
}
bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, const struct path *path,
--
2.43.0On Tue, Feb 06, 2024 at 03:18:50PM -0500, Kent Overstreet wrote:
> We don't want to be settingc sb->s_uuid directly anymore, as there's a
> length field that also has to be set, and this conversion was not
> completely trivial.
>
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: linux-unionfs@vger.kernel.org
> ---
> fs/overlayfs/util.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 0217094c23ea..f1f0ee9a9dff 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -760,13 +760,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
> const struct path *upperpath)
> {
> bool set = false;
> + uuid_t uuid;
> int res;
>
> /* Try to load existing persistent uuid */
> - res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, sb->s_uuid.b,
> + res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, uuid.b,
> UUID_SIZE);
> if (res == UUID_SIZE)
> - return true;
> + goto success;
>
> if (res != -ENODATA)
> goto fail;
> @@ -794,14 +795,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
> }
>
> /* Generate overlay instance uuid */
> - uuid_gen(&sb->s_uuid);
> + uuid_gen(&uuid);
>
> /* Try to store persistent uuid */
> set = true;
> - res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, sb->s_uuid.b,
> + res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, uuid.b,
> UUID_SIZE);
> if (res == 0)
> - return true;
> + goto success;
This is a bit weird. Normally the success case is in line, and we
jump out of line for the fail case. I think this is more better:
if (res)
goto fail;
success:
super_set_uuid(sb, uuid.b, sizeof(uuid));
return true;
>
> fail:
> memset(sb->s_uuid.b, 0, UUID_SIZE);
And then the fail case follows naturally.
-Dave.
--
Dave Chinner
david@fromorbit.com
On Tue, Feb 6, 2024 at 11:49 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Tue, Feb 06, 2024 at 03:18:50PM -0500, Kent Overstreet wrote:
> > We don't want to be settingc sb->s_uuid directly anymore, as there's a
> > length field that also has to be set, and this conversion was not
> > completely trivial.
> >
> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > Cc: Miklos Szeredi <miklos@szeredi.hu>
> > Cc: Amir Goldstein <amir73il@gmail.com>
> > Cc: linux-unionfs@vger.kernel.org
> > ---
> > fs/overlayfs/util.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 0217094c23ea..f1f0ee9a9dff 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -760,13 +760,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
> > const struct path *upperpath)
> > {
> > bool set = false;
> > + uuid_t uuid;
> > int res;
> >
> > /* Try to load existing persistent uuid */
> > - res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, sb->s_uuid.b,
> > + res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, uuid.b,
> > UUID_SIZE);
> > if (res == UUID_SIZE)
> > - return true;
> > + goto success;
> >
> > if (res != -ENODATA)
> > goto fail;
> > @@ -794,14 +795,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
> > }
> >
> > /* Generate overlay instance uuid */
> > - uuid_gen(&sb->s_uuid);
> > + uuid_gen(&uuid);
> >
> > /* Try to store persistent uuid */
> > set = true;
> > - res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, sb->s_uuid.b,
> > + res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, uuid.b,
> > UUID_SIZE);
> > if (res == 0)
> > - return true;
> > + goto success;
>
> This is a bit weird. Normally the success case is in line, and we
> jump out of line for the fail case. I think this is more better:
>
> if (res)
> goto fail;
> success:
> super_set_uuid(sb, uuid.b, sizeof(uuid));
> return true;
> >
> > fail:
> > memset(sb->s_uuid.b, 0, UUID_SIZE);
>
> And then the fail case follows naturally.
>
I agree. Please use the label name
set_uuid:
Also the memset() in fail: case is not needed anymore, because
with your change we do not dirty sb->s_uuid in this function.
Thanks,
Amir.
© 2016 - 2026 Red Hat, Inc.