From: Valerie Aurora <vaurora@redhat.com>
To: Nick Piggin <npiggin@kernel.dk>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
Christoph Hellwig <hch@infradead.org>,
Alan Cox <alan@lxorguk.ukuu.org.uk>,
Andi Kleen <ak@linux.intel.com>,
Greg Kroah-Hartman <gregkh@suse.de>
Subject: Re: [patch 06/10] fs: cleanup files_lock locking
Date: Wed, 18 Aug 2010 15:46:46 -0400 [thread overview]
Message-ID: <20100818194646.GF10850@shell> (raw)
In-Reply-To: <20100817184121.363425312@kernel.dk>
On Wed, Aug 18, 2010 at 04:37:35AM +1000, Nick Piggin wrote:
> fs: cleanup files_lock locking
>
> Lock tty_files with a new spinlock, tty_files_lock; provide helpers to
> manipulate the per-sb files list; unexport the files_lock spinlock.
>
> Cc: linux-kernel@vger.kernel.org
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Acked-by: Andi Kleen <ak@linux.intel.com>
> Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Nick Piggin <npiggin@kernel.dk>
>
> ---
> drivers/char/pty.c | 6 +++++-
> drivers/char/tty_io.c | 26 ++++++++++++++++++--------
> fs/file_table.c | 42 ++++++++++++++++++------------------------
> fs/open.c | 4 ++--
> include/linux/fs.h | 7 ++-----
> include/linux/tty.h | 1 +
> security/selinux/hooks.c | 4 ++--
> 7 files changed, 48 insertions(+), 42 deletions(-)
>
> Index: linux-2.6/security/selinux/hooks.c
> ===================================================================
> --- linux-2.6.orig/security/selinux/hooks.c 2010-08-18 04:04:01.000000000 +1000
> +++ linux-2.6/security/selinux/hooks.c 2010-08-18 04:05:10.000000000 +1000
> @@ -2170,7 +2170,7 @@ static inline void flush_unauthorized_fi
>
> tty = get_current_tty();
> if (tty) {
> - file_list_lock();
> + spin_lock(&tty_files_lock);
> if (!list_empty(&tty->tty_files)) {
> struct inode *inode;
>
> @@ -2186,7 +2186,7 @@ static inline void flush_unauthorized_fi
> drop_tty = 1;
> }
> }
> - file_list_unlock();
> + spin_unlock(&tty_files_lock);
> tty_kref_put(tty);
> }
> /* Reset controlling tty. */
> Index: linux-2.6/drivers/char/pty.c
> ===================================================================
> --- linux-2.6.orig/drivers/char/pty.c 2010-08-18 04:04:01.000000000 +1000
> +++ linux-2.6/drivers/char/pty.c 2010-08-18 04:05:10.000000000 +1000
> @@ -676,7 +676,11 @@ static int ptmx_open(struct inode *inode
>
> set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
> filp->private_data = tty;
> - file_move(filp, &tty->tty_files);
> +
> + file_sb_list_del(filp); /* __dentry_open has put it on the sb list */
> + spin_lock(&tty_files_lock);
> + list_add(&filp->f_u.fu_list, &tty->tty_files);
> + spin_unlock(&tty_files_lock);
>
> retval = devpts_pty_new(inode, tty->link);
> if (retval)
> Index: linux-2.6/drivers/char/tty_io.c
> ===================================================================
> --- linux-2.6.orig/drivers/char/tty_io.c 2010-08-18 04:04:01.000000000 +1000
> +++ linux-2.6/drivers/char/tty_io.c 2010-08-18 04:05:10.000000000 +1000
> @@ -136,6 +136,9 @@ LIST_HEAD(tty_drivers); /* linked list
> DEFINE_MUTEX(tty_mutex);
> EXPORT_SYMBOL(tty_mutex);
>
> +/* Spinlock to protect the tty->tty_files list */
> +DEFINE_SPINLOCK(tty_files_lock);
> +
> static ssize_t tty_read(struct file *, char __user *, size_t, loff_t *);
> static ssize_t tty_write(struct file *, const char __user *, size_t, loff_t *);
> ssize_t redirected_tty_write(struct file *, const char __user *,
> @@ -235,11 +238,11 @@ static int check_tty_count(struct tty_st
> struct list_head *p;
> int count = 0;
>
> - file_list_lock();
> + spin_lock(&tty_files_lock);
> list_for_each(p, &tty->tty_files) {
> count++;
> }
> - file_list_unlock();
> + spin_unlock(&tty_files_lock);
> if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> tty->driver->subtype == PTY_TYPE_SLAVE &&
> tty->link && tty->link->count)
> @@ -519,7 +522,7 @@ void __tty_hangup(struct tty_struct *tty
> workqueue with the lock held */
> check_tty_count(tty, "tty_hangup");
>
> - file_list_lock();
> + spin_lock(&tty_files_lock);
> /* This breaks for file handles being sent over AF_UNIX sockets ? */
> list_for_each_entry(filp, &tty->tty_files, f_u.fu_list) {
> if (filp->f_op->write == redirected_tty_write)
> @@ -530,7 +533,7 @@ void __tty_hangup(struct tty_struct *tty
> __tty_fasync(-1, filp, 0); /* can't block */
> filp->f_op = &hung_up_tty_fops;
> }
> - file_list_unlock();
> + spin_unlock(&tty_files_lock);
>
> tty_ldisc_hangup(tty);
>
> @@ -1424,9 +1427,9 @@ static void release_one_tty(struct work_
> tty_driver_kref_put(driver);
> module_put(driver->owner);
>
> - file_list_lock();
> + spin_lock(&tty_files_lock);
> list_del_init(&tty->tty_files);
> - file_list_unlock();
> + spin_unlock(&tty_files_lock);
>
> put_pid(tty->pgrp);
> put_pid(tty->session);
> @@ -1671,7 +1674,10 @@ int tty_release(struct inode *inode, str
> * - do_tty_hangup no longer sees this file descriptor as
> * something that needs to be handled for hangups.
> */
> - file_kill(filp);
> + spin_lock(&tty_files_lock);
> + BUG_ON(list_empty(&filp->f_u.fu_list));
> + list_del_init(&filp->f_u.fu_list);
> + spin_unlock(&tty_files_lock);
> filp->private_data = NULL;
>
> /*
> @@ -1840,7 +1846,11 @@ got_driver:
> }
>
> filp->private_data = tty;
> - file_move(filp, &tty->tty_files);
> + BUG_ON(list_empty(&filp->f_u.fu_list));
> + file_sb_list_del(filp); /* __dentry_open has put it on the sb list */
> + spin_lock(&tty_files_lock);
> + list_add(&filp->f_u.fu_list, &tty->tty_files);
> + spin_unlock(&tty_files_lock);
> check_tty_count(tty, "tty_open");
> if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> tty->driver->subtype == PTY_TYPE_MASTER)
> Index: linux-2.6/fs/file_table.c
> ===================================================================
> --- linux-2.6.orig/fs/file_table.c 2010-08-18 04:04:01.000000000 +1000
> +++ linux-2.6/fs/file_table.c 2010-08-18 04:05:09.000000000 +1000
> @@ -32,8 +32,7 @@ struct files_stat_struct files_stat = {
> .max_files = NR_FILE
> };
>
> -/* public. Not pretty! */
> -__cacheline_aligned_in_smp DEFINE_SPINLOCK(files_lock);
> +static __cacheline_aligned_in_smp DEFINE_SPINLOCK(files_lock);
>
> /* SLAB cache for file structures */
> static struct kmem_cache *filp_cachep __read_mostly;
> @@ -249,7 +248,7 @@ static void __fput(struct file *file)
> cdev_put(inode->i_cdev);
> fops_put(file->f_op);
> put_pid(file->f_owner.pid);
> - file_kill(file);
> + file_sb_list_del(file);
> if (file->f_mode & FMODE_WRITE)
> drop_file_write_access(file);
> file->f_path.dentry = NULL;
> @@ -328,31 +327,29 @@ struct file *fget_light(unsigned int fd,
> return file;
> }
>
> -
> void put_filp(struct file *file)
> {
> if (atomic_long_dec_and_test(&file->f_count)) {
> security_file_free(file);
> - file_kill(file);
> + file_sb_list_del(file);
> file_free(file);
> }
> }
>
> -void file_move(struct file *file, struct list_head *list)
> +void file_sb_list_add(struct file *file, struct super_block *sb)
> {
> - if (!list)
> - return;
> - file_list_lock();
> - list_move(&file->f_u.fu_list, list);
> - file_list_unlock();
> + spin_lock(&files_lock);
> + BUG_ON(!list_empty(&file->f_u.fu_list));
> + list_add(&file->f_u.fu_list, &sb->s_files);
> + spin_unlock(&files_lock);
> }
>
> -void file_kill(struct file *file)
> +void file_sb_list_del(struct file *file)
> {
> if (!list_empty(&file->f_u.fu_list)) {
> - file_list_lock();
> + spin_lock(&files_lock);
> list_del_init(&file->f_u.fu_list);
> - file_list_unlock();
> + spin_unlock(&files_lock);
> }
> }
>
> @@ -361,7 +358,7 @@ int fs_may_remount_ro(struct super_block
> struct file *file;
>
> /* Check that no files are currently opened for writing. */
> - file_list_lock();
> + spin_lock(&files_lock);
> list_for_each_entry(file, &sb->s_files, f_u.fu_list) {
> struct inode *inode = file->f_path.dentry->d_inode;
>
> @@ -373,10 +370,10 @@ int fs_may_remount_ro(struct super_block
> if (S_ISREG(inode->i_mode) && (file->f_mode & FMODE_WRITE))
> goto too_bad;
> }
> - file_list_unlock();
> + spin_unlock(&files_lock);
> return 1; /* Tis' cool bro. */
> too_bad:
> - file_list_unlock();
> + spin_unlock(&files_lock);
> return 0;
> }
>
> @@ -392,7 +389,7 @@ void mark_files_ro(struct super_block *s
> struct file *f;
>
> retry:
> - file_list_lock();
> + spin_lock(&files_lock);
> list_for_each_entry(f, &sb->s_files, f_u.fu_list) {
> struct vfsmount *mnt;
> if (!S_ISREG(f->f_path.dentry->d_inode->i_mode))
> @@ -408,16 +405,13 @@ retry:
> continue;
> file_release_write(f);
> mnt = mntget(f->f_path.mnt);
> - file_list_unlock();
> - /*
> - * This can sleep, so we can't hold
> - * the file_list_lock() spinlock.
> - */
> + /* This can sleep, so we can't hold the spinlock. */
> + spin_unlock(&files_lock);
> mnt_drop_write(mnt);
> mntput(mnt);
> goto retry;
> }
> - file_list_unlock();
> + spin_unlock(&files_lock);
> }
>
> void __init files_init(unsigned long mempages)
> Index: linux-2.6/fs/open.c
> ===================================================================
> --- linux-2.6.orig/fs/open.c 2010-08-18 04:04:01.000000000 +1000
> +++ linux-2.6/fs/open.c 2010-08-18 04:04:29.000000000 +1000
> @@ -675,7 +675,7 @@ static struct file *__dentry_open(struct
> f->f_path.mnt = mnt;
> f->f_pos = 0;
> f->f_op = fops_get(inode->i_fop);
> - file_move(f, &inode->i_sb->s_files);
> + file_sb_list_add(f, inode->i_sb);
>
> error = security_dentry_open(f, cred);
> if (error)
> @@ -721,7 +721,7 @@ cleanup_all:
> mnt_drop_write(mnt);
> }
> }
> - file_kill(f);
> + file_sb_list_del(f);
> f->f_path.dentry = NULL;
> f->f_path.mnt = NULL;
> cleanup_file:
> Index: linux-2.6/include/linux/fs.h
> ===================================================================
> --- linux-2.6.orig/include/linux/fs.h 2010-08-18 04:04:01.000000000 +1000
> +++ linux-2.6/include/linux/fs.h 2010-08-18 04:05:10.000000000 +1000
> @@ -953,9 +953,6 @@ struct file {
> unsigned long f_mnt_write_state;
> #endif
> };
> -extern spinlock_t files_lock;
> -#define file_list_lock() spin_lock(&files_lock);
> -#define file_list_unlock() spin_unlock(&files_lock);
>
> #define get_file(x) atomic_long_inc(&(x)->f_count)
> #define fput_atomic(x) atomic_long_add_unless(&(x)->f_count, -1, 1)
> @@ -2197,8 +2194,8 @@ static inline void insert_inode_hash(str
> __insert_inode_hash(inode, inode->i_ino);
> }
>
> -extern void file_move(struct file *f, struct list_head *list);
> -extern void file_kill(struct file *f);
> +extern void file_sb_list_add(struct file *f, struct super_block *sb);
> +extern void file_sb_list_del(struct file *f);
> #ifdef CONFIG_BLOCK
> extern void submit_bio(int, struct bio *);
> extern int bdev_read_only(struct block_device *);
> Index: linux-2.6/include/linux/tty.h
> ===================================================================
> --- linux-2.6.orig/include/linux/tty.h 2010-08-18 04:04:01.000000000 +1000
> +++ linux-2.6/include/linux/tty.h 2010-08-18 04:05:10.000000000 +1000
> @@ -470,6 +470,7 @@ extern struct tty_struct *tty_pair_get_t
> extern struct tty_struct *tty_pair_get_pty(struct tty_struct *tty);
>
> extern struct mutex tty_mutex;
> +extern spinlock_t tty_files_lock;
>
> extern void tty_write_unlock(struct tty_struct *tty);
> extern int tty_write_lock(struct tty_struct *tty, int ndelay);
Looks good.
Reviewed-by: Valerie Aurora <vaurora@redhat.com>
-VAL
next prev parent reply other threads:[~2010-08-18 19:46 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-08-17 18:37 [patch 00/10] first set of vfs scale patches Nick Piggin
2010-08-17 18:37 ` [patch 01/10] fs: fix do_lookup false negative Nick Piggin
2010-08-17 22:45 ` Valerie Aurora
2010-08-17 23:04 ` Sage Weil
2010-08-18 13:41 ` Andi Kleen
2010-08-17 18:37 ` [patch 02/10] fs: dentry allocation consolidation Nick Piggin
2010-08-17 22:45 ` Valerie Aurora
2010-08-17 18:37 ` [patch 03/10] apparmor: use task path helpers Nick Piggin
2010-08-17 22:59 ` Valerie Aurora
2010-08-17 18:37 ` [patch 04/10] fs: fs_struct rwlock to spinlock Nick Piggin
2010-08-17 23:14 ` Valerie Aurora
2010-08-20 10:05 ` Nick Piggin
2010-08-17 18:37 ` [patch 05/10] fs: remove extra lookup in __lookup_hash Nick Piggin
2010-08-18 13:57 ` Andi Kleen
2010-08-18 21:13 ` Andi Kleen
2010-08-18 19:34 ` Valerie Aurora
2010-08-17 18:37 ` [patch 06/10] fs: cleanup files_lock locking Nick Piggin
2010-08-18 19:46 ` Valerie Aurora [this message]
2010-08-17 18:37 ` [patch 07/10] tty: fix fu_list abuse Nick Piggin
2010-08-17 18:37 ` [patch 08/10] lglock: introduce special lglock and brlock spin locks Nick Piggin
2010-08-17 18:37 ` [patch 09/10] fs: scale files_lock Nick Piggin
2010-08-17 18:37 ` [patch 10/10] fs: brlock vfsmount_lock Nick Piggin
2010-08-18 14:05 ` Andi Kleen
2010-08-20 10:09 ` Nick Piggin
2010-08-17 21:14 ` [patch 00/10] first set of vfs scale patches Al Viro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100818194646.GF10850@shell \
--to=vaurora@redhat.com \
--cc=ak@linux.intel.com \
--cc=alan@lxorguk.ukuu.org.uk \
--cc=gregkh@suse.de \
--cc=hch@infradead.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=npiggin@kernel.dk \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).