diff mbox series

[18/19] dm-crypt: check if adding pages to clone bio fails

Message ID beea645603eccbb045ad9bb777e05a085b91808a.1680108414.git.johannes.thumshirn@wdc.com (mailing list archive)
State New, archived
Headers show
Series bio: check return values of bio_add_page | expand

Commit Message

Johannes Thumshirn March 29, 2023, 5:06 p.m. UTC
Check if adding pages to clone bio fails and if bail out.

This way we can mark bio_add_pages as __must_check.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 drivers/md/dm-crypt.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Damien Le Moal March 29, 2023, 11:49 p.m. UTC | #1
On 3/30/23 02:06, Johannes Thumshirn wrote:
> Check if adding pages to clone bio fails and if bail out.

Nope. The code retries with direct reclaim until it succeeds. Which is very
suspicious...

> 
> This way we can mark bio_add_pages as __must_check.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

With the commit message fixed,

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Yang Shi March 30, 2023, 12:17 a.m. UTC | #2
On Wed, Mar 29, 2023 at 4:49 PM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> On 3/30/23 02:06, Johannes Thumshirn wrote:
> > Check if adding pages to clone bio fails and if bail out.
>
> Nope. The code retries with direct reclaim until it succeeds. Which is very
> suspicious...

It is not related to bio_add_page() failure. It is used to avoid a
race condition when two processes are depleting the mempool
simultaneously.

IIUC I don't think page merge may happen for dm-crypt, so is
__bio_add_page() good enough? I'm working on this code too, using
__bio_add_page() would make my patch easier.

>
> >
> > This way we can mark bio_add_pages as __must_check.
> >
> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> With the commit message fixed,
>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>
>
> --
> Damien Le Moal
> Western Digital Research
>
>
Damien Le Moal March 30, 2023, 12:24 a.m. UTC | #3
On 3/30/23 09:17, Yang Shi wrote:
> On Wed, Mar 29, 2023 at 4:49 PM Damien Le Moal
> <damien.lemoal@opensource.wdc.com> wrote:
>>
>> On 3/30/23 02:06, Johannes Thumshirn wrote:
>>> Check if adding pages to clone bio fails and if bail out.
>>
>> Nope. The code retries with direct reclaim until it succeeds. Which is very
>> suspicious...
> 
> It is not related to bio_add_page() failure. It is used to avoid a
> race condition when two processes are depleting the mempool
> simultaneously.
> 
> IIUC I don't think page merge may happen for dm-crypt, so is
> __bio_add_page() good enough? I'm working on this code too, using
> __bio_add_page() would make my patch easier.

If the BIO was allocated with enough bvecs, we could use that function. But page
merging reduces overhead, so if it can happen, let's use it.

> 
>>
>>>
>>> This way we can mark bio_add_pages as __must_check.
>>>
>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>
>> With the commit message fixed,
>>
>> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
>>
>>
Yang Shi March 30, 2023, 10:29 p.m. UTC | #4
On Wed, Mar 29, 2023 at 5:24 PM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> On 3/30/23 09:17, Yang Shi wrote:
> > On Wed, Mar 29, 2023 at 4:49 PM Damien Le Moal
> > <damien.lemoal@opensource.wdc.com> wrote:
> >>
> >> On 3/30/23 02:06, Johannes Thumshirn wrote:
> >>> Check if adding pages to clone bio fails and if bail out.
> >>
> >> Nope. The code retries with direct reclaim until it succeeds. Which is very
> >> suspicious...
> >
> > It is not related to bio_add_page() failure. It is used to avoid a
> > race condition when two processes are depleting the mempool
> > simultaneously.
> >
> > IIUC I don't think page merge may happen for dm-crypt, so is
> > __bio_add_page() good enough? I'm working on this code too, using
> > __bio_add_page() would make my patch easier.
>
> If the BIO was allocated with enough bvecs, we could use that function. But page
> merging reduces overhead, so if it can happen, let's use it.

It does allocate BIO with enough bvecs. IIUC it will merge the
adjacent pages? If so page merging may happen. Since dm-crypt does
allocate BIO with enough bvces, so it can't fail if I read the code
correctly. I'm wondering whether we could have a never fail variant.

>
> >
> >>
> >>>
> >>> This way we can mark bio_add_pages as __must_check.
> >>>
> >>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> >>
> >> With the commit message fixed,
> >>
> >> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> >>
> >>
> >> --
> >> Damien Le Moal
> >> Western Digital Research
> >>
> >>
>
> --
> Damien Le Moal
> Western Digital Research
>
diff mbox series

Patch

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 3ba53dc3cc3f..19f7e087c6df 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1693,7 +1693,14 @@  static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
 
 		len = (remaining_size > PAGE_SIZE) ? PAGE_SIZE : remaining_size;
 
-		bio_add_page(clone, page, len, 0);
+		if (!bio_add_page(clone, page, len, 0)) {
+			mempool_free(page, &cc->page_pool);
+			crypt_free_buffer_pages(cc, clone);
+			bio_put(clone);
+			gfp_mask |= __GFP_DIRECT_RECLAIM;
+			goto retry;
+
+		}
 
 		remaining_size -= len;
 	}