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. > _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
