XFS UUID cleanups

XFS UUID cleanups

[Christoph Hellwig]

This series cleans up uuid usage in XFS a bit, inspired by the series from
Amir yesterday (from which the first patch is taken as-is).  This should
be seen as a precursor to more uuid work, so it might make sense to have
in in an immutable separate branch if we're able to get to agreement for
more kernel-wide uuid work.

[PATCH 1/3] xfs: use uuid_copy() helper to abstract uuid_t

[Christoph Hellwig]

From: Amir Goldstein <amir73il@gmail.com>

uuid_t definition is about to change.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_inode_item.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 08cb7d1a4a3a..013cc78d7daf 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -834,9 +834,7 @@ xfs_inode_item_format_convert(
 		in_f->ilf_dsize = in_f32->ilf_dsize;
 		in_f->ilf_ino = in_f32->ilf_ino;
 		/* copy biggest field of ilf_u */
-		memcpy(in_f->ilf_u.ilfu_uuid.__u_bits,
-		       in_f32->ilf_u.ilfu_uuid.__u_bits,
-		       sizeof(uuid_t));
+		uuid_copy(&in_f->ilf_u.ilfu_uuid, &in_f32->ilf_u.ilfu_uuid);
 		in_f->ilf_blkno = in_f32->ilf_blkno;
 		in_f->ilf_len = in_f32->ilf_len;
 		in_f->ilf_boffset = in_f32->ilf_boffset;
@@ -851,9 +849,7 @@ xfs_inode_item_format_convert(
 		in_f->ilf_dsize = in_f64->ilf_dsize;
 		in_f->ilf_ino = in_f64->ilf_ino;
 		/* copy biggest field of ilf_u */
-		memcpy(in_f->ilf_u.ilfu_uuid.__u_bits,
-		       in_f64->ilf_u.ilfu_uuid.__u_bits,
-		       sizeof(uuid_t));
+		uuid_copy(&in_f->ilf_u.ilfu_uuid, &in_f64->ilf_u.ilfu_uuid);
 		in_f->ilf_blkno = in_f64->ilf_blkno;
 		in_f->ilf_len = in_f64->ilf_len;
 		in_f->ilf_boffset = in_f64->ilf_boffset;
-- 
2.11.0

[PATCH 2/3] xfs: use uuid_be to implement the uuid_t type

[Christoph Hellwig]

Use the generic Linux definition to implement our UUID type, this will
allow using more generic infrastructure in the future.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/uuid.h      | 4 ----
 fs/xfs/xfs_linux.h | 3 +++
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/uuid.h b/fs/xfs/uuid.h
index 104db0f3bed6..4f1441ba4fa5 100644
--- a/fs/xfs/uuid.h
+++ b/fs/xfs/uuid.h
@@ -18,10 +18,6 @@
 #ifndef __XFS_SUPPORT_UUID_H__
 #define __XFS_SUPPORT_UUID_H__
 
-typedef struct {
-	unsigned char	__u_bits[16];
-} uuid_t;
-
 extern int uuid_is_nil(uuid_t *uuid);
 extern int uuid_equal(uuid_t *uuid1, uuid_t *uuid2);
 extern void uuid_getnodeuniq(uuid_t *uuid, int fsid [2]);
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index 044fb0e15390..89ee5ec66837 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -19,6 +19,7 @@
 #define __XFS_LINUX__
 
 #include <linux/types.h>
+#include <linux/uuid.h>
 
 /*
  * Kernel specific type declarations for XFS
@@ -38,6 +39,8 @@ typedef __s64			xfs_daddr_t;	/* <disk address> type */
 typedef __u32			xfs_dev_t;
 typedef __u32			xfs_nlink_t;
 
+typedef uuid_be			uuid_t;
+
 #include "xfs_types.h"
 
 #include "kmem.h"
-- 
2.11.0

Re: [PATCH 2/3] xfs: use uuid_be to implement the uuid_t type

[Amir Goldstein]

On Fri, May 5, 2017 at 10:57 AM, Christoph Hellwig <hch@lst.de> wrote:
> Use the generic Linux definition to implement our UUID type, this will
> allow using more generic infrastructure in the future.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>


> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 044fb0e15390..89ee5ec66837 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -19,6 +19,7 @@
>  #define __XFS_LINUX__
>
>  #include <linux/types.h>
> +#include <linux/uuid.h>
>
>  /*
>   * Kernel specific type declarations for XFS
> @@ -38,6 +39,8 @@ typedef __s64                 xfs_daddr_t;    /* <disk address> type */
>  typedef __u32                  xfs_dev_t;
>  typedef __u32                  xfs_nlink_t;
>
> +typedef uuid_be                        uuid_t;
> +

How about moving this typedef to linux/uuid.h.
It's weird to have a non xfs_ prefixed typedef as it was
and placing it here is even more weird.

Yes, only xfs uses uuid_t right now, but this could mark the intentions
in a central place, so other code can follow suit (i.e. libnvdimm) and
start using uuid_t as well.

I realize this series is intended to be xfs internal cleanup,
so could be part of a follow up patch set.

If this is acceptable by Andy, I can re-post my series based on top
of this one to hoist uuid_t and the rest of the xfs helpers to linux/uuid.h.

Amir.

Re: [PATCH 2/3] xfs: use uuid_be to implement the uuid_t type

[Christoph Hellwig]

On Fri, May 05, 2017 at 11:44:16AM +0300, Amir Goldstein wrote:
> How about moving this typedef to linux/uuid.h.

Yes, I think eventually we want that, but for now I tried to keep
it local to XFS.  uuid.h already is a mess with uuid_be/uuid_le and
struct uuid_v1.

I think I'll need to do a series on that first, but this might
run into conflicts with the work that Andy is doing at the moment.

> It's weird to have a non xfs_ prefixed typedef as it was
> and placing it here is even more weird.

We do have a few typedefs like that, but maybe we should eventually
clean them up.

> Yes, only xfs uses uuid_t right now, but this could mark the intentions
> in a central place, so other code can follow suit (i.e. libnvdimm) and
> start using uuid_t as well.

I think libnvdimm would be guid_t.

> If this is acceptable by Andy, I can re-post my series based on top
> of this one to hoist uuid_t and the rest of the xfs helpers to linux/uuid.h.

Sure.  There actually are very few users of uuid_be at the moment,
so it might be a good opportunity to just kill if off ASAP.

uuid_le might take a little more time if it's really worth it.

Re: [PATCH 2/3] xfs: use uuid_be to implement the uuid_t type

[Andy Shevchenko]

On Fri, 2017-05-05 at 10:48 +0200, Christoph Hellwig wrote:
> On Fri, May 05, 2017 at 11:44:16AM +0300, Amir Goldstein wrote:
> > How about moving this typedef to linux/uuid.h.
> 
> Yes, I think eventually we want that, but for now I tried to keep
> it local to XFS.  uuid.h already is a mess with uuid_be/uuid_le and
> struct uuid_v1.
> 
> I think I'll need to do a series on that first, but this might
> run into conflicts with the work that Andy is doing at the moment.

Christoph, I can wait a bit and re-do my patch if we settle down data
types and function name space.

I would really get rid of the mess with UUIDs/GUIDs we have in kernel
(but I'm not FS expert, so, I didn't touch that area at all).

> > It's weird to have a non xfs_ prefixed typedef as it was
> > and placing it here is even more weird.
> 
> We do have a few typedefs like that, but maybe we should eventually
> clean them up.
> 
> > Yes, only xfs uses uuid_t right now, but this could mark the
> > intentions
> > in a central place, so other code can follow suit (i.e. libnvdimm)
> > and
> > start using uuid_t as well.
> 
> I think libnvdimm would be guid_t.

Yes.

EFI, ACPI, this one are the users of uuid_le.
AFAIR EFI defines type as efi_guid_t.

> 
> > If this is acceptable by Andy, I can re-post my series based on top
> > of this one to hoist uuid_t and the rest of the xfs helpers to
> > linux/uuid.h.
> 
> Sure.  There actually are very few users of uuid_be at the moment,
> so it might be a good opportunity to just kill if off ASAP.

Agreed.

> uuid_le might take a little more time if it's really worth it.

We may leave the type and helpers, introduce better ones and encourage
new code to use them instead.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/3] xfs: use uuid_be to implement the uuid_t type

[Christoph Hellwig]

On Fri, May 05, 2017 at 12:42:23PM +0300, Andy Shevchenko wrote:
> Christoph, I can wait a bit and re-do my patch if we settle down data
> types and function name space.

We should probably just work on a common branch.

Re: [PATCH 2/3] xfs: use uuid_be to implement the uuid_t type

[Andy Shevchenko]

On Fri, 2017-05-05 at 11:56 +0200, Christoph Hellwig wrote:
> On Fri, May 05, 2017 at 12:42:23PM +0300, Andy Shevchenko wrote:
> > Christoph, I can wait a bit and re-do my patch if we settle down
> > data
> > types and function name space.
> 
> We should probably just work on a common branch.

Works for me. Are you volunteering to provide a such?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/3] xfs: use uuid_be to implement the uuid_t type

