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

[Pavel Tikhomirov]

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 *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;
>>
> 

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.