Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1184356 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw_semaphore Jan Kara <jack@suse.cz> - 2015-07-15 08:50 +0200
    Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw_semaphore Jan Kara <jack@suse.cz> - 2015-07-16 12:20 +0200
    Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw_semaphore Oleg Nesterov <oleg@redhat.com> - 2015-07-16 19:40 +0200
      Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw_semaphore Dave Chinner <david@fromorbit.com> - 2015-07-17 03:30 +0200

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

On Tue 14-07-15 14:41:13, Dave Hansen wrote:
> On 07/14/2015 02:22 PM, Oleg Nesterov wrote:
> >> Using my little write-1-byte test (under will-it-scale), your 4 patches
> >> improves the number of writes/sec by 12%.  My 3 patches improve the
> >> number of writes/sec by 32%.
> 
> I looked at it again.  I tested with this patch in addition to the ones
> modifying __sb_start/end_write():
> 
> 	https://lkml.org/lkml/2015/6/24/682
> 
> That is where the performance delta came from.  Your patches (plus the
> fsnotify optimization) perform very similarly to my approach.
> 
> Yours remove so much code that I think they are the preferable approach.
> 
> They don't compile with lockdep on, btw. :)

Great, thanks for hashing it out. So I'm also in favor of Oleg's approach
as well. We just have to wait until he fixes the outstanding issues with
his code. Dave, just send your fsnotify patch separately to AKPM - he
usually merges fsnotify stuff. Thanks.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1185697

On Thu 16-07-15 00:30:27, Dave Hansen wrote:
> On 07/16/2015 12:26 AM, Jan Kara wrote:
> > So Dave's patches would go in only in the next merge window anyway so we
> > still have like two-three weeks to decide which patchset to take. If you
> > think it will take you longer, then merging Dave's patches makes some sense
> > although I personally don't think the issue is so important that we have to
> > fix it ASAP and eventual delay of one more release would be OK for me.
> 
> I thought of one more thing.
> 
> Oleg's patches are fast in the uncontended case.  My testing of his code
> so far was all single-threaded.  Will it slow down if we throw a bunch
> of threads at it an see contention?  I don't think my RCU approach will.
> 
> I can benchmark in the next few days on a larger machine.