[Christoph Hellwig]

On Fri, May 05, 2017 at 01:06:28PM +0300, Andy Shevchenko wrote:
> On Fri, 2017-05-05 at 11:56 +0200, Christoph Hellwig wrote:
> > On Fri, May 05, 2017 at 12:42:23PM +0300, Andy Shevchenko wrote:
> > > Christoph, I can wait a bit and re-do my patch if we settle down
> > > data
> > > types and function name space.
> > 
> > We should probably just work on a common branch.
> 
> Works for me. Are you volunteering to provide a such?

Sure, I can set one up in one of my infradead trees.

Re: [PATCH 2/3] xfs: use uuid_be to implement the uuid_t type

[Amir Goldstein]

[ added CC linux-fsdevel ]

On Fri, May 5, 2017 at 1:10 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Fri, May 05, 2017 at 01:06:28PM +0300, Andy Shevchenko wrote:
>> On Fri, 2017-05-05 at 11:56 +0200, Christoph Hellwig wrote:
>> > On Fri, May 05, 2017 at 12:42:23PM +0300, Andy Shevchenko wrote:
>> > > Christoph, I can wait a bit and re-do my patch if we settle down
>> > > data
>> > > types and function name space.
>> >
>> > We should probably just work on a common branch.
>>
>> Works for me. Are you volunteering to provide a such?
>
> Sure, I can set one up in one of my infradead trees.

