Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753905AbbDQWRQ (ORCPT ); Fri, 17 Apr 2015 18:17:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53197 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751275AbbDQWRO (ORCPT ); Fri, 17 Apr 2015 18:17:14 -0400 Date: Sat, 18 Apr 2015 00:16:48 +0200 From: Mateusz Guzik To: Eric Dumazet Cc: Alexander Viro , Andrew Morton , "Paul E. McKenney" , Yann Droneaud , Konstantin Khlebnikov , 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 Message-ID: <20150417221646.GA15589@mguzik> References: <20150416121628.GA20615@mguzik> <1429307216.7346.255.camel@edumazet-glaptop2.roam.corp.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1429307216.7346.255.camel@edumazet-glaptop2.roam.corp.google.com> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2727 Lines: 83 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-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/