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

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: Chandan Babu R <chandanrlinux@gmail.com>, linux-xfs@vger.kernel.org
Subject: Re: [PATCH V2 09/12] xfs: Rename XFS_IOC_BULKSTAT to XFS_IOC_BULKSTAT_V5
Date: Wed, 28 Jul 2021 09:17:13 +1000	[thread overview]
Message-ID: <20210727231713.GX664593@dread.disaster.area> (raw)
In-Reply-To: <20210727230002.GT559212@magnolia>

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.
-- 
Dave Chinner
david@fromorbit.com