Christoph, Andy,

Our discussions now span so many threads and different CC lists
(ACPI uuid_le, hoist xfs uuid_t, tmpfs gen s_uuid, ubifs copy s_uuid, ...)
so I picked this relatively young and less overloaded thread.
Since converting filesystems and uuid_be seems to be first order of
business, I only added linux-fsdevel to CC. If you think that other lists
should be included at this point in the discussion, feel free to add them.
Let's try to stick to this thread going forward.

To avoid stepping over each other toes, I propose the following work sequence:

This patch set by Christoph:
- re-define xfs's  uuid_t as uuid_be (this patch)
- more xfs uuid cleanups

V3 of 'hoist xfs uuid helpers' by me:
- hoist uuid_t typedef to uuid.h
- hoist helpers uuid_equal()/uuid_copy() to uuid.h
- create helper uuid_is_null()
- convert xfs to use the new uuid_is_null()

Kill off uuid_be (I can do it):
- Create helper variants uuid_cmp()/uuid_gen()/uuid_to_bin()
- Convery uuid_be users to uuid_t and uuid_ helpers
- Kill off uuid_be and uuid_be_ helpers

Promote uuid_t to VFS and filesystems (I can do it):
- Convert sb->s_uuid to uuid_t
- Convert some sample fs (tmpfs, ext2, ext4, ..) to uuid_t
- Use uuid_copy() and uuid_gen() where due

Kill off generate_random_uuid (Christoph?):
- Not sure what you're intentions are wrt to all users that use
  this helper to initialize a char[16] var?

Maybe repeat the uuid_be->uuid_t process with uuid_le->guid_t (Andy?):
- I don't think any of the above interferes with your work to convert
ACPI to uuid_le?
- You can actually start this work in parallel to uuid_be->uuid_t, because
  it will create trivial merge conflicts that we can easily sort out
on the common
  development branch

Thoughts?

Amir.

Re: [PATCH 2/3] xfs: use uuid_be to implement the uuid_t type

[Christoph Hellwig]

On Wed, May 10, 2017 at 11:39:27AM +0300, Amir Goldstein wrote:
> To avoid stepping over each other toes, I propose the following work sequence:

I agree on the list, but I've started a tree that has a slightly different
sequence:

