On 9/14/16 4:26 PM, Dave Chinner wrote:
> Bad wrap case:
>
> flfirst = 10 (invalid!)
> fllast = 0
> flcount = 2
> agfl_size = 10
> active = 0 - 10 + 1
> = -9 + agfl_size
> = 1 (correct as flfirst is invalid!)
Ok, I guess maybe I got confused w.r.t. the actual accounting vs.
the expected/correct accounting.
I saw "active = 1" and thought no, that's wrong, there are 2.
But active is what it /should/ be not what it /is/ ?
More comments would help I suppose, around either the first assignment
or the variable declaration, for this and for flcount.
(I guess flcount is actual number on the list, active is what
it /should/ be?)
> if (flfirst > agfl_size) not true
> if (flfirst == agfl_size) true, warn
>
> if (fllast > agfl_size) not true
> if (fllast == agfl_size) not true
>
> if (flcount > agfl_size + 1) not true
> if (flcount == agfl_size) not true
>
> if (active <= 0) not true
> if (flcount > active + 1) not true
> if (flcount < active) not true
> if (flcount != active) true, warn
>
> So, two correction needed warnings, which is correct because both
> flfirst and flcount need fixing due to being off by one.
ok
> The bad fllast case:
>
> flfirst = 5
> fllast = 10 (invalid!)
> flcount = 6
> agfl_size = 10
> active = fllast - flfirst + 1
> = 10 - 5 + 1
> = 6 (invalid as fllast is out of range)
>
> if (flfirst > agfl_size) not true
> if (flfirst == agfl_size) not true
>
> if (fllast > agfl_size) not true
> if (fllast == agfl_size) true, warn
>
> if (flcount > agfl_size + 1) not true
> if (flcount == agfl_size) not true
>
> if (active <= 0) not true
> if (flcount > active + 1) not true
> if (flcount < active) not true
> if (flcount != active) not true
>
> So we've got a warning that a fllast correction will take place,
> which will also correct the flcount once the fllast index is
> updated.
>
> But I think the case you are concerned about is a corner case of
> this one - a full AGFL, right? Something that, in practice, will
> never happen as no single transaction will free or require a full
> AGFL worth of blocks in a single allocation/free operation.
Actually no, I hadn't caught that one ;)
> As such, I didn't actually consider it or test that case, so, it may
> be wrong. Let's run the numbers:
>
> flfirst = 0
> fllast = 10 (invalid!)
> flcount = 11 (invalid!)
> agfl_size = 10
> active = fllast - flfirst + 1
> = 10 - 0 + 1
> = 11 (invalid as fllast is out of range)
>
> if (flfirst > agfl_size) not true
> if (flfirst == agfl_size) not true
>
> if (fllast > agfl_size) not true
> if (fllast == agfl_size) true, warn
>
> if (flcount > agfl_size + 1) not true
> if (flcount == agfl_size) not true
>
> if (active <= 0) not true
> if (flcount > active + 1) not true
> if (flcount < active) not true
> if (flcount != active) not true
>
> So it issues the same fllast warning as the not full, not wrapped
> case, and that correction also fixes up the flcount. Maybe it's the
> full wrapped case, which may be a pure flcount error?
>
> flfirst = 5
> fllast = 4
> flcount = 11 (invalid!)
> agfl_size = 10
> active = fllast - flfirst + 1
> = 4 - 5 + 1
> = 10 (different to flcount!)
>
> if (flfirst > agfl_size) not true
> if (flfirst == agfl_size) not true
>
> if (fllast > agfl_size) not true
> if (fllast == agfl_size) not true
>
> if (flcount > agfl_size + 1) not true
> if (flcount == agfl_size) not true
> (yes, that should warn)
>
> if (active <= 0) not true
> if (flcount > active + 1) not true
> if (flcount < active) not true
> if (flcount != active) true, warn
>
> So we do get a warning about the flcount being wrong because it's
> only one block different to the calculated active size (and hence it
> will be corrected), but no warning from the flcount being out of
> range by one block. So, yes, I think you are right, Eric, that the
> check should be against agfl_size + 1, even it it means we get
> multiple warnings...
Ah, ok :) So this is essentially:
- if (flcount == agfl_size)
+ if (flcount == agfl_size + 1)
xfs_warn(mp, "AGF %u: freelist count needs correction(1)",
be32_to_cpu(agf->agf_seqno));
for your patch, right. Glad we agree. :)
-Eric
> Thanks for thinking about this case!
>
> Cheers,
>
> Dave.
>
|