[RFC][PATCH 1/5]: Move tty lookup/reopen to caller

[RFC][PATCH 1/5]: Move tty lookup/reopen to caller

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

get_node() cleanup in devpts

This patchset adds inode/tty parameter to devpts interfaces and uses
this new parameter to remove the unnecessary get_node() calls in devpts
(as pointed out by Peter Anvin).

Alan, if these are getting in your way, I can hold off for now, but
just wanted some quick comments from you on patch 1 and from Peter
on get_node() cleanup.

----

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 1/5]: Move tty lookup/reopen to caller

Move tty_driver_lookup_tty() and tty_reopen() from tty_init_dev()
into tty_open() (one of the two callers of tty_init_dev()).  These
calls are not really required in ptmx_open(), the other caller,
since ptmx_open() would be setting up a new tty.

Changelog[v2]:
	- remove the lookup and reopen calls from ptmx_open
	- merge with recent changes to ttydev tree

---
 drivers/char/tty_io.c |   31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

Index: linux-2.6.27-rc3-tty/drivers/char/tty_io.c
===================================================================
--- linux-2.6.27-rc3-tty.orig/drivers/char/tty_io.c	2008-08-28 15:26:34.000000000 -0700
+++ linux-2.6.27-rc3-tty/drivers/char/tty_io.c	2008-08-28 16:16:29.000000000 -0700
@@ -1366,19 +1366,6 @@ struct tty_struct *tty_init_dev(struct t
 	struct tty_struct *tty;
 	int retval;
 
-	/* check whether we're reopening an existing tty */
-	tty = tty_driver_lookup_tty(driver, idx);
-
-	if (IS_ERR(tty))
-		return tty;
-
-	if (tty) {
-		retval = tty_reopen(tty);
-		if (retval)
-			return ERR_PTR(retval);
-		return tty;
-	}
-
 	/* Check if pty master is being opened multiple times */
 	if (driver->subtype == PTY_TYPE_MASTER &&
 		(driver->flags & TTY_DRIVER_DEVPTS_MEM) && !first_ok)
@@ -1780,7 +1767,7 @@ void tty_release_dev(struct file *filp)
 
 static int __tty_open(struct inode *inode, struct file *filp)
 {
-	struct tty_struct *tty;
+	struct tty_struct *tty = NULL;
 	int noctty, retval;
 	struct tty_driver *driver;
 	int index;
@@ -1837,7 +1824,21 @@ retry_open:
 		return -ENODEV;
 	}
 got_driver:
-	tty = tty_init_dev(driver, index, 0);
+	if (!tty) {
+		/* check whether we're reopening an existing tty */
+		tty = tty_driver_lookup_tty(driver, index);
+
+		if (IS_ERR(tty))
+			return PTR_ERR(tty);
+	}
+
+	if (tty) {
+		retval = tty_reopen(tty);
+		if (retval)
+			tty = ERR_PTR(retval);
+	} else
+		tty = tty_init_dev(driver, index, 0);
+
 	mutex_unlock(&tty_mutex);
 	tty_driver_kref_put(driver);
 	if (IS_ERR(tty))

[RFC][PATCH 2/5]: Add an instance parameter devpts interfaces

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 2/5]: Add an instance parameter devpts interfaces

Pass-in 'inode' or 'tty' parameter to devpts interfaces.  With multiple
devpts instances, these parameters will be used in subsequent patches
to identify the instance of devpts mounted. The parameters also help
simplify devpts implementation.

Changelog[v3]:  
	- minor changes due to merge with ttydev updates
	- rename parameters to emphasize they are ptmx or pts inodes
	- pass-in tty_struct * to devpts_pty_kill() (this will help
	  cleanup the get_node() call in a subsequent patch)

---
 drivers/char/pty.c         |   18 ++++++++++--------
 drivers/char/tty_io.c      |   14 ++++++++------
 fs/devpts/inode.c          |   11 ++++++-----
 include/linux/devpts_fs.h  |   31 +++++++++++++++++++++----------
 include/linux/tty_driver.h |    3 ++-
 5 files changed, 47 insertions(+), 30 deletions(-)

