[RFC PATCH 1/6] shmem: use alloc_file instead of init_file

[RFC PATCH 1/6] shmem: use alloc_file instead of init_file

[Eric Paris]

shmem uses get_empty_filp() and then init_file().  Their is no good reason
not to just use alloc_file() like everything else.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 mm/shmem.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 356dd99..831f8bb 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2640,32 +2640,32 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
 	if (!dentry)
 		goto put_memory;
 
-	error = -ENFILE;
-	file = get_empty_filp();
-	if (!file)
-		goto put_dentry;
-
 	error = -ENOSPC;
 	inode = shmem_get_inode(root->d_sb, S_IFREG | S_IRWXUGO, 0, flags);
 	if (!inode)
-		goto close_file;
+		goto put_dentry;
 
 	d_instantiate(dentry, inode);
 	inode->i_size = size;
 	inode->i_nlink = 0;	/* It is unlinked */
-	init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
-		  &shmem_file_operations);
+
+	error = -ENFILE;
+	file = alloc_file(shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
+			  &shmem_file_operations);
+	if (!file)
+		goto put_dentry;
 
 #ifndef CONFIG_MMU
 	error = ramfs_nommu_expand_for_mapping(inode, size);
 	if (error)
 		goto close_file;
 #endif
-	ima_counts_get(file);
 	return file;
 
+#ifndef CONFIG_MMU
 close_file:
-	put_filp(file);
+	fput(file);
+#endif
 put_dentry:
 	dput(dentry);
 put_memory:

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC PATCH 2/6] pipes: use alloc-file instead of duplicating code

[Eric Paris]

The pipe code duplicates the functionality of alloc-file and init-file.  Use
the generic vfs functions instead of duplicating code.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 fs/pipe.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index ae17d02..5d6c969 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1028,20 +1028,17 @@ void free_write_pipe(struct file *f)
 
 struct file *create_read_pipe(struct file *wrf, int flags)
 {
-	struct file *f = get_empty_filp();
-	if (!f)
-		return ERR_PTR(-ENFILE);
-
-	/* Grab pipe from the writer */
-	f->f_path = wrf->f_path;
-	path_get(&wrf->f_path);
-	f->f_mapping = wrf->f_path.dentry->d_inode->i_mapping;
+	struct file *f;
+	struct dentry *dentry = wrf->f_path.dentry;
+	struct vfsmount *mnt = wrf->f_path.mnt;
 
-	f->f_pos = 0;
+	dentry = dget(dentry);
+	f = alloc_file(mnt, dentry, FMODE_READ, &read_pipefifo_fops);
+	if (!f) {
+		dput(dentry);
+		return ERR_PTR(-ENFILE);
+	}
 	f->f_flags = O_RDONLY | (flags & O_NONBLOCK);
-	f->f_op = &read_pipefifo_fops;
-	f->f_mode = FMODE_READ;
-	f->f_version = 0;
 
 	return f;
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 2/6] pipes: use alloc-file instead of duplicating code

[Miklos Szeredi]

On Thu, 03 Dec 2009, Eric Paris wrote:
> The pipe code duplicates the functionality of alloc-file and init-file.  Use
> the generic vfs functions instead of duplicating code.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>

Acked-by: Miklos Szeredi <miklos@szeredi.hu>

As a side note: I wonder why we aren't passing a "struct path" to
alloc_file() and why are the refcount rules wrt. dentries/vfsmounts so
weird?

> ---
> 
>  fs/pipe.c |   21 +++++++++------------
>  1 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index ae17d02..5d6c969 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -1028,20 +1028,17 @@ void free_write_pipe(struct file *f)
>  
>  struct file *create_read_pipe(struct file *wrf, int flags)
>  {
> -	struct file *f = get_empty_filp();
> -	if (!f)
> -		return ERR_PTR(-ENFILE);
> -
> -	/* Grab pipe from the writer */
> -	f->f_path = wrf->f_path;
> -	path_get(&wrf->f_path);
> -	f->f_mapping = wrf->f_path.dentry->d_inode->i_mapping;
> +	struct file *f;
> +	struct dentry *dentry = wrf->f_path.dentry;
> +	struct vfsmount *mnt = wrf->f_path.mnt;
>  
> -	f->f_pos = 0;
> +	dentry = dget(dentry);
> +	f = alloc_file(mnt, dentry, FMODE_READ, &read_pipefifo_fops);
> +	if (!f) {
> +		dput(dentry);
> +		return ERR_PTR(-ENFILE);
> +	}
>  	f->f_flags = O_RDONLY | (flags & O_NONBLOCK);
> -	f->f_op = &read_pipefifo_fops;
> -	f->f_mode = FMODE_READ;
> -	f->f_version = 0;
>  
>  	return f;
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 2/6] pipes: use alloc-file instead of duplicating code

[Eric Paris]

On Fri, 2009-12-04 at 07:08 +0100, Miklos Szeredi wrote:
> On Thu, 03 Dec 2009, Eric Paris wrote:
> > The pipe code duplicates the functionality of alloc-file and init-file.  Use
> > the generic vfs functions instead of duplicating code.
> > 
> > Signed-off-by: Eric Paris <eparis@redhat.com>
> 
> Acked-by: Miklos Szeredi <miklos@szeredi.hu>
> 
> As a side note: I wonder why we aren't passing a "struct path" to
> alloc_file() and why are the refcount rules wrt. dentries/vfsmounts so
> weird?