(1) renamed uuid_be to uuid_t and uuid_le to guid_t, but keep the old
    name for compat for now.
    Including these items from your list:
> - Create helper variants uuid_cmp()/uuid_gen()/uuid_to_bin()

(2): kill uuid_v1 by adding a union to uuid_t that has the v1 meaning
     (and maybe more in the future)

(3) > This patch set by Christoph:
> - re-define xfs's  uuid_t as uuid_be (this patch)
> - more xfs uuid cleanups


(4): > V3 of 'hoist xfs uuid helpers' by me:
> - hoist uuid_t typedef to uuid.h
> - hoist helpers uuid_equal()/uuid_copy() to uuid.h
> - create helper uuid_is_null()
> - convert xfs to use the new uuid_is_null()

(but now moved over the to plain uuid_t type)

(5) > Promote uuid_t to VFS and filesystems (I can do it):
> - Convert sb->s_uuid to uuid_t
> - Convert some sample fs (tmpfs, ext2, ext4, ..) to uuid_t
> - Use uuid_copy() and uuid_gen() where due

And the rest for latter

> Kill off uuid_be (I can do it):
> - Convery uuid_be users to uuid_t and uuid_ helpers
> - Kill off uuid_be and uuid_be_ helpers
> 
> Kill off generate_random_uuid (Christoph?):
> - Not sure what you're intentions are wrt to all users that use
>   this helper to initialize a char[16] var?

They really should be converted to either uuid_t or guid_t, or
just use prandom_bytes if they really aren't either a UUID or GUID

> Maybe repeat the uuid_be->uuid_t process with uuid_le->guid_t (Andy?):
> - I don't think any of the above interferes with your work to convert
> ACPI to uuid_le?
> - You can actually start this work in parallel to uuid_be->uuid_t, because
>   it will create trivial merge conflicts that we can easily sort out
> on the common
>   development branch

With my work I'm right now at item (5) of my list, I'll send it out
later this afternoon.

Re: [PATCH 2/3] xfs: use uuid_be to implement the uuid_t type

[Andy Shevchenko]

On Wed, 2017-05-10 at 14:01 +0200, Christoph Hellwig wrote:
> On Wed, May 10, 2017 at 11:39:27AM +0300, Amir Goldstein wrote:
> > To avoid stepping over each other toes, I propose the following work
> > sequence:
> 
> I agree on the list, but I've started a tree that has a slightly
> different
> sequence:
> 
> (1) renamed uuid_be to uuid_t and uuid_le to guid_t, but keep the old
>     name for compat for now.
>     Including these items from your list:
> > - Create helper variants uuid_cmp()/uuid_gen()/uuid_to_bin()
> 
> (2): kill uuid_v1 by adding a union to uuid_t that has the v1 meaning
>      (and maybe more in the future)
> 
> (3) > This patch set by Christoph:
> > - re-define xfs's  uuid_t as uuid_be (this patch)
> > - more xfs uuid cleanups
> 
> 
> (4): > V3 of 'hoist xfs uuid helpers' by me:
> > - hoist uuid_t typedef to uuid.h
> > - hoist helpers uuid_equal()/uuid_copy() to uuid.h
> > - create helper uuid_is_null()
> > - convert xfs to use the new uuid_is_null()
> 
> (but now moved over the to plain uuid_t type)
> 
> (5) > Promote uuid_t to VFS and filesystems (I can do it):
> > - Convert sb->s_uuid to uuid_t
> > - Convert some sample fs (tmpfs, ext2, ext4, ..) to uuid_t
> > - Use uuid_copy() and uuid_gen() where due
> 
> And the rest for latter
> 
> > Kill off uuid_be (I can do it):
> > - Convery uuid_be users to uuid_t and uuid_ helpers
> > - Kill off uuid_be and uuid_be_ helpers
> > 
> > Kill off generate_random_uuid (Christoph?):
> > - Not sure what you're intentions are wrt to all users that use
> >   this helper to initialize a char[16] var?
> 
> They really should be converted to either uuid_t or guid_t, or
> just use prandom_bytes if they really aren't either a UUID or GUID
> 
> > Maybe repeat the uuid_be->uuid_t process with uuid_le->guid_t
> > (Andy?):
> > - I don't think any of the above interferes with your work to
> > convert
> > ACPI to uuid_le?
> > - You can actually start this work in parallel to uuid_be->uuid_t,
> > because
> >   it will create trivial merge conflicts that we can easily sort out
> > on the common
> >   development branch
> 
> With my work I'm right now at item (5) of my list, I'll send it out
> later this afternoon.


