Message ID | 1493756363-5113-1-git-send-email-amir73il@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, May 2, 2017 at 1:19 PM, Amir Goldstein <amir73il@gmail.com> wrote: > We need a helper for VFS to check if struct super_block field s_uuid > was filled by the filesystem, before consumers can use it. > > The libnvdimm code already has a private helper to check if uuid is null > and the helper name is not using a private namespace prefix, which > prevents us from using the same helper name as a common function. > > Hoist the libnvdimm helper as an inline helper to linux/uuid.h. > > Suggested-by: Christoph Hellwig <hch@lst.de> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: David Howells <dhowells@redhat.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Acked-by: Dan Williams <dan.j.williams@intel.com>
On Tue, 2017-05-02 at 23:19 +0300, Amir Goldstein wrote: > We need a helper for VFS to check if struct super_block field s_uuid > was filled by the filesystem, before consumers can use it. > > The libnvdimm code already has a private helper to check if uuid is > null > and the helper name is not using a private namespace prefix, which > prevents us from using the same helper name as a common function. > Hmm... Have you checked my branch here: https://bitbucket.org/andy-shev/linux/branch/topic%2Fuuid ? Probably not. I have number of patches to make UUID API used kernel wide. ...including helpers for null UUID check: https://bitbucket.org/andy-shev/linux/commits/79029ebe2c32830f82effc0f0b 62cce2b6eb7fdb?at=topic/uuid > Hoist the libnvdimm helper as an inline helper to linux/uuid.h. > > Suggested-by: Christoph Hellwig <hch@lst.de> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: David Howells <dhowells@redhat.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > drivers/nvdimm/btt_devs.c | 8 +------- > include/linux/uuid.h | 7 +++++++ > 2 files changed, 8 insertions(+), 7 deletions(-) > > Christoph, > > Following your suggestion to provide a helper for checking if > filesystem > had filled sb->s_uuid, here is a patch to add that helper. > > I considered hoisting xfs's uuid_is_nil() helper and uuid_t to uuid.h, > but > that would have been more painful, so just moved this simple helper > instead. > We can continue debating which implementation is better, but that > would be > futile... > > If in the future xfs uuid table code is going to be moved to VFS, we > should > probably move variants of the xfs/uuid.c functions to lib/uuid.c if > those > variants don't already exist. > > CC'ing the maintainers of clearcache and EVM/IMA to see if this helper > should be added (as sanity?) to their code before accessing sb- > >s_uuid. > > I truely hope that EVM/IMA signatures do not expect xfs/ubifs to keep > exporting null s_uuid, because if they do, then starting to export > s_uuid > for xfs/ubifs may require a new config/mkfs/mount option and that > would > be a shame. > > Cheers, > Amir. > > diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c > index 97dd292..d0fcda2 100644 > --- a/drivers/nvdimm/btt_devs.c > +++ b/drivers/nvdimm/btt_devs.c > @@ -17,6 +17,7 @@ > #include <linux/slab.h> > #include <linux/fs.h> > #include <linux/mm.h> > +#include <linux/uuid.h> > #include "nd-core.h" > #include "btt.h" > #include "nd.h" > @@ -222,13 +223,6 @@ struct device *nd_btt_create(struct nd_region > *nd_region) > return dev; > } > > -static bool uuid_is_null(u8 *uuid) > -{ > - static const u8 null_uuid[16]; > - > - return (memcmp(uuid, null_uuid, 16) == 0); > -} > - > /** > * nd_btt_arena_is_valid - check if the metadata layout is valid > * @nd_btt: device with BTT geometry and backing device info > diff --git a/include/linux/uuid.h b/include/linux/uuid.h > index 4dff73a..d3f8656 100644 > --- a/include/linux/uuid.h > +++ b/include/linux/uuid.h > @@ -58,6 +58,13 @@ static inline int uuid_be_cmp(const uuid_be u1, > const uuid_be u2) > return memcmp(&u1, &u2, sizeof(uuid_be)); > } > > +static inline bool uuid_is_null(u8 *uuid) > +{ > + static const u8 null_uuid[16]; > + > + return (memcmp(uuid, null_uuid, 16) == 0); > +} > + > void generate_random_uuid(unsigned char uuid[16]); > > extern void uuid_le_gen(uuid_le *u);
On Wed, May 3, 2017 at 12:13 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, 2017-05-02 at 23:19 +0300, Amir Goldstein wrote: >> We need a helper for VFS to check if struct super_block field s_uuid >> was filled by the filesystem, before consumers can use it. >> >> The libnvdimm code already has a private helper to check if uuid is >> null >> and the helper name is not using a private namespace prefix, which >> prevents us from using the same helper name as a common function. >> > > Hmm... > Have you checked my branch here: > https://bitbucket.org/andy-shev/linux/branch/topic%2Fuuid > ? > Hi Andy, No I have not seen your patches. When do you plan to post them? > Probably not. > I have number of patches to make UUID API used kernel wide. > That looks nice, but I see you did not get to converting many other users that represent uuid as u8[16], like libnvdimm, bluetooth, various filesystems and users of sb->s_uuid (IMA). > ...including helpers for null UUID check: > https://bitbucket.org/andy-shev/linux/commits/79029ebe2c32830f82effc0f0b > 62cce2b6eb7fdb?at=topic/uuid > I wouldn't mind using is_null_uuid_le(sb->s_uuid) myself and that is how I implemented the test in my current patch set, but I can hear the voices of those saying that there should be a 'natural' helper uuid_is_null(u8 *) for the users that represent uuid as u8[16] (i.e. filesystems and sb->s_uuid). Also, I suggest naming your helpers uuid_{le|be}_is_null() to maintain the uuid_* naming convention with other helpers you added to uuid.h. Cheers, Amir.
On Wed, 2017-05-03 at 13:05 +0300, Amir Goldstein wrote: > On Wed, May 3, 2017 at 12:13 PM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, 2017-05-02 at 23:19 +0300, Amir Goldstein wrote: > > > We need a helper for VFS to check if struct super_block field > > > s_uuid > > > was filled by the filesystem, before consumers can use it. > > > > > > The libnvdimm code already has a private helper to check if uuid > > > is > > > null > > > and the helper name is not using a private namespace prefix, which > > > prevents us from using the same helper name as a common function. > > > > > > > Hmm... > > Have you checked my branch here: > > https://bitbucket.org/andy-shev/linux/branch/topic%2Fuuid > > ? > > > > Hi Andy, > > No I have not seen your patches. When do you plan to post them? Some of them had been sent in different series recently and ~year ago. Some need to be polished and tested first. So, somewhere in the future for sure. > > Probably not. > > I have number of patches to make UUID API used kernel wide. > > > > That looks nice, but I see you did not get to converting many other > users that represent uuid as u8[16], like libnvdimm, bluetooth, > various filesystems and users of sb->s_uuid (IMA). Yes, my main goal is to make ACPI glue layer to use UUID API. I didn't touch and I won't touch filesystems since it's a huge area which I have no knowledge of. For the rest I might be interested to do, but have no time. > > > ...including helpers for null UUID check: > > https://bitbucket.org/andy-shev/linux/commits/79029ebe2c32830f82effc > > 0f0b > > 62cce2b6eb7fdb?at=topic/uuid > > > > I wouldn't mind using is_null_uuid_le(sb->s_uuid) myself and that is > how > I implemented the test in my current patch set, but I can hear the > voices of > those saying that there should be a 'natural' helper uuid_is_null(u8 > *) for the > users that represent uuid as u8[16] (i.e. filesystems and sb->s_uuid). u8 * doesn't represent UUID as a type. Perhaps we need to reflect this in the name of the function somehow. > Also, I suggest naming your helpers uuid_{le|be}_is_null() to maintain > the uuid_* naming convention with other helpers you added to uuid.h. I'm open to a name that satisfies most of us. So, no objections here.
On Wed, May 3, 2017 at 2:00 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, 2017-05-03 at 13:05 +0300, Amir Goldstein wrote: >> On Wed, May 3, 2017 at 12:13 PM, Andy Shevchenko [...] >> I can hear the voices of >> those saying that there should be a 'natural' helper uuid_is_null(u8 >> *) for the >> users that represent uuid as u8[16] (i.e. filesystems and sb->s_uuid). > > u8 * doesn't represent UUID as a type. > Perhaps we need to reflect this in the name of the function somehow. > OK, since you have no time nor the intention to convert filesystem code to use uuid_le (not should you), I see 2 options for filesystems/VFS: 1. Use the simple u8* libnvdimm helper proposed by this patch 2. Hoist uuid_t + helpers from fs/xfs/uuid.* to linux/uuid.h lib/uuid.c If option #2 is preferred, I think we should conform to libuuid's helper name uuid_is_null() (instead of freebsd's uuid_is_nil()) and maybe convert libnvdimm and bluetooth to use the xfs helpers as well. Christoph, Since you got me started on this helper, do you have a preference? Amir.
On Wed, 2017-05-03 at 15:42 +0300, Amir Goldstein wrote: > On Wed, May 3, 2017 at 2:00 PM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, 2017-05-03 at 13:05 +0300, Amir Goldstein wrote: > > > On Wed, May 3, 2017 at 12:13 PM, Andy Shevchenko > I can hear the voices of > > > those saying that there should be a 'natural' helper > > > uuid_is_null(u8 > > > *) for the > > > users that represent uuid as u8[16] (i.e. filesystems and sb- > > > >s_uuid). > > > > u8 * doesn't represent UUID as a type. > > Perhaps we need to reflect this in the name of the function somehow. > > > > OK, since you have no time nor the intention to convert filesystem > code > to use uuid_le uuid_be I suppose. I noticed core developers doesn't support endianess feature of UUID: https://lkml.org/lkml/2016/5/25/724 That's also why I don't want to touch filesystem stuff. > (not should you), I see 2 options for filesystems/VFS: > > 1. Use the simple u8* libnvdimm helper proposed by this patch > 2. Hoist uuid_t + helpers from fs/xfs/uuid.* to linux/uuid.h > lib/uuid.c > > If option #2 is preferred, I think we should conform to libuuid's > helper > name uuid_is_null() (instead of freebsd's uuid_is_nil()) I think option #2 is preferred and we need actually to submit some generic uuid_{be|le}_cmp() helpers. For now it's blocked by Gcc bug. We may remove const from the prototypes for now and add them later when the bug will be fixed. > and maybe > convert libnvdimm and bluetooth to use the xfs helpers as well. > > Christoph, > > Since you got me started on this helper, do you have a preference?
On Wed, 2017-05-03 at 19:35 +0300, Andy Shevchenko wrote: > On Wed, 2017-05-03 at 15:42 +0300, Amir Goldstein wrote: > > On Wed, May 3, 2017 at 2:00 PM, Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Wed, 2017-05-03 at 13:05 +0300, Amir Goldstein wrote: > > > > On Wed, May 3, 2017 at 12:13 PM, Andy Shevchenko > > I can hear the voices of > > > > those saying that there should be a 'natural' helper > > > > uuid_is_null(u8 > > > > *) for the > > > > users that represent uuid as u8[16] (i.e. filesystems and sb- > > > > > s_uuid). > > > > > > u8 * doesn't represent UUID as a type. > > > Perhaps we need to reflect this in the name of the function > > > somehow. > > > > > > > OK, since you have no time nor the intention to convert filesystem > > code > > to use uuid_le > > uuid_be I suppose. I noticed core developers doesn't support endianess > feature of UUID: https://lkml.org/lkml/2016/5/25/724 > > That's also why I don't want to touch filesystem stuff. > > > (not should you), I see 2 options for filesystems/VFS: > > > > 1. Use the simple u8* libnvdimm helper proposed by this patch > > 2. Hoist uuid_t + helpers from fs/xfs/uuid.* to linux/uuid.h > > lib/uuid.c > > > > If option #2 is preferred, I think we should conform to libuuid's > > helper > > name uuid_is_null() (instead of freebsd's uuid_is_nil()) I renamed as proposed and updated my branch. By the way, notice that uuid_v1 type (which had been moved to uuid.h already) is the same what xfs uses as uuid_t. So, helpers against it we can prefix with uuid_v1_[foo_bar()].
On Wed, May 3, 2017 at 8:15 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > By the way, notice that uuid_v1 type (which had been moved to uuid.h > already) is the same what xfs uses as uuid_t. > > So, helpers against it we can prefix with uuid_v1_[foo_bar()]. > I really don't see why we can't use the exact same helpers found in fs/xfs/uuid.c and uuid_t as well for that matter. They are mostly similar to the libuuid API that filesystems tools use in userspace and they don't collide with your uuid_{le|be} helpers.
On Wed, 2017-05-03 at 21:34 +0300, Amir Goldstein wrote: > On Wed, May 3, 2017 at 8:15 PM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > > By the way, notice that uuid_v1 type (which had been moved to uuid.h > > already) is the same what xfs uses as uuid_t. > > > > So, helpers against it we can prefix with uuid_v1_[foo_bar()]. > > > > I really don't see why we can't use the exact same helpers > found in fs/xfs/uuid.c and uuid_t as well for that matter. Does it make sense to have two types that defines the same? > They are mostly similar to the libuuid API that filesystems > tools use in userspace and they don't collide with your > uuid_{le|be} helpers. Okay, if you are insisting just move them to the header, we may simplify it later, though I think that there is no makes sense to do any effort that will be neglected in the future (so, just movement is fine, but using it elsewhere I prefer to have 1 type and const for parameters).
On Thu, May 4, 2017 at 11:21 AM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, 2017-05-03 at 21:34 +0300, Amir Goldstein wrote: >> On Wed, May 3, 2017 at 8:15 PM, Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >> >> > >> > By the way, notice that uuid_v1 type (which had been moved to uuid.h >> > already) is the same what xfs uses as uuid_t. >> > >> > So, helpers against it we can prefix with uuid_v1_[foo_bar()]. >> > >> >> I really don't see why we can't use the exact same helpers >> found in fs/xfs/uuid.c and uuid_t as well for that matter. > > Does it make sense to have two types that defines the same? > >> They are mostly similar to the libuuid API that filesystems >> tools use in userspace and they don't collide with your >> uuid_{le|be} helpers. > > Okay, if you are insisting just move them to the header, we may simplify > it later, though I think that there is no makes sense to do any effort > that will be neglected in the future (so, just movement is fine, but > using it elsewhere I prefer to have 1 type and const for parameters). > Preparing to post a series with typedef struct uuid_v1 uuid_t; I think you'll like it... stay tuned.
diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c index 97dd292..d0fcda2 100644 --- a/drivers/nvdimm/btt_devs.c +++ b/drivers/nvdimm/btt_devs.c @@ -17,6 +17,7 @@ #include <linux/slab.h> #include <linux/fs.h> #include <linux/mm.h> +#include <linux/uuid.h> #include "nd-core.h" #include "btt.h" #include "nd.h" @@ -222,13 +223,6 @@ struct device *nd_btt_create(struct nd_region *nd_region) return dev; } -static bool uuid_is_null(u8 *uuid) -{ - static const u8 null_uuid[16]; - - return (memcmp(uuid, null_uuid, 16) == 0); -} - /** * nd_btt_arena_is_valid - check if the metadata layout is valid * @nd_btt: device with BTT geometry and backing device info diff --git a/include/linux/uuid.h b/include/linux/uuid.h index 4dff73a..d3f8656 100644 --- a/include/linux/uuid.h +++ b/include/linux/uuid.h @@ -58,6 +58,13 @@ static inline int uuid_be_cmp(const uuid_be u1, const uuid_be u2) return memcmp(&u1, &u2, sizeof(uuid_be)); } +static inline bool uuid_is_null(u8 *uuid) +{ + static const u8 null_uuid[16]; + + return (memcmp(uuid, null_uuid, 16) == 0); +} + void generate_random_uuid(unsigned char uuid[16]); extern void uuid_le_gen(uuid_le *u);
We need a helper for VFS to check if struct super_block field s_uuid was filled by the filesystem, before consumers can use it. The libnvdimm code already has a private helper to check if uuid is null and the helper name is not using a private namespace prefix, which prevents us from using the same helper name as a common function. Hoist the libnvdimm helper as an inline helper to linux/uuid.h. Suggested-by: Christoph Hellwig <hch@lst.de> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: David Howells <dhowells@redhat.com> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- drivers/nvdimm/btt_devs.c | 8 +------- include/linux/uuid.h | 7 +++++++ 2 files changed, 8 insertions(+), 7 deletions(-) Christoph, Following your suggestion to provide a helper for checking if filesystem had filled sb->s_uuid, here is a patch to add that helper. I considered hoisting xfs's uuid_is_nil() helper and uuid_t to uuid.h, but that would have been more painful, so just moved this simple helper instead. We can continue debating which implementation is better, but that would be futile... If in the future xfs uuid table code is going to be moved to VFS, we should probably move variants of the xfs/uuid.c functions to lib/uuid.c if those variants don't already exist. CC'ing the maintainers of clearcache and EVM/IMA to see if this helper should be added (as sanity?) to their code before accessing sb->s_uuid. I truely hope that EVM/IMA signatures do not expect xfs/ubifs to keep exporting null s_uuid, because if they do, then starting to export s_uuid for xfs/ubifs may require a new config/mkfs/mount option and that would be a shame. Cheers, Amir.