| Message ID | 20220623175157.1715274-5-shr@fb.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io-uring/xfs: support async buffered writes | expand |
On Thu, Jun 23, 2022 at 10:51:47AM -0700, Stefan Roesch wrote: > Add the kiocb flags parameter to the function iomap_page_create(). > Depending on the value of the flags parameter it enables different gfp > flags. > > No intended functional changes in this patch. [...] > @@ -226,7 +234,7 @@ static int iomap_read_inline_data(const struct iomap_iter *iter, > if (WARN_ON_ONCE(size > iomap->length)) > return -EIO; > if (offset > 0) > - iop = iomap_page_create(iter->inode, folio); > + iop = iomap_page_create(iter->inode, folio, iter->flags); > else > iop = to_iomap_page(folio); I really don't like what this change has done to this file. I'm modifying this function, and I start thinking "Well, hang on, if flags has IOMAP_NOWAIT set, then GFP_NOWAIT can fail, and iop will be NULL, so we'll end up marking the entire folio uptodate when really we should only be marking some blocks uptodate, so we should really be failing the entire read if the allocation failed, but maybe it's OK because IOMAP_NOWAIT is never set in this path". I don't know how we fix this. Maybe return ERR_PTR(-ENOMEM) or -EAGAIN if the memory allocation fails (leaving the NULL return for "we don't need an iop"). Thoughts?
On Fri, Mar 03, 2023 at 04:51:10AM +0000, Matthew Wilcox wrote: > On Thu, Jun 23, 2022 at 10:51:47AM -0700, Stefan Roesch wrote: > > Add the kiocb flags parameter to the function iomap_page_create(). > > Depending on the value of the flags parameter it enables different gfp > > flags. > > > > No intended functional changes in this patch. > > [...] > > > @@ -226,7 +234,7 @@ static int iomap_read_inline_data(const struct iomap_iter *iter, > > if (WARN_ON_ONCE(size > iomap->length)) > > return -EIO; > > if (offset > 0) > > - iop = iomap_page_create(iter->inode, folio); > > + iop = iomap_page_create(iter->inode, folio, iter->flags); > > else > > iop = to_iomap_page(folio); > > I really don't like what this change has done to this file. I'm > modifying this function, and I start thinking "Well, hang on, if > flags has IOMAP_NOWAIT set, then GFP_NOWAIT can fail, and iop > will be NULL, so we'll end up marking the entire folio uptodate > when really we should only be marking some blocks uptodate, so > we should really be failing the entire read if the allocation > failed, but maybe it's OK because IOMAP_NOWAIT is never set in > this path". > > I don't know how we fix this. Maybe return ERR_PTR(-ENOMEM) or > -EAGAIN if the memory allocation fails (leaving the NULL return > for "we don't need an iop"). Thoughts? I don't see any problem with that, aside from being pre-coffee and on vacation for the rest of today. ;) --D
On 3/3/23 8:53 AM, Darrick J. Wong wrote: > > > On Fri, Mar 03, 2023 at 04:51:10AM +0000, Matthew Wilcox wrote: >> On Thu, Jun 23, 2022 at 10:51:47AM -0700, Stefan Roesch wrote: >>> Add the kiocb flags parameter to the function iomap_page_create(). >>> Depending on the value of the flags parameter it enables different gfp >>> flags. >>> >>> No intended functional changes in this patch. >> >> [...] >> >>> @@ -226,7 +234,7 @@ static int iomap_read_inline_data(const struct iomap_iter *iter, >>> if (WARN_ON_ONCE(size > iomap->length)) >>> return -EIO; >>> if (offset > 0) >>> - iop = iomap_page_create(iter->inode, folio); >>> + iop = iomap_page_create(iter->inode, folio, iter->flags); >>> else >>> iop = to_iomap_page(folio); >> >> I really don't like what this change has done to this file. I'm >> modifying this function, and I start thinking "Well, hang on, if >> flags has IOMAP_NOWAIT set, then GFP_NOWAIT can fail, and iop >> will be NULL, so we'll end up marking the entire folio uptodate >> when really we should only be marking some blocks uptodate, so >> we should really be failing the entire read if the allocation >> failed, but maybe it's OK because IOMAP_NOWAIT is never set in >> this path". >> >> I don't know how we fix this. Maybe return ERR_PTR(-ENOMEM) or >> -EAGAIN if the memory allocation fails (leaving the NULL return >> for "we don't need an iop"). Thoughts? > > I don't see any problem with that, aside from being pre-coffee and on > vacation for the rest of today. ;) > > --D If IOMAP_NOWAIT is set, and the allocation fails, we should return -EAGAIN, so the write request is retried in the slow path. --Stefan
On Fri, Mar 03, 2023 at 09:29:30AM -0800, Stefan Roesch wrote: > If IOMAP_NOWAIT is set, and the allocation fails, we should return > -EAGAIN, so the write request is retried in the slow path. Yes. Another vote for doing the ERR_PTR. willy, are you going to look into that yourself or are you waiting for someone to take care of it?
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index d2a9f699e17e..3c97b713f831 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -44,20 +44,28 @@ static inline struct iomap_page *to_iomap_page(struct folio *folio) static struct bio_set iomap_ioend_bioset; static struct iomap_page * -iomap_page_create(struct inode *inode, struct folio *folio) +iomap_page_create(struct inode *inode, struct folio *folio, unsigned int flags) { struct iomap_page *iop = to_iomap_page(folio); unsigned int nr_blocks = i_blocks_per_folio(inode, folio); + gfp_t gfp; if (iop || nr_blocks <= 1) return iop; + if (flags & IOMAP_NOWAIT) + gfp = GFP_NOWAIT; + else + gfp = GFP_NOFS | __GFP_NOFAIL; + iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)), - GFP_NOFS | __GFP_NOFAIL); - spin_lock_init(&iop->uptodate_lock); - if (folio_test_uptodate(folio)) - bitmap_fill(iop->uptodate, nr_blocks); - folio_attach_private(folio, iop); + gfp); + if (iop) { + spin_lock_init(&iop->uptodate_lock); + if (folio_test_uptodate(folio)) + bitmap_fill(iop->uptodate, nr_blocks); + folio_attach_private(folio, iop); + } return iop; } @@ -226,7 +234,7 @@ static int iomap_read_inline_data(const struct iomap_iter *iter, if (WARN_ON_ONCE(size > iomap->length)) return -EIO; if (offset > 0) - iop = iomap_page_create(iter->inode, folio); + iop = iomap_page_create(iter->inode, folio, iter->flags); else iop = to_iomap_page(folio); @@ -264,7 +272,7 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter, return iomap_read_inline_data(iter, folio); /* zero post-eof blocks as the page may be mapped */ - iop = iomap_page_create(iter->inode, folio); + iop = iomap_page_create(iter->inode, folio, iter->flags); iomap_adjust_read_range(iter->inode, folio, &pos, length, &poff, &plen); if (plen == 0) goto done; @@ -547,7 +555,7 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, size_t len, struct folio *folio) { const struct iomap *srcmap = iomap_iter_srcmap(iter); - struct iomap_page *iop = iomap_page_create(iter->inode, folio); + struct iomap_page *iop; loff_t block_size = i_blocksize(iter->inode); loff_t block_start = round_down(pos, block_size); loff_t block_end = round_up(pos + len, block_size); @@ -558,6 +566,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, return 0; folio_clear_error(folio); + iop = iomap_page_create(iter->inode, folio, iter->flags); + do { iomap_adjust_read_range(iter->inode, folio, &block_start, block_end - block_start, &poff, &plen); @@ -1329,7 +1339,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc, struct writeback_control *wbc, struct inode *inode, struct folio *folio, u64 end_pos) { - struct iomap_page *iop = iomap_page_create(inode, folio); + struct iomap_page *iop = iomap_page_create(inode, folio, 0); struct iomap_ioend *ioend, *next; unsigned len = i_blocksize(inode); unsigned nblocks = i_blocks_per_folio(inode, folio);