[Devel] [RFC PATCH vz9 v6 01/62] dm-ploop: md_pages map all pages at creation time

[Pavel Tikhomirov]

On 12/30/24 15:12, Alexander Atanasov wrote:
> On 24.12.24 12:13, Pavel Tikhomirov wrote:
>>
> 
> 
> I'll reply here to all the comments.
> 
> - kmap_atomic creates a temporary mapping on the current CPU only and we 
> want to create permanent mappings for the pages.
> - kmap_atomic is deprecated - we want to avoid it.

agreed

> - we want to use READ/WRITE_ONCE everywhere since we try to avoid taking 
> locks. may be it is a good idea to move it to a different patch

So it looks like d_bat_entries, bat_entries and bat_levels can change 
concurrently, thus we use, *_ONCE to keep them stable in one scope. OK.

> - this places are not atomic context but they are creating a non- 
> preemptive one, that's the side effect of kmap_atomic. code is used when 
> loading deltas, i.e. not performance critical

The function ploop_prepare_bat_update is always called under 
spin_lock_irqsave(&ploop->bat_lock), that is atomic context, one can not 
sleep there. So calling kmap looks invalid there.

> 
>>
>> On 12/6/24 05:55, Alexander Atanasov wrote:
>>> md_page is always present in memory. In that case
>>> md_page->page could be always be mapped and we would not need to perform
>>> kmap_atomic/kunmap_atomic during each lookup
>>>
>>> https://virtuozzo.atlassian.net/browse/VSTOR-91659
>>> Suggested-by: Denis V. Lunev <den at openvz.org>
>>> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
>>> ---
>>>   drivers/md/dm-ploop-bat.c | 21 ++++++++-------------
>>>   drivers/md/dm-ploop-cmd.c | 33 ++++++++++++---------------------
>>>   drivers/md/dm-ploop-map.c | 35 +++++++++++++----------------------
>>>   drivers/md/dm-ploop.h     | 14 +++++++-------
>>>   4 files changed, 40 insertions(+), 63 deletions(-)
>>>
>> ...
>>> @@ -265,9 +263,9 @@ static int ploop_write_zero_cluster_sync(struct 
>>> ploop *ploop,
>>>       int i;
>>>       for (i = 0; i < pio->bi_vcnt; i++) {
>>> -        data = kmap_atomic(pio->bi_io_vec[i].bv_page);
>>> +        data = kmap(pio->bi_io_vec[i].bv_page);
>>
>> This hunk seems unrelated to original md->kmpage idea and just does 
>> kmap_atomic -> kmap replacement. Should it go to separate patch? 
>> Should we prove in commit message that it is safe to call kmap here 
>> (non-atomic context)?
>>
>>>           memset(data, 0, PAGE_SIZE);
>>> -        kunmap_atomic(data);
>>> +        kunmap(data);
>>>       }
>>>       return ploop_write_cluster_sync(ploop, pio, clu);
>> ...
>>> @@ -909,9 +908,10 @@ static int ploop_prepare_bat_update(struct ploop 
>>> *ploop, struct md_page *md,
>>>       piwb->pio = pio = ploop_alloc_pio(ploop, GFP_NOIO);
>>>       if (!page || !pio)
>>>           goto err;
>>> +    piwb->kmpage = kmap(piwb->bat_page);
>>
>> Same question why it is safe to switch kmap_atomic to kmap?
>>
>>>       ploop_init_pio(ploop, REQ_OP_WRITE, pio);
>>> -    bat_entries = kmap_atomic(md->page);
>>> +    bat_entries = md->kmpage;
>>>       write_lock_irq(&ploop->bat_rwlock);
>>>       md->piwb = piwb;
>> ...
>>
>>> @@ -759,14 +754,12 @@ static void notify_delta_merged(struct ploop 
>>> *ploop, u8 level,
>>>                        * 1)next delta (which became renumbered) or
>>>                        * 2)prev delta (if !@forward).
>>>                        */
>>> -                     bat_entries[i] = d_bat_entries[i];
>>> +                     WRITE_ONCE(bat_entries[i], 
>>> READ_ONCE(d_bat_entries[i]));
>>>                       if (!forward)
>>>                               md->bat_levels[i] = level - 1;
>>>                       else
>>>                               md->bat_levels[i] = level;
>>>               }
>>> -             kunmap_atomic(bat_entries);
>>> -             kunmap_atomic(d_bat_entries);
>>>               if (stop)
>>>                       break;
>>>               d_md = ploop_md_next_entry(d_md);
>> ...
>>> @@ -463,11 +464,10 @@ static inline bool 
>>> ploop_md_page_cluster_is_in_top_delta(struct ploop *ploop,
>>>           return false;
>>>       }
>>> -    bat_entries = kmap_atomic(md->page);
>>> -    if (bat_entries[clu] == BAT_ENTRY_NONE ||
>>> -        md->bat_levels[clu] < ploop_top_level(ploop))
>>> +    bat_entries = md->kmpage;
>>> +    if (READ_ONCE(bat_entries[clu]) == BAT_ENTRY_NONE ||
>>> +        READ_ONCE(md->bat_levels[clu]) < ploop_top_level(ploop))
>>
>> Should we explain in commit message why we need to use READ_ONCE/ 
>> WRITE_ONCE above?
>>
>>>           ret = false;
>>> -    kunmap_atomic(bat_entries);
>>>       return ret;
>>>   }
>>
> 

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