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

[Dave Chinner <david@fromorbit.com>]

On Fri, Oct 03, 2014 at 05:05:46PM -0500, Ben Myers wrote:
> From: Ben Myers <bpm@sgi.com>
> 
> 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@sgi.com>
> ---
>  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@fromorbit.com