| Message ID | 20240206201858.952303-3-kent.overstreet@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | filesystem visibililty ioctls | expand |
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. -Dave.
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.
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,
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(-)