Anand Avati <avati@xxxxxxxxxx> writes: > Implement ->direct_IO() method in aops. The ->direct_IO() method combines > the existing fuse_direct_read/fuse_direct_write with a generic > fuse_loop_rwv (inspired by do_loop_readv/writev) to neatly re-use the existing > direct_io framework. > > Reaching ->direct_IO() in the read path via generic_file_aio_read ensures > proper synchronization with page cache with its existing framework. > > Reaching ->direct_IO() in the write path via fuse_file_aio_write is made > to come via generic_file_direct_write() which makes it play nice with > the page cache w.r.t other mmap pages etc. > > On files marked 'direct_io' by the filesystem server, IO always follows > the fuse_direct_read/write path. There is no effect of fcntl(O_DIRECT) > and it always succeeds. > > On files not marked with 'direct_io' by the filesystem server, the IO > path depends on O_DIRECT flag by the application. This can be passed > at the time of open() as well as via fcntl(). > > Note that asynchronous O_DIRECT iocb jobs are completed synchronously > always (this has been the case with FUSE even before this patch) I'm generally fine with this approach, but see comments inline. > > Signed-off-by: Anand Avati <avati@xxxxxxxxxx> > --- > fs/fuse/dir.c | 3 -- > fs/fuse/file.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 68 insertions(+), 8 deletions(-) > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c > index 2066328..7e5dbd0 100644 > --- a/fs/fuse/dir.c > +++ b/fs/fuse/dir.c > @@ -387,9 +387,6 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry, > if (fc->no_create) > return -ENOSYS; > > - if (flags & O_DIRECT) > - return -EINVAL; > - > forget = fuse_alloc_forget(); > if (!forget) > return -ENOMEM; > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index 4a199fd..3c6b8bc 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -194,10 +194,6 @@ int fuse_open_common(struct inode *inode, struct file *file, bool isdir) > struct fuse_conn *fc = get_fuse_conn(inode); > int err; > > - /* VFS checks this, but only _after_ ->open() */ > - if (file->f_flags & O_DIRECT) > - return -EINVAL; > - > err = generic_file_open(inode, file); > if (err) > return err; > @@ -932,6 +928,7 @@ static ssize_t fuse_file_aio_write(struct kiocb *iocb, const struct iovec *iov, > struct file *file = iocb->ki_filp; > struct address_space *mapping = file->f_mapping; > size_t count = 0; > + size_t ocount = 0; > ssize_t written = 0; > struct inode *inode = mapping->host; > ssize_t err; > @@ -939,10 +936,13 @@ static ssize_t fuse_file_aio_write(struct kiocb *iocb, const struct iovec *iov, > > WARN_ON(iocb->ki_pos != pos); > > - err = generic_segment_checks(iov, &nr_segs, &count, VERIFY_READ); > + ocount = 0; > + err = generic_segment_checks(iov, &nr_segs, &ocount, VERIFY_READ); > if (err) > return err; > > + count = ocount; > + > mutex_lock(&inode->i_mutex); > vfs_check_frozen(inode->i_sb, SB_FREEZE_WRITE); > > @@ -962,6 +962,13 @@ static ssize_t fuse_file_aio_write(struct kiocb *iocb, const struct iovec *iov, > > file_update_time(file); > > + if (file->f_flags & O_DIRECT) { > + written = generic_file_direct_write(iocb, iov, &nr_segs, > + pos, &iocb->ki_pos, > + count, ocount); It looks to me like this will deadlock on i_mutex (fuse_direct_write() will try to lock it again, while it is already locked). Was this code path tested at all? Also generic_file_direct_write() may return a short count in which case the write should be finished using buffered write. Thanks, Miklos > + goto out; > + } > + > iov_iter_init(&i, iov, nr_segs, count, 0); > written = fuse_perform_write(file, mapping, &i, pos); > if (written >= 0) > @@ -2077,6 +2084,61 @@ int fuse_notify_poll_wakeup(struct fuse_conn *fc, > return 0; > } > > + > +static ssize_t fuse_loop_rwv(struct file *filp, const struct iovec *iov, > + unsigned long nr_segs, loff_t *ppos, io_fn_t fn) > +{ > + const struct iovec *vector = iov; > + ssize_t ret = 0; > + > + while (nr_segs > 0) { > + void __user *base; > + size_t len; > + ssize_t nr; > + > + base = vector->iov_base; > + len = vector->iov_len; > + vector++; > + nr_segs--; > + > + nr = fn(filp, base, len, ppos); > + > + if (nr < 0) { > + if (!ret) > + ret = nr; > + break; > + } > + ret += nr; > + if (nr != len) > + break; > + } > + > + return ret; > +} > + > + > +static ssize_t > +fuse_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov, > + loff_t offset, unsigned long nr_segs) > +{ > + ssize_t ret = 0; > + struct file *file = NULL; > + loff_t pos = 0; > + io_fn_t fn = NULL; > + > + file = iocb->ki_filp; > + pos = offset; > + > + if (rw == WRITE) > + fn = (io_fn_t)fuse_direct_write; > + else > + fn = fuse_direct_read; > + > + ret = fuse_loop_rwv(file, iov, nr_segs, &pos, fn); > + > + return ret; > +} > + > static const struct file_operations fuse_file_operations = { > .llseek = fuse_file_llseek, > .read = do_sync_read, > @@ -2120,6 +2182,7 @@ static const struct address_space_operations fuse_file_aops = { > .readpages = fuse_readpages, > .set_page_dirty = __set_page_dirty_nobuffers, > .bmap = fuse_bmap, > + .direct_IO = fuse_direct_IO, > }; > > void fuse_init_file_inode(struct inode *inode) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html