It's probably because of the slightly weird refcnt rules that it asks
for the dentry and vfsmount separately rather than as a struct path.
The rules make perfect sense if you consider

d_alloc()  <-- reference on dentry
d_instantiate()
alloc_file() <-- reference on vfsmount
  so here file->f_path() is all good.

Which a number of callers user.  They make less sense when you consider
something that is not allocating the dentry right there (like this path)

dget(dentry);  <-- reference here
alloc_file() <-- reference on vfsmount;
  so here file->f_path is all good.

It would be a reasonable interface if it took a struct path and then
took a reference on the struct path.  The second case would look more
clean, but the first case would turn into

d_alloc()
d_instantiate()
alloc_file()
d_put() /* matches d_alloc() */

and

alloc_file()

Is this better?  I'll gladly do it if other think so it makes more
sense....

-Eric

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 2/6] pipes: use alloc-file instead of duplicating code

[Jens Axboe]

On Thu, Dec 03 2009, Eric Paris wrote:
> The pipe code duplicates the functionality of alloc-file and init-file.  Use
> the generic vfs functions instead of duplicating code.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>

Acked-by: Jens Axboe <jens.axboe@oracle.com>

-- 
Jens Axboe

[RFC PATCH 3/6] inotify: use alloc_file instead of doing it internally

[Eric Paris]

inotify basically duplicates everything from alloc-file and init-file.  Use
the generic vfs functions instead.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 fs/notify/inotify/inotify_user.c |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index c40894a..3e03803 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -725,6 +725,7 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
 	struct fsnotify_group *group;
 	struct user_struct *user;
 	struct file *filp;
+	struct dentry *dentry;
 	int fd, ret;
 
 	/* Check the IN_* constants for consistency.  */
@@ -738,12 +739,6 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
 	if (fd < 0)
 		return fd;
 
-	filp = get_empty_filp();
-	if (!filp) {
-		ret = -ENFILE;
-		goto out_put_fd;
-	}
-
 	user = get_current_user();
 	if (unlikely(atomic_read(&user->inotify_devs) >=
 			inotify_max_user_instances)) {
@@ -758,11 +753,12 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
 		goto out_free_uid;
 	}
 
-	filp->f_op = &inotify_fops;
-	filp->f_path.mnt = mntget(inotify_mnt);
-	filp->f_path.dentry = dget(inotify_mnt->mnt_root);
-	filp->f_mapping = filp->f_path.dentry->d_inode->i_mapping;
-	filp->f_mode = FMODE_READ;
+	dentry = dget(inotify_mnt->mnt_root);
+	filp = alloc_file(inotify_mnt, dentry, FMODE_READ, &inotify_fops);
+	if (!filp) {
+		ret = -ENFILE;
+		goto out_dput;
+	}
 	filp->f_flags = O_RDONLY | (flags & O_NONBLOCK);
 	filp->private_data = group;
 
@@ -771,11 +767,10 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
 	fd_install(fd, filp);
 
 	return fd;
-
+out_dput:
+	dput(dentry);
 out_free_uid:
 	free_uid(user);
-	put_filp(filp);
-out_put_fd:
 	put_unused_fd(fd);
 	return ret;
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 3/6] inotify: use alloc_file instead of doing it internally

[Miklos Szeredi]

On Thu, 03 Dec 2009, Eric Paris wrote:
> inotify basically duplicates everything from alloc-file and init-file.  Use
> the generic vfs functions instead.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>

Acked-by: Miklos Szeredi <miklos@szeredi.hu>

> ---
> 
>  fs/notify/inotify/inotify_user.c |   23 +++++++++--------------
>  1 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index c40894a..3e03803 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -725,6 +725,7 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
>  	struct fsnotify_group *group;
>  	struct user_struct *user;
>  	struct file *filp;
> +	struct dentry *dentry;
>  	int fd, ret;
>  
>  	/* Check the IN_* constants for consistency.  */
> @@ -738,12 +739,6 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
>  	if (fd < 0)
>  		return fd;
>  
> -	filp = get_empty_filp();
> -	if (!filp) {
> -		ret = -ENFILE;
> -		goto out_put_fd;
> -	}
> -
>  	user = get_current_user();
>  	if (unlikely(atomic_read(&user->inotify_devs) >=
>  			inotify_max_user_instances)) {
> @@ -758,11 +753,12 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
>  		goto out_free_uid;
>  	}
>  
> -	filp->f_op = &inotify_fops;
> -	filp->f_path.mnt = mntget(inotify_mnt);
> -	filp->f_path.dentry = dget(inotify_mnt->mnt_root);
> -	filp->f_mapping = filp->f_path.dentry->d_inode->i_mapping;
> -	filp->f_mode = FMODE_READ;
> +	dentry = dget(inotify_mnt->mnt_root);
> +	filp = alloc_file(inotify_mnt, dentry, FMODE_READ, &inotify_fops);
> +	if (!filp) {
> +		ret = -ENFILE;
> +		goto out_dput;
> +	}
>  	filp->f_flags = O_RDONLY | (flags & O_NONBLOCK);
>  	filp->private_data = group;
>  
> @@ -771,11 +767,10 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
>  	fd_install(fd, filp);
>  
>  	return fd;
> -
> +out_dput:
> +	dput(dentry);
>  out_free_uid:
>  	free_uid(user);
> -	put_filp(filp);
> -out_put_fd:
>  	put_unused_fd(fd);
>  	return ret;
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC PATCH 4/6] networking: rework socket to fd mapping using alloc-file

[Eric Paris]

