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.