On Thu, Oct 15, 2020 at 05:43:33PM +0100, Matthew Wilcox wrote:
> On Thu, Oct 15, 2020 at 10:42:03AM +0100, Christoph Hellwig wrote:
> > > +static void iomap_read_page_end_io(struct bio_vec *bvec,
> > > + struct completion *done, bool error)
> >
> > I really don't like the parameters here. Part of the problem is
> > that ctx is only assigned to bi_private conditionally, which can
> > easily be fixed. The other part is the strange bool error when
> > we can just pass on bi_stats. See the patch at the end of what
> > I'd do intead.
>
> I prefer assigning ctx conditionally to propagating the knowledge
> that !rac means synchronous. I've gone with this:
And I really hate these kinds of conditional assignments. If the
->rac check is too raw please just add an explicit
bool synchronous : 1;
flag.
> @@ -340,16 +335,12 @@ iomap_readpage(struct page *page, const struct iomap_ops *
> ops)
>
> if (ctx.bio) {
> submit_bio(ctx.bio);
> + if (ret > 0)
> + ret = blk_status_to_errno(ctx.status);
> }
>
> - wait_for_completion(&ctx.done);
> if (ret >= 0)
> - ret = blk_status_to_errno(ctx.status);
> - if (ret == 0)
> return AOP_UPDATED_PAGE;
> unlock_page(page);
> return ret;
>
>
> ... there's no need to call blk_status_to_errno if we never submitted a bio.
True. I'd still prefer the AOP_UPDATED_PAGE as the fallthrough case
and an explicit goto out_unlock, though.
