[PATCH] xfs: move the define for superblock magic to uapi

[PATCH] xfs: move the define for superblock magic to uapi

[Adam Borowski]

Needed by users of fstatfs().

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 fs/xfs/libxfs/xfs_format.h | 3 ++-
 include/uapi/linux/magic.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 059bc44c27e8..837863c57e8c 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -14,6 +14,7 @@
  * xfs_da_format.h, which log and log item formats are defined in
  * xfs_log_format.h. Everything else goes here.
  */
+#include <linux/magic.h>
 
 struct xfs_mount;
 struct xfs_trans;
@@ -26,7 +27,7 @@ struct xfs_ifork;
  * Fits into a sector-sized buffer at address 0 of each allocation group.
  * Only the first of these is ever updated except during growfs.
  */
-#define	XFS_SB_MAGIC		0x58465342	/* 'XFSB' */
+#define	XFS_SB_MAGIC		XFS_SUPER_MAGIC /* 0x58465342 = 'XFSB' */
 #define	XFS_SB_VERSION_1	1		/* 5.3, 6.0.1, 6.1 */
 #define	XFS_SB_VERSION_2	2		/* 6.2 - attributes */
 #define	XFS_SB_VERSION_3	3		/* 6.2 - new inode version */
diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h
index 1a6fee974116..96c24478d8ce 100644
--- a/include/uapi/linux/magic.h
+++ b/include/uapi/linux/magic.h
@@ -29,6 +29,7 @@
 #define HPFS_SUPER_MAGIC	0xf995e849
 #define ISOFS_SUPER_MAGIC	0x9660
 #define JFFS2_SUPER_MAGIC	0x72b6
+#define XFS_SUPER_MAGIC		0x58465342	/* "XFSB" */
 #define PSTOREFS_MAGIC		0x6165676C
 #define EFIVARFS_MAGIC		0xde5e81e4
 #define HOSTFS_SUPER_MAGIC	0x00c0ffee
-- 
2.19.0

Re: [PATCH] xfs: move the define for superblock magic to uapi

[Darrick J. Wong]

On Fri, Oct 05, 2018 at 02:27:41PM +0200, Adam Borowski wrote:
> Needed by users of fstatfs().

NAK.

The XFS superblock magic number is part of the on-disk format definition
of XFS. It belongs with all the other on-disk format definitions in this
file.

I don't think it's a good idea for userspace to associate s_magic with
userspace API feature sets, though the cat's long escaped the bag on
that one.

With that being reality, "the XFS superblock magic number" has a
different semantic meaning than "the agreed upon statfs.f_type value for
XFS", which means the latter should have a different symbol name to
reflect that difference.

--D

> 
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>

> ---
>  fs/xfs/libxfs/xfs_format.h | 3 ++-
>  include/uapi/linux/magic.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 059bc44c27e8..837863c57e8c 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -14,6 +14,7 @@
>   * xfs_da_format.h, which log and log item formats are defined in
>   * xfs_log_format.h. Everything else goes here.
>   */
> +#include <linux/magic.h>
>  
>  struct xfs_mount;
>  struct xfs_trans;
> @@ -26,7 +27,7 @@ struct xfs_ifork;
>   * Fits into a sector-sized buffer at address 0 of each allocation group.
>   * Only the first of these is ever updated except during growfs.
>   */
> -#define	XFS_SB_MAGIC		0x58465342	/* 'XFSB' */
> +#define	XFS_SB_MAGIC		XFS_SUPER_MAGIC /* 0x58465342 = 'XFSB' */
>  #define	XFS_SB_VERSION_1	1		/* 5.3, 6.0.1, 6.1 */
>  #define	XFS_SB_VERSION_2	2		/* 6.2 - attributes */
>  #define	XFS_SB_VERSION_3	3		/* 6.2 - new inode version */
> diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h
> index 1a6fee974116..96c24478d8ce 100644
> --- a/include/uapi/linux/magic.h
> +++ b/include/uapi/linux/magic.h
> @@ -29,6 +29,7 @@
>  #define HPFS_SUPER_MAGIC	0xf995e849
>  #define ISOFS_SUPER_MAGIC	0x9660
>  #define JFFS2_SUPER_MAGIC	0x72b6
> +#define XFS_SUPER_MAGIC		0x58465342	/* "XFSB" */
>  #define PSTOREFS_MAGIC		0x6165676C
>  #define EFIVARFS_MAGIC		0xde5e81e4
>  #define HOSTFS_SUPER_MAGIC	0x00c0ffee
> -- 
> 2.19.0
> 