I'm fine with above and it looks like I could rebase my current work
whenever (1) has been done.

Looking forward to repository with changes I can rebase on top of.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/3] xfs: use uuid_be to implement the uuid_t type

[Christoph Hellwig]

On Wed, May 10, 2017 at 03:54:55PM +0300, Andy Shevchenko wrote:
> I'm fine with above and it looks like I could rebase my current work
> whenever (1) has been done.
> 
> Looking forward to repository with changes I can rebase on top of.

Here is what I currently have:

http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/uuid-types

It hasn't survived the buildbot yet, so take it with a grain of salt.

[PATCH 3/3] xfs: remove uuid_getnodeuniq and xfs_uu_t

[Christoph Hellwig]

And just opencode retreiving the UUID v1 information in the only caller
using get_unaligned_be*.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/uuid.c      | 25 -------------------------
 fs/xfs/uuid.h      |  1 -
 fs/xfs/xfs_mount.c |  5 ++++-
 3 files changed, 4 insertions(+), 27 deletions(-)

diff --git a/fs/xfs/uuid.c b/fs/xfs/uuid.c
index b83f76b6d410..937deaf2128d 100644
--- a/fs/xfs/uuid.c
+++ b/fs/xfs/uuid.c
@@ -17,31 +17,6 @@
  */
 #include <xfs.h>
 
-/* IRIX interpretation of an uuid_t */
-typedef struct {
-	__be32	uu_timelow;
-	__be16	uu_timemid;
-	__be16	uu_timehi;
-	__be16	uu_clockseq;
-	__be16	uu_node[3];
-} xfs_uu_t;
-
-/*
- * uuid_getnodeuniq - obtain the node unique fields of a UUID.
- *
- * This is not in any way a standard or condoned UUID function;
- * it just something that's needed for user-level file handles.
- */
-void
-uuid_getnodeuniq(uuid_t *uuid, int fsid [2])
-{
-	xfs_uu_t *uup = (xfs_uu_t *)uuid;
-
-	fsid[0] = (be16_to_cpu(uup->uu_clockseq) << 16) |
-		   be16_to_cpu(uup->uu_timemid);
-	fsid[1] = be32_to_cpu(uup->uu_timelow);
-}
-
 int
 uuid_is_nil(uuid_t *uuid)
 {
diff --git a/fs/xfs/uuid.h b/fs/xfs/uuid.h
index 4f1441ba4fa5..65b5cebf2f99 100644
--- a/fs/xfs/uuid.h
+++ b/fs/xfs/uuid.h
@@ -20,7 +20,6 @@
 
 extern int uuid_is_nil(uuid_t *uuid);
 extern int uuid_equal(uuid_t *uuid1, uuid_t *uuid2);
-extern void uuid_getnodeuniq(uuid_t *uuid, int fsid [2]);
 
 static inline void
 uuid_copy(uuid_t *dst, uuid_t *src)
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 688ebff1f663..5bf1edb0986d 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -789,7 +789,10 @@ xfs_mountfs(
 	 *  Copies the low order bits of the timestamp and the randomly
 	 *  set "sequence" number out of a UUID.
 	 */
-	uuid_getnodeuniq(&sbp->sb_uuid, mp->m_fixedfsid);
+	mp->m_fixedfsid[0] =
+		(get_unaligned_be16(&sbp->sb_uuid.b[8]) << 16) |
+		 get_unaligned_be16(&sbp->sb_uuid.b[4]);
+	mp->m_fixedfsid[1] = get_unaligned_be32(&sbp->sb_uuid.b[0]);
 
 	mp->m_dmevmask = 0;	/* not persistent; set after each mount */
 
-- 
2.11.0

Re: [PATCH 3/3] xfs: remove uuid_getnodeuniq and xfs_uu_t

[Amir Goldstein]

On Fri, May 5, 2017 at 10:57 AM, Christoph Hellwig <hch@lst.de> wrote:
> And just opencode retreiving the UUID v1 information in the only caller
> using get_unaligned_be*.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Nice.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>