[Devel] [PATCH vz9 v1 55/63] dm-ploop: rework bat completion logic

[Pavel Tikhomirov]

On 1/24/25 23:36, Alexander Atanasov wrote:
> @@ -915,28 +918,31 @@ static void ploop_free_piwb(struct ploop_index_wb *piwb)
>   	kfree(piwb);
>   }
>   
> +
> +static void ploop_bat_write_finish(struct pio *pio, void *piwb_ptr,
> +				     blk_status_t bi_status);
>   static void ploop_put_piwb(struct ploop_index_wb *piwb)
>   {
>   	if (atomic_dec_and_test(&piwb->count)) {
> -		struct ploop *ploop = piwb->ploop;
> -		/*
> -		 * Index wb failed. Mark clusters as unallocated again.
> -		 * piwb->count is zero, so all data writers compeleted.
> -		 */
> -		if (piwb->bi_status)
> -			ploop_advance_local_after_bat_wb(ploop, piwb, false);
> +
> +		ploop_bat_write_finish(piwb->pio, piwb, piwb->bi_status);

It looks like piwb->bi_status can be read directly from piwb inside 
ploop_bat_write_finish so we can ommit the third argument.

>   
>   		if (piwb->comp) {
>   			if (piwb->comp_bi_status)
>   				*piwb->comp_bi_status = piwb->bi_status;
>   			complete(piwb->comp);
>   		}
> +		/*
> +		 * Status is set from first call to ploop_bat_write_complete
> +		 * zero keeps it as is
> +		 */
> +
>   		ploop_free_piwb(piwb);
>   	}
>   }
>   
>   /* This handler is called after BAT is updated. */
> -static void ploop_bat_write_complete(struct pio *pio, void *piwb_ptr,
> +static void ploop_bat_write_finish(struct pio *pio, void *piwb_ptr,
>   				     blk_status_t bi_status)
>   {
>   	struct ploop_index_wb *piwb = piwb_ptr;
> @@ -950,30 +956,30 @@ static void ploop_bat_write_complete(struct pio *pio, void *piwb_ptr,
>   	struct llist_node *pos, *t;
>   	struct llist_node *ll_cow_pios;
>   	struct llist_node *ll_ready_pios;
> -	int completed = atomic_read(&piwb->count) == 1;
> -
> -	if (completed) {
> -		/* We are the last count so it is safe to advance bat */
> -		if (!bi_status) {
> -			/*
> -			 * Success: now update local BAT copy. We could do this
> -			 * from our delayed work, but we want to publish new
> -			 * mapping in the fastest way. This must be done before
> -			 * data bios completion, since right after we complete
> -			 * a bio, subsequent read wants to see written data
> -			 * (ploop_map() wants to see not zero bat_entries[.]).
> -			 */
> -			ploop_advance_local_after_bat_wb(ploop, piwb, true);
> -		}
> +
> +	if (!bi_status) {
> +		/*
> +		 * Success: now update local BAT copy. We could do this
> +		 * from our delayed work, but we want to publish new
> +		 * mapping in the fastest way. This must be done before
> +		 * data bios completion, since right after we complete
> +		 * a bio, subsequent read wants to see written data
> +		 * (ploop_map() wants to see not zero bat_entries[.]).
> +		 */
> +		ploop_advance_local_after_bat_wb(ploop, piwb, true);
> +	} else {
> +		/*
> +		 * Index wb failed. Mark clusters as unallocated again.
> +		 * piwb->count is zero, so all data writers compeleted.
> +		 */
> +		ploop_advance_local_after_bat_wb(ploop, piwb, false);
>   	}
>   
>   	spin_lock_irqsave(&piwb->lock, flags);
> -	if (completed)
> -		piwb->completed = completed;
> -	piwb->bi_status = bi_status;
> -	ll_ready_pios = llist_reverse_order(llist_del_all(&piwb->llready_data_pios));
> +	ll_ready_pios = llist_del_all(&piwb->llready_data_pios);
> +	if (bi_status)
> +		piwb->bi_status = bi_status;

The function ploop_bat_write_finish is always called with bi_status 
argument set to piwb->bi_status, so this setting bi_status back to 
piwb->bi_status seems excess.

>   	spin_unlock_irqrestore(&piwb->lock, flags);
> -
>   	ll_cow_pios = llist_reverse_order(llist_del_all(&piwb->cow_llist));
>   
>   	/*
...
> @@ -1000,11 +1006,17 @@ static void ploop_bat_write_complete(struct pio *pio, void *piwb_ptr,
>   			ploop_dispatch_pios(ploop, flush_pio, NULL);
>   		piwb->flush_pio = NULL;
>   	}
> +}
> +
> +static void ploop_bat_write_complete(struct pio *pio, void *piwb_ptr,
> +				     blk_status_t bi_status)
> +
> +{
> +	struct ploop_index_wb *piwb = piwb_ptr;
> +
> +	if (bi_status)
> +		piwb->bi_status = bi_status;

As we've already set it here beforehand.

>   
> -	/*
> -	 * In case of update BAT is failed, dst_clusters will be
> -	 * set back to holes_bitmap on last put_piwb().
> -	 */
>   	ploop_put_piwb(piwb);
>   }
>