Messages in this thread | | | Date | Wed, 03 Feb 2016 11:05:32 -0800 | From | Nikhilesh Reddy <> | Subject | Re: [PATCH v5] fuse: Add support for passthrough read/write |
| |
On 02/01/2016 11:45 AM, Jann Horn wrote: > On Mon, Feb 01, 2016 at 11:28:51AM -0800, Nikhilesh Reddy wrote: >> On Mon 01 Feb 2016 11:15:56 AM PST, Jann Horn wrote: >>> On Mon, Feb 01, 2016 at 10:56:27AM -0800, Nikhilesh Reddy wrote: >>>> diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c >>> [...] >>>> +static ssize_t fuse_passthrough_read_write_iter(struct kiocb *iocb, >>>> + struct iov_iter *iter, int do_write) >>>> +{ >>>> + ssize_t ret_val; >>>> + struct fuse_file *ff; >>>> + struct file *fuse_file, *passthrough_filp; >>>> + struct inode *fuse_inode, *passthrough_inode; >>>> + >>>> + ff = iocb->ki_filp->private_data; >>>> + fuse_file = iocb->ki_filp; >>>> + passthrough_filp = ff->passthrough_filp; >>>> + >>>> + /* lock passthrough file to prevent it from being released */ >>>> + get_file(passthrough_filp); >>>> + iocb->ki_filp = passthrough_filp; >>>> + fuse_inode = fuse_file->f_path.dentry->d_inode; >>>> + passthrough_inode = file_inode(passthrough_filp); >>>> + >>>> + if (do_write) { >>>> + if (!passthrough_filp->f_op->write_iter) >>>> + return -EIO; >>>> + ret_val = passthrough_filp->f_op->write_iter(iocb, iter); >>>> + >>>> + if (ret_val >= 0 || ret_val == -EIOCBQUEUED) { >>>> + fsstack_copy_inode_size(fuse_inode, passthrough_inode); >>>> + fsstack_copy_attr_times(fuse_inode, passthrough_inode); >>>> + } >>>> + } else { >>>> + if (!passthrough_filp->f_op->read_iter) >>>> + return -EIO; >>>> + ret_val = passthrough_filp->f_op->read_iter(iocb, iter); >>>> + if (ret_val >= 0 || ret_val == -EIOCBQUEUED) >>>> + fsstack_copy_attr_atime(fuse_inode, passthrough_inode); >>>> + } >>>> + >>>> + iocb->ki_filp = fuse_file; >>>> + >>>> + /* unlock passthrough file */ >>>> + fput(passthrough_filp); >>> >>> Why the get_file() and fput() in this method? This doesn't look right. There >>> is no lock you're releasing between get_file() and fput(). What are they >>> intended for? >> >> Hi >> >> Thanks for reviewing the code. >> >> The passthrough file could be released under our feet say if the userspace >> fuse daemon crashed or was killed ( while we are processing the read or the >> write) causing bad things to happen. >> The calls here are to increase the count temporarily and then decrease it >> so that we dont release in the middle of a write and everything is >> gracefully handled... >> >> I have a comment right before the get_file call above saying the same thing. >> Please let me know if you have any more questions. > > If that is the case, why can't the passthrough file be released before the > get_file() call, e.g. while the core processing the filesystem read request > is entering fuse_passthrough_read_write_iter()? > > As far as I can tell, you can drop the get_file() and fput() calls. > fuse_setup_passthrough() already took a reference to the file for you, that > reference can only be dropped in fuse_passthrough_release(), and the VFS > ensures that no release call happens while a read or write is pending. > I just feel uncomfortable with dropping them. I thought they could be released ( i/o ) takes longer than the actual execution... but if i can be sure of it then maybe..
-- Thanks Nikhilesh Reddy
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
| |