Hi, Hannes!
在 2025/07/08 15:47, Hannes Reinecke 写道:
[...]
+ case BitUnwritten:
+ pctl->state[pos] = level_456 ? BitNeedSync : BitDirty;
This really looks as if we should use WRITE_ONCE() ...
Sorry for the late reply, we're just writing one byte here, either old
value or new value with be read concurrently, I think we don't need
WRITE_ONCE() here, and READ_ONCE() on the reader side to prevent reading
strange value.
[...]
+ if (!test_bit(LLPageDirty, &pctl->flags))
+ set_bit(LLPageDirty, &pctl->flags);
+
test_and_set_bit?
We don't need to guarantee atomicity here, so perhaps test_bit() and
set_bit() will have less overhead?
[...]
+ for (pos = bit * io_size; pos < (bit + 1) * io_size; pos++) {
+ if (pos == offset)
+ continue;
+ if (pctl->state[pos] == BitDirty ||
+ pctl->state[pos] == BitNeedSync) {
+ llbitmap_infect_dirty_bits(llbitmap, pctl, bit, offset);
+ return;
Hmm. That looks _so_ inefficient. A loop within a loop... Wouldn't it be
possible to use XOR or something to flip several bits at once?
This is a good question. I was thinking this will only be executed once
for every daemon_sleep seconds, and the additional overhead is fine, and
perf results do confirm that.
For XOR, we'll have to convert the enum type llbitmap_stage to one state
per bit, and there are total 7 states already and not good for future
expansion.
[...]
>> +static void llbitmap_pending_timer_fn(struct timer_list *t)
>> +{
>> + struct llbitmap *llbitmap = from_timer(llbitmap, t, pending_timer);
>> +
>> + if (work_busy(&llbitmap->daemon_work)) {
>> + pr_warn("daemon_work not finished\n");
>> + set_bit(BITMAP_DAEMON_BUSY, &llbitmap->flags);
>> + return;
>> + }
>> +
>
> Do you really need to check this?
> Wouldn't it be easier to just run the daemon, which should devolve in a
> no-op if no work is to be done?
This is the same reason as below, if the daemon get stuck and doesn't
finish in the last daemon_sleep seconds, just print a warning message
for now, and the BITMAP_DAEMON_BUSY is used for debuging perpose.
[...]
+ llbitmap_suspend(llbitmap, idx);
+ llbitmap_state_machine(llbitmap, start, end,
BitmapActionDaemon);
How do you ensure that the daemon doesn't get stuck trying to write/read
individual pages? Shouldn't there be some sort of 'emergency exit'?
Perhaps llbitmap_suspend_timeout() and give up clearing dirty bits if
failed? If inflight writes get stuck, suspend will forbit all new
writers to be issued.
Thanks,
Kuai
