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

From: Mateusz Guzik <mguzik@redhat.com>
To: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	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: Sat, 18 Apr 2015 00:16:48 +0200	[thread overview]
Message-ID: <20150417221646.GA15589@mguzik> (raw)
In-Reply-To: <1429307216.7346.255.camel@edumazet-glaptop2.roam.corp.google.com>

On Fri, Apr 17, 2015 at 02:46:56PM -0700, Eric Dumazet wrote:
> On Thu, 2015-04-16 at 14:16 +0200, Mateusz Guzik wrote:
> > Hi,
> > 
> > Currently obtaining a new file descriptor results in locking fdtable
> > twice - once in order to reserve a slot and second time to fill it
> 
> ...
> 
> 
> >  void __fd_install(struct files_struct *files, unsigned int fd,
> >  		struct file *file)
> >  {
> > +	unsigned long seq;
> 
> 	unsigned int seq;
> 
> >  	struct fdtable *fdt;
> > -	spin_lock(&files->file_lock);
> > -	fdt = files_fdtable(files);
> > -	BUG_ON(fdt->fd[fd] != NULL);
> > -	rcu_assign_pointer(fdt->fd[fd], file);
> > -	spin_unlock(&files->file_lock);
> > +
> > +	rcu_read_lock();
> > +	do {
> > +		seq = read_seqcount_begin(&files->fdt_seqcount);
> > +		fdt = files_fdtable_seq(files);
> > +		/*
> > +		 * Entry in the table can already be equal to file if we
> > +		 * had to restart and copy_fdtable picked up our update.
> > +		 */
> > +		BUG_ON(!(fdt->fd[fd] == NULL || fdt->fd[fd] == file));
> > +		rcu_assign_pointer(fdt->fd[fd], file);
> > +		smp_mb();
> > +	} while (__read_seqcount_retry(&files->fdt_seqcount, seq));
> > +	rcu_read_unlock();
> >  }
> >  
> 
> So one problem here is :
> 
> As soon as  rcu_assign_pointer(fdt->fd[fd], file) is done, and other cpu
> does one expand_fdtable() and releases files->file_lock, another cpu can
> close(fd).
> 
> Then another cpu can reuse the [fd] now empty slot and install a new
> file in it.
> 
> Then this cpu will crash here :
> 
> BUG_ON(!(fdt->fd[fd] == NULL || fdt->fd[fd] == file));
> 

Ouch, this is so obvious now that you mention it. Really stupid
mistake on my side.

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.

I'll play with this.

-- 
Mateusz Guzik