On Tue, May 30 2023 at 3:43P -0400,
Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
>
>
> On Tue, 30 May 2023, Mike Snitzer wrote:
>
> > On Tue, May 30 2023 at 11:13P -0400,
> > Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
> >
> > > Hi
> > >
> > > I nack this. This just adds code that can't ever be executed.
> > >
> > > dm-crypt already allocates enough entries in the vector (see "unsigned int
> > > nr_iovecs = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;"), so bio_add_page can't
> > > fail.
> > >
> > > If you want to add __must_check to bio_add_page, you should change the
> > > dm-crypt code to:
> > > if (!bio_add_page(clone, page, len, 0)) {
> > > WARN(1, "this can't happen");
> > > return NULL;
> > > }
> > > and not write recovery code for a can't-happen case.
> >
> > Thanks for the review Mikulas. But the proper way forward, in the
> > context of this patchset, is to simply change bio_add_page() to
> > __bio_add_page()
> >
> > Subject becomes: "dm crypt: use __bio_add_page to add single page to clone bio"
> >
> > And header can explain that "crypt_alloc_buffer() already allocates
> > enough entries in the clone bio's vector, so bio_add_page can't fail".
> >
> > Mike
>
> Yes, __bio_add_page would look nicer. But bio_add_page can merge adjacent
> pages into a single bvec entry and __bio_add_page can't (I don't know how
> often the merging happens or what is the performance implication of
> non-merging).
>
> I think that for the next merge window, we can apply this patch:
> https://listman.redhat.com/archives/dm-devel/2023-May/054046.html
> which makes this discussion irrelevant. (you can change bio_add_page to
> __bio_add_page in it)
Yes, your patch is on my TODO list. I've rebased my dm-6.5 branch on
the latest block 6.5 branch. I'll be reviewing/rebasing/applying your
patch soon.
Mike
