Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Artem Bityutskiy <dedekind1@gmail.com>]

Nick, thanks for serialization suggestion.

On Thu, 2010-05-27 at 17:22 +1000, Nick Piggin wrote:
> Yeah, we definitely don't want to add global cacheline writes in the
> common case. Also I don't know why you do the strange -1 value. I
> couldn't seem to find where you defined bdi_arm_supers_timer();

It is in mm/backing-dev.c:376 in today's Linus' tree. The -1 is used to
indicate that 'sync_supers()' is in progress and avoid arming timer in
that case. But yes, this is not really needed.

> But why doesn't this work?
> 
>   sb->s_dirty = 1;
>   smp_mb(); /* corresponding MB is in test_and_clear_bit */

AFAIU, test_and_clear_bit assumes 2 barriers - before the test and after
the clear. Then I do not really understand why this smp_mb is needed.

>   if (unlikely(!supers_timer_armed)) {
>     if (!test_and_set_bit(0, &supers_timer_armed))
>         bdi_arm_supers_timer();
>   }
>     
>   vs
> 
>   supers_timer_armed = 0;
> again:
>   sync_supers();
>   if (test_and_clear_bit(0, &supers_timer_armed))
>     goto again;

AFAIU, the following should be fine, no?:


    if (unlikely(!supers_timer_armed))
        if (!test_and_set_bit(0, &supers_timer_armed))
            bdi_arm_supers_timer();

vs

again:
    sync_supers();
    if (test_and_clear_bit(0, &supers_timer_armed))
        goto again;

I assume that it is harmless to run 'bdi_arm_supers_timer()'
concurrently;

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)