Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw_semaphore

[Oleg Nesterov <oleg@redhat.com>]

On 07/14, Jan Kara wrote:
>
>   Hello,
>
> On Mon 13-07-15 23:25:36, Oleg Nesterov wrote:
> > Al, Jan, could you comment? I mean the intent, the patches are
> > obviously not for inclusion yet.
>
> Thanks for the patches! Hum, what do people have with freeze protection
> these days? Noone cared about it for years and sudddently two patch sets
> within a month :) Anyway, have you seen the patch set from Dave Hansen?
>
> It is here: https://lkml.org/lkml/2015/6/24/682
> He modifies the freezing primitives in a different way. AFAICS the
> resulting performance of the fast path should be about the same.

At first glance, 2-3 do something similar, yes...

> So unless
> I'm missing something and there is a significant performance advantage to
> Dave's patches I'm all for using a generic primitive you suggest.

I think percpu_rw_semaphore looks a bit better. And even a bit faster.
And it will not block __sb_start_write() entirely while freeze_super()
sleeps in synchronize_rcu().

freeze_super() should be faster too after rcu_sync changes, but this
is not that important.

But again, to me the main advantage is that we can use the generic
primitives and remove this nontrivial code in fs/super.c.

> Can you perhaps work with Dave on some common resolution?

Dave, what do you think? Will you agree with percpu_rw_semaphore ?

> > 	- __sb_start_write() will be a little bit faster, but this
> > 	  is minor.
>
> Actually Dave could measure the gain achieved by removing the barrier. It
> would be good to verify that your patches achieve a similar gain.

The fast path in percpu_down_read() is really fast, it does a plain
LOAD plus __this_cpu_add() under preempt_disable(). I doubt this can
be improved. The actual code is:

	static bool update_fast_ctr(struct percpu_rw_semaphore *brw, unsigned int val)
	{
		bool success = false;

		preempt_disable();
		if (likely(!atomic_read(&brw->write_ctr))) {
			__this_cpu_add(*brw->fast_read_ctr, val);
			success = true;
		}
		preempt_enable();

		return success;
	}

	void percpu_down_read(struct percpu_rw_semaphore *brw)
	{
		might_sleep();
		if (likely(update_fast_ctr(brw, +1))) {
			rwsem_acquire_read(&brw->rw_sem.dep_map, 0, 0, _RET_IP_);
			return;
		}

		down_read(&brw->rw_sem);
		atomic_inc(&brw->slow_read_ctr);
		__up_read(&brw->rw_sem);
	}


> > 	- Fix get_super_thawed(), it will spin if MS_RDONLY...
> >
> > 	  It is not clear to me what exactly should we do, but this
> > 	  doesn't look hard. Perhaps it can just return if MS_RDONLY.
>
> What's the exact problem here?

Note that freeze_super() does not do sb_wait_write() (which blocks
__sb_start_write) if MS_RDONLY. This means that after 1/4 get_super_thawed()
will spin until SB_UNFROZEN in this case, sb_start_write() won't block. But
please forget, this is not the problem. I mean, afaics this is easy to fix,
but the initial version should just keep ->wait_unfrozen specially for
get_super_thawed(), then we can remove it in a separate patch.

Thanks!

Oleg.