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.
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.
Hence returning -EOVERFLOW because the extent count is greater than
what can be held in the struct bstat will stop those programs from
running properly to completion.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