Re: [PATCH] xfs: move the define for superblock magic to uapi

[Adam Borowski]

On Fri, Oct 05, 2018 at 09:06:00AM -0700, Darrick J. Wong wrote:
> On Fri, Oct 05, 2018 at 02:27:41PM +0200, Adam Borowski wrote:
> > Needed by users of fstatfs().
> 
> NAK.
> 
> The XFS superblock magic number is part of the on-disk format definition
> of XFS. It belongs with all the other on-disk format definitions in this
> file.
> 
> I don't think it's a good idea for userspace to associate s_magic with
> userspace API feature sets, though the cat's long escaped the bag on
> that one.
> 
> With that being reality, "the XFS superblock magic number" has a
> different semantic meaning than "the agreed upon statfs.f_type value for
> XFS", which means the latter should have a different symbol name to
> reflect that difference.

Ie, you'd be ok with the same value to be defined in two places -- do I
understand this right?

> > --- a/fs/xfs/libxfs/xfs_format.h
> > -#define	XFS_SB_MAGIC		0x58465342	/* 'XFSB' */
> > +#define	XFS_SB_MAGIC		XFS_SUPER_MAGIC /* 0x58465342 = 'XFSB' */
> > --- a/include/uapi/linux/magic.h
> >  #define JFFS2_SUPER_MAGIC	0x72b6
> > +#define XFS_SUPER_MAGIC		0x58465342	/* "XFSB" */
> >  #define PSTOREFS_MAGIC		0x6165676C


Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀ 
⣾⠁⢰⠒⠀⣿⡁ 10 people enter a bar: 1 who understands binary,
⢿⡄⠘⠷⠚⠋⠀ 1 who doesn't, D who prefer to write it as hex,
⠈⠳⣄⠀⠀⠀⠀ and 1 who narrowly avoided an off-by-one error.

Re: [PATCH] xfs: move the define for superblock magic to uapi

[Darrick J. Wong]

On Fri, Oct 05, 2018 at 08:10:32PM +0200, Adam Borowski wrote:
> On Fri, Oct 05, 2018 at 09:06:00AM -0700, Darrick J. Wong wrote:
> > On Fri, Oct 05, 2018 at 02:27:41PM +0200, Adam Borowski wrote:
> > > Needed by users of fstatfs().
> > 
> > NAK.
> > 
> > The XFS superblock magic number is part of the on-disk format definition
> > of XFS. It belongs with all the other on-disk format definitions in this
> > file.
> > 
> > I don't think it's a good idea for userspace to associate s_magic with
> > userspace API feature sets, though the cat's long escaped the bag on
> > that one.
> > 
> > With that being reality, "the XFS superblock magic number" has a
> > different semantic meaning than "the agreed upon statfs.f_type value for
> > XFS", which means the latter should have a different symbol name to
> > reflect that difference.
> 
> Ie, you'd be ok with the same value to be defined in two places -- do I
> understand this right?

I'd be fine with it, but let's see what the other XFS maintainers think.
I still don't like the practice of inferring behaviors from magic
numbers, but at least we'd decouple the disk format from the UABI. :)

> > > --- a/fs/xfs/libxfs/xfs_format.h
> > > -#define	XFS_SB_MAGIC		0x58465342	/* 'XFSB' */
> > > +#define	XFS_SB_MAGIC		XFS_SUPER_MAGIC /* 0x58465342 = 'XFSB' */
> > > --- a/include/uapi/linux/magic.h
> > >  #define JFFS2_SUPER_MAGIC	0x72b6
> > > +#define XFS_SUPER_MAGIC		0x58465342	/* "XFSB" */
> > >  #define PSTOREFS_MAGIC		0x6165676C

