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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
