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

[Mateusz Guzik <mguzik@redhat.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