Re: [RFC PATCH] fs: use a sequence counter instead of file_lock in fd_install

From: Mateusz Guzik <mguzik@redhat.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Eric Dumazet <eric.dumazet@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
	Yann Droneaud <ydroneaud@opteya.com>,
	Konstantin Khlebnikov <khlebnikov@yandex-team.ru>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] fs: use a sequence counter instead of file_lock in fd_install
Date: Mon, 20 Apr 2015 15:43:27 +0200	[thread overview]
Message-ID: <20150420134326.GC2513@mguzik> (raw)
In-Reply-To: <20150420130633.GA2513@mguzik>

On Mon, Apr 20, 2015 at 03:06:33PM +0200, Mateusz Guzik wrote:
> On Sat, Apr 18, 2015 at 12:02:52AM +0100, Al Viro wrote:
> > On Sat, Apr 18, 2015 at 12:16:48AM +0200, Mateusz Guzik wrote:
> > 
> > > I would say this makes the use of seq counter impossible. Even if we
> > > decided to fall back to a lock on retry, we cannot know what to do if
> > > the slot is reserved - it very well could be that something called
> > > close, and something else reserved the slot, so putting the file inside
> > > could be really bad. In fact we would be putting a file for which we
> > > don't have a reference anymore.
> > > 
> > > However, not all hope is lost and I still think we can speed things up.
> > > 
> > > A locking primitive which only locks stuff for current cpu and has
> > > another mode where it locks stuff for all cpus would do the trick just
> > > fine. I'm not a linux guy, quick search suggests 'lglock' would do what
> > > I want.
> > > 
> > > table reallocation is an extremely rare operation, so this should be
> > > fine. It would take the lock 'globally' for given table.
> > 
> > It would also mean percpu_alloc() for each descriptor table...
> 
> Well as it was noted I have not checked how it's implemented at the time
> of writing the message. I agree embedding something like this into files
> struct is a non-starter.
> 
> I would say this could work with a small set of locks, selected by hashing
> struct files pointer.
> 
> Table resizing is supposed to be extremely rare - most processes should
> not need it at all (if they do, the default size is too small and should
> be adjusted). Not only that, the lock is only needed if the process in
> question is multithreaded.
> 
> So I would say this would not contend in real-world workloads, but still
> looks crappy.
> 
> Unfortunately the whole thing loses original appeal of a simple hack
> with no potential perfomrance drawbacks. Maybe I'll hack it up later and
> run some tests anyway.
> 

I just came up with another stupid hack, but this time it could really
work just fine.

Note that the entire issue stems from the fact that the table can be
resized at any moment. If only we had a guarantee the table "stands
still", we would not even need that sequence couner. fd_install could
just plop the file in.

So a stupid hack which comes to mind tells the kernel to make sure the
table is big enough and then never resize it ever again (inherited on
fork, cleared on exec):
prctl(FDTABLE_SIZE_FIXED, BIGNUM);

or

dup2(0, BIGNUM); /* sizes the table appropriately */
close(BIGNUM);
prctl(FDTABLE_SIZE_FIXED);

Thoughts?
-- 
Mateusz Guzik