| Message ID | 20220518233709.1937634-5-shr@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | io-uring/xfs: support async buffered writes | expand |
On Wed, May 18, 2022 at 04:36:55PM -0700, Stefan Roesch wrote: > This adds async buffered write support to iomap. The support is focused > on the changes necessary to support XFS with iomap. > > Support for other filesystems might require additional changes. What would those other changes be? Inline data support should not matter here, so I guess it is buffer_head support? Please spell out the actual limitations instead of the use case. Preferably including asserts in the code to catch the case of a file system trying to use the now supported cases. > > Signed-off-by: Stefan Roesch <shr@fb.com> > --- > fs/iomap/buffered-io.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 6b06fd358958..b029e2b10e07 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -580,12 +580,18 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, > size_t from = offset_in_folio(folio, pos), to = from + len; > size_t poff, plen; > gfp_t gfp = GFP_NOFS | __GFP_NOFAIL; > + bool no_wait = (iter->flags & IOMAP_NOWAIT); > + > + if (no_wait) Does thi flag really buy us anything? My preference woud be to see the IOMAP_NOWAIT directy as that is easier for me to read than trying to figure out what no_wait actually means. > + gfp = GFP_NOWAIT; > > if (folio_test_uptodate(folio)) > return 0; > folio_clear_error(folio); > > iop = iomap_page_create_gfp(iter->inode, folio, nr_blocks, gfp); And maybe the btter iomap_page_create inteface would be one that passes the flags so that we can centralize the gfp_t selection. > @@ -602,6 +608,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, > if (WARN_ON_ONCE(iter->flags & IOMAP_UNSHARE)) > return -EIO; > folio_zero_segments(folio, poff, from, to, poff + plen); > + } else if (no_wait) { > + return -EAGAIN; > } else { > int status = iomap_read_folio_sync(block_start, folio, > poff, plen, srcmap); That's a somewhat unnatural code flow. I'd much prefer: } else { int status; if (iter->flags & IOMAP_NOWAIT) return -EAGAIN; iomap_read_folio_sync(block_start, folio, poff, plen, srcmap); Or maybe even pass the iter to iomap_read_folio_sync and just do the IOMAP_NOWAIT check there.
On 5/19/22 1:25 AM, Christoph Hellwig wrote: > On Wed, May 18, 2022 at 04:36:55PM -0700, Stefan Roesch wrote: >> This adds async buffered write support to iomap. The support is focused >> on the changes necessary to support XFS with iomap. >> >> Support for other filesystems might require additional changes. > > What would those other changes be? Inline data support should not > matter here, so I guess it is buffer_head support? Please spell out > the actual limitations instead of the use case. Preferably including > asserts in the code to catch the case of a file system trying to use > the now supported cases. > I was only trying to make the point that I haven't enabled this on other filesystems than XFS. Removing the statement as it causes confusion. >> >> Signed-off-by: Stefan Roesch <shr@fb.com> >> --- >> fs/iomap/buffered-io.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c >> index 6b06fd358958..b029e2b10e07 100644 >> --- a/fs/iomap/buffered-io.c >> +++ b/fs/iomap/buffered-io.c >> @@ -580,12 +580,18 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, >> size_t from = offset_in_folio(folio, pos), to = from + len; >> size_t poff, plen; >> gfp_t gfp = GFP_NOFS | __GFP_NOFAIL; >> + bool no_wait = (iter->flags & IOMAP_NOWAIT); >> + >> + if (no_wait) > > Does thi flag really buy us anything? My preference woud be to see > the IOMAP_NOWAIT directy as that is easier for me to read than trying to > figure out what no_wait actually means. > Removed the no_wait variable and instead used the flag check directly in the code. >> + gfp = GFP_NOWAIT; >> >> if (folio_test_uptodate(folio)) >> return 0; >> folio_clear_error(folio); >> >> iop = iomap_page_create_gfp(iter->inode, folio, nr_blocks, gfp); > > And maybe the btter iomap_page_create inteface would be one that passes > the flags so that we can centralize the gfp_t selection. > >> @@ -602,6 +608,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, >> if (WARN_ON_ONCE(iter->flags & IOMAP_UNSHARE)) >> return -EIO; >> folio_zero_segments(folio, poff, from, to, poff + plen); >> + } else if (no_wait) { >> + return -EAGAIN; >> } else { >> int status = iomap_read_folio_sync(block_start, folio, >> poff, plen, srcmap); > > That's a somewhat unnatural code flow. I'd much prefer: > I made the below change. > } else { > int status; > > if (iter->flags & IOMAP_NOWAIT) > return -EAGAIN; > iomap_read_folio_sync(block_start, folio, > poff, plen, srcmap); > > Or maybe even pass the iter to iomap_read_folio_sync and just do the > IOMAP_NOWAIT check there.
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 6b06fd358958..b029e2b10e07 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -580,12 +580,18 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, size_t from = offset_in_folio(folio, pos), to = from + len; size_t poff, plen; gfp_t gfp = GFP_NOFS | __GFP_NOFAIL; + bool no_wait = (iter->flags & IOMAP_NOWAIT); + + if (no_wait) + gfp = GFP_NOWAIT; if (folio_test_uptodate(folio)) return 0; folio_clear_error(folio); iop = iomap_page_create_gfp(iter->inode, folio, nr_blocks, gfp); + if (no_wait && !iop) + return -EAGAIN; do { iomap_adjust_read_range(iter->inode, folio, &block_start, @@ -602,6 +608,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, if (WARN_ON_ONCE(iter->flags & IOMAP_UNSHARE)) return -EIO; folio_zero_segments(folio, poff, from, to, poff + plen); + } else if (no_wait) { + return -EAGAIN; } else { int status = iomap_read_folio_sync(block_start, folio, poff, plen, srcmap); @@ -632,6 +640,9 @@ static int iomap_write_begin(const struct iomap_iter *iter, loff_t pos, unsigned fgp = FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE | FGP_NOFS; int status = 0; + if (iter->flags & IOMAP_NOWAIT) + fgp |= FGP_NOWAIT; + BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length); if (srcmap != &iter->iomap) BUG_ON(pos + len > srcmap->offset + srcmap->length); @@ -789,6 +800,10 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) * Otherwise there's a nasty deadlock on copying from the * same page as we're writing to, without it being marked * up-to-date. + * + * For async buffered writes the assumption is that the user + * page has already been faulted in. This can be optimized by + * faulting the user page in the prepare phase of io-uring. */ if (unlikely(fault_in_iov_iter_readable(i, bytes) == bytes)) { status = -EFAULT; @@ -844,6 +859,9 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i, }; int ret; + if (iocb->ki_flags & IOCB_NOWAIT) + iter.flags |= IOMAP_NOWAIT; + while ((ret = iomap_iter(&iter, ops)) > 0) iter.processed = iomap_write_iter(&iter, i); if (iter.pos == iocb->ki_pos)
This adds async buffered write support to iomap. The support is focused on the changes necessary to support XFS with iomap. Support for other filesystems might require additional changes. Signed-off-by: Stefan Roesch <shr@fb.com> --- fs/iomap/buffered-io.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)