Re: [PATCH v2 11/11] md/md-llbitmap: introduce new lockless bitmap

[Yu Kuai <yukuai1@huaweicloud.com>]

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