Currently the networking code does interesting things allocating its struct
file and file descriptors.  This patch attempts to unify all of that and
simplify the error paths.  It is also a part of my patch series trying to get
rid of init-file and get-empty_filp and friends.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 net/socket.c |  122 +++++++++++++++++++++-------------------------------------
 1 files changed, 45 insertions(+), 77 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 402abb3..41ac0b1 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -355,32 +355,24 @@ static const struct dentry_operations sockfs_dentry_operations = {
  *	but we take care of internal coherence yet.
  */
 
-static int sock_alloc_fd(struct file **filep, int flags)
+static int sock_alloc_fd(struct file **filep, struct socket *sock, int flags)
 {
-	int fd;
-
-	fd = get_unused_fd_flags(flags);
-	if (likely(fd >= 0)) {
-		struct file *file = get_empty_filp();
-
-		*filep = file;
-		if (unlikely(!file)) {
-			put_unused_fd(fd);
-			return -ENFILE;
-		}
-	} else
-		*filep = NULL;
-	return fd;
-}
-
-static int sock_attach_fd(struct socket *sock, struct file *file, int flags)
-{
-	struct dentry *dentry;
+	int fd, rc;
+	struct file *file;
+	struct dentry *dentry = NULL;
 	struct qstr name = { .name = "" };
 
+	fd = get_unused_fd_flags(flags & O_CLOEXEC);
+	if (unlikely(fd < 0)) {
+		rc = fd;
+		goto out_err;
+	}
+
 	dentry = d_alloc(sock_mnt->mnt_sb->s_root, &name);
-	if (unlikely(!dentry))
-		return -ENOMEM;
+	if (unlikely(!dentry)) {
+		rc = -ENOMEM;
+		goto out_err;
+	}
 
 	dentry->d_op = &sockfs_dentry_operations;
 	/*
@@ -391,32 +383,37 @@ static int sock_attach_fd(struct socket *sock, struct file *file, int flags)
 	dentry->d_flags &= ~DCACHE_UNHASHED;
 	d_instantiate(dentry, SOCK_INODE(sock));
 
+	file = alloc_file(sock_mnt, dentry, FMODE_READ | FMODE_WRITE,
+			  &socket_file_ops);
+	if (unlikely(!file)) {
+		rc = -ENFILE;
+		goto out_err;
+	}
+
 	sock->file = file;
-	init_file(file, sock_mnt, dentry, FMODE_READ | FMODE_WRITE,
-		  &socket_file_ops);
 	SOCK_INODE(sock)->i_fop = &socket_file_ops;
 	file->f_flags = O_RDWR | (flags & O_NONBLOCK);
-	file->f_pos = 0;
 	file->private_data = sock;
 
-	return 0;
+	return fd;
+out_err:
+	if (fd >= 0)
+		put_unused_fd(fd);
+	if (dentry)
+		dput(dentry);
+	*filep = NULL;
+	return rc;
 }
 
 int sock_map_fd(struct socket *sock, int flags)
 {
 	struct file *newfile;
-	int fd = sock_alloc_fd(&newfile, flags);
-
-	if (likely(fd >= 0)) {
-		int err = sock_attach_fd(sock, newfile, flags);
+	int fd;
 
-		if (unlikely(err < 0)) {
-			put_filp(newfile);
-			put_unused_fd(fd);
-			return err;
-		}
+	fd = sock_alloc_fd(&newfile, sock, flags);
+	if (likely(fd >= 0))
 		fd_install(fd, newfile);
-	}
+
 	return fd;
 }
 
@@ -1384,35 +1381,22 @@ SYSCALL_DEFINE4(socketpair, int, family, int, type, int, protocol,
 
 	err = sock_create(family, type, protocol, &sock2);
 	if (err < 0)
-		goto out_release_1;
+		goto out_release_sock_1;
 
 	err = sock1->ops->socketpair(sock1, sock2);
 	if (err < 0)
-		goto out_release_both;
+		goto out_release_sock_2;
 
-	fd1 = sock_alloc_fd(&newfile1, flags & O_CLOEXEC);
+	fd1 = sock_alloc_fd(&newfile1, sock1, flags & (O_CLOEXEC | O_NONBLOCK));
 	if (unlikely(fd1 < 0)) {
 		err = fd1;
-		goto out_release_both;
+		goto out_release_sock_2;
 	}
 
-	fd2 = sock_alloc_fd(&newfile2, flags & O_CLOEXEC);
+	fd2 = sock_alloc_fd(&newfile2, sock2, flags & (O_CLOEXEC | O_NONBLOCK));
 	if (unlikely(fd2 < 0)) {
 		err = fd2;
-		put_filp(newfile1);
-		put_unused_fd(fd1);
-		goto out_release_both;
-	}
-
-	err = sock_attach_fd(sock1, newfile1, flags & O_NONBLOCK);
-	if (unlikely(err < 0)) {
-		goto out_fd2;
-	}
-
-	err = sock_attach_fd(sock2, newfile2, flags & O_NONBLOCK);
-	if (unlikely(err < 0)) {
-		fput(newfile1);
-		goto out_fd1;
+		goto out_release_fd_1;
 	}
 
 	audit_fd_pair(fd1, fd2);
@@ -1432,22 +1416,15 @@ SYSCALL_DEFINE4(socketpair, int, family, int, type, int, protocol,
 	sys_close(fd1);
 	return err;
 
-out_release_both:
+out_release_fd_1:
+	fput(newfile1);
+	put_unused_fd(fd1);
+out_release_sock_2:
 	sock_release(sock2);
-out_release_1:
+out_release_sock_1:
 	sock_release(sock1);
 out:
 	return err;
-
-out_fd2:
-	put_filp(newfile1);
-	sock_release(sock1);
-out_fd1:
-	put_filp(newfile2);
-	sock_release(sock2);
-	put_unused_fd(fd1);
-	put_unused_fd(fd2);
-	goto out;
 }
 
 /*
@@ -1551,17 +1528,13 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
 	 */
 	__module_get(newsock->ops->owner);
 
-	newfd = sock_alloc_fd(&newfile, flags & O_CLOEXEC);
+	newfd = sock_alloc_fd(&newfile, newsock, flags & (O_CLOEXEC | O_NONBLOCK));
 	if (unlikely(newfd < 0)) {
 		err = newfd;
 		sock_release(newsock);
 		goto out_put;
 	}
 
-	err = sock_attach_fd(newsock, newfile, flags & O_NONBLOCK);
-	if (err < 0)
-		goto out_fd_simple;
-
 	err = security_socket_accept(sock, newsock);
 	if (err)
 		goto out_fd;
@@ -1591,11 +1564,6 @@ out_put:
 	fput_light(sock->file, fput_needed);
 out:
 	return err;
-out_fd_simple:
-	sock_release(newsock);
-	put_filp(newfile);
-	put_unused_fd(newfd);
-	goto out_put;
 out_fd:
 	fput(newfile);
 	put_unused_fd(newfd);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 4/6] networking: rework socket to fd mapping using alloc-file

[David Miller]

From: Eric Paris <eparis@redhat.com>
Date: Thu, 03 Dec 2009 14:59:17 -0500

> Currently the networking code does interesting things allocating its struct
> file and file descriptors.  This patch attempts to unify all of that and
> simplify the error paths.  It is also a part of my patch series trying to get
> rid of init-file and get-empty_filp and friends.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>

I'm fine with this:

Acked-by: David S. Miller <davem@davemloft.net>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 4/6] networking: rework socket to fd mapping using alloc-file

