Re: [PATCH -V11 2/9] vfs: Add name to file handle conversion support

[Aneesh Kumar K. V]

On Fri, 21 May 2010 18:15:07 -0400, "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> On Thu, May 20, 2010 at 01:05:31PM +0530, Aneesh Kumar K.V wrote:
> > This patch add a new superblock field unsigned char s_uuid[16]
> > to store UUID mapping for the file system. The s_uuid[16] is used to
> > identify the file system apart of file_handle
> 
> Sorry, I lost track of the previous argument.  I thought that the
> decision was to return just the file-identifying part of the filehandle
> and let userspace tack on the uuid if that's what it wants?  That seems
> the more flexible approach at least.

Having UUID as a part of a handle enables us to use the handle directly
in the userspace as a file identifier. In most case UUID should work as
a identifier for the file system. What is changed now is even though in
name to handle conversion we add UUID, we don't use UUID to identify the
file system in handle to open path. Rather userspace should map UUID or
what ever unique identifier for the file system it have decided to use
to a mountdir fd and use that to identify the file system in
sys_open_by_handle_at syscall.

UUID is now a part of handle only as a easy way to get the 16 byte
unique handler for the file system. Userspace applications like NFS
server still have to make sure that for the list of exported file system
whether UUID is the correct unique identifier. If yes, they can directly
use the file handle returned from the syscall. If not NFS server will
have to find an alternative to uniquely identify the file system.

So instead of doing

sys_name_to_handle("/tmp/a", handle);
statfs("/tmp/a")
memcpy(handle.f_fsid, statfs.f_fsid);

we can now do 

sys_name_to_handle("/tmp/a, handle);


> 
> > 
> > Acked-by: Serge Hallyn <serue@xxxxxxxxxx>
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
> > ---
> >  fs/exportfs/expfs.c      |    2 +-
> >  fs/open.c                |  100 ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/fs.h       |   11 +++++
> >  include/linux/syscalls.h |    3 +
> >  4 files changed, 115 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> > index cfee0f0..d103c31 100644
> > --- a/fs/exportfs/expfs.c
> > +++ b/fs/exportfs/expfs.c
> > @@ -352,7 +352,7 @@ int exportfs_encode_fh(struct dentry *dentry, struct fid *fid, int *max_len,
> >  	const struct export_operations *nop = dentry->d_sb->s_export_op;
> >  	int error;
> >  
> > -	if (nop->encode_fh)
> > +	if (nop && nop->encode_fh)
> 
> Is there any legitimate reason to call this with nop == NULL?  If not,
> this should be a BUG() if we want to test for the case at all.

For name_to_handle i have added a check to make sure we return
-EOPNOTSUPP. 

       if (!path.mnt->mnt_sb->s_export_op ||
		!path.mnt->mnt_sb->s_export_op->fh_to_dentry) {
		ret = -EOPNOTSUPP;
		goto out_path;
	}

So may be i can drop that nop == NULL check. 

We want to retain the null check in exportfs_decode_fh because
application can call open_by_handle with any mountdirfd that can be for
a file system that doesn't support export operations. 

> 
> Some user documentation for the new interface would be helpful.  (The
> sort of information that would go into the man page eventually, if not
> actually in man page format.)


Will add that in the next iteration.

> 
> >  		error = nop->encode_fh(dentry, fid->raw, max_len, connectable);
> >  	else
> >  		error = export_encode_fh(dentry, fid, max_len, connectable);
> > diff --git a/fs/open.c b/fs/open.c
> > index 74e5cd9..c4c2577 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -30,6 +30,7 @@
> >  #include <linux/falloc.h>
> >  #include <linux/fs_struct.h>
> >  #include <linux/ima.h>
> > +#include <linux/exportfs.h>
> >  
> >  #include "internal.h"
> >  
> > @@ -1206,3 +1207,102 @@ int nonseekable_open(struct inode *inode, struct file *filp)
> >  }
> >  
> >  EXPORT_SYMBOL(nonseekable_open);
> > +
> > +#ifdef CONFIG_EXPORTFS
> > +/* limit the handle size to some value */
> > +#define MAX_HANDLE_SZ 4096
> > +static long do_sys_name_to_handle(struct path *path,
> > +			struct file_handle __user *ufh)
> > +{
> > +	int retval;
> > +	int handle_size;
> > +	struct super_block *sb;
> > +	struct file_handle f_handle;
> > +	struct file_handle *handle = NULL;
> > +
> > +	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) {
> > +		retval = -EFAULT;
> > +		goto err_out;
> > +	}
> > +	if (f_handle.handle_size > MAX_HANDLE_SZ) {
> > +		retval = -EINVAL;
> > +		goto err_out;
> > +	}
> > +	handle = kmalloc(sizeof(struct file_handle) + f_handle.handle_size,
> > +			GFP_KERNEL);
> > +	if (!handle) {
> > +		retval = -ENOMEM;
> > +		goto err_out;
> > +	}
> > +	handle_size = f_handle.handle_size;
> > +
> > +	/* we ask for a non connected handle */
> > +	retval = exportfs_encode_fh(path->dentry,
> > +				(struct fid *)handle->f_handle,
> > +				&handle_size,  0);
> > +	/* convert handle size to bytes */
> > +	handle_size *= sizeof(u32);
> 
> I'm confused about units:
> 
> 	- the max_len parameter is in 4-byte units.
> 	- So handle_size was also in 4-byte units before the above line.
> 	- So f_handle->handle_size was also in 4-byte units?  Then why
> 	  were we passing it to kmalloc?  And isn't that a confusing
> 	  convention for the length passed in from userspace?

The units passed from userpace should be in bytes. So yes i missed a 

handle_size = f_handle.handle_size >> 2;


> 
> > +	handle->handle_type = retval;
> 
> Don't we need to check for the special case retval == 255?

encode_f returns retval == 255 when we don't have enough space to copy the handle right ?
which should be captured by 

if (handle_size <= f_handle.handle_size) 

Are there any other error condition indicated by retval == 255 ? IIUC
encode_fh have two possible returns

a) retval == 255 if handle_size is small. I updated to put the required
handle_size in max_len. So i am capture this via a handle_size check.
b) Successfully copied the handle 

did i miss any other return ?


Thanks for reviewing the patches

-aneesh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/