Messages in this thread | | | From | Amir Goldstein <> | Date | Wed, 7 Feb 2024 08:19:22 +0200 | Subject | Re: [PATCH v2 2/7] overlayfs: Convert to super_set_uuid() |
| |
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, uuidb, > > 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.
| |