You'd change xfs_fs_fill_super and xfs_fs_statfs too, right?

--D

> 
> 
> Meow!
> -- 
> ⢀⣴⠾⠻⢶⣦⠀ 
> ⣾⠁⢰⠒⠀⣿⡁ 10 people enter a bar: 1 who understands binary,
> ⢿⡄⠘⠷⠚⠋⠀ 1 who doesn't, D who prefer to write it as hex,
> ⠈⠳⣄⠀⠀⠀⠀ and 1 who narrowly avoided an off-by-one error.

Re: [PATCH] xfs: move the define for superblock magic to uapi

[Darrick J. Wong]

On Fri, Oct 05, 2018 at 08:10:32PM +0200, Adam Borowski wrote:
> On Fri, Oct 05, 2018 at 09:06:00AM -0700, Darrick J. Wong wrote:
> > On Fri, Oct 05, 2018 at 02:27:41PM +0200, Adam Borowski wrote:
> > > Needed by users of fstatfs().
> > 
> > NAK.
> > 
> > The XFS superblock magic number is part of the on-disk format definition
> > of XFS. It belongs with all the other on-disk format definitions in this
> > file.
> > 
> > I don't think it's a good idea for userspace to associate s_magic with
> > userspace API feature sets, though the cat's long escaped the bag on
> > that one.
> > 
> > With that being reality, "the XFS superblock magic number" has a
> > different semantic meaning than "the agreed upon statfs.f_type value for
> > XFS", which means the latter should have a different symbol name to
> > reflect that difference.
> 
> Ie, you'd be ok with the same value to be defined in two places -- do I
> understand this right?
> 
> > > --- a/fs/xfs/libxfs/xfs_format.h
> > > -#define	XFS_SB_MAGIC		0x58465342	/* 'XFSB' */
> > > +#define	XFS_SB_MAGIC		XFS_SUPER_MAGIC /* 0x58465342 = 'XFSB' */
> > > --- a/include/uapi/linux/magic.h
> > >  #define JFFS2_SUPER_MAGIC	0x72b6
> > > +#define XFS_SUPER_MAGIC		0x58465342	/* "XFSB" */
> > >  #define PSTOREFS_MAGIC		0x6165676C

Sorry, hit send too fast.

include/uapi/linux/magic.h would get:

#define XFS_STATFS_MAGIC	0x58465342	/* 'XFSB' */

fs/xfs/libxfs/xfs_format.h would not be changed at all:

#define XFS_SB_MAGIC		0x58465342	/* 'XFSB' */

fs/xfs/xfs_super.c would then be changed to:

xfs_fs_statfs(...) {
	statp->f_type = XFS_STATFS_MAGIC;
}

xfs_fs_fill_super(...) {
	sb->s_magic = XFS_STATFS_MAGIC;
}

We're making two separate promises here:

   "The on-disk superblock magic is XFS_SB_MAGIC."

    "When XFS is the filesystem, statfs will return XFS_STATFS_MAGIC."

Two completely independent #defines.  It just happens to be coincidental
that they have the same numeric value.

--D

> 
> 
> Meow!
> -- 
> ⢀⣴⠾⠻⢶⣦⠀ 
> ⣾⠁⢰⠒⠀⣿⡁ 10 people enter a bar: 1 who understands binary,
> ⢿⡄⠘⠷⠚⠋⠀ 1 who doesn't, D who prefer to write it as hex,
> ⠈⠳⣄⠀⠀⠀⠀ and 1 who narrowly avoided an off-by-one error.

Re: [PATCH] xfs: move the define for superblock magic to uapi

[Dave Chinner]

