| Message ID | a77b34fe-dbe7-388f-c559-932b1cd44583@fastmail.fm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] fuse: DIO writes always use the same code path | expand |
On Wed, Jul 05, 2023 at 12:23:40PM +0200, Bernd Schubert wrote: > @@ -1377,37 +1375,10 @@ static ssize_t fuse_cache_write_iter(struct kiocb > *iocb, struct iov_iter *from) > if (err) > goto out; > > - if (iocb->ki_flags & IOCB_DIRECT) { > - loff_t pos = iocb->ki_pos; > - written = generic_file_direct_write(iocb, from); After this generic_file_direct_write becomes unused outside of mm/filemap.c, please add a patch to the series to mark it static. > + written = fuse_perform_write(iocb, mapping, from, iocb->ki_pos); > + if (written >= 0) > + iocb->ki_pos += written; This needs to be updated to the new fuse_perform_write calling conventions in Linus tree.
On 7/6/23 16:43, Christoph Hellwig wrote: > On Wed, Jul 05, 2023 at 12:23:40PM +0200, Bernd Schubert wrote: >> @@ -1377,37 +1375,10 @@ static ssize_t fuse_cache_write_iter(struct kiocb >> *iocb, struct iov_iter *from) >> if (err) >> goto out; >> >> - if (iocb->ki_flags & IOCB_DIRECT) { >> - loff_t pos = iocb->ki_pos; >> - written = generic_file_direct_write(iocb, from); > > After this generic_file_direct_write becomes unused outside of > mm/filemap.c, please add a patch to the series to mark it static. > >> + written = fuse_perform_write(iocb, mapping, from, iocb->ki_pos); >> + if (written >= 0) >> + iocb->ki_pos += written; > > This needs to be updated to the new fuse_perform_write calling > conventions in Linus tree. > Thanks a lot for your review, I will address it when I'm back from vacation in two weeks. I had seen your DIO patch series, but didn't notice it was merged already.
Hi Bernd, On 7/5/23 18:23, Bernd Schubert wrote: > From: Bernd Schubert <bschubert@ddn.com> > > In commit 153524053bbb0d27bb2e0be36d1b46862e9ce74c DIO > writes can be handled in parallel, as long as the file > is not extended. So far this only works when daemon/server > side set FOPEN_DIRECT_IO and FOPEN_PARALLEL_DIRECT_WRITES, > but O_DIRECT (iocb->ki_flags & IOCB_DIRECT) went another > code path that doesn't have the parallel DIO write > optimization. > Given that fuse_direct_write_iter has to handle page writes > and invalidation anyway (for mmap), the DIO handler in > fuse_cache_write_iter() is removed and DIO writes are now > only handled by fuse_direct_write_iter(). > > Note: Correctness of this patch depends on a non-merged > series from Hao Xu <hao.xu@linux.dev> > ( fuse: add a new fuse init flag to relax restrictions in no cache mode) > --- > fs/fuse/file.c | 38 +++++--------------------------------- > 1 file changed, 5 insertions(+), 33 deletions(-) > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index 89d97f6188e0..1490329b536c 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -1337,11 +1337,9 @@ static ssize_t fuse_cache_write_iter(struct kiocb > *iocb, struct iov_iter *from) > struct file *file = iocb->ki_filp; > struct address_space *mapping = file->f_mapping; > ssize_t written = 0; > - ssize_t written_buffered = 0; > struct inode *inode = mapping->host; > ssize_t err; > struct fuse_conn *fc = get_fuse_conn(inode); > - loff_t endbyte = 0; > > if (fc->writeback_cache) { > /* Update size (EOF optimization) and mode (SUID clearing) */ > @@ -1377,37 +1375,10 @@ static ssize_t fuse_cache_write_iter(struct > kiocb *iocb, struct iov_iter *from) > if (err) > goto out; > > - if (iocb->ki_flags & IOCB_DIRECT) { > - loff_t pos = iocb->ki_pos; > - written = generic_file_direct_write(iocb, from); > - if (written < 0 || !iov_iter_count(from)) > - goto out; > - > - pos += written; > - > - written_buffered = fuse_perform_write(iocb, mapping, from, pos); > - if (written_buffered < 0) { > - err = written_buffered; > - goto out; > - } > - endbyte = pos + written_buffered - 1; > - > - err = filemap_write_and_wait_range(file->f_mapping, pos, > - endbyte); > - if (err) > - goto out; > - > - invalidate_mapping_pages(file->f_mapping, > - pos >> PAGE_SHIFT, > - endbyte >> PAGE_SHIFT); > + written = fuse_perform_write(iocb, mapping, from, iocb->ki_pos); > + if (written >= 0) > + iocb->ki_pos += written; > > - written += written_buffered; > - iocb->ki_pos = pos + written_buffered; > - } else { > - written = fuse_perform_write(iocb, mapping, from, iocb->ki_pos); > - if (written >= 0) > - iocb->ki_pos += written; > - } > out: > current->backing_dev_info = NULL; > inode_unlock(inode); > @@ -1691,7 +1662,8 @@ static ssize_t fuse_file_write_iter(struct kiocb > *iocb, struct iov_iter *from) > if (FUSE_IS_DAX(inode)) > return fuse_dax_write_iter(iocb, from); > > - if (!(ff->open_flags & FOPEN_DIRECT_IO)) > + if (!(ff->open_flags & FOPEN_DIRECT_IO) && > + !(iocb->ki_flags & IOCB_DIRECT)) > return fuse_cache_write_iter(iocb, from); > else > return fuse_direct_write_iter(iocb, from); > For normal direct io(IOCB_DIRECT set, FOPEN_DIRECT_IO not set), it now goes to fuse_direct_write_iter() but the thing is the previous patchset I sent adds page flush and invalidation in FOPEN_DIRECT_IO and/or fc->direct_io_relax, so I guess this part(flush and invalidation) is not included in the normal direct io code path. Regards, Hao
On July 17, 2023 10:03:11 AM GMT+02:00, Hao Xu <hao.xu@linux.dev> wrote: >Hi Bernd, > >On 7/5/23 18:23, Bernd Schubert wrote: >> From: Bernd Schubert <bschubert@ddn.com> >> >> In commit 153524053bbb0d27bb2e0be36d1b46862e9ce74c DIO >> writes can be handled in parallel, as long as the file >> is not extended. So far this only works when daemon/server >> side set FOPEN_DIRECT_IO and FOPEN_PARALLEL_DIRECT_WRITES, >> but O_DIRECT (iocb->ki_flags & IOCB_DIRECT) went another >> code path that doesn't have the parallel DIO write >> optimization. >> Given that fuse_direct_write_iter has to handle page writes >> and invalidation anyway (for mmap), the DIO handler in >> fuse_cache_write_iter() is removed and DIO writes are now >> only handled by fuse_direct_write_iter(). >> >> Note: Correctness of this patch depends on a non-merged >> series from Hao Xu <hao.xu@linux.dev> >> ( fuse: add a new fuse init flag to relax restrictions in no cache mode) >> --- >> fs/fuse/file.c | 38 +++++--------------------------------- >> 1 file changed, 5 insertions(+), 33 deletions(-) >> >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c >> index 89d97f6188e0..1490329b536c 100644 >> --- a/fs/fuse/file.c >> +++ b/fs/fuse/file.c >> @@ -1337,11 +1337,9 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) >> struct file *file = iocb->ki_filp; >> struct address_space *mapping = file->f_mapping; >> ssize_t written = 0; >> - ssize_t written_buffered = 0; >> struct inode *inode = mapping->host; >> ssize_t err; >> struct fuse_conn *fc = get_fuse_conn(inode); >> - loff_t endbyte = 0; >> >> if (fc->writeback_cache) { >> /* Update size (EOF optimization) and mode (SUID clearing) */ >> @@ -1377,37 +1375,10 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) >> if (err) >> goto out; >> >> - if (iocb->ki_flags & IOCB_DIRECT) { >> - loff_t pos = iocb->ki_pos; >> - written = generic_file_direct_write(iocb, from); >> - if (written < 0 || !iov_iter_count(from)) >> - goto out; >> - >> - pos += written; >> - >> - written_buffered = fuse_perform_write(iocb, mapping, from, pos); >> - if (written_buffered < 0) { >> - err = written_buffered; >> - goto out; >> - } >> - endbyte = pos + written_buffered - 1; >> - >> - err = filemap_write_and_wait_range(file->f_mapping, pos, >> - endbyte); >> - if (err) >> - goto out; >> - >> - invalidate_mapping_pages(file->f_mapping, >> - pos >> PAGE_SHIFT, >> - endbyte >> PAGE_SHIFT); >> + written = fuse_perform_write(iocb, mapping, from, iocb->ki_pos); >> + if (written >= 0) >> + iocb->ki_pos += written; >> >> - written += written_buffered; >> - iocb->ki_pos = pos + written_buffered; >> - } else { >> - written = fuse_perform_write(iocb, mapping, from, iocb->ki_pos); >> - if (written >= 0) >> - iocb->ki_pos += written; >> - } >> out: >> current->backing_dev_info = NULL; >> inode_unlock(inode); >> @@ -1691,7 +1662,8 @@ static ssize_t fuse_file_write_iter(struct kiocb *iocb, struct iov_iter *from) >> if (FUSE_IS_DAX(inode)) >> return fuse_dax_write_iter(iocb, from); >> >> - if (!(ff->open_flags & FOPEN_DIRECT_IO)) >> + if (!(ff->open_flags & FOPEN_DIRECT_IO) && >> + !(iocb->ki_flags & IOCB_DIRECT)) >> return fuse_cache_write_iter(iocb, from); >> else >> return fuse_direct_write_iter(iocb, from); >> > >For normal direct io(IOCB_DIRECT set, FOPEN_DIRECT_IO not set), it now >goes to fuse_direct_write_iter() but the thing is the previous patchset >I sent adds page flush and invalidation in FOPEN_DIRECT_IO >and/or fc->direct_io_relax, so I guess this part(flush and invalidation) >is not included in the normal direct io code path. > >Regards, >Hao > Hi Hao, I'm going to rebase to for-next and create a single patch set that should handle that, but only next week. Thanks, Bernd
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 89d97f6188e0..1490329b536c 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1337,11 +1337,9 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) struct file *file = iocb->ki_filp; struct address_space *mapping = file->f_mapping; ssize_t written = 0; - ssize_t written_buffered = 0; struct inode *inode = mapping->host; ssize_t err; struct fuse_conn *fc = get_fuse_conn(inode); - loff_t endbyte = 0; if (fc->writeback_cache) {
From: Bernd Schubert <bschubert@ddn.com> In commit 153524053bbb0d27bb2e0be36d1b46862e9ce74c DIO writes can be handled in parallel, as long as the file is not extended. So far this only works when daemon/server side set FOPEN_DIRECT_IO and FOPEN_PARALLEL_DIRECT_WRITES, but O_DIRECT (iocb->ki_flags & IOCB_DIRECT) went another code path that doesn't have the parallel DIO write optimization. Given that fuse_direct_write_iter has to handle page writes and invalidation anyway (for mmap), the DIO handler in fuse_cache_write_iter() is removed and DIO writes are now only handled by fuse_direct_write_iter(). Note: Correctness of this patch depends on a non-merged series from Hao Xu <hao.xu@linux.dev> ( fuse: add a new fuse init flag to relax restrictions in no cache mode) --- fs/fuse/file.c | 38 +++++--------------------------------- 1 file changed, 5 insertions(+), 33 deletions(-) /* Update size (EOF optimization) and mode (SUID clearing) */ @@ -1377,37 +1375,10 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) if (err) goto out; - if (iocb->ki_flags & IOCB_DIRECT) { - loff_t pos = iocb->ki_pos; - written = generic_file_direct_write(iocb, from); - if (written < 0 || !iov_iter_count(from)) - goto out; - - pos += written; - - written_buffered = fuse_perform_write(iocb, mapping, from, pos); - if (written_buffered < 0) { - err = written_buffered; - goto out; - } - endbyte = pos + written_buffered - 1; - - err = filemap_write_and_wait_range(file->f_mapping, pos, - endbyte); - if (err) - goto out; - - invalidate_mapping_pages(file->f_mapping, - pos >> PAGE_SHIFT, - endbyte >> PAGE_SHIFT); + written = fuse_perform_write(iocb, mapping, from, iocb->ki_pos); + if (written >= 0) + iocb->ki_pos += written; - written += written_buffered; - iocb->ki_pos = pos + written_buffered; - } else { - written = fuse_perform_write(iocb, mapping, from, iocb->ki_pos); - if (written >= 0) - iocb->ki_pos += written; - } out: current->backing_dev_info = NULL; inode_unlock(inode); @@ -1691,7 +1662,8 @@ static ssize_t fuse_file_write_iter(struct kiocb *iocb, struct iov_iter *from) if (FUSE_IS_DAX(inode)) return fuse_dax_write_iter(iocb, from); - if (!(ff->open_flags & FOPEN_DIRECT_IO)) + if (!(ff->open_flags & FOPEN_DIRECT_IO) && + !(iocb->ki_flags & IOCB_DIRECT)) return fuse_cache_write_iter(iocb, from); else return fuse_direct_write_iter(iocb, from);