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.
>
> 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.
>
> 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.
>
--
chandan
