[Devel] [RFC PATCH vz9 v6 44/62] dm-ploop: introduce pio runner threads

[Alexander Atanasov]

On 21.01.25 9:15, Pavel Tikhomirov wrote:
> 
> 
> On 1/21/25 01:34, Alexander Atanasov wrote:
>> On 20.01.25 12:08, Pavel Tikhomirov wrote:
>>>
>>>
>>> On 12/6/24 05:56, Alexander Atanasov wrote:
>>>> +static inline int ploop_runners_add_work(struct ploop *ploop, 
>>>> struct pio *pio)
>>>> +{
>>>> +    int i;
>>>> +    struct ploop_worker *wrkr;
>>>> +
>>>> +    if (++ploop->last_used_runner >= ploop->nkt_runners)
>>>> +        ploop->last_used_runner = 0;
>>>> +    wrkr = ploop->kt_runners[ploop->last_used_runner];
>>>
>>> This can potentially lead to out of bound read, as we don't have 
>>> locks around wrapping last_used_runner increment to zero (i.e. it's 
>>> not atomic), here we can see ploop->last_used_runner >= nkt_runners.
>>
>> Reworked - runners are circular list - READ_ONCE/WRITE_ONCE used to 
>> switch to ->next /see updated patches/
> 
> In updated patches:
> 
> +static inline int ploop_runners_add_work(struct ploop *ploop, struct 
> pio *pio)
> +{
> +       struct ploop_worker *wrkr;
> +
> +       wrkr = READ_ONCE(ploop->last_used_runner)->next; // A
> +       WRITE_ONCE(ploop->last_used_runner, wrkr); // B
> 
> No out of bound now. But note that as we have two operations thus it's 
> not atomic, and thus this list iteration can go "backwards" sometimes:
> 
> A1 -> A2 -> B2 -> A3 -> B3 -> ... AN -> BN -> B1


Yes, we may send two requests to same runner or skip but this does not 
matter - we do not require strict scheduling here.

> 
> 
>>
>>
>>>
>>> Note: I see (in final version, with all patches applied) multiple 
>>> stacks which probably can run concurrently:
>>>
>>>    +-< ploop_runners_add_work
>>>      +-< ploop_index_wb_submit
>>>      | +-< ploop_grow_relocate_cluster
>>>      | | +-< ploop_process_resize_cmd
>>>      | | | +-< ploop_resize
>>>      | | | | +-< ploop_message
>>>      | +-< ploop_grow_update_header
>>>      | | +-< ploop_process_resize_cmd
>>>      | | | +-< ploop_resize
>>>      | | | | +-< ploop_message
>>>      | +-< ploop_submit_metadata_writeback
>>>      | | +-< do_ploop_run_work
>>>      | | | +-< do_ploop_work
>>>      | | | +-< ploop_worker
>>>      +-< process_ploop_fsync_work
>>>      | +-< do_ploop_run_work
>>>      | | +-< do_ploop_work
>>>      | | +-< ploop_worker
>>>      +-< ploop_runners_add_work_list
>>>      | +-< do_ploop_run_work
>>>      | | +-< do_ploop_work
>>>      | | +-< ploop_worker
>>>
>>>> +
>>>> +    atomic_inc(&ploop->kt_worker->inflight_pios);
>>>> +    llist_add((struct llist_node *)(&pio->list), &wrkr->work_llist);
>>>> +    wake_up_process(wrkr->task);
>>>> +
>>>> +    return 0;
>>>> +}
>>>
>>
> 

-- 
Regards,
Alexander Atanasov