[PATCH for 4.15] libxfs: fix dev_t handling in inode forks

[PATCH for 4.15] libxfs: fix dev_t handling in inode forks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In commit 699f676961 ("xfs: remove if_rdev"), some ancient definitions
of MAJOR/MINOR/MKDEV were ported into libxfs.  However, platform_defs.h
already has IRIX_DEV_* macros which encode a (major, minor) tuple into a
(major:14, minor:18) u32 format.  libxfs writes these values to disk
without any further interpretation.

Existing libxfs clients pass these encoded device numbers into libxfs,
which means that we cannot suddenly start interpreting the bit fields
differently.  The ported MAJOR/MINOR does this (major:8, minor:8), with
the result that proto file specs for block/char devices write
bit-shifted garbage into i_rdev.

(Or at least it would write garbage if libxfs_ialloc hadn't also lost
the ability to set i_rdev in the same patch...)

Therefore, just keep using the "IRIX" device encoding macros in
platform_defs.h, and actually set i_rdev in libxfs_ialloc when someone
tries to create a device file.  This fixes a regression in xfs/019.

Cc: sandeen@sandeen.net
Fixes: 699f67696151d750a58c321b072360ccc694837b
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_priv.h |   18 ++++--------------
 libxfs/util.c        |    1 +
 2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index 8c3f3b7..4c083b9 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -87,23 +87,13 @@ extern char    *progname;
 #undef ASSERT
 #define ASSERT(ex) assert(ex)
 
-#define MAJOR(dev)      ((dev)>>8)
-#define MINOR(dev)      ((dev) & 0xff)
-#define MKDEV(ma,mi) ((ma)<<8 | (mi))
-
-static inline unsigned sysv_major(uint32_t dev)
-{
-        return (dev >> 18) & 0x3fff;
-}
-
-static inline unsigned sysv_minor(uint32_t dev)
-{
-        return dev & 0x3ffff;
-}
+#define MKDEV(major, minor)	IRIX_MKDEV(major, minor)
+#define sysv_major(dev)		IRIX_DEV_MAJOR(dev)
+#define sysv_minor(dev)		IRIX_DEV_MINOR(dev)
 
 static inline uint32_t sysv_encode_dev(dev_t dev)
 {
-        return MINOR(dev) | (MAJOR(dev) << 18);
+        return IRIX_DEV_MINOR(dev) | (IRIX_DEV_MAJOR(dev) << 18);
 }
 
 #ifndef EWRONGFS