Well, his semaphore is percpu as well so I don't think there should be a
difference (there may be some difference when freeze is in progress but
that doesn't really matter). But it's always good to verify.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186126

On 07/16, Jan Kara wrote:
>
> On Wed 15-07-15 20:19:20, Oleg Nesterov wrote:
> >
> > Perhaps it makes to merge other 2 patches from Dave first? (those which
> > change __sb_start/end_write to rely on RCU). Afaics these changes are
> > straightforward and correct. Although I'd suggest to use preempt_disable()
> > and synchronize_sched() instead. I will be happy to (try to) make this
> > conversion on top of his changes.
> >
> > Because I do not want to delay the performance improvements and I do not
> > know when exactly I'll send the next version: I need to finish the previous
> > discussion about rcu_sync first. And the necessary changes in fs/super.c
> > depend on whether percpu_rw_semaphore will have rcu_sync or not (not too
> > much, only destroy_super() depends, but still).
> >
> > And of course, I am worried that I missed something and percpu_rw_semaphore
> > can't work for some reason. The code in fs/super.c looks simple, but it
> > seems that filesystems do the "strange" things with lockdep at least.
>
> So Dave's patches would go in only in the next merge window anyway so we
> still have like two-three weeks to decide which patchset to take.

OK, good.

> If you
> think it will take you longer,

Hopefully not.

> then merging Dave's patches makes some sense
> although I personally don't think the issue is so important that we have to
> fix it ASAP and eventual delay of one more release would be OK for me.

OK. I will try to do this in any case, I just wanted to say that I can
equally do this on top of Dave's patches.

To remind, I need to finish the discussion about percpu_rw_semaphore
and rcu_sync, then I'll try to make v2.

And. The biggest problem is lockdep. Everything else looks really simple
although of course I could miss something. And not only because the
filesystems abuse lockdep and thus we need some cleanups first. Unless
I am totally confused fs/super.c needs some cleanups (and fixes) too,
with or without this conversion. Say, acquire_freeze_lock() logic does
do not look right:

	- wait_event(.frozen < level) without rwsem_acquire_read() is
	  just wrong from lockdep perspective. If we are going to deadlock
	  because the caller is buggy, lockdep can't warn us.

	- __sb_start_write() can race with thaw_super() + freeze_super(),
	  and after "goto retry" the 2nd  acquire_freeze_lock() is wrong.

	- The "tell lockdep we are doing trylock" hack doesn't look nice.

	  I think this is correct, but this logic should be more explicit.
	  Yes, the recursive read_lock() is fine if we hold the lock on
	  higher level. But we do not need to fool lockdep. If we can not
	  deadlock in this case (and I agree we can't) we should simply use
	  wait == F consistently.

Something like this (not even compiled tested):

	static int
	do_sb_start_write(struct super_block *sb, int level, bool wait, unsigned long ip)
	{

		if (wait)
			rwsem_acquire_read(&sb->s_writers.lock_map[level-1], 0, 0, ip);
	retry:
		if (unlikely(sb->s_writers.frozen >= level)) {
			if (!wait)
				return 0;
			wait_event(sb->s_writers.wait_unfrozen,
				   sb->s_writers.frozen < level);
		}

		percpu_counter_inc(&sb->s_writers.counter[level-1]);
		/*
		 * Make sure counter is updated before we check for frozen.
		 * freeze_super() first sets frozen and then checks the counter.
		 */
		smp_mb();
		if (unlikely(sb->s_writers.frozen >= level)) {
			__sb_end_write(sb, level);
			goto retry;
		}

		if (!wait)
			rwsem_acquire_read(&sb->s_writers.lock_map[level-1], 0, 1, ip);
		return 1;
	}

	/*
	 * This is an internal function, please use sb_start_{write,pagefault,intwrite}
	 * instead.
	 */
	int __sb_start_write(struct super_block *sb, int level, bool wait)
	{
		bool cantbelocked = false;
		int ret;

	#ifdef CONFIG_LOCKDEP
		/*
		 * We want lockdep to tell us about possible deadlocks with freezing but
		 * it's it bit tricky to properly instrument it. Getting a freeze protection
		 * works as getting a read lock but there are subtle problems. XFS for example
		 * gets freeze protection on internal level twice in some cases, which is OK
		 * only because we already hold a freeze protection also on higher level. Due
		 * to these cases we have to use wait == F (trylock mode) which must not fail.
		 */
		if (wait) {
			int i;

			for (i = 0; i < level - 1; i++)
				if (lock_is_held(&sb->s_writers.lock_map[i])) {
					cantbelocked = true;
					break;
				}
		}
	#endif
		ret = do_sb_start_write(sb, level, wait && !cantbelocked, _RET_IP_);
		WARN_ON(cantbelocked & !ret);
		return ret;
	}

This should not generate the additional code if CONFIG_LOCKDEP=n and
After this patch it will be trivial to convert __sb_start_write(), but
we need some more cleanups. And perhaps I'll send some changes (like
above) separately, because again, I think they make sense in any case.

In short: I'll try to make v2 asap, but this is all I can promise ;)

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186359

On Thu, Jul 16, 2015 at 07:32:56PM +0200, Oleg Nesterov wrote:
> 
> 	#ifdef CONFIG_LOCKDEP
> 		/*
> 		 * We want lockdep to tell us about possible deadlocks with freezing but
> 		 * it's it bit tricky to properly instrument it. Getting a freeze protection
> 		 * works as getting a read lock but there are subtle problems. XFS for example
> 		 * gets freeze protection on internal level twice in some cases, which is OK

Sorry, I've missed something here - where is XFS nesting
sb_start_intwrite() calls?  XFS only has a call to
sb_start_intwrite() in xfs_trans_alloc() and an open coded
equivalent in xfs_iomap_write_unwritten(). However, we cannot create
nested transaction contexts as doing so is *guaranteed to deadlock
the journal*. (e.g. why we use PF_FSTRANS to trigger GFP_NOFS
allocation as it prevents direct memory reclaim from causing nested
transactions.) Hence if we taking multiple FREEZE_FS level
references at a time then there is a bug that needs fixing....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web