Re: [PATCH -V3 3/5] vfs: Add open by file handle support

["Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com>]

On Thu, 22 Apr 2010 13:22:32 -0600, Andreas Dilger <adilger@sun.com> wrote:
> On 2010-04-22, at 12:15, Aneesh Kumar K.V wrote:
> > +long do_sys_open_by_handle(struct file_handle *fh, int flags)
> > +{
> 
> Note this is inconsistent with the patch description that says that
> the syscall is open_by_handle_at().  While the syscall is still using
> the "_at()" suffix, it is no longer passing any directory.

Fixed. 

> 
> > +	sb = fs_get_sb(&fh->fsid);
> > +	if (!sb)
> > +		return -ESTALE;
> 
> I was going to say that it would be nice to allow a fallback to the "_at" directory of the filesystem, but since the handle is specific to the exact filesystem it was created on anyway (inode number+generation, not even an rsync would be enough) the only way the handle is useful on another node is if the filesystem was cloned at the block device level (or is a shared filesystem identical on all nodes) so the UUID should be identical too.
> 
> > +	 * Find the vfsmount for this superblock in the
> > +	 * current namespace
> > +	 */
> > +	mnt = fs_get_vfsmount(current, sb);
> > +	if (!mnt) {
> > +		deactivate_super(sb);
> > +		return -ESTALE;
> > +	}
> > +
> > +	dentry = handle_to_dentry(mnt, fh);
> > +	if (IS_ERR(dentry)) {
> > +		mntput(mnt);
> > +		deactivate_super(sb);
> > +		return PTR_ERR(dentry);
> > +	}
> 
> Why is this code doing "manual" cleanup instead of setting retval and
> goto out_sb: or out_mnt: type labels that only do partial cleanup?

Fixed

> 
> > +	fsnotify_open(filp->f_path.dentry);
> > +	fd_install(fd, filp);
> > +	mntput(mnt);
> > +	deactivate_super(sb);
> > +	return fd;
> 
> This could set retval = fd and goto out_mnt: (skipping dput(dentry)) in conjunction with the change proposed below.
> 
> > +err_out:
> > +	mntput(mnt);
> > +	deactivate_super(sb);
> > +	dput(dentry);
> > +	return retval;
> > +}
> 
> I think it makes sense to do the dput(dentry) first, since it will be holding a reference on the sb and vfsmnt, and it also allows code simplification:
> 
> out_err:
>        dput(dentry);
> out_mnt:
>        mntput(mnt);
> out_sb:
>        deactivate_super(sb);
> 
>        return retval;
> }
>

Fixed

I sent a V4 version of the patch with compat syscall support.

-aneesh