On Fri, Oct 05, 2018 at 11:23:56AM -0700, Darrick J. Wong wrote:
> On Fri, Oct 05, 2018 at 08:10:32PM +0200, Adam Borowski wrote:
> > On Fri, Oct 05, 2018 at 09:06:00AM -0700, Darrick J. Wong wrote:
> > > On Fri, Oct 05, 2018 at 02:27:41PM +0200, Adam Borowski wrote:
> > > > Needed by users of fstatfs().
> > > 
> > > NAK.
> > > 
> > > The XFS superblock magic number is part of the on-disk format definition
> > > of XFS. It belongs with all the other on-disk format definitions in this
> > > file.
> > > 
> > > I don't think it's a good idea for userspace to associate s_magic with
> > > userspace API feature sets, though the cat's long escaped the bag on
> > > that one.
> > > 
> > > With that being reality, "the XFS superblock magic number" has a
> > > different semantic meaning than "the agreed upon statfs.f_type value for
> > > XFS", which means the latter should have a different symbol name to
> > > reflect that difference.
> > 
> > Ie, you'd be ok with the same value to be defined in two places -- do I
> > understand this right?
> > 
> > > > --- a/fs/xfs/libxfs/xfs_format.h
> > > > -#define	XFS_SB_MAGIC		0x58465342	/* 'XFSB' */
> > > > +#define	XFS_SB_MAGIC		XFS_SUPER_MAGIC /* 0x58465342 = 'XFSB' */
> > > > --- a/include/uapi/linux/magic.h
> > > >  #define JFFS2_SUPER_MAGIC	0x72b6
> > > > +#define XFS_SUPER_MAGIC		0x58465342	/* "XFSB" */
> > > >  #define PSTOREFS_MAGIC		0x6165676C
> 
> Sorry, hit send too fast.
> 
> include/uapi/linux/magic.h would get:
> 
> #define XFS_STATFS_MAGIC	0x58465342	/* 'XFSB' */

$ man statfs |grep 0x5846
           XFS_SUPER_MAGIC       0x58465342
$

-Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH v2] xfs: add a define for statfs magic to uapi

[Adam Borowski]

Needed by userspace programs that call fstatfs().

It'd be natural to publish XFS_SB_MAGIC in uapi, but while these two have
identical values, they have different semantic meaning: one is an enum
cookie meant for statfs, the other a signature of the on-disk format.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
> > include/uapi/linux/magic.h would get:
> >
> > #define XFS_STATFS_MAGIC      0x58465342      /* 'XFSB' */
>
> $ man statfs |grep 0x5846
>            XFS_SUPER_MAGIC       0x58465342

Indeed, not only it is documented this way, but almost every other
filesystem has the define named FOO_SUPER_MAGIC.  Thus, inventing something
else doesn't seem to be a good idea to me.

On the other hand, this might be considered bikeshedding, and you're the
maintainer of this particular shed...


 fs/xfs/xfs_super.c         | 5 +++--
 include/uapi/linux/magic.h | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 207ee302b1bb..90b08051a4f3 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -43,6 +43,7 @@
 #include <linux/dax.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <linux/magic.h>
 #include <linux/mount.h>
 #include <linux/mempool.h>
 #include <linux/writeback.h>
