Re: [PATCH V5 14/16] xfs: Enable bulkstat ioctl to support 64-bit per-inode extent counters

[Chandan Babu R <chandan.babu@oracle.com>]

On 07 Feb 2022 at 10:26, Chandan Babu R wrote:
> On 02 Feb 2022 at 00:54, Darrick J. Wong wrote:
>> On Fri, Jan 21, 2022 at 10:48:55AM +0530, Chandan Babu R wrote:
>>> The following changes are made to enable userspace to obtain 64-bit extent
>>> counters,
>>> 1. Carve out a new 64-bit field xfs_bulkstat->bs_extents64 from
>>>    xfs_bulkstat->bs_pad[] to hold 64-bit extent counter.
>>> 2. Define the new flag XFS_BULK_IREQ_BULKSTAT for userspace to indicate that
>>>    it is capable of receiving 64-bit extent counters.
>>> 
>>> Suggested-by: Darrick J. Wong <djwong@kernel.org>
>>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>>> ---
>>>  fs/xfs/libxfs/xfs_fs.h | 12 ++++++++----
>>>  fs/xfs/xfs_ioctl.c     |  3 +++
>>>  fs/xfs/xfs_itable.c    | 27 +++++++++++++++++++++++++--
>>>  fs/xfs/xfs_itable.h    |  7 ++++++-
>>>  fs/xfs/xfs_iwalk.h     |  7 +++++--
>>>  5 files changed, 47 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
>>> index 42bc39501d81..4e12530eb518 100644
>>> --- a/fs/xfs/libxfs/xfs_fs.h
>>> +++ b/fs/xfs/libxfs/xfs_fs.h
>>> @@ -393,7 +393,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_extents;	/* 32-bit data fork extent counter */
>>>  	uint32_t	bs_aextents;	/* attribute number of extents	*/
>>>  	uint16_t	bs_version;	/* structure version		*/
>>>  	uint16_t	bs_forkoff;	/* inode fork offset in bytes	*/
>>> @@ -402,8 +402,9 @@ struct xfs_bulkstat {
>>>  	uint16_t	bs_checked;	/* checked inode metadata	*/
>>>  	uint16_t	bs_mode;	/* type and mode		*/
>>>  	uint16_t	bs_pad2;	/* zeroed			*/
>>> +	uint64_t	bs_extents64;	/* 64-bit data fork extent counter */
>>>  
>>> -	uint64_t	bs_pad[7];	/* zeroed			*/
>>> +	uint64_t	bs_pad[6];	/* zeroed			*/
>>>  };
>>>  
>>>  #define XFS_BULKSTAT_VERSION_V1	(1)
>>> @@ -484,8 +485,11 @@ struct xfs_bulk_ireq {
>>>   */
>>>  #define XFS_BULK_IREQ_SPECIAL	(1 << 1)
>>>  
>>> -#define XFS_BULK_IREQ_FLAGS_ALL	(XFS_BULK_IREQ_AGNO | \
>>> -				 XFS_BULK_IREQ_SPECIAL)
>>> +#define XFS_BULK_IREQ_NREXT64	(1 << 2)
>>
>> This needs a comment specifying the behavior of this flag.
>>
>> If the flag is set and the data fork extent count fits in both fields,
>> will they both be filled out?
>
> If the flag is set, xfs_bulkstat->bs_extents64 field will be assigned the data
> fork extent count and xfs_bulkstat->bs_extents will be set to 0
> (xfs_bulkstat() allocates xfs_bstat_chunk->buf by invoking kmem_zalloc()).
>
> If the flag is not set, xfs_bulkstat->bs_extents field will be assigned the
> data fork extent count and xfs_bulkstat->bs_extents64 will be set to 0.
>
>>
>> If the flag is set but the data fork extent count only fits in
>> bs_extents64, what will be written to bs_extents?
>
> bs_extents will be set to zero.
>
>>
>> If the flag is not set and the data fork extent count won't fit in
>> bs_extents, do we return an error value?  Fill it with garbage?
>>
>
> In this case, we return -EOVERFLOW and the contents of bs_extents will be set
> to zero. This happens because xfs_bulkstat() will return success even if
> xfs_iwalk() returned an error provided that we already have collected details
> about one more inodes. The next call to xfs_ioc_bulkstat() will start from the
> problematic inode. Here we allocate xfs_bstat_chunk->buf using kmem_zalloc()
> which zeroes the contents of the allocated memory and returns -EOVERFLOW to
> userspace.
>

Sorry, I forgot to mention that I will add the above data points as a comment
before posting the next version of the patchset.

-- 
chandan