[Eric Paris]

On Thu, 2009-12-03 at 14:00 -0800, David Miller wrote:
> From: Eric Paris <eparis@redhat.com>
> Date: Thu, 03 Dec 2009 14:59:17 -0500
> 
> > Currently the networking code does interesting things allocating its struct
> > file and file descriptors.  This patch attempts to unify all of that and
> > simplify the error paths.  It is also a part of my patch series trying to get
> > rid of init-file and get-empty_filp and friends.
> > 
> > Signed-off-by: Eric Paris <eparis@redhat.com>
> 
> I'm fine with this:
> 
> Acked-by: David S. Miller <davem@davemloft.net>

It's actually busted, I forgot to actually pass back the new file in
sock_alloc_fd().  But I've got a fixed version and will resend the
series once I see other comments....

inc diff below in case anyone is trying to test this series.

diff --git a/net/socket.c b/net/socket.c
index 41ac0b1..6620421 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -390,6 +390,7 @@ static int sock_alloc_fd(struct file **filep, struct
socket *sock, int flags)
                goto out_err;
        }
 
+       *filep = file;
        sock->file = file;
        SOCK_INODE(sock)->i_fop = &socket_file_ops;
        file->f_flags = O_RDWR | (flags & O_NONBLOCK);



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 4/6] networking: rework socket to fd mapping using alloc-file

[Miklos Szeredi]

On Thu, 03 Dec 2009, Eric Paris wrote:
> @@ -391,32 +383,37 @@ static int sock_attach_fd(struct socket *sock, struct file *file, int flags)
>  	dentry->d_flags &= ~DCACHE_UNHASHED;
>  	d_instantiate(dentry, SOCK_INODE(sock));
>  
> +	file = alloc_file(sock_mnt, dentry, FMODE_READ | FMODE_WRITE,
> +			  &socket_file_ops);
> +	if (unlikely(!file)) {
> +		rc = -ENFILE;
> +		goto out_err;
> +	}
> +
>  	sock->file = file;
> -	init_file(file, sock_mnt, dentry, FMODE_READ | FMODE_WRITE,
> -		  &socket_file_ops);
>  	SOCK_INODE(sock)->i_fop = &socket_file_ops;
>  	file->f_flags = O_RDWR | (flags & O_NONBLOCK);
> -	file->f_pos = 0;
>  	file->private_data = sock;
>  
> -	return 0;
> +	return fd;
> +out_err:
> +	if (fd >= 0)
> +		put_unused_fd(fd);
> +	if (dentry)
> +		dput(dentry);

Could you please use separate labels intead of conditionals here?

Thanks,
Miklos

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC PATCH 5/6] vfs: make init-file static

[Eric Paris]

