[dm-devel] Re: [PATCH] dm snapshot: stop merging using a completion

[Mikulas Patocka]

On Sat, 5 Dec 2009, Mike Snitzer wrote:

> Switch stop_merge() from using a busy loop to a completion event.
> 
> stop_merge() now requests merging be shutdown using the
> 'merge_completion' pointer (instead of the 'merge_shutdown' flag).  This
> is accomplished by testing if 'merge_completion' is not NULL in
> snapshot_merge_process().  stop_merge() allocates its completion on the
> stack and assigns it to the 'merge_completion' pointer in the snapshot.
> 'merge_completion' is protected by the snapshot's lock.
> 
> Also changed the 'merge_running' flag from int to atomic_t.

No, there's a bug:

>  static void stop_merge(struct dm_snapshot *s)
>  {
> -	while (s->merge_running) {
> -		s->merge_shutdown = 1;
> -		msleep(1);
> +	DECLARE_COMPLETION_ONSTACK(merge_stopped);
> +	if (atomic_read(&s->merge_running)) {

--- if the merge stops exactly at this point (because it gets finished or 
because of an i/o error), we are waiting for a completion that will be 
never signalled.

> +		down_write(&s->lock);
> +		s->merge_completion = &merge_stopped;
> +		up_write(&s->lock);
> +		wait_for_completion(&merge_stopped);
>  	}
> -	s->merge_shutdown = 0;
>  }
>  
>  /*

For Alasdair: do you get the problem? If I write it with msleep() 
correctly, you keep on complaining how unclean it is --- if it is written 
with completions and it is wrong (because they are just harder to use 
correctly than simple variables and msleep), you tend to support it. Now 
you see in practice how complex constructs tend to trigger bugs.

Mike: I thought that the completion would be in struct dm_snapshot. But 
maybe, try it with wait_on_bit / wake_up_bit / test_bit / set_bit etc., it 
may be easier than completions.

Mikulas