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

From: Chandan Babu R <chandanrlinux@gmail.com>
To: Dave Chinner <david@fromorbit.com>
Cc: "Darrick J. Wong" <djwong@kernel.org>, 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 12:26:41 +0530	[thread overview]
Message-ID: <875ywvvsd2.fsf@garuda> (raw)
In-Reply-To: <20210727231713.GX664593@dread.disaster.area>

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