[Devel] [RFC PATCH vz9 v6 19/62] dm-ploop: simplify llseek

[Alexander Atanasov]

On 20.01.25 13:35, Pavel Tikhomirov wrote:
> 
> 
> On 1/20/25 19:21, Alexander Atanasov wrote:
>> On 20.01.25 6:06, Pavel Tikhomirov wrote:
>>>
>>>
>>> On 12/6/24 05:55, Alexander Atanasov wrote:
>>>> From: Andrey Zhadchenko <andrey.zhadchenko at virtuozzo.com>
>>>>
>>>> Do not bother with locked clusters. For llseek we can ignore all
>>>> concurrent operations.
>>>> Do not call ploop_bat_entries() on every cluster as this is slow.
>>>> Just remember md and read it directly.
>>>>
>>>> https://virtuozzo.atlassian.net/browse/VSTOR-91817
>>>> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko at virtuozzo.com>
>>>> ---
>>>>   drivers/md/dm-ploop-map.c | 27 ++++++++-------------------
>>>>   1 file changed, 8 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
>>>> index 7ecf79fcd2cc..ad7ca7d43dfc 100644
>>>> --- a/drivers/md/dm-ploop-map.c
>>>> +++ b/drivers/md/dm-ploop-map.c
>>>> @@ -2088,32 +2088,21 @@ 
>>>> ALLOW_ERROR_INJECTION(ploop_prepare_reloc_index_wb, ERRNO);
>>>>   loff_t ploop_llseek_hole(struct dm_target *ti, loff_t offset, int 
>>>> whence)
>>>>   {
>>>>       struct ploop *ploop = ti->private;
>>>> -    u32 clu, dst_clu;
>>>> +    u32 clu, dst_clu, id, *bat;
>>>> +    struct md_page *md;
>>>>       loff_t result;
>>>>       clu = SEC_TO_CLU(ploop, to_sector(offset) + ploop->skip_off);
>>>> +    id = U32_MAX;
>>>>       while (clu < ploop->nr_bat_entries) {
>>>> -
>>>> -        /*
>>>> -         * Assume a locked cluster to have a data.
>>>> -         * In worst case someone will read zeroes
>>>> -         */
>>>> -        if (ploop_postpone_if_cluster_locked(ploop, NULL, clu)) {
>>>> -            if (whence & SEEK_DATA)
>>>> -                break;
>>>> -            if (whence & SEEK_HOLE) {
>>>> -                clu++;
>>>> -                continue;
>>>> -            }
>>>> +        if (id != ploop_bat_clu_to_page_nr(clu)) {
>>>> +            id = ploop_bat_clu_to_page_nr(clu);
>>>> +            md = ploop_md_page_find(ploop, id);
>>>
>>> We can probably put BUG_ON(!md); here same as in ploop_bat_entries.
>>
>> I think it is better to have :
>>
>>              if (!md)
>>                  return -EINVAL;
>>
>>
>> this is a system call so we can return error instead of crashing - 
>> BUG_ON use is discouraged.
>>
>> EINVAL is listed in llseek man page.
> 
> We probably have BUG_ON here because this situation was totally 
> unexpected here and might indicate e.g. memory corruption, and in case 
> of memory corruption crashing the system is probably OK instead of 
> gracefully returning the error for syscall and letting it degrade further.
> 
> But I'm not sure about the intent, I just prefer to leave the bugon 
> there it was.

It's dangerous to have BUG_ON here, userspace may craft a bad request to 
crash the system. If we have memory corruption it will be cought on next 
ploop_bat_entries which have a BUG_ON.

> 
>>
>>
>>>
>>>>           }
>>>> -        /*
>>>> -         * It *may* be possible to speed this up by iterating over 
>>>> clusters
>>>> -         * in mapped metadata page, but this is a bit problematic
>>>> -         * since we want to check if cluster is locked
>>>> -         */
>>>> -        dst_clu = ploop_bat_entries(ploop, clu, NULL, NULL);
>>>> +        bat = md->kmpage;
>>>> +        dst_clu = READ_ONCE(bat[ploop_bat_clu_idx_in_page(clu)]);
>>>>           if (whence & SEEK_DATA && dst_clu != BAT_ENTRY_NONE)
>>>>               break;
>>>
>>
> 

-- 
Regards,
Alexander Atanasov