Re: dm: add memory barrier before waitqueue_active

[Mike Snitzer <snitzer@redhat.com>]

On Tue, Feb 05 2019 at  5:09am -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Hi
> 
> Please submit patch this to Linus before 5.0 is released.
> 
> Mikulas
> 
> 
> 
> waitqueue_active without preceding barrier is unsafe, see the comment
> before waitqueue_active definition in include/linux/wait.h.
> 
> This patch changes it to wq_has_sleeper.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: 6f75723190d8 ("dm: remove the pending IO accounting")
> 
> ---
>  drivers/md/dm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/md/dm.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm.c	2019-02-04 20:18:03.000000000 +0100
> +++ linux-2.6/drivers/md/dm.c	2019-02-04 20:18:03.000000000 +0100
> @@ -699,7 +699,7 @@ static void end_io_acct(struct dm_io *io
>  				    true, duration, &io->stats_aux);
>  
>  	/* nudge anyone waiting on suspend queue */
> -	if (unlikely(waitqueue_active(&md->wait)))
> +	if (unlikely(wq_has_sleeper(&md->wait)))
>  		wake_up(&md->wait);
>  }
>  

This could be applicable to dm-rq.c:rq_completed() too...

but I'm not following where you think we benefit from adding the
smp_mb() to end_io_acct() please be explicit about your concern.

Focusing on bio-based DM, your concern is end_io_acct()'s wake_up() will
race with its, or some other cpus', preceding generic_end_io_acct()
percpu accounting?
- and so dm_wait_for_completion()'s !md_in_flight() condition will
  incorrectly determine there is outstanding IO due to end_io_acct()'s
  missing smp_mb()?
- SO dm_wait_for_completion() would go back to top its loop and may
  never get woken up again via wake_up(&md->wait)?

The thing is in both callers that use this pattern:
    
    if (unlikely(waitqueue_active(&md->wait)))
		wake_up(&md->wait);

the condition (namely IO accounting) will have already been updated via
generic_end_io_acct() (in terms of part_dec_in_flight() percpu updates).
So to me, using smp_mb() here is fairly pointless when you consider the
condition that the waiter (dm_wait_for_completion) will be using is
_not_ the byproduct of a single store.

Again, for bio-based DM, block core is performing atomic percpu updates
across N cpus.  And the dm_wait_for_completion() waiter is doing percpu
totalling via md_in_flight_bios().

Could be there is still an issue here.. but I'm not quite seeing it.
Cc'ing Jens to get his thoughts.

Thanks,
Mike