diff --git a/libxfs/util.c b/libxfs/util.c
index 90f96f8..792a17f 100644
--- a/libxfs/util.c
+++ b/libxfs/util.c
@@ -336,6 +336,7 @@ libxfs_ialloc(
 	case S_IFBLK:
 		ip->i_d.di_format = XFS_DINODE_FMT_DEV;
 		flags |= XFS_ILOG_DEV;
+		VFS_I(ip)->i_rdev = rdev;
 		break;
 	case S_IFREG:
 	case S_IFDIR:

Re: [PATCH for 4.15] libxfs: fix dev_t handling in inode forks

[Eric Sandeen]

On 11/15/17 7:15 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In commit 699f676961 ("xfs: remove if_rdev"), some ancient definitions
> of MAJOR/MINOR/MKDEV were ported into libxfs.  However, platform_defs.h
> already has IRIX_DEV_* macros which encode a (major, minor) tuple into a
> (major:14, minor:18) u32 format.  libxfs writes these values to disk
> without any further interpretation.

Thanks for spotting, that was dumb on my part.  Rather than fixing what
I intended to be a RFC/WIP branch with this commit, I plan to include
your changes in the original commit to fix it up, credit you, and
rebase that branch.  There are a couple other reasons to rebase as well.

After 4.14 gets pushed out I'll merge the libxfs-4.15 sync branch to for-next.

-Eric

Re: [PATCH for 4.15] libxfs: fix dev_t handling in inode forks

[Christoph Hellwig]

Hmm.

I guess we should just clean up the calling conventions, and keep
the conversion helpers in xfs_linux.h so that xfsprogs can turn them
into noops.

Something like the untested patch for the kernel space, xfsprogs would
then just stub out the two inlines to return the passed in value.

---
>From e0ed26ab66774c1611e71a3c290efbe4e21d483c Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 17 Nov 2017 17:55:48 +0100
Subject: xfs: abstract out dev_t conversions

And move them to xfs_linux.h so that xfsprogs can stub them out more
easily.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_inode_fork.c |  8 ++------
 fs/xfs/xfs_linux.h             | 10 ++++++++++
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 1c90ec41e9df..c79a1616b79d 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -42,11 +42,6 @@ STATIC int xfs_iformat_local(xfs_inode_t *, xfs_dinode_t *, int, int);
 STATIC int xfs_iformat_extents(xfs_inode_t *, xfs_dinode_t *, int);
 STATIC int xfs_iformat_btree(xfs_inode_t *, xfs_dinode_t *, int);
 
-static inline dev_t xfs_to_linux_dev_t(xfs_dev_t dev)
-{
-	return MKDEV(sysv_major(dev) & 0x1ff, sysv_minor(dev));
-}
-
 /*
  * Copy inode type and data and attr format specific information from the
  * on-disk inode to the in-core inode and fork structures.  For fifos, devices,
@@ -792,7 +787,8 @@ xfs_iflush_fork(
 	case XFS_DINODE_FMT_DEV:
 		if (iip->ili_fields & XFS_ILOG_DEV) {
 			ASSERT(whichfork == XFS_DATA_FORK);
-			xfs_dinode_put_rdev(dip, sysv_encode_dev(VFS_I(ip)->i_rdev));
+			xfs_dinode_put_rdev(dip,
+					linux_to_xfs_dev_t(VFS_I(ip)->i_rdev));
 		}
 		break;
 
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index 6282bfc1afa9..99562ec0de56 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -204,6 +204,16 @@ static inline kgid_t xfs_gid_to_kgid(uint32_t gid)
 	return make_kgid(&init_user_ns, gid);
 }
 
+static inline dev_t xfs_to_linux_dev_t(xfs_dev_t dev)
+{
+	return MKDEV(sysv_major(dev) & 0x1ff, sysv_minor(dev));
+}
+
+static inline xfs_dev_t linux_to_xfs_dev_t(dev_t dev)
+{
+	return sysv_encode_dev(dev);
+}
+
 /*
  * Various platform dependent calls that don't fit anywhere else
  */
-- 
2.14.2

Re: [PATCH for 4.15] libxfs: fix dev_t handling in inode forks

[Eric Sandeen]

On 11/17/17 10:57 AM, Christoph Hellwig wrote:
> Hmm.
> 
> I guess we should just clean up the calling conventions, and keep
> the conversion helpers in xfs_linux.h so that xfsprogs can turn them
> into noops.
> 
> Something like the untested patch for the kernel space, xfsprogs would
> then just stub out the two inlines to return the passed in value.
> 
> ---
> From e0ed26ab66774c1611e71a3c290efbe4e21d483c Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 17 Nov 2017 17:55:48 +0100
> Subject: xfs: abstract out dev_t conversions
> 
> And move them to xfs_linux.h so that xfsprogs can stub them out more
> easily.

Ok I'm a little confused by this, actually (sorry for the late reply).

I added an i_rdev to the "vfs inode" in xfsprogs... but if that's
really supposed to be a "linux inode" then we probably want to keep
the conversions to/from linux dev_t format when it's stored there,
even if it's never actually used as such.

So I'm not sure no-ops are the right answer.  That'd work, but it
seems odd to carry around the xfs device format in the "linux" inode
even in xfsprogs, from a "least surprise" POV.

So moving these to the header file is (was) fine, but I'll probably
keep the translation in xfsprogs.

Unless I'm missing something ...

-Eric


> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.c |  8 ++------
>  fs/xfs/xfs_linux.h             | 10 ++++++++++
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 1c90ec41e9df..c79a1616b79d 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -42,11 +42,6 @@ STATIC int xfs_iformat_local(xfs_inode_t *, xfs_dinode_t *, int, int);
>  STATIC int xfs_iformat_extents(xfs_inode_t *, xfs_dinode_t *, int);
>  STATIC int xfs_iformat_btree(xfs_inode_t *, xfs_dinode_t *, int);
>  
> -static inline dev_t xfs_to_linux_dev_t(xfs_dev_t dev)
> -{
> -	return MKDEV(sysv_major(dev) & 0x1ff, sysv_minor(dev));
> -}
> -
>  /*
>   * Copy inode type and data and attr format specific information from the
>   * on-disk inode to the in-core inode and fork structures.  For fifos, devices,
> @@ -792,7 +787,8 @@ xfs_iflush_fork(
>  	case XFS_DINODE_FMT_DEV:
>  		if (iip->ili_fields & XFS_ILOG_DEV) {
>  			ASSERT(whichfork == XFS_DATA_FORK);
> -			xfs_dinode_put_rdev(dip, sysv_encode_dev(VFS_I(ip)->i_rdev));
> +			xfs_dinode_put_rdev(dip,
> +					linux_to_xfs_dev_t(VFS_I(ip)->i_rdev));
>  		}
>  		break;
>  
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 6282bfc1afa9..99562ec0de56 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -204,6 +204,16 @@ static inline kgid_t xfs_gid_to_kgid(uint32_t gid)
>  	return make_kgid(&init_user_ns, gid);
>  }
>  
> +static inline dev_t xfs_to_linux_dev_t(xfs_dev_t dev)
> +{
> +	return MKDEV(sysv_major(dev) & 0x1ff, sysv_minor(dev));
> +}
> +
> +static inline xfs_dev_t linux_to_xfs_dev_t(dev_t dev)
> +{
> +	return sysv_encode_dev(dev);
> +}
> +
>  /*
>   * Various platform dependent calls that don't fit anywhere else
>   */
> 

Re: [PATCH for 4.15] libxfs: fix dev_t handling in inode forks

[Eric Sandeen]

On 11/27/17 1:26 PM, Eric Sandeen wrote:
> On 11/17/17 10:57 AM, Christoph Hellwig wrote:
>> Hmm.
>>
>> I guess we should just clean up the calling conventions, and keep
>> the conversion helpers in xfs_linux.h so that xfsprogs can turn them
>> into noops.
>>
>> Something like the untested patch for the kernel space, xfsprogs would
>> then just stub out the two inlines to return the passed in value.
>>
>> ---
>> From e0ed26ab66774c1611e71a3c290efbe4e21d483c Mon Sep 17 00:00:00 2001
>> From: Christoph Hellwig <hch@lst.de>
>> Date: Fri, 17 Nov 2017 17:55:48 +0100
>> Subject: xfs: abstract out dev_t conversions
>>
>> And move them to xfs_linux.h so that xfsprogs can stub them out more
>> easily.
> 
> Ok I'm a little confused by this, actually (sorry for the late reply).
> 
> I added an i_rdev to the "vfs inode" in xfsprogs... but if that's
> really supposed to be a "linux inode" then we probably want to keep
> the conversions to/from linux dev_t format when it's stored there,
> even if it's never actually used as such.
> 
> So I'm not sure no-ops are the right answer.  That'd work, but it
> seems odd to carry around the xfs device format in the "linux" inode
> even in xfsprogs, from a "least surprise" POV.
> 
> So moving these to the header file is (was) fine, but I'll probably
> keep the translation in xfsprogs.
> 
> Unless I'm missing something ...

Maybe what I'm missing is that it's stupid to carry all those conversions
around if they're not needed.   ;)  

I'll stuff the xfs format device number into the "linux" i_rdev, with a comment
stating that it's so.

-Eric

Re: [PATCH for 4.15] libxfs: fix dev_t handling in inode forks

[Christoph Hellwig]

On Mon, Nov 27, 2017 at 02:16:47PM -0600, Eric Sandeen wrote:
> Maybe what I'm missing is that it's stupid to carry all those conversions
> around if they're not needed.   ;)  
> 
> I'll stuff the xfs format device number into the "linux" i_rdev, with a comment

Yes, that was the idea.