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

[Pavel Tikhomirov]

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.