On Wed, Aug 11, 2021 at 10:11:56AM -0700, Dan Williams wrote:
> On Wed, Aug 11, 2021 at 9:59 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Wed, Aug 11, 2021 at 11:05:55AM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 09, 2021 at 03:28:40PM -0700, Dan Williams wrote:
> > > > In preparation for CXL labels that move the uuid to a different offset
> > > > in the label, add nsl_{ref,get,validate}_uuid(). These helpers use the
> > > > proper uuid_t type. That type definition predated the libnvdimm
> > > > subsystem, so now is as a good a time as any to convert all the uuid
> > > > handling in the subsystem to uuid_t to match the helpers.
> > > >
> > > > As for the whitespace changes, all new code is clang-format compliant.
> > >
> > > Thanks, looks good to me!
> > > Reviewed-by: Andy Shevchenko <[email protected]>
> >
> > Sorry, I'm in doubt this Rb stays. See below.
> >
> > ...
> >
> > > > struct btt_sb {
> > > > u8 signature[BTT_SIG_LEN];
> > > > - u8 uuid[16];
> > > > - u8 parent_uuid[16];
> > > > + uuid_t uuid;
> > > > + uuid_t parent_uuid;
> >
> > uuid_t type is internal to the kernel. This seems to be an ABI?
>
> No, it's not a user ABI, this is an on-disk metadata structure. uuid_t
> is approprirate.
So, changing size of the structure is forbidden after this change, right?
I don't like this. It means we always stuck with this type to be like this and
no change will be allowed.
> > > > __le32 flags;
> > > > __le16 version_major;
> > > > __le16 version_minor;
> >
> > ...
> >
> > > > struct nd_namespace_label {
> > > > - u8 uuid[NSLABEL_UUID_LEN];
> > > > + uuid_t uuid;
> >
> > So seems this.
> >
> > > > u8 name[NSLABEL_NAME_LEN];
> > > > __le32 flags;
> > > > __le16 nlabel;
> >
> > ...
> >
> > I'm not familiar with FS stuff, but looks to me like unwanted changes.
> > In such cases you have to use export/import APIs. otherwise you make the
> > type
> > carved in stone without even knowing that it's part of an ABI or some
> > hardware
> > / firmware interfaces.
>
> Can you clarify the concern? Carving the intent that these 16-bytes
> are meant to be treated as UUID in stone is deliberate.
It's a bit surprise to me. Do we have any documentation on that?
How do we handle such types in kernel that covers a lot of code?
--
With Best Regards,
Andy Shevchenko
