Dave Chinner <david@xxxxxxxxxxxxx> 于2022年9月12日周一 07:12写道：
> Given that all the types and comparisons involved are 64 bit
> unsigned:
>
> typedef uint64_t xfs_fileoff_t; /* block number in a file */
>
> #define XFS_FILEOFF_MAX(a,b) max_t(xfs_fileoff_t, (a), (b))
>
> xfs_fileoff_t br_startoff;
>
> xfs_fileoff_t lastaddr = 0;
> xfs_fileoff_t lowest, max;
>
> We end up with the following calculations (in FSBs, not bytes):
>
> lowest + len = 0x800000ULL + 1
> = 0x800001ULL
>
> got.br_startoff - max = 0ULL - 0x800000
> = 0xffffffffff800000ULL
>
> and so the existing check is:
>
> if (0 >= 0x800001ULL && 0xffffffffff800000 >= 1)
>
> which evaluates as false because the extent that was found is not
> beyond the initial offset (first_unused) that we need to start
> searching at.
>
> With your modification, this would now evaluate as:
>
> if (0xffffffffff800000 >= 1)
>
> Because of the underflow, this would then evaluate as true and we'd
> return 0 as the first unused offset. This is incorrect as we do not
> have a hole at offset 0, nor is it within the correct directory
> offset segment, nor is it within the search bounds we have
> specified.
>
> If these were all signed types, then your proposed code might be
> correct. But they are unsigned and hence we have to ensure that we
> handle overflow/underflow appropriately.
>
> Which leads me to ask: did you test this change before you send
> it to the list?
>
I am so sorry about the mistake, and thanks for your elaboration about
this problem. it indeed teaches me a lesson about the necessity of test
even for the simplest change.
By the way, theoretically, in order to solve this, I wonder if we could
change the code in the following way:
====
xfs_bmap_first_unused(
/*
* See if the hole before this extent will work.
*/
- if (got.br_startoff >= lowest + len &&
- got.br_startoff - max >= len)
+ if (got.br_startoff >= max + len)
break;
====
Thanks,
Stephen.