init-file is no longer used by anything except alloc_file.  Make it static and
remove from headers.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 fs/file_table.c      |   73 ++++++++++++++++++++++----------------------------
 include/linux/file.h |    3 --
 2 files changed, 32 insertions(+), 44 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 4bef4c0..0f9d2f2 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -150,53 +150,16 @@ fail:
 EXPORT_SYMBOL(get_empty_filp);
 
 /**
- * alloc_file - allocate and initialize a 'struct file'
- * @mnt: the vfsmount on which the file will reside
- * @dentry: the dentry representing the new file
- * @mode: the mode with which the new file will be opened
- * @fop: the 'struct file_operations' for the new file
- *
- * Use this instead of get_empty_filp() to get a new
- * 'struct file'.  Do so because of the same initialization
- * pitfalls reasons listed for init_file().  This is a
- * preferred interface to using init_file().
- *
- * If all the callers of init_file() are eliminated, its
- * code should be moved into this function.
- */
-struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
-		fmode_t mode, const struct file_operations *fop)
-{
-	struct file *file;
-
-	file = get_empty_filp();
-	if (!file)
-		return NULL;
-
-	init_file(file, mnt, dentry, mode, fop);
-	return file;
-}
-EXPORT_SYMBOL(alloc_file);
-
-/**
  * init_file - initialize a 'struct file'
  * @file: the already allocated 'struct file' to initialized
  * @mnt: the vfsmount on which the file resides
  * @dentry: the dentry representing this file
  * @mode: the mode the file is opened with
  * @fop: the 'struct file_operations' for this file
- *
- * Use this instead of setting the members directly.  Doing so
- * avoids making mistakes like forgetting the mntget() or
- * forgetting to take a write on the mnt.
- *
- * Note: This is a crappy interface.  It is here to make
- * merging with the existing users of get_empty_filp()
- * who have complex failure logic easier.  All users
- * of this should be moving to alloc_file().
  */
-int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
-	   fmode_t mode, const struct file_operations *fop)
+static int init_file(struct file *file, struct vfsmount *mnt,
+		     struct dentry *dentry, fmode_t mode,
+		     const struct file_operations *fop)
 {
 	int error = 0;
 	file->f_path.dentry = dentry;
@@ -218,7 +181,35 @@ int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
 	}
 	return error;
 }
-EXPORT_SYMBOL(init_file);
+
+/**
+ * alloc_file - allocate and initialize a 'struct file'
+ * @mnt: the vfsmount on which the file will reside
+ * @dentry: the dentry representing the new file
+ * @mode: the mode with which the new file will be opened
+ * @fop: the 'struct file_operations' for the new file
+ *
+ * Use this instead of get_empty_filp() to get a new
+ * 'struct file'.  Do so because of the same initialization
+ * pitfalls reasons listed for init_file().  This is a
+ * preferred interface to using init_file().
+ *
+ * If all the callers of init_file() are eliminated, its
+ * code should be moved into this function.
+ */
+struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
+		fmode_t mode, const struct file_operations *fop)
+{
+	struct file *file;
+
+	file = get_empty_filp();
+	if (!file)
+		return NULL;
+
+	init_file(file, mnt, dentry, mode, fop);
+	return file;
+}
+EXPORT_SYMBOL(alloc_file);
 
 void fput(struct file *file)
 {
diff --git a/include/linux/file.h b/include/linux/file.h
index 335a0a5..6a8d361 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -18,9 +18,6 @@ extern void drop_file_write_access(struct file *file);
 struct file_operations;
 struct vfsmount;
 struct dentry;
-extern int init_file(struct file *, struct vfsmount *mnt,
-		struct dentry *dentry, fmode_t mode,
-		const struct file_operations *fop);
 extern struct file *alloc_file(struct vfsmount *, struct dentry *dentry,
 		fmode_t mode, const struct file_operations *fop);
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 5/6] vfs: make init-file static

[Serge E. Hallyn]

Quoting Eric Paris (eparis@redhat.com):
> init-file is no longer used by anything except alloc_file.  Make it static and
> remove from headers.

Should these go through a deprecation period?  (Same for the next patch)

> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
> 
>  fs/file_table.c      |   73 ++++++++++++++++++++++----------------------------
>  include/linux/file.h |    3 --
>  2 files changed, 32 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 4bef4c0..0f9d2f2 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -150,53 +150,16 @@ fail:
>  EXPORT_SYMBOL(get_empty_filp);
> 
>  /**
> - * alloc_file - allocate and initialize a 'struct file'
> - * @mnt: the vfsmount on which the file will reside
> - * @dentry: the dentry representing the new file
> - * @mode: the mode with which the new file will be opened
> - * @fop: the 'struct file_operations' for the new file
> - *
> - * Use this instead of get_empty_filp() to get a new
> - * 'struct file'.  Do so because of the same initialization
> - * pitfalls reasons listed for init_file().  This is a
> - * preferred interface to using init_file().
> - *
> - * If all the callers of init_file() are eliminated, its
> - * code should be moved into this function.
> - */
> -struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
> -		fmode_t mode, const struct file_operations *fop)
> -{
> -	struct file *file;
> -
> -	file = get_empty_filp();
> -	if (!file)
> -		return NULL;
> -
> -	init_file(file, mnt, dentry, mode, fop);
> -	return file;
> -}
> -EXPORT_SYMBOL(alloc_file);
> -
> -/**
>   * init_file - initialize a 'struct file'
>   * @file: the already allocated 'struct file' to initialized
>   * @mnt: the vfsmount on which the file resides
>   * @dentry: the dentry representing this file
>   * @mode: the mode the file is opened with
>   * @fop: the 'struct file_operations' for this file
> - *
> - * Use this instead of setting the members directly.  Doing so
> - * avoids making mistakes like forgetting the mntget() or
> - * forgetting to take a write on the mnt.
> - *
> - * Note: This is a crappy interface.  It is here to make
> - * merging with the existing users of get_empty_filp()
> - * who have complex failure logic easier.  All users
> - * of this should be moving to alloc_file().
>   */
> -int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
> -	   fmode_t mode, const struct file_operations *fop)
> +static int init_file(struct file *file, struct vfsmount *mnt,
> +		     struct dentry *dentry, fmode_t mode,
> +		     const struct file_operations *fop)
>  {
>  	int error = 0;
>  	file->f_path.dentry = dentry;
> @@ -218,7 +181,35 @@ int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
>  	}
>  	return error;
>  }
> -EXPORT_SYMBOL(init_file);
> +
> +/**
> + * alloc_file - allocate and initialize a 'struct file'
> + * @mnt: the vfsmount on which the file will reside
> + * @dentry: the dentry representing the new file
> + * @mode: the mode with which the new file will be opened
> + * @fop: the 'struct file_operations' for the new file
> + *
> + * Use this instead of get_empty_filp() to get a new
> + * 'struct file'.  Do so because of the same initialization
> + * pitfalls reasons listed for init_file().  This is a
> + * preferred interface to using init_file().
> + *
> + * If all the callers of init_file() are eliminated, its
> + * code should be moved into this function.
> + */
> +struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
> +		fmode_t mode, const struct file_operations *fop)
> +{
> +	struct file *file;
> +
> +	file = get_empty_filp();
> +	if (!file)
> +		return NULL;
> +
> +	init_file(file, mnt, dentry, mode, fop);
> +	return file;
> +}
> +EXPORT_SYMBOL(alloc_file);
> 
>  void fput(struct file *file)
>  {
> diff --git a/include/linux/file.h b/include/linux/file.h
> index 335a0a5..6a8d361 100644
> --- a/include/linux/file.h
> +++ b/include/linux/file.h
> @@ -18,9 +18,6 @@ extern void drop_file_write_access(struct file *file);
>  struct file_operations;
>  struct vfsmount;
>  struct dentry;
> -extern int init_file(struct file *, struct vfsmount *mnt,
> -		struct dentry *dentry, fmode_t mode,
> -		const struct file_operations *fop);
>  extern struct file *alloc_file(struct vfsmount *, struct dentry *dentry,
>  		fmode_t mode, const struct file_operations *fop);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 5/6] vfs: make init-file static

