Hi all,
This patch aims to hoist the ext4 get/set ioctls to the VFS so we have a
common interface for tools such as coreutils. The second patch adds support
for FS_IOC_GETFSUUID in xfs (with FS_IOC_SETFSUUID planned for future patches).
Comments and feedback appreciated!
Catherine
Catherine Hoang (2):
fs: hoist get/set UUID ioctls
xfs: add FS_IOC_GETFSUUID ioctl
fs/ext4/ext4.h | 13 ++-----------
fs/xfs/xfs_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++
include/uapi/linux/fs.h | 11 +++++++++++
3 files changed, 49 insertions(+), 11 deletions(-)
--
2.25.1
Add a new ioctl to retrieve the UUID of a mounted xfs filesystem. This is a
precursor to adding the SETFSUUID ioctl.
Signed-off-by: Catherine Hoang <[email protected]>
Reviewed-by: Allison Henderson <[email protected]>
---
fs/xfs/xfs_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 1f783e979629..cf77715afe9e 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1865,6 +1865,39 @@ xfs_fs_eofblocks_from_user(
return 0;
}
+static int
+xfs_ioctl_getuuid(
+ struct xfs_mount *mp,
+ struct fsuuid __user *ufsuuid)
+{
+ struct fsuuid fsuuid;
+ __u8 uuid[UUID_SIZE];
+
+ if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
+ return -EFAULT;
+
+ if (fsuuid.fsu_len == 0) {
+ fsuuid.fsu_len = UUID_SIZE;
+ if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len,
+ sizeof(fsuuid.fsu_len)))
+ return -EFAULT;
+ return 0;
+ }
+
+ if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0)
+ return -EINVAL;
+
+ spin_lock(&mp->m_sb_lock);
+ memcpy(uuid, &mp->m_sb.sb_uuid, UUID_SIZE);
+ spin_unlock(&mp->m_sb_lock);
+
+ fsuuid.fsu_len = UUID_SIZE;
+ if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) ||
+ copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
+ return -EFAULT;
+ return 0;
+}
+
/*
* These long-unused ioctls were removed from the official ioctl API in 5.17,
* but retain these definitions so that we can log warnings about them.
@@ -2153,6 +2186,9 @@ xfs_file_ioctl(
return error;
}
+ case FS_IOC_GETFSUUID:
+ return xfs_ioctl_getuuid(mp, arg);
+
default:
return -ENOTTY;
}
--
2.25.1
On Fri, Nov 18, 2022 at 01:14:08PM -0800, Catherine Hoang wrote:
> Add a new ioctl to retrieve the UUID of a mounted xfs filesystem. This is a
> precursor to adding the SETFSUUID ioctl.
>
> Signed-off-by: Catherine Hoang <[email protected]>
> Reviewed-by: Allison Henderson <[email protected]>
LGTM,
Reviewed-by: Darrick J. Wong <[email protected]>
--D
> ---
> fs/xfs/xfs_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 1f783e979629..cf77715afe9e 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1865,6 +1865,39 @@ xfs_fs_eofblocks_from_user(
> return 0;
> }
>
> +static int
> +xfs_ioctl_getuuid(
> + struct xfs_mount *mp,
> + struct fsuuid __user *ufsuuid)
> +{
> + struct fsuuid fsuuid;
> + __u8 uuid[UUID_SIZE];
> +
> + if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
> + return -EFAULT;
> +
> + if (fsuuid.fsu_len == 0) {
> + fsuuid.fsu_len = UUID_SIZE;
> + if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len,
> + sizeof(fsuuid.fsu_len)))
> + return -EFAULT;
> + return 0;
> + }
> +
> + if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0)
> + return -EINVAL;
> +
> + spin_lock(&mp->m_sb_lock);
> + memcpy(uuid, &mp->m_sb.sb_uuid, UUID_SIZE);
> + spin_unlock(&mp->m_sb_lock);
> +
> + fsuuid.fsu_len = UUID_SIZE;
> + if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) ||
> + copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
> + return -EFAULT;
> + return 0;
> +}
> +
> /*
> * These long-unused ioctls were removed from the official ioctl API in 5.17,
> * but retain these definitions so that we can log warnings about them.
> @@ -2153,6 +2186,9 @@ xfs_file_ioctl(
> return error;
> }
>
> + case FS_IOC_GETFSUUID:
> + return xfs_ioctl_getuuid(mp, arg);
> +
> default:
> return -ENOTTY;
> }
> --
> 2.25.1
>
On Fri, Nov 18, 2022 at 01:14:08PM -0800, Catherine Hoang wrote:
> Add a new ioctl to retrieve the UUID of a mounted xfs filesystem. This is a
> precursor to adding the SETFSUUID ioctl.
>
> Signed-off-by: Catherine Hoang <[email protected]>
> Reviewed-by: Allison Henderson <[email protected]>
> ---
> fs/xfs/xfs_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 1f783e979629..cf77715afe9e 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1865,6 +1865,39 @@ xfs_fs_eofblocks_from_user(
> return 0;
> }
>
> +static int
> +xfs_ioctl_getuuid(
> + struct xfs_mount *mp,
> + struct fsuuid __user *ufsuuid)
> +{
> + struct fsuuid fsuuid;
> + __u8 uuid[UUID_SIZE];
uuid_t, please, not an open coded uuid_t.
> +
> + if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
> + return -EFAULT;
I still think this failing to copy the flex array member and then
having to declare a local uuid buffer is an ugly wart, not just on
the API side of things.
> + if (fsuuid.fsu_len == 0) {
> + fsuuid.fsu_len = UUID_SIZE;
XFS uses sizeof(uuid_t) for the size of it's uuids, not UUID_SIZE.
> + if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len,
> + sizeof(fsuuid.fsu_len)))
> + return -EFAULT;
> + return 0;
> + }
> +
> + if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0)
> + return -EINVAL;
> +
> + spin_lock(&mp->m_sb_lock);
> + memcpy(uuid, &mp->m_sb.sb_uuid, UUID_SIZE);
> + spin_unlock(&mp->m_sb_lock);
Hmmmm. Shouldn't we be promoting xfs_fs_get_uuid() to xfs_super.c
(without the pNFS warning!) and calling that here, rather than open
coding another "get the XFS superblock UUID" function here?
i.e.
if (fsuuid.fsu_flags != 0)
return -EINVAL;
error = xfs_fs_get_uuid(&mp->m_sb, uuid, &fsuuid.fsu_len, NULL);
if (error)
return -EINVAL;
Also, uuid_copy()?
Cheers,
Dave.
--
Dave Chinner
[email protected]
On Fri, Nov 18, 2022 at 01:14:06PM -0800, Catherine Hoang wrote:
> Hi all,
>
> This patch aims to hoist the ext4 get/set ioctls to the VFS so we have a
> common interface for tools such as coreutils. The second patch adds support
> for FS_IOC_GETFSUUID in xfs (with FS_IOC_SETFSUUID planned for future patches).
FWIW, the next version needs to be cc'd to
[email protected] because we're talking about lifting an
ioctl to the VFS layer...
Cheers,
Dave.
--
Dave Chinner
[email protected]
On Tue, Nov 22, 2022 at 08:02:23AM +1100, Dave Chinner wrote:
> On Fri, Nov 18, 2022 at 01:14:08PM -0800, Catherine Hoang wrote:
> > Add a new ioctl to retrieve the UUID of a mounted xfs filesystem. This is a
> > precursor to adding the SETFSUUID ioctl.
> >
> > Signed-off-by: Catherine Hoang <[email protected]>
> > Reviewed-by: Allison Henderson <[email protected]>
> > ---
> > fs/xfs/xfs_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 36 insertions(+)
> >
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 1f783e979629..cf77715afe9e 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1865,6 +1865,39 @@ xfs_fs_eofblocks_from_user(
> > return 0;
> > }
> >
> > +static int
> > +xfs_ioctl_getuuid(
> > + struct xfs_mount *mp,
> > + struct fsuuid __user *ufsuuid)
> > +{
> > + struct fsuuid fsuuid;
> > + __u8 uuid[UUID_SIZE];
>
> uuid_t, please, not an open coded uuid_t.
>
> > +
> > + if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
> > + return -EFAULT;
>
> I still think this failing to copy the flex array member and then
> having to declare a local uuid buffer is an ugly wart, not just on
> the API side of things.
>
> > + if (fsuuid.fsu_len == 0) {
> > + fsuuid.fsu_len = UUID_SIZE;
>
> XFS uses sizeof(uuid_t) for the size of it's uuids, not UUID_SIZE.
>
> > + if (copy_to_user(&ufsuuid->fsu_len, &fsuuid.fsu_len,
> > + sizeof(fsuuid.fsu_len)))
> > + return -EFAULT;
> > + return 0;
> > + }
> > +
> > + if (fsuuid.fsu_len < UUID_SIZE || fsuuid.fsu_flags != 0)
> > + return -EINVAL;
> > +
> > + spin_lock(&mp->m_sb_lock);
> > + memcpy(uuid, &mp->m_sb.sb_uuid, UUID_SIZE);
> > + spin_unlock(&mp->m_sb_lock);
>
> Hmmmm. Shouldn't we be promoting xfs_fs_get_uuid() to xfs_super.c
> (without the pNFS warning!) and calling that here, rather than open
> coding another "get the XFS superblock UUID" function here?
I disagree that it's worth the effort to create a helper function to
wrap four lines of code, particularly since the pnfs code has this extra
weird wart of returning the byte offset(?) of the uuid location.
> i.e.
> if (fsuuid.fsu_flags != 0)
> return -EINVAL;
>
> error = xfs_fs_get_uuid(&mp->m_sb, uuid, &fsuuid.fsu_len, NULL);
> if (error)
> return -EINVAL;
>
> Also, uuid_copy()?
Why does xfs_fs_get_uuid use memcpy then? Did the compiler reject the
u8* -> uuid_t * type conversion?
Alternately there's export_uuid().
--D
> Cheers,
>
> Dave.
>
> --
> Dave Chinner
> [email protected]
On Mon, Nov 21, 2022 at 02:18:47PM -0800, Darrick J. Wong wrote:
> On Tue, Nov 22, 2022 at 08:02:23AM +1100, Dave Chinner wrote:
> > On Fri, Nov 18, 2022 at 01:14:08PM -0800, Catherine Hoang wrote:
> > i.e.
> > if (fsuuid.fsu_flags != 0)
> > return -EINVAL;
> >
> > error = xfs_fs_get_uuid(&mp->m_sb, uuid, &fsuuid.fsu_len, NULL);
> > if (error)
> > return -EINVAL;
> >
> > Also, uuid_copy()?
>
> Why does xfs_fs_get_uuid use memcpy then? Did the compiler reject the
> u8* -> uuid_t * type conversion?
No idea, I've completely forgotten about the reasons for the code
being written that way.
These days people seem to care about making the compiler do all the
type checking and type conversions for us. The use of UUIDs and
various types within XFS has been quite ad hoc, so I'm just
suggesting that we improve it somewhat.
Using types and APIs that mean we don't have to code the length of
UUIDs everywhere seems like a Good Idea for newly written code...
> Alternately there's export_uuid().
But we don't need that if we use uuid_t for all the local
representations of the UUID....
Cheers,
Dave.
--
Dave Chinner
[email protected]