On Mon, Sep 12, 2022 at 02:39:23PM +0800, Stephen Zhang wrote: > 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) Er... what problem does this solve? Is there a defect in this range checking code? Why not leave it alone, since that's less retesting for all of us. --D > break; > ==== > > Thanks, > > Stephen.
