Re: [RFC][PATCH] filesystem helpers for custom 'struct file's

[Christoph Hellwig <hch@infradead.org>]

On Wed, Sep 20, 2006 at 08:36:38AM -0700, Dave Hansen wrote:
> Some filesystems forego the vfs and may_open() and create their
> own 'struct file's.
> 
> This patch creates a couple of helper functions which can be
> used by these filesystems, and will provide a unified place
> which the r/o bind mount code may patch.
> 
> Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
> ---
> 
>  lxc-dave/include/linux/file.h |    8 ++++++++
>  lxc-dave/fs/file_table.c      |   30 ++++++++++++++++++++++++++++++
>  lxc-dave/fs/hugetlbfs/inode.c |   22 +++++++++-------------
>  lxc-dave/mm/shmem.c           |    7 ++-----
>  lxc-dave/mm/tiny-shmem.c      |   24 +++++++++---------------
>  lxc-dave/net/socket.c         |   17 ++++++++---------
>  6 files changed, 66 insertions(+), 42 deletions(-)
> 
> diff -puN include/linux/file.h~B0-helpers-for-custom-struct_files include/linux/file.h
> --- lxc/include/linux/file.h~B0-helpers-for-custom-struct_files	2006-09-19 10:36:47.000000000 -0700
> +++ lxc-dave/include/linux/file.h	2006-09-19 10:36:57.000000000 -0700
> @@ -67,6 +67,14 @@ struct files_struct {
>  extern void FASTCALL(__fput(struct file *));
>  extern void FASTCALL(fput(struct file *));
>  
> +struct file_operations;
> +struct vfsmount;
> +struct dentry;
> +extern int f_init(struct file *, struct vfsmount *, struct dentry *dentry,
> +		mode_t mode, const struct file_operations *fop);
> +extern struct file *f_alloc(struct vfsmount *, struct dentry *dentry,
> +		mode_t mode, const struct file_operations *fop);
> +

To be honest I really hate this names.  init_file and alloc_file sound
a lot better.

> +int f_init(struct file *file, struct vfsmount *mnt,
> +	   struct dentry *dentry, mode_t mode,
> +	   const struct file_operations *fop)
> +{
> +	int error = 0;
> +	file->f_vfsmnt = mntget(mnt);
> +	file->f_dentry = dentry;
> +	file->f_mapping = dentry->d_inode->i_mapping;
> +	file->f_mode = mode;
> +	file->f_op = fop;
> +	return error;
> +}
> +
> +EXPORT_SYMBOL(f_init);

Do we really need this one?  I think we should only need the
alloc version.

> diff -puN mm/shmem.c~B0-helpers-for-custom-struct_files mm/shmem.c
> --- lxc/mm/shmem.c~B0-helpers-for-custom-struct_files	2006-09-19 10:36:47.000000000 -0700
> +++ lxc-dave/mm/shmem.c	2006-09-19 10:36:57.000000000 -0700
> @@ -2411,11 +2411,8 @@ struct file *shmem_file_setup(char *name
>  	d_instantiate(dentry, inode);
>  	inode->i_size = size;
>  	inode->i_nlink = 0;	/* It is unlinked */
> -	file->f_vfsmnt = mntget(shm_mnt);
> -	file->f_dentry = dentry;
> -	file->f_mapping = inode->i_mapping;
> -	file->f_op = &shmem_file_operations;
> -	file->f_mode = FMODE_WRITE | FMODE_READ;
> +	f_init(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> +			&shmem_file_operations);
>  	return file;
>  

This should be able to use the alloc variant by moving the file allocation
after the inode allocation.  Which makes a lot of sense anyway as
shmem_get_inode can fail for accounting reasons.

>  {
> +	struct dentry *dentry;
>  	struct qstr this;
>  	char name[32];
>  
> @@ -356,18 +357,16 @@ static int sock_attach_fd(struct socket 
>  	this.name = name;
>  	this.hash = SOCK_INODE(sock)->i_ino;
>  
> -	file->f_dentry = d_alloc(sock_mnt->mnt_sb->s_root, &this);
> -	if (unlikely(!file->f_dentry))
> +	dentry = d_alloc(sock_mnt->mnt_sb->s_root, &this);
> +	if (unlikely(!dentry))
>  		return -ENOMEM;
>  
> -	file->f_dentry->d_op = &sockfs_dentry_operations;
> -	d_add(file->f_dentry, SOCK_INODE(sock));
> -	file->f_vfsmnt = mntget(sock_mnt);
> -	file->f_mapping = file->f_dentry->d_inode->i_mapping;
> -
> +	dentry->d_op = &sockfs_dentry_operations;
> +	d_add(dentry, SOCK_INODE(sock));
> +	f_init(file, sock_mnt, dentry, FMODE_READ | FMODE_WRITE,
> +			&socket_file_ops);
> +	SOCK_INODE(sock)->i_fop = &socket_file_ops;
>  	sock->file = file;
> -	file->f_op = SOCK_INODE(sock)->i_fop = &socket_file_ops;
> -	file->f_mode = FMODE_READ | FMODE_WRITE;
>  	file->f_flags = O_RDWR;
>  	file->f_pos = 0;
>  	file->private_data = sock;

Similarly the socket code wants a rework.  The current split into sock_alloc_fd
and sock_attach_fd is highly confusing and the allocation of the file struct
before the inode doesn't make a lot of sense.