Index: linux-2.6.27-rc3-tty/drivers/char/pty.c
===================================================================
--- linux-2.6.27-rc3-tty.orig/drivers/char/pty.c	2008-08-28 16:16:29.000000000 -0700
+++ linux-2.6.27-rc3-tty/drivers/char/pty.c	2008-08-28 16:17:04.000000000 -0700
@@ -60,7 +60,7 @@ static void pty_close(struct tty_struct 
 		set_bit(TTY_OTHER_CLOSED, &tty->flags);
 #ifdef CONFIG_UNIX98_PTYS
 		if (tty->driver == ptm_driver)
-			devpts_pty_kill(tty->index);
+			devpts_pty_kill(tty->link);
 #endif
 		tty_vhangup(tty->link);
 	}
@@ -451,9 +451,10 @@ static int pty_unix98_ioctl(struct tty_s
  *	This provides our locking.
  */
 
-static struct tty_struct *ptm_unix98_lookup(struct tty_driver *driver, int idx)
+static struct tty_struct *ptm_unix98_lookup(struct tty_driver *driver,
+		struct inode *ptm_inode, int idx)
 {
-	struct tty_struct *tty = devpts_get_tty(idx);
+	struct tty_struct *tty = devpts_get_tty(ptm_inode, idx);
 	if (tty)
 		tty = tty->link;
 	return tty;
@@ -468,9 +469,10 @@ static struct tty_struct *ptm_unix98_loo
  *	This provides our locking.
  */
 
-static struct tty_struct *pts_unix98_lookup(struct tty_driver *driver, int idx)
+static struct tty_struct *pts_unix98_lookup(struct tty_driver *driver,
+		struct inode *pts_inode, int idx)
 {
-	struct tty_struct *tty = devpts_get_tty(idx);
+	struct tty_struct *tty = devpts_get_tty(pts_inode, idx);
 	/* Master must be open before slave */
 	if (!tty)
 		return ERR_PTR(-EIO);
@@ -598,7 +600,7 @@ static int __ptmx_open(struct inode *ino
 	nonseekable_open(inode, filp);
 
 	/* find a device that is not in use. */
-	index = devpts_new_index();
+	index = devpts_new_index(inode);
 	if (index < 0)
 		return index;
 
@@ -615,7 +617,7 @@ static int __ptmx_open(struct inode *ino
 	filp->private_data = tty;
 	file_move(filp, &tty->tty_files);
 
-	retval = devpts_pty_new(tty->link);
+	retval = devpts_pty_new(inode, tty->link);
 	if (retval)
 		goto out1;
 
@@ -626,7 +628,7 @@ out1:
 	tty_release_dev(filp);
 	return retval;
 out:
-	devpts_kill_index(index);
+	devpts_kill_index(inode, index);
 	return retval;
 }
 
Index: linux-2.6.27-rc3-tty/drivers/char/tty_io.c
===================================================================
--- linux-2.6.27-rc3-tty.orig/drivers/char/tty_io.c	2008-08-28 16:16:29.000000000 -0700
+++ linux-2.6.27-rc3-tty/drivers/char/tty_io.c	2008-08-28 16:17:59.000000000 -0700
@@ -1203,12 +1203,13 @@ static void tty_line_name(struct tty_dri
  *	be held until the 'fast-open' is also done. Will change once we
  *	have refcounting in the driver and per driver locking
  */
-struct tty_struct *tty_driver_lookup_tty(struct tty_driver *driver, int idx)
+struct tty_struct *tty_driver_lookup_tty(struct tty_driver *driver,
+		struct inode *inode, int idx)
 {
 	struct tty_struct *tty;
 
 	if (driver->ops->lookup)
-		return driver->ops->lookup(driver, idx);
+		return driver->ops->lookup(driver, inode, idx);
 
 	tty = driver->ttys[idx];
 	return tty;
@@ -1529,10 +1530,11 @@ void tty_release_dev(struct file *filp)
 	int	devpts;
 	int	idx;
 	char	buf[64];
+	struct 	inode *inode;
 
+	inode = filp->f_path.dentry->d_inode;
 	tty = (struct tty_struct *)filp->private_data;
-	if (tty_paranoia_check(tty, filp->f_path.dentry->d_inode,
-							"tty_release_dev"))
+	if (tty_paranoia_check(tty, inode, "tty_release_dev"))
 		return;
 
 	check_tty_count(tty, "tty_release_dev");
@@ -1741,7 +1743,7 @@ void tty_release_dev(struct file *filp)
 
 	/* Make this pty number available for reallocation */
 	if (devpts)
-		devpts_kill_index(idx);
+		devpts_kill_index(inode, idx);
 }
 
 /**
@@ -1826,7 +1828,7 @@ retry_open:
 got_driver:
 	if (!tty) {
 		/* check whether we're reopening an existing tty */
-		tty = tty_driver_lookup_tty(driver, index);
+		tty = tty_driver_lookup_tty(driver, inode, index);
 
 		if (IS_ERR(tty))
 			return PTR_ERR(tty);
Index: linux-2.6.27-rc3-tty/fs/devpts/inode.c
===================================================================
--- linux-2.6.27-rc3-tty.orig/fs/devpts/inode.c	2008-08-28 16:16:29.000000000 -0700
+++ linux-2.6.27-rc3-tty/fs/devpts/inode.c	2008-08-28 17:59:15.000000000 -0700
@@ -177,7 +177,7 @@ static struct dentry *get_node(int num)
 	return lookup_one_len(s, root, sprintf(s, "%d", num));
 }
 
-int devpts_new_index(void)
+int devpts_new_index(struct inode *ptmx_inode)
 {
 	int index;
 	int ida_ret;
@@ -205,14 +205,14 @@ retry:
 	return index;
 }
 
-void devpts_kill_index(int idx)
+void devpts_kill_index(struct inode *ptmx_inode, int idx)
 {
 	mutex_lock(&allocated_ptys_lock);
 	ida_remove(&allocated_ptys, idx);
 	mutex_unlock(&allocated_ptys_lock);
 }
 
-int devpts_pty_new(struct tty_struct *tty)
+int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty)
 {
 	int number = tty->index; /* tty layer puts index from devpts_new_index() in here */
 	struct tty_driver *driver = tty->driver;
@@ -245,7 +245,7 @@ int devpts_pty_new(struct tty_struct *tt
 	return 0;
 }
 
-struct tty_struct *devpts_get_tty(int number)
+struct tty_struct *devpts_get_tty(struct inode *pts_inode, int number)
 {
 	struct dentry *dentry = get_node(number);
 	struct tty_struct *tty;
@@ -262,8 +262,9 @@ struct tty_struct *devpts_get_tty(int nu
 	return tty;
 }
 
-void devpts_pty_kill(int number)
+void devpts_pty_kill(struct tty_struct *tty)
 {
+	int number = tty->index;
 	struct dentry *dentry = get_node(number);
 
 	if (!IS_ERR(dentry)) {
Index: linux-2.6.27-rc3-tty/include/linux/devpts_fs.h
===================================================================
--- linux-2.6.27-rc3-tty.orig/include/linux/devpts_fs.h	2008-08-28 16:16:29.000000000 -0700
+++ linux-2.6.27-rc3-tty/include/linux/devpts_fs.h	2008-08-28 16:17:04.000000000 -0700
@@ -17,20 +17,31 @@
 
 #ifdef CONFIG_UNIX98_PTYS
 
-int devpts_new_index(void);
-void devpts_kill_index(int idx);
-int devpts_pty_new(struct tty_struct *tty);      /* mknod in devpts */
-struct tty_struct *devpts_get_tty(int number);	 /* get tty structure */
-void devpts_pty_kill(int number);		 /* unlink */
+int devpts_new_index(struct inode *ptmx_inode);
+void devpts_kill_index(struct inode *ptmx_inode, int idx);
+/* mknod in devpts */
+int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty);
+/* get tty structure */
+struct tty_struct *devpts_get_tty(struct inode *pts_inode, int number);
+/* unlink */
+void devpts_pty_kill(struct tty_struct *tty);
 
 #else
 
 /* Dummy stubs in the no-pty case */
-static inline int devpts_new_index(void) { return -EINVAL; }
-static inline void devpts_kill_index(int idx) { }
-static inline int devpts_pty_new(struct tty_struct *tty) { return -EINVAL; }
-static inline struct tty_struct *devpts_get_tty(int number) { return NULL; }
-static inline void devpts_pty_kill(int number) { }
+static inline int devpts_new_index(struct inode *ptmx_inode) { return -EINVAL; }
+static inline void devpts_kill_index(struct inode *ptmx_inode, int idx) { }
+static inline int devpts_pty_new(struct inode *ptmx_inode,
+				struct tty_struct *tty)
+{
+	return -EINVAL;
+}
+static inline struct tty_struct *devpts_get_tty(struct inode *pts_inode,
+		int number)
+{
+	return NULL;
+}
+static inline void devpts_pty_kill(struct tty_struct *tty) { }
 
 #endif
 
Index: linux-2.6.27-rc3-tty/include/linux/tty_driver.h
===================================================================
--- linux-2.6.27-rc3-tty.orig/include/linux/tty_driver.h	2008-08-28 16:16:29.000000000 -0700
+++ linux-2.6.27-rc3-tty/include/linux/tty_driver.h	2008-08-28 16:17:04.000000000 -0700
@@ -225,7 +225,8 @@ struct tty_struct;
 struct tty_driver;
 
 struct tty_operations {
-	struct tty_struct * (*lookup)(struct tty_driver *driver, int idx);
+	struct tty_struct * (*lookup)(struct tty_driver *driver,
+			struct inode *inode, int idx);
 	int  (*install)(struct tty_driver *driver, struct tty_struct *tty);
 	void (*remove)(struct tty_driver *driver, struct tty_struct *tty);
 	int  (*open)(struct tty_struct * tty, struct file * filp);

[RFC][PATCH 3/5]: Simplify devpts_get_tty()

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 3/5]: Simplify devpts_get_tty()

As pointed out by H. Peter Anvin, since the inode for the pty is known,
we don't need to look it up.

---
 fs/devpts/inode.c |   17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

Index: linux-2.6.27-rc3-tty/fs/devpts/inode.c
===================================================================
--- linux-2.6.27-rc3-tty.orig/fs/devpts/inode.c	2008-08-28 17:59:15.000000000 -0700
+++ linux-2.6.27-rc3-tty/fs/devpts/inode.c	2008-08-28 18:01:11.000000000 -0700
@@ -27,6 +27,7 @@
 #define DEVPTS_SUPER_MAGIC 0x1cd1
 
 #define DEVPTS_DEFAULT_MODE 0600
+#define PTMX_MINOR	2
 
 extern int pty_limit;			/* Config limit on Unix98 ptys */
 static DEFINE_IDA(allocated_ptys);
@@ -247,19 +248,11 @@ int devpts_pty_new(struct inode *ptmx_in
 
 struct tty_struct *devpts_get_tty(struct inode *pts_inode, int number)
 {
-	struct dentry *dentry = get_node(number);
-	struct tty_struct *tty;
-
-	tty = NULL;
-	if (!IS_ERR(dentry)) {
-		if (dentry->d_inode)
-			tty = dentry->d_inode->i_private;
-		dput(dentry);
-	}
-
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	BUG_ON(pts_inode->i_rdev == MKDEV(TTYAUX_MAJOR, PTMX_MINOR));
 
-	return tty;
+	if (pts_inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
+		return (struct tty_struct *)pts_inode->i_private;
+	return NULL;
 }
 
 void devpts_pty_kill(struct tty_struct *tty)

[RFC][PATCH 4/5]: Simplify devpts_pty_new()

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 4/5]: Simplify devpts_pty_new()

devpts_pty_new() is called when setting up a new pty and would not
will not have an existing dentry or inode for the pty. So don't bother
looking for an existing dentry - just create a new one.

---
 fs/devpts/inode.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Index: linux-2.6.27-rc3-tty/fs/devpts/inode.c
===================================================================
--- linux-2.6.27-rc3-tty.orig/fs/devpts/inode.c	2008-08-28 18:01:11.000000000 -0700
+++ linux-2.6.27-rc3-tty/fs/devpts/inode.c	2008-08-28 18:46:55.000000000 -0700
@@ -220,6 +220,7 @@ int devpts_pty_new(struct inode *ptmx_in
 	dev_t device = MKDEV(driver->major, driver->minor_start+number);
 	struct dentry *dentry;
 	struct inode *inode = new_inode(devpts_mnt->mnt_sb);
+	char s[12];
 
 	/* We're supposed to be given the slave end of a pty */
 	BUG_ON(driver->type != TTY_DRIVER_TYPE_PTY);
@@ -235,9 +236,13 @@ int devpts_pty_new(struct inode *ptmx_in
 	init_special_inode(inode, S_IFCHR|config.mode, device);
 	inode->i_private = tty;
 
-	dentry = get_node(number);
-	if (!IS_ERR(dentry) && !dentry->d_inode) {
-		d_instantiate(dentry, inode);
+	sprintf(s, "%d", number);
+
+	mutex_lock(&devpts_root->d_inode->i_mutex);
+
+	dentry = d_alloc_name(devpts_root, s);
+	if (!IS_ERR(dentry)) {
+		d_add(dentry, inode);
 		fsnotify_create(devpts_root->d_inode, dentry);
 	}

[RFC][PATCH 5/5]: Simplify devpts_pty_kill

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 5/5]: Simplify devpts_pty_kill

When creating a new pty, save the pty's inode in the tty->driver_data.
Use this inode in pty_kill() to identify the devpts instance. Since
we now have the inode for the pty, we can skip get_node() lookup and
remove the unused get_node().

TODO: 
	- check if the mutex_lock is needed in pty_kill().

---
 fs/devpts/inode.c |   29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

Index: linux-2.6.27-rc3-tty/fs/devpts/inode.c
===================================================================
--- linux-2.6.27-rc3-tty.orig/fs/devpts/inode.c	2008-08-28 18:46:55.000000000 -0700
+++ linux-2.6.27-rc3-tty/fs/devpts/inode.c	2008-08-28 18:47:08.000000000 -0700
@@ -170,14 +170,6 @@ static struct file_system_type devpts_fs
  * to the System V naming convention
  */
 
-static struct dentry *get_node(int num)
-{
-	char s[12];
-	struct dentry *root = devpts_root;
-	mutex_lock(&root->d_inode->i_mutex);
-	return lookup_one_len(s, root, sprintf(s, "%d", num));
-}
-
 int devpts_new_index(struct inode *ptmx_inode)
 {
 	int index;
@@ -235,6 +227,7 @@ int devpts_pty_new(struct inode *ptmx_in
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	init_special_inode(inode, S_IFCHR|config.mode, device);
 	inode->i_private = tty;
+	tty->driver_data = inode;
 
 	sprintf(s, "%d", number);
 
@@ -262,18 +255,20 @@ struct tty_struct *devpts_get_tty(struct
 
 void devpts_pty_kill(struct tty_struct *tty)
 {
-	int number = tty->index;
-	struct dentry *dentry = get_node(number);
+	struct inode *inode = tty->driver_data;
+	struct dentry *dentry;
 
-	if (!IS_ERR(dentry)) {
-		struct inode *inode = dentry->d_inode;
-		if (inode) {
-			inode->i_nlink--;
-			d_delete(dentry);
-			dput(dentry);
-		}
+	BUG_ON(inode->i_rdev == MKDEV(TTYAUX_MAJOR, PTMX_MINOR));
+
+	mutex_lock(&devpts_root->d_inode->i_mutex);
+
+	dentry = d_find_alias(inode);
+	if (dentry && !IS_ERR(dentry)) {
+		inode->i_nlink--;
+		d_delete(dentry);
 		dput(dentry);
 	}
+
 	mutex_unlock(&devpts_root->d_inode->i_mutex);
 }

Re: [RFC][PATCH 1/5]: Move tty lookup/reopen to caller

[Alan Cox]

On Fri, 29 Aug 2008 00:00:01 -0700
sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:

> get_node() cleanup in devpts
> 
> This patchset adds inode/tty parameter to devpts interfaces and uses
> this new parameter to remove the unnecessary get_node() calls in devpts
> (as pointed out by Peter Anvin).
> 
> Alan, if these are getting in your way, I can hold off for now, but
> just wanted some quick comments from you on patch 1 and from Peter
> on get_node() cleanup.

These all look good. Added to the ttydev tree stack - will need Sign offs.