lkml.org 
[lkml]   [2015]   [Jun]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] fs/file.c: don't acquire files->file_lock in fd_install()
    On Tue, Apr 28, 2015 at 09:25:03PM -0700, Eric Dumazet wrote:

    > @@ -553,11 +572,20 @@ void __fd_install(struct files_struct *files, unsigned int fd,
    > struct file *file)
    > {
    > struct fdtable *fdt;
    > - spin_lock(&files->file_lock);
    > - fdt = files_fdtable(files);
    > +
    > + rcu_read_lock_sched();
    > +
    > + while (unlikely(files->resize_in_progress)) {
    > + rcu_read_unlock_sched();
    > + wait_event(files->resize_wait, !files->resize_in_progress);
    > + rcu_read_lock_sched();
    > + }
    > + /* coupled with smp_wmb() in expand_fdtable() */
    > + smp_rmb();
    > + fdt = rcu_dereference_sched(files->fdt);
    > BUG_ON(fdt->fd[fd] != NULL);
    > rcu_assign_pointer(fdt->fd[fd], file);
    > - spin_unlock(&files->file_lock);
    > + rcu_read_unlock_sched();

    Umm... You've taken something that was safe to use in atomic contexts
    and turned into something that might wait for GFP_KERNEL allocation; what's
    to guarantee that no users get broken by that? At the very least, you want
    to slap might_sleep() in there - the actual sleep is going to be very rare,
    so it would be an extremely hard to reproduce and debug.

    AFAICS, all current in-tree users should be safe, but fd_install() is exported
    and quiet changes of that sort are rather antisocial. Generally I don't give
    a damn about out-of-tree code, but this one is over the top.

    I _think_ it's otherwise OK, but please, add might_sleep() *AND* a note in
    Documentation/filesystems/porting.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2015-06-22 04:41    [W:4.158 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site