[Nick Piggin]

On Thu, Dec 03, 2009 at 04:44:02PM -0600, Serge E. Hallyn wrote:
> Quoting Eric Paris (eparis@redhat.com):
> > init-file is no longer used by anything except alloc_file.  Make it static and
> > remove from headers.

These look like pretty good changes to me, FWIW. I had found myself
wondering about this recently so it's good to see it improved.


> Should these go through a deprecation period?  (Same for the next patch)

Maybe waiting a release or two couldn't hurt. Unless Eric you
have some other changes in mind after these?

 
> > Signed-off-by: Eric Paris <eparis@redhat.com>
> > ---
> > 
> >  fs/file_table.c      |   73 ++++++++++++++++++++++----------------------------
> >  include/linux/file.h |    3 --
> >  2 files changed, 32 insertions(+), 44 deletions(-)
> > 
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index 4bef4c0..0f9d2f2 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -150,53 +150,16 @@ fail:
> >  EXPORT_SYMBOL(get_empty_filp);
> > 
> >  /**
> > - * alloc_file - allocate and initialize a 'struct file'
> > - * @mnt: the vfsmount on which the file will reside
> > - * @dentry: the dentry representing the new file
> > - * @mode: the mode with which the new file will be opened
> > - * @fop: the 'struct file_operations' for the new file
> > - *
> > - * Use this instead of get_empty_filp() to get a new
> > - * 'struct file'.  Do so because of the same initialization
> > - * pitfalls reasons listed for init_file().  This is a
> > - * preferred interface to using init_file().
> > - *
> > - * If all the callers of init_file() are eliminated, its
> > - * code should be moved into this function.
> > - */
> > -struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
> > -		fmode_t mode, const struct file_operations *fop)
> > -{
> > -	struct file *file;
> > -
> > -	file = get_empty_filp();
> > -	if (!file)
> > -		return NULL;
> > -
> > -	init_file(file, mnt, dentry, mode, fop);
> > -	return file;
> > -}
> > -EXPORT_SYMBOL(alloc_file);
> > -
> > -/**
> >   * init_file - initialize a 'struct file'
> >   * @file: the already allocated 'struct file' to initialized
> >   * @mnt: the vfsmount on which the file resides
> >   * @dentry: the dentry representing this file
> >   * @mode: the mode the file is opened with
> >   * @fop: the 'struct file_operations' for this file
> > - *
> > - * Use this instead of setting the members directly.  Doing so
> > - * avoids making mistakes like forgetting the mntget() or
> > - * forgetting to take a write on the mnt.
> > - *
> > - * Note: This is a crappy interface.  It is here to make
> > - * merging with the existing users of get_empty_filp()
> > - * who have complex failure logic easier.  All users
> > - * of this should be moving to alloc_file().
> >   */
> > -int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
> > -	   fmode_t mode, const struct file_operations *fop)
> > +static int init_file(struct file *file, struct vfsmount *mnt,
> > +		     struct dentry *dentry, fmode_t mode,
> > +		     const struct file_operations *fop)
> >  {
> >  	int error = 0;
> >  	file->f_path.dentry = dentry;
> > @@ -218,7 +181,35 @@ int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
> >  	}
> >  	return error;
> >  }
> > -EXPORT_SYMBOL(init_file);
> > +
> > +/**
> > + * alloc_file - allocate and initialize a 'struct file'
> > + * @mnt: the vfsmount on which the file will reside
> > + * @dentry: the dentry representing the new file
> > + * @mode: the mode with which the new file will be opened
> > + * @fop: the 'struct file_operations' for the new file
> > + *
> > + * Use this instead of get_empty_filp() to get a new
> > + * 'struct file'.  Do so because of the same initialization
> > + * pitfalls reasons listed for init_file().  This is a
> > + * preferred interface to using init_file().
> > + *
> > + * If all the callers of init_file() are eliminated, its
> > + * code should be moved into this function.
> > + */
> > +struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
> > +		fmode_t mode, const struct file_operations *fop)
> > +{
> > +	struct file *file;
> > +
> > +	file = get_empty_filp();
> > +	if (!file)
> > +		return NULL;
> > +
> > +	init_file(file, mnt, dentry, mode, fop);
> > +	return file;
> > +}
> > +EXPORT_SYMBOL(alloc_file);
> > 
> >  void fput(struct file *file)
> >  {
> > diff --git a/include/linux/file.h b/include/linux/file.h
> > index 335a0a5..6a8d361 100644
> > --- a/include/linux/file.h
> > +++ b/include/linux/file.h
> > @@ -18,9 +18,6 @@ extern void drop_file_write_access(struct file *file);
> >  struct file_operations;
> >  struct vfsmount;
> >  struct dentry;
> > -extern int init_file(struct file *, struct vfsmount *mnt,
> > -		struct dentry *dentry, fmode_t mode,
> > -		const struct file_operations *fop);
> >  extern struct file *alloc_file(struct vfsmount *, struct dentry *dentry,
> >  		fmode_t mode, const struct file_operations *fop);
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC PATCH 6/6] fs: move get_empty_filp() deffinition to internal.h

