diff mbox series

[V2,09/12] xfs: Rename XFS_IOC_BULKSTAT to XFS_IOC_BULKSTAT_V5

Message ID 20210726114541.24898-10-chandanrlinux@gmail.com (mailing list archive)
State Deferred, archived
Headers show
Series xfs: Extend per-inode extent counters | expand

Commit Message

Chandan Babu R July 26, 2021, 11:45 a.m. UTC 
This commit renames XFS_IOC_BULKSTAT to XFS_IOC_BULKSTAT_V5 to allow a future
commit to extend bulkstat facility to support 64-bit extent counters. To this
end, this commit also renames xfs_bulkstat->bs_extents field to
xfs_bulkstat->bs_extents32.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_fs.h |  4 ++--
 fs/xfs/xfs_ioctl.c     | 27 ++++++++++++++++++++++-----
 fs/xfs/xfs_ioctl32.c   |  7 +++++++
 fs/xfs/xfs_itable.c    |  4 ++--
 fs/xfs/xfs_itable.h    |  1 +
 5 files changed, 34 insertions(+), 9 deletions(-)

Comments

Darrick J. Wong July 27, 2021, 10:54 p.m. UTC | #1 
On Mon, Jul 26, 2021 at 05:15:38PM +0530, Chandan Babu R wrote:
> This commit renames XFS_IOC_BULKSTAT to XFS_IOC_BULKSTAT_V5 to allow a future
> commit to extend bulkstat facility to support 64-bit extent counters. To this
> end, this commit also renames xfs_bulkstat->bs_extents field to
> xfs_bulkstat->bs_extents32.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_fs.h |  4 ++--
>  fs/xfs/xfs_ioctl.c     | 27 ++++++++++++++++++++++-----
>  fs/xfs/xfs_ioctl32.c   |  7 +++++++
>  fs/xfs/xfs_itable.c    |  4 ++--
>  fs/xfs/xfs_itable.h    |  1 +
>  5 files changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index 2594fb647384..d760a969599e 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -394,7 +394,7 @@ struct xfs_bulkstat {
>  	uint32_t	bs_extsize_blks; /* extent size hint, blocks	*/
>  
>  	uint32_t	bs_nlink;	/* number of links		*/
> -	uint32_t	bs_extents;	/* number of extents		*/
> +	uint32_t	bs_extents32;	/* number of extents		*/

I wish I'd thought of this when we introduced bulkstat v5 so you
wouldn't have had to do this.

(I might have more to say in the bulkstat v6 patch review.)
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  	uint32_t	bs_aextents;	/* attribute number of extents	*/
>  	uint16_t	bs_version;	/* structure version		*/
>  	uint16_t	bs_forkoff;	/* inode fork offset in bytes	*/
> @@ -853,7 +853,7 @@ struct xfs_scrub_metadata {
>  #define XFS_IOC_FSGEOMETRY_V4	     _IOR ('X', 124, struct xfs_fsop_geom_v4)
>  #define XFS_IOC_GOINGDOWN	     _IOR ('X', 125, uint32_t)
>  #define XFS_IOC_FSGEOMETRY	     _IOR ('X', 126, struct xfs_fsop_geom)
> -#define XFS_IOC_BULKSTAT	     _IOR ('X', 127, struct xfs_bulkstat_req)
> +#define XFS_IOC_BULKSTAT_V5	     _IOR ('X', 127, struct xfs_bulkstat_req)
>  #define XFS_IOC_INUMBERS	     _IOR ('X', 128, struct xfs_inumbers_req)
>  /*	FIEXCHANGE_RANGE ----------- hoisted 129	 */
>  /*	XFS_IOC_GETFSUUID ---------- deprecated 140	 */
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 2da90f81e6e3..85f45340cb16 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -715,6 +715,8 @@ xfs_fsbulkstat_one_fmt(
>  {
>  	struct xfs_bstat		bs1;
>  
> +	ASSERT(breq->version == XFS_BULKSTAT_VERSION_V1);
> +
>  	xfs_bulkstat_to_bstat(breq->mp, &bs1, bstat);
>  	if (copy_to_user(breq->ubuffer, &bs1, sizeof(bs1)))
>  		return -EFAULT;
> @@ -728,6 +730,8 @@ xfs_fsinumbers_fmt(
>  {
>  	struct xfs_inogrp		ig1;
>  
> +	ASSERT(breq->version == XFS_INUMBERS_VERSION_V1);
> +
>  	xfs_inumbers_to_inogrp(&ig1, igrp);
>  	if (copy_to_user(breq->ubuffer, &ig1, sizeof(struct xfs_inogrp)))
>  		return -EFAULT;
> @@ -787,14 +791,17 @@ xfs_ioc_fsbulkstat(
>  	 */
>  	if (cmd == XFS_IOC_FSINUMBERS) {
>  		breq.startino = lastino ? lastino + 1 : 0;
> +		breq.version = XFS_INUMBERS_VERSION_V1;
>  		error = xfs_inumbers(&breq, xfs_fsinumbers_fmt);
>  		lastino = breq.startino - 1;
>  	} else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE) {
>  		breq.startino = lastino;
>  		breq.icount = 1;
> +		breq.version = XFS_BULKSTAT_VERSION_V1;
>  		error = xfs_bulkstat_one(&breq, xfs_fsbulkstat_one_fmt);
>  	} else {	/* XFS_IOC_FSBULKSTAT */
>  		breq.startino = lastino ? lastino + 1 : 0;
> +		breq.version = XFS_BULKSTAT_VERSION_V1;
>  		error = xfs_bulkstat(&breq, xfs_fsbulkstat_one_fmt);
>  		lastino = breq.startino - 1;
>  	}
> @@ -819,6 +826,8 @@ xfs_bulkstat_fmt(
>  	struct xfs_ibulk		*breq,
>  	const struct xfs_bulkstat	*bstat)
>  {
> +	ASSERT(breq->version == XFS_BULKSTAT_VERSION_V5);
> +
>  	if (copy_to_user(breq->ubuffer, bstat, sizeof(struct xfs_bulkstat)))
>  		return -EFAULT;
>  	return xfs_ibulk_advance(breq, sizeof(struct xfs_bulkstat));
> @@ -918,13 +927,15 @@ STATIC int
>  xfs_ioc_bulkstat(
>  	struct file			*file,
>  	unsigned int			cmd,
> -	struct xfs_bulkstat_req __user	*arg)
> +	struct xfs_bulkstat_req __user	*arg,
> +	int				version)
>  {
>  	struct xfs_mount		*mp = XFS_I(file_inode(file))->i_mount;
>  	struct xfs_bulk_ireq		hdr;
>  	struct xfs_ibulk		breq = {
>  		.mp			= mp,
>  		.mnt_userns		= file_mnt_user_ns(file),
> +		.version		= version,
>  	};
>  	int				error;
>  
> @@ -960,6 +971,8 @@ xfs_inumbers_fmt(
>  	struct xfs_ibulk		*breq,
>  	const struct xfs_inumbers	*igrp)
>  {
> +	ASSERT(breq->version == XFS_INUMBERS_VERSION_V5);
> +
>  	if (copy_to_user(breq->ubuffer, igrp, sizeof(struct xfs_inumbers)))
>  		return -EFAULT;
>  	return xfs_ibulk_advance(breq, sizeof(struct xfs_inumbers));
> @@ -970,11 +983,13 @@ STATIC int
>  xfs_ioc_inumbers(
>  	struct xfs_mount		*mp,
>  	unsigned int			cmd,
> -	struct xfs_inumbers_req __user	*arg)
> +	struct xfs_inumbers_req __user	*arg,
> +	int				version)
>  {
>  	struct xfs_bulk_ireq		hdr;
>  	struct xfs_ibulk		breq = {
>  		.mp			= mp,
> +		.version		= version,
>  	};
>  	int				error;
>  
> @@ -1882,10 +1897,12 @@ xfs_file_ioctl(
>  	case XFS_IOC_FSINUMBERS:
>  		return xfs_ioc_fsbulkstat(filp, cmd, arg);
>  
> -	case XFS_IOC_BULKSTAT:
> -		return xfs_ioc_bulkstat(filp, cmd, arg);
> +	case XFS_IOC_BULKSTAT_V5:
> +		return xfs_ioc_bulkstat(filp, cmd, arg,
> +				XFS_BULKSTAT_VERSION_V5);
>  	case XFS_IOC_INUMBERS:
> -		return xfs_ioc_inumbers(mp, cmd, arg);
> +		return xfs_ioc_inumbers(mp, cmd, arg,
> +				XFS_INUMBERS_VERSION_V5);
>  
>  	case XFS_IOC_FSGEOMETRY_V1:
>  		return xfs_ioc_fsgeometry(mp, arg, 3);
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index bef19060f4aa..d9a7abc209b5 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -88,6 +88,8 @@ xfs_fsinumbers_fmt_compat(
>  	struct xfs_inogrp		ig1;
>  	struct xfs_inogrp		*igrp = &ig1;
>  
> +	ASSERT(breq->version == XFS_INUMBERS_VERSION_V1);
> +
>  	xfs_inumbers_to_inogrp(&ig1, ig);
>  
>  	if (put_user(igrp->xi_startino,   &p32->xi_startino) ||
> @@ -177,6 +179,8 @@ xfs_fsbulkstat_one_fmt_compat(
>  	struct xfs_bstat		bs1;
>  	struct xfs_bstat		*buffer = &bs1;
>  
> +	ASSERT(breq->version == XFS_BULKSTAT_VERSION_V1);
> +
>  	xfs_bulkstat_to_bstat(breq->mp, &bs1, bstat);
>  
>  	if (put_user(buffer->bs_ino,	  &p32->bs_ino)		||
> @@ -293,15 +297,18 @@ xfs_compat_ioc_fsbulkstat(
>  	 */
>  	if (cmd == XFS_IOC_FSINUMBERS_32) {
>  		breq.startino = lastino ? lastino + 1 : 0;
> +		breq.version = XFS_INUMBERS_VERSION_V1;
>  		error = xfs_inumbers(&breq, inumbers_func);
>  		lastino = breq.startino - 1;
>  	} else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE_32) {
>  		breq.startino = lastino;
>  		breq.icount = 1;
> +		breq.version = XFS_BULKSTAT_VERSION_V1;
>  		error = xfs_bulkstat_one(&breq, bs_one_func);
>  		lastino = breq.startino;
>  	} else if (cmd == XFS_IOC_FSBULKSTAT_32) {
>  		breq.startino = lastino ? lastino + 1 : 0;
> +		breq.version = XFS_BULKSTAT_VERSION_V1;
>  		error = xfs_bulkstat(&breq, bs_one_func);
>  		lastino = breq.startino - 1;
>  	} else {
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 65c194cdea79..f0daa65e61ff 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -134,7 +134,7 @@ xfs_bulkstat_one_int(
>  
>  	buf->bs_xflags = xfs_ip2xflags(ip);
>  	buf->bs_extsize_blks = ip->i_extsize;
> -	buf->bs_extents = xfs_ifork_nextents(&ip->i_df);
> +	buf->bs_extents32 = xfs_ifork_nextents(&ip->i_df);
>  	xfs_bulkstat_health(ip, buf);
>  	buf->bs_aextents = xfs_ifork_nextents(ip->i_afp);
>  	buf->bs_forkoff = XFS_IFORK_BOFF(ip);
> @@ -356,7 +356,7 @@ xfs_bulkstat_to_bstat(
>  	bs1->bs_blocks = bstat->bs_blocks;
>  	bs1->bs_xflags = bstat->bs_xflags;
>  	bs1->bs_extsize = XFS_FSB_TO_B(mp, bstat->bs_extsize_blks);
> -	bs1->bs_extents = bstat->bs_extents;
> +	bs1->bs_extents = bstat->bs_extents32;
>  	bs1->bs_gen = bstat->bs_gen;
>  	bs1->bs_projid_lo = bstat->bs_projectid & 0xFFFF;
>  	bs1->bs_forkoff = bstat->bs_forkoff;
> diff --git a/fs/xfs/xfs_itable.h b/fs/xfs/xfs_itable.h
> index f5a13f69883a..ee4d8d0461ca 100644
> --- a/fs/xfs/xfs_itable.h
> +++ b/fs/xfs/xfs_itable.h
> @@ -14,6 +14,7 @@ struct xfs_ibulk {
>  	unsigned int		icount;   /* number of elements in ubuffer */
>  	unsigned int		ocount;   /* number of records returned */
>  	unsigned int		flags;    /* see XFS_IBULK_FLAG_* */
> +	int			version;  /* structure version to be returned */
>  };
>  
>  /* Only iterate within the same AG as startino */
> -- 
> 2.30.2
>
Darrick J. Wong July 27, 2021, 11 p.m. UTC | #2 
On Tue, Jul 27, 2021 at 03:54:00PM -0700, Darrick J. Wong wrote:
> On Mon, Jul 26, 2021 at 05:15:38PM +0530, Chandan Babu R wrote:
> > This commit renames XFS_IOC_BULKSTAT to XFS_IOC_BULKSTAT_V5 to allow a future
> > commit to extend bulkstat facility to support 64-bit extent counters. To this
> > end, this commit also renames xfs_bulkstat->bs_extents field to
> > xfs_bulkstat->bs_extents32.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_fs.h |  4 ++--
> >  fs/xfs/xfs_ioctl.c     | 27 ++++++++++++++++++++++-----
> >  fs/xfs/xfs_ioctl32.c   |  7 +++++++
> >  fs/xfs/xfs_itable.c    |  4 ++--
> >  fs/xfs/xfs_itable.h    |  1 +
> >  5 files changed, 34 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > index 2594fb647384..d760a969599e 100644
> > --- a/fs/xfs/libxfs/xfs_fs.h
> > +++ b/fs/xfs/libxfs/xfs_fs.h
> > @@ -394,7 +394,7 @@ struct xfs_bulkstat {
> >  	uint32_t	bs_extsize_blks; /* extent size hint, blocks	*/
> >  
> >  	uint32_t	bs_nlink;	/* number of links		*/
> > -	uint32_t	bs_extents;	/* number of extents		*/
> > +	uint32_t	bs_extents32;	/* number of extents		*/
> 
> I wish I'd thought of this when we introduced bulkstat v5 so you
> wouldn't have had to do this.
> 
> (I might have more to say in the bulkstat v6 patch review.)
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Actually, I take that back, I have things to say /now/. :)

Rather than adding a whole new ioctl definition which (I haven't looked
at the xfsprogs changes) likely creates a bunch of churn in userspace,
what if we added a XFS_IBULK_ flag for supporting large extent counts?
There's also quite a bit of reserved padding space in xfs_bulk_ireq, so
perhaps we should define one of those padding u64 as a op-specific flag
field that would be a way to pass bulkstat-specific flags to the
relevant operations.  That way the 64-bit extent counts are merely a
variant on bulkstat v5 instead of a whole new format.

I doubt very many bulkstat users have fully gone over to V5 yet; telling
them we have a V6 already isn't going to be fun... :/

--D

> --D
> 
> >  	uint32_t	bs_aextents;	/* attribute number of extents	*/
> >  	uint16_t	bs_version;	/* structure version		*/
> >  	uint16_t	bs_forkoff;	/* inode fork offset in bytes	*/
> > @@ -853,7 +853,7 @@ struct xfs_scrub_metadata {
> >  #define XFS_IOC_FSGEOMETRY_V4	     _IOR ('X', 124, struct xfs_fsop_geom_v4)
> >  #define XFS_IOC_GOINGDOWN	     _IOR ('X', 125, uint32_t)
> >  #define XFS_IOC_FSGEOMETRY	     _IOR ('X', 126, struct xfs_fsop_geom)
> > -#define XFS_IOC_BULKSTAT	     _IOR ('X', 127, struct xfs_bulkstat_req)
> > +#define XFS_IOC_BULKSTAT_V5	     _IOR ('X', 127, struct xfs_bulkstat_req)
> >  #define XFS_IOC_INUMBERS	     _IOR ('X', 128, struct xfs_inumbers_req)
> >  /*	FIEXCHANGE_RANGE ----------- hoisted 129	 */
> >  /*	XFS_IOC_GETFSUUID ---------- deprecated 140	 */
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 2da90f81e6e3..85f45340cb16 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -715,6 +715,8 @@ xfs_fsbulkstat_one_fmt(
> >  {
> >  	struct xfs_bstat		bs1;
> >  
> > +	ASSERT(breq->version == XFS_BULKSTAT_VERSION_V1);
> > +
> >  	xfs_bulkstat_to_bstat(breq->mp, &bs1, bstat);
> >  	if (copy_to_user(breq->ubuffer, &bs1, sizeof(bs1)))
> >  		return -EFAULT;
> > @@ -728,6 +730,8 @@ xfs_fsinumbers_fmt(
> >  {
> >  	struct xfs_inogrp		ig1;
> >  
> > +	ASSERT(breq->version == XFS_INUMBERS_VERSION_V1);
> > +
> >  	xfs_inumbers_to_inogrp(&ig1, igrp);
> >  	if (copy_to_user(breq->ubuffer, &ig1, sizeof(struct xfs_inogrp)))
> >  		return -EFAULT;
> > @@ -787,14 +791,17 @@ xfs_ioc_fsbulkstat(
> >  	 */
> >  	if (cmd == XFS_IOC_FSINUMBERS) {
> >  		breq.startino = lastino ? lastino + 1 : 0;
> > +		breq.version = XFS_INUMBERS_VERSION_V1;
> >  		error = xfs_inumbers(&breq, xfs_fsinumbers_fmt);
> >  		lastino = breq.startino - 1;
> >  	} else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE) {
> >  		breq.startino = lastino;
> >  		breq.icount = 1;
> > +		breq.version = XFS_BULKSTAT_VERSION_V1;
> >  		error = xfs_bulkstat_one(&breq, xfs_fsbulkstat_one_fmt);
> >  	} else {	/* XFS_IOC_FSBULKSTAT */
> >  		breq.startino = lastino ? lastino + 1 : 0;
> > +		breq.version = XFS_BULKSTAT_VERSION_V1;
> >  		error = xfs_bulkstat(&breq, xfs_fsbulkstat_one_fmt);
> >  		lastino = breq.startino - 1;
> >  	}
> > @@ -819,6 +826,8 @@ xfs_bulkstat_fmt(
> >  	struct xfs_ibulk		*breq,
> >  	const struct xfs_bulkstat	*bstat)
> >  {
> > +	ASSERT(breq->version == XFS_BULKSTAT_VERSION_V5);
> > +
> >  	if (copy_to_user(breq->ubuffer, bstat, sizeof(struct xfs_bulkstat)))
> >  		return -EFAULT;
> >  	return xfs_ibulk_advance(breq, sizeof(struct xfs_bulkstat));
> > @@ -918,13 +927,15 @@ STATIC int
> >  xfs_ioc_bulkstat(
> >  	struct file			*file,
> >  	unsigned int			cmd,
> > -	struct xfs_bulkstat_req __user	*arg)
> > +	struct xfs_bulkstat_req __user	*arg,
> > +	int				version)
> >  {
> >  	struct xfs_mount		*mp = XFS_I(file_inode(file))->i_mount;
> >  	struct xfs_bulk_ireq		hdr;
> >  	struct xfs_ibulk		breq = {
> >  		.mp			= mp,
> >  		.mnt_userns		= file_mnt_user_ns(file),
> > +		.version		= version,
> >  	};
> >  	int				error;
> >  
> > @@ -960,6 +971,8 @@ xfs_inumbers_fmt(
> >  	struct xfs_ibulk		*breq,
> >  	const struct xfs_inumbers	*igrp)
> >  {
> > +	ASSERT(breq->version == XFS_INUMBERS_VERSION_V5);
> > +
> >  	if (copy_to_user(breq->ubuffer, igrp, sizeof(struct xfs_inumbers)))
> >  		return -EFAULT;
> >  	return xfs_ibulk_advance(breq, sizeof(struct xfs_inumbers));
> > @@ -970,11 +983,13 @@ STATIC int
> >  xfs_ioc_inumbers(
> >  	struct xfs_mount		*mp,
> >  	unsigned int			cmd,
> > -	struct xfs_inumbers_req __user	*arg)
> > +	struct xfs_inumbers_req __user	*arg,
> > +	int				version)
> >  {
> >  	struct xfs_bulk_ireq		hdr;
> >  	struct xfs_ibulk		breq = {
> >  		.mp			= mp,
> > +		.version		= version,
> >  	};
> >  	int				error;
> >  
> > @@ -1882,10 +1897,12 @@ xfs_file_ioctl(
> >  	case XFS_IOC_FSINUMBERS:
> >  		return xfs_ioc_fsbulkstat(filp, cmd, arg);
> >  
> > -	case XFS_IOC_BULKSTAT:
> > -		return xfs_ioc_bulkstat(filp, cmd, arg);
> > +	case XFS_IOC_BULKSTAT_V5:
> > +		return xfs_ioc_bulkstat(filp, cmd, arg,
> > +				XFS_BULKSTAT_VERSION_V5);
> >  	case XFS_IOC_INUMBERS:
> > -		return xfs_ioc_inumbers(mp, cmd, arg);
> > +		return xfs_ioc_inumbers(mp, cmd, arg,
> > +				XFS_INUMBERS_VERSION_V5);
> >  
> >  	case XFS_IOC_FSGEOMETRY_V1:
> >  		return xfs_ioc_fsgeometry(mp, arg, 3);
> > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> > index bef19060f4aa..d9a7abc209b5 100644
> > --- a/fs/xfs/xfs_ioctl32.c
> > +++ b/fs/xfs/xfs_ioctl32.c
> > @@ -88,6 +88,8 @@ xfs_fsinumbers_fmt_compat(
> >  	struct xfs_inogrp		ig1;
> >  	struct xfs_inogrp		*igrp = &ig1;
> >  
> > +	ASSERT(breq->version == XFS_INUMBERS_VERSION_V1);
> > +
> >  	xfs_inumbers_to_inogrp(&ig1, ig);
> >  
> >  	if (put_user(igrp->xi_startino,   &p32->xi_startino) ||
> > @@ -177,6 +179,8 @@ xfs_fsbulkstat_one_fmt_compat(
> >  	struct xfs_bstat		bs1;
> >  	struct xfs_bstat		*buffer = &bs1;
> >  
> > +	ASSERT(breq->version == XFS_BULKSTAT_VERSION_V1);
> > +
> >  	xfs_bulkstat_to_bstat(breq->mp, &bs1, bstat);
> >  
> >  	if (put_user(buffer->bs_ino,	  &p32->bs_ino)		||
> > @@ -293,15 +297,18 @@ xfs_compat_ioc_fsbulkstat(
> >  	 */
> >  	if (cmd == XFS_IOC_FSINUMBERS_32) {
> >  		breq.startino = lastino ? lastino + 1 : 0;
> > +		breq.version = XFS_INUMBERS_VERSION_V1;
> >  		error = xfs_inumbers(&breq, inumbers_func);
> >  		lastino = breq.startino - 1;
> >  	} else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE_32) {
> >  		breq.startino = lastino;
> >  		breq.icount = 1;
> > +		breq.version = XFS_BULKSTAT_VERSION_V1;
> >  		error = xfs_bulkstat_one(&breq, bs_one_func);
> >  		lastino = breq.startino;
> >  	} else if (cmd == XFS_IOC_FSBULKSTAT_32) {
> >  		breq.startino = lastino ? lastino + 1 : 0;
> > +		breq.version = XFS_BULKSTAT_VERSION_V1;
> >  		error = xfs_bulkstat(&breq, bs_one_func);
> >  		lastino = breq.startino - 1;
> >  	} else {
> > diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> > index 65c194cdea79..f0daa65e61ff 100644
> > --- a/fs/xfs/xfs_itable.c
> > +++ b/fs/xfs/xfs_itable.c
> > @@ -134,7 +134,7 @@ xfs_bulkstat_one_int(
> >  
> >  	buf->bs_xflags = xfs_ip2xflags(ip);
> >  	buf->bs_extsize_blks = ip->i_extsize;
> > -	buf->bs_extents = xfs_ifork_nextents(&ip->i_df);
> > +	buf->bs_extents32 = xfs_ifork_nextents(&ip->i_df);
> >  	xfs_bulkstat_health(ip, buf);
> >  	buf->bs_aextents = xfs_ifork_nextents(ip->i_afp);
> >  	buf->bs_forkoff = XFS_IFORK_BOFF(ip);
> > @@ -356,7 +356,7 @@ xfs_bulkstat_to_bstat(
> >  	bs1->bs_blocks = bstat->bs_blocks;
> >  	bs1->bs_xflags = bstat->bs_xflags;
> >  	bs1->bs_extsize = XFS_FSB_TO_B(mp, bstat->bs_extsize_blks);
> > -	bs1->bs_extents = bstat->bs_extents;
> > +	bs1->bs_extents = bstat->bs_extents32;
> >  	bs1->bs_gen = bstat->bs_gen;
> >  	bs1->bs_projid_lo = bstat->bs_projectid & 0xFFFF;
> >  	bs1->bs_forkoff = bstat->bs_forkoff;
> > diff --git a/fs/xfs/xfs_itable.h b/fs/xfs/xfs_itable.h
> > index f5a13f69883a..ee4d8d0461ca 100644
> > --- a/fs/xfs/xfs_itable.h
> > +++ b/fs/xfs/xfs_itable.h
> > @@ -14,6 +14,7 @@ struct xfs_ibulk {
> >  	unsigned int		icount;   /* number of elements in ubuffer */
> >  	unsigned int		ocount;   /* number of records returned */
> >  	unsigned int		flags;    /* see XFS_IBULK_FLAG_* */
> > +	int			version;  /* structure version to be returned */
> >  };
> >  
> >  /* Only iterate within the same AG as startino */
> > -- 
> > 2.30.2
> >
Dave Chinner July 27, 2021, 11:17 p.m. UTC | #3 
On Tue, Jul 27, 2021 at 04:00:02PM -0700, Darrick J. Wong wrote:
> On Tue, Jul 27, 2021 at 03:54:00PM -0700, Darrick J. Wong wrote:
> > On Mon, Jul 26, 2021 at 05:15:38PM +0530, Chandan Babu R wrote:
> > > This commit renames XFS_IOC_BULKSTAT to XFS_IOC_BULKSTAT_V5 to allow a future
> > > commit to extend bulkstat facility to support 64-bit extent counters. To this
> > > end, this commit also renames xfs_bulkstat->bs_extents field to
> > > xfs_bulkstat->bs_extents32.
> > > 
> > > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_fs.h |  4 ++--
> > >  fs/xfs/xfs_ioctl.c     | 27 ++++++++++++++++++++++-----
> > >  fs/xfs/xfs_ioctl32.c   |  7 +++++++
> > >  fs/xfs/xfs_itable.c    |  4 ++--
> > >  fs/xfs/xfs_itable.h    |  1 +
> > >  5 files changed, 34 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > > index 2594fb647384..d760a969599e 100644
> > > --- a/fs/xfs/libxfs/xfs_fs.h
> > > +++ b/fs/xfs/libxfs/xfs_fs.h
> > > @@ -394,7 +394,7 @@ struct xfs_bulkstat {
> > >  	uint32_t	bs_extsize_blks; /* extent size hint, blocks	*/
> > >  
> > >  	uint32_t	bs_nlink;	/* number of links		*/
> > > -	uint32_t	bs_extents;	/* number of extents		*/
> > > +	uint32_t	bs_extents32;	/* number of extents		*/
> > 
> > I wish I'd thought of this when we introduced bulkstat v5 so you
> > wouldn't have had to do this.
> > 
> > (I might have more to say in the bulkstat v6 patch review.)
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> Actually, I take that back, I have things to say /now/. :)
> 
> Rather than adding a whole new ioctl definition which (I haven't looked
> at the xfsprogs changes) likely creates a bunch of churn in userspace,
> what if we added a XFS_IBULK_ flag for supporting large extent counts?
> There's also quite a bit of reserved padding space in xfs_bulk_ireq, so
> perhaps we should define one of those padding u64 as a op-specific flag
> field that would be a way to pass bulkstat-specific flags to the
> relevant operations.  That way the 64-bit extent counts are merely a
> variant on bulkstat v5 instead of a whole new format.

Yup, this.

The only reason for creating a new revision of the ioctl is if we've
run out of expansion space in the existing ioctl structures to cater
for new information we want to export to userspace.

Cheers,

Dave.
Chandan Babu R July 28, 2021, 6:56 a.m. UTC | #4 
On 28 Jul 2021 at 04:47, Dave Chinner wrote:
> On Tue, Jul 27, 2021 at 04:00:02PM -0700, Darrick J. Wong wrote:
>> On Tue, Jul 27, 2021 at 03:54:00PM -0700, Darrick J. Wong wrote:
>> > On Mon, Jul 26, 2021 at 05:15:38PM +0530, Chandan Babu R wrote:
>> > > This commit renames XFS_IOC_BULKSTAT to XFS_IOC_BULKSTAT_V5 to allow a future
>> > > commit to extend bulkstat facility to support 64-bit extent counters. To this
>> > > end, this commit also renames xfs_bulkstat->bs_extents field to
>> > > xfs_bulkstat->bs_extents32.
>> > >
>> > > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
>> > > ---
>> > >  fs/xfs/libxfs/xfs_fs.h |  4 ++--
>> > >  fs/xfs/xfs_ioctl.c     | 27 ++++++++++++++++++++++-----
>> > >  fs/xfs/xfs_ioctl32.c   |  7 +++++++
>> > >  fs/xfs/xfs_itable.c    |  4 ++--
>> > >  fs/xfs/xfs_itable.h    |  1 +
>> > >  5 files changed, 34 insertions(+), 9 deletions(-)
>> > >
>> > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
>> > > index 2594fb647384..d760a969599e 100644
>> > > --- a/fs/xfs/libxfs/xfs_fs.h
>> > > +++ b/fs/xfs/libxfs/xfs_fs.h
>> > > @@ -394,7 +394,7 @@ struct xfs_bulkstat {
>> > >  	uint32_t	bs_extsize_blks; /* extent size hint, blocks	*/
>> > >
>> > >  	uint32_t	bs_nlink;	/* number of links		*/
>> > > -	uint32_t	bs_extents;	/* number of extents		*/
>> > > +	uint32_t	bs_extents32;	/* number of extents		*/
>> >
>> > I wish I'd thought of this when we introduced bulkstat v5 so you
>> > wouldn't have had to do this.
>> >
>> > (I might have more to say in the bulkstat v6 patch review.)
>> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>>
>> Actually, I take that back, I have things to say /now/. :)
>>
>> Rather than adding a whole new ioctl definition which (I haven't looked
>> at the xfsprogs changes) likely creates a bunch of churn in userspace,
>> what if we added a XFS_IBULK_ flag for supporting large extent counts?
>> There's also quite a bit of reserved padding space in xfs_bulk_ireq, so
>> perhaps we should define one of those padding u64 as a op-specific flag
>> field that would be a way to pass bulkstat-specific flags to the
>> relevant operations.  That way the 64-bit extent counts are merely a
>> variant on bulkstat v5 instead of a whole new format.
>
> Yup, this.
>
> The only reason for creating a new revision of the ioctl is if we've
> run out of expansion space in the existing ioctl structures to cater
> for new information we want to export to userspace.
>

Thanks for the suggestion. I will make the relevant changes before posting the
next version of the patchset.

--
chandan
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index 2594fb647384..d760a969599e 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -394,7 +394,7 @@  struct xfs_bulkstat {
 	uint32_t	bs_extsize_blks; /* extent size hint, blocks	*/
 
 	uint32_t	bs_nlink;	/* number of links		*/
-	uint32_t	bs_extents;	/* number of extents		*/
+	uint32_t	bs_extents32;	/* number of extents		*/
 	uint32_t	bs_aextents;	/* attribute number of extents	*/
 	uint16_t	bs_version;	/* structure version		*/
 	uint16_t	bs_forkoff;	/* inode fork offset in bytes	*/
@@ -853,7 +853,7 @@  struct xfs_scrub_metadata {
 #define XFS_IOC_FSGEOMETRY_V4	     _IOR ('X', 124, struct xfs_fsop_geom_v4)
 #define XFS_IOC_GOINGDOWN	     _IOR ('X', 125, uint32_t)
 #define XFS_IOC_FSGEOMETRY	     _IOR ('X', 126, struct xfs_fsop_geom)
-#define XFS_IOC_BULKSTAT	     _IOR ('X', 127, struct xfs_bulkstat_req)
+#define XFS_IOC_BULKSTAT_V5	     _IOR ('X', 127, struct xfs_bulkstat_req)
 #define XFS_IOC_INUMBERS	     _IOR ('X', 128, struct xfs_inumbers_req)
 /*	FIEXCHANGE_RANGE ----------- hoisted 129	 */
 /*	XFS_IOC_GETFSUUID ---------- deprecated 140	 */
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 2da90f81e6e3..85f45340cb16 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -715,6 +715,8 @@  xfs_fsbulkstat_one_fmt(
 {
 	struct xfs_bstat		bs1;
 
+	ASSERT(breq->version == XFS_BULKSTAT_VERSION_V1);
+
 	xfs_bulkstat_to_bstat(breq->mp, &bs1, bstat);
 	if (copy_to_user(breq->ubuffer, &bs1, sizeof(bs1)))
 		return -EFAULT;
@@ -728,6 +730,8 @@  xfs_fsinumbers_fmt(
 {
 	struct xfs_inogrp		ig1;
 
+	ASSERT(breq->version == XFS_INUMBERS_VERSION_V1);
+
 	xfs_inumbers_to_inogrp(&ig1, igrp);
 	if (copy_to_user(breq->ubuffer, &ig1, sizeof(struct xfs_inogrp)))
 		return -EFAULT;
@@ -787,14 +791,17 @@  xfs_ioc_fsbulkstat(
 	 */
 	if (cmd == XFS_IOC_FSINUMBERS) {
 		breq.startino = lastino ? lastino + 1 : 0;
+		breq.version = XFS_INUMBERS_VERSION_V1;
 		error = xfs_inumbers(&breq, xfs_fsinumbers_fmt);
 		lastino = breq.startino - 1;
 	} else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE) {
 		breq.startino = lastino;
 		breq.icount = 1;
+		breq.version = XFS_BULKSTAT_VERSION_V1;
 		error = xfs_bulkstat_one(&breq, xfs_fsbulkstat_one_fmt);
 	} else {	/* XFS_IOC_FSBULKSTAT */
 		breq.startino = lastino ? lastino + 1 : 0;
+		breq.version = XFS_BULKSTAT_VERSION_V1;
 		error = xfs_bulkstat(&breq, xfs_fsbulkstat_one_fmt);
 		lastino = breq.startino - 1;
 	}
@@ -819,6 +826,8 @@  xfs_bulkstat_fmt(
 	struct xfs_ibulk		*breq,
 	const struct xfs_bulkstat	*bstat)
 {
+	ASSERT(breq->version == XFS_BULKSTAT_VERSION_V5);
+
 	if (copy_to_user(breq->ubuffer, bstat, sizeof(struct xfs_bulkstat)))
 		return -EFAULT;
 	return xfs_ibulk_advance(breq, sizeof(struct xfs_bulkstat));
@@ -918,13 +927,15 @@  STATIC int
 xfs_ioc_bulkstat(
 	struct file			*file,
 	unsigned int			cmd,
-	struct xfs_bulkstat_req __user	*arg)
+	struct xfs_bulkstat_req __user	*arg,
+	int				version)
 {
 	struct xfs_mount		*mp = XFS_I(file_inode(file))->i_mount;
 	struct xfs_bulk_ireq		hdr;
 	struct xfs_ibulk		breq = {
 		.mp			= mp,
 		.mnt_userns		= file_mnt_user_ns(file),
+		.version		= version,
 	};
 	int				error;
 
@@ -960,6 +971,8 @@  xfs_inumbers_fmt(
 	struct xfs_ibulk		*breq,
 	const struct xfs_inumbers	*igrp)
 {
+	ASSERT(breq->version == XFS_INUMBERS_VERSION_V5);
+
 	if (copy_to_user(breq->ubuffer, igrp, sizeof(struct xfs_inumbers)))
 		return -EFAULT;
 	return xfs_ibulk_advance(breq, sizeof(struct xfs_inumbers));
@@ -970,11 +983,13 @@  STATIC int
 xfs_ioc_inumbers(
 	struct xfs_mount		*mp,
 	unsigned int			cmd,
-	struct xfs_inumbers_req __user	*arg)
+	struct xfs_inumbers_req __user	*arg,
+	int				version)
 {
 	struct xfs_bulk_ireq		hdr;
 	struct xfs_ibulk		breq = {
 		.mp			= mp,
+		.version		= version,
 	};
 	int				error;
 
@@ -1882,10 +1897,12 @@  xfs_file_ioctl(
 	case XFS_IOC_FSINUMBERS:
 		return xfs_ioc_fsbulkstat(filp, cmd, arg);
 
-	case XFS_IOC_BULKSTAT:
-		return xfs_ioc_bulkstat(filp, cmd, arg);
+	case XFS_IOC_BULKSTAT_V5:
+		return xfs_ioc_bulkstat(filp, cmd, arg,
+				XFS_BULKSTAT_VERSION_V5);
 	case XFS_IOC_INUMBERS:
-		return xfs_ioc_inumbers(mp, cmd, arg);
+		return xfs_ioc_inumbers(mp, cmd, arg,
+				XFS_INUMBERS_VERSION_V5);
 
 	case XFS_IOC_FSGEOMETRY_V1:
 		return xfs_ioc_fsgeometry(mp, arg, 3);
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index bef19060f4aa..d9a7abc209b5 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -88,6 +88,8 @@  xfs_fsinumbers_fmt_compat(
 	struct xfs_inogrp		ig1;
 	struct xfs_inogrp		*igrp = &ig1;
 
+	ASSERT(breq->version == XFS_INUMBERS_VERSION_V1);
+
 	xfs_inumbers_to_inogrp(&ig1, ig);
 
 	if (put_user(igrp->xi_startino,   &p32->xi_startino) ||
@@ -177,6 +179,8 @@  xfs_fsbulkstat_one_fmt_compat(
 	struct xfs_bstat		bs1;
 	struct xfs_bstat		*buffer = &bs1;
 
+	ASSERT(breq->version == XFS_BULKSTAT_VERSION_V1);
+
 	xfs_bulkstat_to_bstat(breq->mp, &bs1, bstat);
 
 	if (put_user(buffer->bs_ino,	  &p32->bs_ino)		||
@@ -293,15 +297,18 @@  xfs_compat_ioc_fsbulkstat(
 	 */
 	if (cmd == XFS_IOC_FSINUMBERS_32) {
 		breq.startino = lastino ? lastino + 1 : 0;
+		breq.version = XFS_INUMBERS_VERSION_V1;
 		error = xfs_inumbers(&breq, inumbers_func);
 		lastino = breq.startino - 1;
 	} else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE_32) {
 		breq.startino = lastino;
 		breq.icount = 1;
+		breq.version = XFS_BULKSTAT_VERSION_V1;
 		error = xfs_bulkstat_one(&breq, bs_one_func);
 		lastino = breq.startino;
 	} else if (cmd == XFS_IOC_FSBULKSTAT_32) {
 		breq.startino = lastino ? lastino + 1 : 0;
+		breq.version = XFS_BULKSTAT_VERSION_V1;
 		error = xfs_bulkstat(&breq, bs_one_func);
 		lastino = breq.startino - 1;
 	} else {
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 65c194cdea79..f0daa65e61ff 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -134,7 +134,7 @@  xfs_bulkstat_one_int(
 
 	buf->bs_xflags = xfs_ip2xflags(ip);
 	buf->bs_extsize_blks = ip->i_extsize;
-	buf->bs_extents = xfs_ifork_nextents(&ip->i_df);
+	buf->bs_extents32 = xfs_ifork_nextents(&ip->i_df);
 	xfs_bulkstat_health(ip, buf);
 	buf->bs_aextents = xfs_ifork_nextents(ip->i_afp);
 	buf->bs_forkoff = XFS_IFORK_BOFF(ip);
@@ -356,7 +356,7 @@  xfs_bulkstat_to_bstat(
 	bs1->bs_blocks = bstat->bs_blocks;
 	bs1->bs_xflags = bstat->bs_xflags;
 	bs1->bs_extsize = XFS_FSB_TO_B(mp, bstat->bs_extsize_blks);
-	bs1->bs_extents = bstat->bs_extents;
+	bs1->bs_extents = bstat->bs_extents32;
 	bs1->bs_gen = bstat->bs_gen;
 	bs1->bs_projid_lo = bstat->bs_projectid & 0xFFFF;
 	bs1->bs_forkoff = bstat->bs_forkoff;
diff --git a/fs/xfs/xfs_itable.h b/fs/xfs/xfs_itable.h
index f5a13f69883a..ee4d8d0461ca 100644
--- a/fs/xfs/xfs_itable.h
+++ b/fs/xfs/xfs_itable.h
@@ -14,6 +14,7 @@  struct xfs_ibulk {
 	unsigned int		icount;   /* number of elements in ubuffer */
 	unsigned int		ocount;   /* number of records returned */
 	unsigned int		flags;    /* see XFS_IBULK_FLAG_* */
+	int			version;  /* structure version to be returned */
 };
 
 /* Only iterate within the same AG as startino */