Re: [RESEND PATCH v9 04/14] iomap: Add flags parameter to iomap_page_create()

Darrick J. Wong
2023-03-03T16:54:45Z

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