[Eric Paris]

All users outside of fs/ of get_empty_filp() have been removed.  This patch
moves the definition from the include/ directory to internal.h so no new
users crop up and removes the EXPORT_SYMBOL.  I'd love to see open intents
stop using it too, but that's a problem for another day and a smarter
developer!

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 fs/file_table.c    |    4 ++--
 fs/internal.h      |    1 +
 fs/namei.c         |    2 ++
 fs/open.c          |    2 ++
 include/linux/fs.h |    1 -
 5 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 0f9d2f2..629a167 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -24,6 +24,8 @@
 
 #include <asm/atomic.h>
 
+#include "internal.h"
+
 /* sysctl tunables... */
 struct files_stat_struct files_stat = {
 	.max_files = NR_FILE
@@ -147,8 +149,6 @@ fail:
 	return NULL;
 }
 
-EXPORT_SYMBOL(get_empty_filp);
-
 /**
  * init_file - initialize a 'struct file'
  * @file: the already allocated 'struct file' to initialized
diff --git a/fs/internal.h b/fs/internal.h
index 515175b..f67cd14 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -79,6 +79,7 @@ extern void chroot_fs_refs(struct path *, struct path *);
  * file_table.c
  */
 extern void mark_files_ro(struct super_block *);
+extern struct file *get_empty_filp(void);
 
 /*
  * super.c
diff --git a/fs/namei.c b/fs/namei.c
index 87f97ba..d7ecd2f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -35,6 +35,8 @@
 #include <linux/fs_struct.h>
 #include <asm/uaccess.h>
 
+#include "internal.h"
+
 #define ACC_MODE(x) ("\000\004\002\006"[(x)&O_ACCMODE])
 
 /* [Feb-1997 T. Schoebel-Theuer]
diff --git a/fs/open.c b/fs/open.c
index fa3bf4c..ebb74d4 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -31,6 +31,8 @@
 #include <linux/falloc.h>
 #include <linux/fs_struct.h>
 
+#include "internal.h"
+
 int vfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 {
 	int retval = -ENODEV;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5de1bab..b8ed6bf 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2192,7 +2192,6 @@ static inline void insert_inode_hash(struct inode *inode) {
 	__insert_inode_hash(inode, inode->i_ino);
 }
 
-extern struct file * get_empty_filp(void);
 extern void file_move(struct file *f, struct list_head *list);
 extern void file_kill(struct file *f);
 #ifdef CONFIG_BLOCK

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 6/6] fs: move get_empty_filp() deffinition to internal.h

[Miklos Szeredi]

On Thu, 03 Dec 2009, Eric Paris wrote:
> All users outside of fs/ of get_empty_filp() have been removed.  This patch
> moves the definition from the include/ directory to internal.h so no new
> users crop up and removes the EXPORT_SYMBOL.  I'd love to see open intents
> stop using it too, but that's a problem for another day and a smarter
> developer!

ACK for 5/6 and 6/6.

Thanks,
Miklos

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 1/6] shmem: use alloc_file instead of init_file

[Miklos Szeredi]

On Thu, 03 Dec 2009, Eric Paris wrote:
> shmem uses get_empty_filp() and then init_file().  Their is no good reason
> not to just use alloc_file() like everything else.

There's a more in this patch, though, and none of that is explained...

> 
> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
> 
>  mm/shmem.c |   20 ++++++++++----------
>  1 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 356dd99..831f8bb 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2640,32 +2640,32 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
>  	if (!dentry)
>  		goto put_memory;
>  
> -	error = -ENFILE;
> -	file = get_empty_filp();
> -	if (!file)
> -		goto put_dentry;
> -
>  	error = -ENOSPC;
>  	inode = shmem_get_inode(root->d_sb, S_IFREG | S_IRWXUGO, 0, flags);
>  	if (!inode)
> -		goto close_file;
> +		goto put_dentry;
>  
>  	d_instantiate(dentry, inode);
>  	inode->i_size = size;
>  	inode->i_nlink = 0;	/* It is unlinked */
> -	init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> -		  &shmem_file_operations);
> +
> +	error = -ENFILE;
> +	file = alloc_file(shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> +			  &shmem_file_operations);
> +	if (!file)
> +		goto put_dentry;
>  
>  #ifndef CONFIG_MMU
>  	error = ramfs_nommu_expand_for_mapping(inode, size);
>  	if (error)
>  		goto close_file;
>  #endif
> -	ima_counts_get(file);