@@ -1097,7 +1098,7 @@ xfs_fs_statfs(
 	xfs_extlen_t		lsize;
 	int64_t			ffree;
 
-	statp->f_type = XFS_SB_MAGIC;
+	statp->f_type = XFS_SUPER_MAGIC;
 	statp->f_namelen = MAXNAMELEN - 1;
 
 	id = huge_encode_dev(mp->m_ddev_targp->bt_dev);
@@ -1650,7 +1651,7 @@ xfs_fs_fill_super(
 	 * we must configure the block size in the superblock before we run the
 	 * full mount process as the mount process can lookup and cache inodes.
 	 */
-	sb->s_magic = XFS_SB_MAGIC;
+	sb->s_magic = XFS_SUPER_MAGIC;
 	sb->s_blocksize = mp->m_sb.sb_blocksize;
 	sb->s_blocksize_bits = ffs(sb->s_blocksize) - 1;
 	sb->s_maxbytes = xfs_max_file_offset(sb->s_blocksize_bits);
diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h
index 1a6fee974116..96c24478d8ce 100644
--- a/include/uapi/linux/magic.h
+++ b/include/uapi/linux/magic.h
@@ -29,6 +29,7 @@
 #define HPFS_SUPER_MAGIC	0xf995e849
 #define ISOFS_SUPER_MAGIC	0x9660
 #define JFFS2_SUPER_MAGIC	0x72b6
+#define XFS_SUPER_MAGIC		0x58465342	/* "XFSB" */
 #define PSTOREFS_MAGIC		0x6165676C
 #define EFIVARFS_MAGIC		0xde5e81e4
 #define HOSTFS_SUPER_MAGIC	0x00c0ffee
-- 
2.19.1

Re: [PATCH v2] xfs: add a define for statfs magic to uapi

[Darrick J. Wong]

On Sun, Oct 07, 2018 at 12:08:11AM +0200, Adam Borowski wrote:
> Needed by userspace programs that call fstatfs().
> 
> It'd be natural to publish XFS_SB_MAGIC in uapi, but while these two have
> identical values, they have different semantic meaning: one is an enum
> cookie meant for statfs, the other a signature of the on-disk format.
> 
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> ---
> > > include/uapi/linux/magic.h would get:
> > >
> > > #define XFS_STATFS_MAGIC      0x58465342      /* 'XFSB' */
> >
> > $ man statfs |grep 0x5846
> >            XFS_SUPER_MAGIC       0x58465342
> 
> Indeed, not only it is documented this way, but almost every other
> filesystem has the define named FOO_SUPER_MAGIC.  Thus, inventing something
> else doesn't seem to be a good idea to me.
> 
> On the other hand, this might be considered bikeshedding, and you're the
> maintainer of this particular shed...
> 
> 
>  fs/xfs/xfs_super.c         | 5 +++--
>  include/uapi/linux/magic.h | 1 +
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 207ee302b1bb..90b08051a4f3 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -43,6 +43,7 @@
>  #include <linux/dax.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
> +#include <linux/magic.h>
>  #include <linux/mount.h>
>  #include <linux/mempool.h>
>  #include <linux/writeback.h>
> @@ -1097,7 +1098,7 @@ xfs_fs_statfs(
>  	xfs_extlen_t		lsize;
>  	int64_t			ffree;
>  
> -	statp->f_type = XFS_SB_MAGIC;
> +	statp->f_type = XFS_SUPER_MAGIC;
>  	statp->f_namelen = MAXNAMELEN - 1;
>  
>  	id = huge_encode_dev(mp->m_ddev_targp->bt_dev);
> @@ -1650,7 +1651,7 @@ xfs_fs_fill_super(
>  	 * we must configure the block size in the superblock before we run the
>  	 * full mount process as the mount process can lookup and cache inodes.
>  	 */
> -	sb->s_magic = XFS_SB_MAGIC;
> +	sb->s_magic = XFS_SUPER_MAGIC;
>  	sb->s_blocksize = mp->m_sb.sb_blocksize;
>  	sb->s_blocksize_bits = ffs(sb->s_blocksize) - 1;
>  	sb->s_maxbytes = xfs_max_file_offset(sb->s_blocksize_bits);
> diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h
> index 1a6fee974116..96c24478d8ce 100644
> --- a/include/uapi/linux/magic.h
> +++ b/include/uapi/linux/magic.h
> @@ -29,6 +29,7 @@
>  #define HPFS_SUPER_MAGIC	0xf995e849
>  #define ISOFS_SUPER_MAGIC	0x9660
>  #define JFFS2_SUPER_MAGIC	0x72b6
> +#define XFS_SUPER_MAGIC		0x58465342	/* "XFSB" */

Looks ok to me...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  #define PSTOREFS_MAGIC		0x6165676C
>  #define EFIVARFS_MAGIC		0xde5e81e4
>  #define HOSTFS_SUPER_MAGIC	0x00c0ffee
> -- 
> 2.19.1
>