xfs
[Top] [All Lists]

Re: [PATCH 16/16] xfs: add versioned fsgeom ioctl with utf8version field

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [PATCH 16/16] xfs: add versioned fsgeom ioctl with utf8version field
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 7 Oct 2014 08:13:51 +1100
Cc: linux-fsdevel@xxxxxxxxxxxxxxx, olaf@xxxxxxx, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20141003220546.GP1865@xxxxxxx>
References: <20141003214758.GY1865@xxxxxxx> <20141003220546.GP1865@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Oct 03, 2014 at 05:05:46PM -0500, Ben Myers wrote:
> From: Ben Myers <bpm@xxxxxxx>
> 
> This adds a utf8version field to the xfs_fs_geom structure.  An
> important characteristic of this version of the ioctl is that
> fsgeo.version needs to be set by the caller to specify which version of
> the structure to return.
> 
> Signed-off-by: Ben Myers <bpm@xxxxxxx>
> ---
>  fs/xfs/xfs_fs.h    | 31 +++++++++++++++++++++++++++++++
>  fs/xfs/xfs_fsops.c | 13 ++++++++++++-
>  fs/xfs/xfs_fsops.h |  2 +-
>  fs/xfs/xfs_ioctl.c | 31 +++++++++++++++++++++++++++++++
>  4 files changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
> index fd45cbe..2f4d430 100644
> --- a/fs/xfs/xfs_fs.h
> +++ b/fs/xfs/xfs_fs.h
> @@ -206,6 +206,34 @@ typedef struct xfs_fsop_geom_v2 {
>       __u32           logsunit;       /* log stripe unit, bytes */
>  } xfs_fsop_geom_v2_t;
>  
> +/*
> + * Output for XFS_IOC_FSGEOMETRY
> + */
> +typedef struct xfs_fsop_geom {
> +     __u32           blocksize;      /* filesystem (data) block size */
> +     __u32           rtextsize;      /* realtime extent size         */
> +     __u32           agblocks;       /* fsblocks in an AG            */
> +     __u32           agcount;        /* number of allocation groups  */
> +     __u32           logblocks;      /* fsblocks in the log          */
> +     __u32           sectsize;       /* (data) sector size, bytes    */
> +     __u32           inodesize;      /* inode size in bytes          */
> +     __u32           imaxpct;        /* max allowed inode space(%)   */
> +     __u64           datablocks;     /* fsblocks in data subvolume   */
> +     __u64           rtblocks;       /* fsblocks in realtime subvol  */
> +     __u64           rtextents;      /* rt extents in realtime subvol*/
> +     __u64           logstart;       /* starting fsblock of the log  */
> +     unsigned char   uuid[16];       /* unique id of the filesystem  */
> +     __u32           sunit;          /* stripe unit, fsblocks        */
> +     __u32           swidth;         /* stripe width, fsblocks       */
> +     __s32           version;        /* structure version            */
> +     __u32           flags;          /* superblock version flags     */
> +     __u32           logsectsize;    /* log sector size, bytes       */
> +     __u32           rtsectsize;     /* realtime sector size, bytes  */
> +     __u32           dirblocksize;   /* directory block size, bytes  */
> +     __u32           logsunit;       /* log stripe unit, bytes */
> +     __u32           utf8version;    /* Unicode version              */
> +} xfs_fsop_geom_t;

New structure defintion, not a redefinition of the old name, please.
Drop the typedef, and the structure needs to be 64 bit size
aligned so we don't get problems with 32 bit userspace on 64 bit
kernels (e.g. we have a v1 compat ioctl handler because of this
issue).

Further, lets avoid needing to rev the ioctl again in future by
adding a bunch of "must be zero" padding to the new structure so we
can extend the information we push to userspace easily. i.e. padding
only becomes meaningful when the superblock flag that exposes
meaning is set. i.e. userspace can do this to conditionally access
the ut8version value if it is meaningful:

        utf8_ver = 0;
        if (geo.flags & XFS_FSOP_GEOM_FLAGS_UTF8)
                utf8_ver = geo->utf8version;

i.e. let's make the new structure forwards compatible with new
features...

> @@ -115,6 +117,15 @@ xfs_fs_geometry(
>                               XFS_FSOP_GEOM_FLAGS_LOGV2 : 0);
>               geo->logsunit = mp->m_sb.sb_logsunit;
>       }
> +     if (new_version >= XFS_FSOP_GEOM_VERSION5) {
> +             geo->version = XFS_FSOP_GEOM_VERSION5;
> +             geo->flags |= (xfs_sb_version_hasutf8(&mp->m_sb) ?
> +                             XFS_FSOP_GEOM_FLAGS_UTF8 : 0);
> +             geo->utf8version = mp->m_sb.sb_utf8version;
> +             if (bytes)
> +                     *bytes = sizeof(xfs_fsop_geom_v2_t) +
> +                              sizeof(geo->utf8version);

Further indication that the *bytes variable should die.

> +     }
>       return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_fsops.h b/fs/xfs/xfs_fsops.h
> index 74e1fee..b723f36 100644
> --- a/fs/xfs/xfs_fsops.h
> +++ b/fs/xfs/xfs_fsops.h
> @@ -18,7 +18,7 @@
>  #ifndef __XFS_FSOPS_H__
>  #define      __XFS_FSOPS_H__
>  
> -extern int xfs_fs_geometry(xfs_mount_t *mp, xfs_fsop_geom_v2_t *geo,
> +extern int xfs_fs_geometry(xfs_mount_t *mp, void *buffer,
>               int new_version, size_t *bytes);
>  extern int xfs_growfs_data(xfs_mount_t *mp, xfs_growfs_data_t *in);
>  extern int xfs_growfs_log(xfs_mount_t *mp, xfs_growfs_log_t *in);
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 1657ce5..6308680 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -859,6 +859,34 @@ xfs_ioc_fsgeometry_v2(
>       return 0;
>  }
>  
> +STATIC int
> +xfs_ioc_fsgeometry(
> +     struct xfs_mount        *mp,
> +     void                    __user *arg)
> +{
> +     xfs_fsop_geom_t         fsgeo;
> +     int                     version, error;
> +     size_t                  bytes;
> +
> +     /* offsetof(version)? XXX just get 32 bits? */
> +     if (copy_from_user(&fsgeo, arg, sizeof(xfs_fsop_geom_v1_t)))
> +             return -EFAULT;

It's best to copy in the entire structure rather than play offset
games.

> +     version = fsgeo.version;
> +
> +     if (version < XFS_FSOP_GEOM_VERSION5)
> +             return -EINVAL;

Here it rejects anything that is not a v3 structure aware of the
unicode extensions, which means it breaks any old recompiled
application that hasn't been updated to support
XFS_FSOP_GEOM_VERSION5 despite the fact that they will compile
against headers with the new definition without warnings or errors.

> +
> +     memset(&fsgeo, 0, sizeof(fsgeo));
> +     error = xfs_fs_geometry(mp, &fsgeo, version, &bytes);
> +     if (error)
> +             return error;
> +
> +     if (copy_to_user(arg, &fsgeo, bytes))
> +             return -EFAULT;

and you can use sizeof(struct xfs_fs_geom_v3) here instead of bytes.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>