Where's this gone?

>  	return file;
>  
> +#ifndef CONFIG_MMU
>  close_file:

I suggest moving this piece of cleanup into the ifdef above, instead
of adding more ifdefs.

> -	put_filp(file);
> +	fput(file);

OK, put_filp() seems to have been wrong here, but please document it
in the changelog.

> +#endif
>  put_dentry:
>  	dput(dentry);
>  put_memory:
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 1/6] shmem: use alloc_file instead of init_file

[Eric Paris]

On Fri, 2009-12-04 at 06:58 +0100, Miklos Szeredi wrote:
> On Thu, 03 Dec 2009, Eric Paris wrote:
> > shmem uses get_empty_filp() and then init_file().  Their is no good reason
> > not to just use alloc_file() like everything else.
> 
> There's a more in this patch, though, and none of that is explained...
> 
> > 
> > Signed-off-by: Eric Paris <eparis@redhat.com>
> > ---
> > 
> >  mm/shmem.c |   20 ++++++++++----------
> >  1 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 356dd99..831f8bb 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2640,32 +2640,32 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
> >  	if (!dentry)
> >  		goto put_memory;
> >  
> > -	error = -ENFILE;
> > -	file = get_empty_filp();
> > -	if (!file)
> > -		goto put_dentry;
> > -
> >  	error = -ENOSPC;
> >  	inode = shmem_get_inode(root->d_sb, S_IFREG | S_IRWXUGO, 0, flags);
> >  	if (!inode)
> > -		goto close_file;
> > +		goto put_dentry;
> >  
> >  	d_instantiate(dentry, inode);
> >  	inode->i_size = size;
> >  	inode->i_nlink = 0;	/* It is unlinked */
> > -	init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> > -		  &shmem_file_operations);
> > +
> > +	error = -ENFILE;
> > +	file = alloc_file(shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> > +			  &shmem_file_operations);
> > +	if (!file)
> > +		goto put_dentry;
> >  
> >  #ifndef CONFIG_MMU
> >  	error = ramfs_nommu_expand_for_mapping(inode, size);
> >  	if (error)
> >  		goto close_file;
> >  #endif
> > -	ima_counts_get(file);
> 
> Where's this gone?

That's the impetuous for the whole patch series.  I originally did the
ima rework first an the vfs changes second, but decided to push the vfs
stuff first and I guess I forgot to back out this bit of ima change I
had done.  This will be dropped for actual submission. 

> >  	return file;
> >  
> > +#ifndef CONFIG_MMU
> >  close_file:
> 
> I suggest moving this piece of cleanup into the ifdef above, instead
> of adding more ifdefs.

Ok.

> > -	put_filp(file);
> > +	fput(file);
> 
> OK, put_filp() seems to have been wrong here, but please document it
> in the changelog.

It was only wrong in the ifndef !CONFIG_MMU case.  The other error path
which ended up here used it correctly.  In any case, I'll mention it in
the changelog.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 1/6] shmem: use alloc_file instead of init_file

[Serge E. Hallyn]

Quoting Eric Paris (eparis@redhat.com):
> shmem uses get_empty_filp() and then init_file().  Their is no good reason
> not to just use alloc_file() like everything else.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>

So,

Acked-by: Serge Hallyn <serue@us.ibm.com>

to the first 3 patches.  I'll review #4 when you resend.  In principle, ack
also to 5 and 6, but for the sake of out-of-tree filesystems I think deprecating
for a version or two would be worthwhile.  Of course, if your ima patches also
go through, then the out-of-three filesystems will spit out ima warnings anyway,
but they can consider that further pursuation to switch :)

Thanks, Eric.

-serge

> ---
> 
>  mm/shmem.c |   20 ++++++++++----------
>  1 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 356dd99..831f8bb 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2640,32 +2640,32 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
>  	if (!dentry)
>  		goto put_memory;
> 
> -	error = -ENFILE;
> -	file = get_empty_filp();
> -	if (!file)
> -		goto put_dentry;
> -
>  	error = -ENOSPC;
>  	inode = shmem_get_inode(root->d_sb, S_IFREG | S_IRWXUGO, 0, flags);
>  	if (!inode)
> -		goto close_file;
> +		goto put_dentry;
> 
>  	d_instantiate(dentry, inode);
>  	inode->i_size = size;
>  	inode->i_nlink = 0;	/* It is unlinked */
> -	init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> -		  &shmem_file_operations);
> +
> +	error = -ENFILE;
> +	file = alloc_file(shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> +			  &shmem_file_operations);
> +	if (!file)
> +		goto put_dentry;
> 
>  #ifndef CONFIG_MMU
>  	error = ramfs_nommu_expand_for_mapping(inode, size);
>  	if (error)
>  		goto close_file;
>  #endif
> -	ima_counts_get(file);
>  	return file;
> 
> +#ifndef CONFIG_MMU
>  close_file:
> -	put_filp(file);
> +	fput(file);
> +#endif
>  put_dentry:
>  	dput(dentry);
>  put_memory: