Re: [PATCH V7 15/17] xfs: Enable bulkstat ioctl to support 64-bit per-inode extent counters

[Dave Chinner <david@fromorbit.com>]

On Mon, Mar 07, 2022 at 07:16:57PM +0530, Chandan Babu R wrote:
> On 07 Mar 2022 at 10:43, Dave Chinner wrote:
> > On Sat, Mar 05, 2022 at 06:15:37PM +0530, Chandan Babu R wrote:
> >> On 04 Mar 2022 at 13:39, Dave Chinner wrote:
> >> > On Tue, Mar 01, 2022 at 04:09:36PM +0530, Chandan Babu R wrote:
> >> >> @@ -102,7 +104,27 @@ 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);
> >> >> +
> >> >> +	nextents = xfs_ifork_nextents(&ip->i_df);
> >> >> +	if (!(bc->breq->flags & XFS_IBULK_NREXT64)) {
> >> >> +		xfs_extnum_t	max_nextents = XFS_MAX_EXTCNT_DATA_FORK_OLD;
> >> >> +
> >> >> +		if (unlikely(XFS_TEST_ERROR(false, mp,
> >> >> +				XFS_ERRTAG_REDUCE_MAX_IEXTENTS)))
> >> >> +			max_nextents = 10;
> >> >> +
> >> >> +		if (nextents > max_nextents) {
> >> >> +			xfs_iunlock(ip, XFS_ILOCK_SHARED);
> >> >> +			xfs_irele(ip);
> >> >> +			error = -EOVERFLOW;
> >> >> +			goto out;
> >> >> +		}
> >> >
> >> > This just seems wrong. This will cause a total abort of the bulkstat
> >> > pass which will just be completely unexpected by any application
> >> > taht does not know about 64 bit extent counts. Most of them likely
> >> > don't even care about the extent count in the data being returned.
> >> >
> >> > Really, I think this should just set the extent count to the MAX
> >> > number and just continue onwards, otherwise existing application
> >> > will not be able to bulkstat a filesystem with large extents counts
> >> > in it at all.
> >> >
> >> 
> >> Actually, I don't know much about how applications use bulkstat. I am
> >> dependent on guidance from other developers who are well versed on this
> >> topic. I will change the code to return maximum extent count if the value
> >> overflows older extent count limits.
> >
> > They tend to just run in a loop until either no more inodes are to
> > be found or an error occurs. bulkstat loops don't expect errors to
> > be reported - it's hard to do something based on all inodes if you
> > get errors reading then inodes part way through. There's no way for
> > the application to tell where it should restart scanning - the
> > bulkstat iteration cookie is controlled by the kernel, and I don't
> > think we update it on error.
> 
> xfs_bulkstat() has the following,
> 
>         kmem_free(bc.buf);
> 
>         /*
>          * We found some inodes, so clear the error status and return them.
>          * The lastino pointer will point directly at the inode that triggered
>          * any error that occurred, so on the next call the error will be
>          * triggered again and propagated to userspace as there will be no
>          * formatted inodes in the buffer.
>          */
>         if (breq->ocount > 0)
>                 error = 0;
> 
>         return error;
> 
> The above will help the userspace process to issue another bulkstat call which
> beging from the inode causing an error.

ANd then it returns with a cookie pointing at the overflowed inode,
and we try that one first on the next loop, triggering -EOVERFLOW
with breq->ocount == 0.

Or maybe we have two inodes in a row that trigger EOVERFLOW, so even
if we skip the first and return to userspace, we trip the second on
the next call and boom...

> > e.g. see fstests src/bstat.c and src/bulkstat_unlink_test*.c - they
> > simply abort if bulkstat fails. Same goes for xfsdump common/util.c
> > and dump/content.c - they just error out and return and don't try to
> > continue further.
> 
> I made the following changes to src/bstat.c,
> 
> diff --git a/src/bstat.c b/src/bstat.c
> index 3f3dc2c6..0e72190e 100644
> --- a/src/bstat.c
> +++ b/src/bstat.c
> @@ -143,7 +143,19 @@ main(int argc, char **argv)
>  	bulkreq.ubuffer = t;
>  	bulkreq.ocount  = &count;
>  
> -	while ((ret = xfsctl(name, fsfd, XFS_IOC_FSBULKSTAT, &bulkreq)) == 0) {
> +	while (1) {
> +		ret = xfsctl(name, fsfd, XFS_IOC_FSBULKSTAT, &bulkreq);
> +		if (ret == -1) {
> +			if (errno == EOVERFLOW) {
> +				printf("Skipping inode %llu.\n",  last+1);
> +				++last;
> +				continue;
> +			}
> +
> +			perror("xfsctl");
> +			exit(1);
> +		}
> +
>  		total += count;
>  
> 
> Executing the script at
> https://gist.github.com/chandanr/f2d147fa20a681e1508e182b5b7cdb00 provides the
> following output,
> 
> ...
> 
> ino 128 mode 040755 nlink 3 uid 0 gid 0 rdev 0
> blksize 4096 size 37 blocks 0 xflags 0 extsize 0
> atime Thu Jan  1 00:00:00.000000000 1970
> mtime Mon Mar  7 13:06:30.051339892 2022
> ctime Mon Mar  7 13:06:30.051339892 2022
> extents 0 0 gen 0
> DMI: event mask 0x00000000 state 0x0000
> 
> Skipping inode 131.
> 
> ino 132 mode 040755 nlink 2 uid 0 gid 0 rdev 0
> blksize 4096 size 97 blocks 0 xflags 0 extsize 0
> atime Mon Mar  7 13:06:30.051339892 2022
> mtime Mon Mar  7 13:06:30.083339892 2022
> ctime Mon Mar  7 13:06:30.083339892 2022
> extents 0 0 gen 548703887
> DMI: event mask 0x00000000 state 0x0000
> 
> ...
> 
> The above illustrates that userspace programs can be modified to use lastip to
> skip inodes which cause bulkstat ioctl to return with an error.

Yes, I know they can be modified to handle it - that is not the
concern here. The concern is that this new error can potentially
break the *unmodified* applications already out there. e.g. xfsdump
may just stop dumping a filesystem half way through because it
doesn't handle unexpected errors like this sanely. But we can't tie
a version of xfsdump to a specific kernel feature, so we have to
make sure that buklstat from older builds of xfsdump will still
iterate through the entire filesystem without explicit EOVERFLOW
support...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com