RE: [PATCH 2/2] md: dm-verity: allow parallel processing of bio blocks
From: yael.chemla
Date: Tue Mar 27 2018 - 17:27:56 EST
> -----Original Message-----
> From: Mike Snitzer <snitzer@xxxxxxxxxx>
> Sent: Tuesday, 27 March 2018 16:17
> To: yael.chemla@xxxxxxxxxxxx
> Cc: 'Alasdair Kergon' <agk@xxxxxxxxxx>; dm-devel@xxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; ofir.drang@xxxxxxxxx; 'Yael Chemla'
> <yael.chemla@xxxxxxx>; 'Eric Biggers' <ebiggers3@xxxxxxxxx>
> Subject: Re: [PATCH 2/2] md: dm-verity: allow parallel processing of bio blocks
>
> On Tue, Mar 27 2018 at 4:55am -0400,
> yael.chemla@xxxxxxxxxxxx <yael.chemla@xxxxxxxxxxxx> wrote:
>
> > Hi Mike
> > I need to rewrite these patches according to issues you and Eric Biggers
> mentioned.
> > please drop this v1 patch.
>
> They've been dropped. BUT please do note that the patches I pushed to
> linux-dm.git were rebased ontop of the 'check_at_most_once' patch.
Thank you so much for many style, formatting and other issues fixes and also for
integration of 'check_at_most_once' patch, it saved me several review iterations.
>
> I never did get an answer about how the sg array is free'd in certain error
> paths (see "FIXME:" in the 2nd patch).
>
Regarding free of sg in two error paths, you were correct.
I fixed it by placing several error labels to differentiate each handling.
I also noted that reqdata_arr[b].req was not released properly, this is also fixed.
following is a diff of my fix based on your modifications.
(I can send it in a patch format, but it doesn't include a fix for Eric Biggers comments)
@@ -573,10 +573,9 @@ static void verity_verify_io(struct dm_verity_io *io)
verity_bv_skip_block(v, io, &io->iter);
continue;
}
-
reqdata_arr[b].req = ahash_request_alloc(v->tfm, GFP_NOIO);
if (unlikely(reqdata_arr[b].req == NULL))
- goto err_memfree;
+ goto err_mem_req;
ahash_request_set_tfm(reqdata_arr[b].req, v->tfm);
/* +1 for the salt buffer */
@@ -586,7 +585,7 @@ static void verity_verify_io(struct dm_verity_io *io)
GFP_NOIO);
if (!sg) {
DMERR_LIMIT("%s: kmalloc_array failed", __func__);
- goto err_memfree;
+ goto err_mem_sg;
}
sg_init_table(sg, num_of_buffs);
// FIXME: if we 'err_memfree' (or continue;) below how does this sg get kfree()'d?
@@ -595,7 +594,7 @@ static void verity_verify_io(struct dm_verity_io *io)
reqdata_arr[b].want_digest,
&reqdata_arr[b].fec_io, &is_zero);
if (unlikely(r < 0))
- goto err_memfree;
+ goto err_mem;
if (is_zero) {
/*
@@ -605,7 +604,7 @@ static void verity_verify_io(struct dm_verity_io *io)
r = verity_for_bv_block(v, io, &io->iter,
verity_bv_zero);
if (unlikely(r < 0))
- goto err_memfree;
+ goto err_mem;
verity_cb_complete(iodata, r);
continue;
}
@@ -644,7 +643,11 @@ static void verity_verify_io(struct dm_verity_io *io)
}
return;
-err_memfree:
+err_mem:
+ kfree(sg);
+err_mem_sg:
+ ahash_request_free(reqdata_arr[b].req);
+err_mem_req:
/*
* reduce expected requests by the number of unsent
* requests, -1 accounting for the current block
atomic_sub(blocks - b - 1, &iodata->expected_reqs);
verity_cb_complete(iodata, -EIO);
> Also, I fixed some issues I saw in error paths, and lots of formatting.
>
> I'll be pretty frustrated if you submit v2 that is blind to the kinds of changes I
> made.
>
I took your modifications and working upon it.
> I'll send you a private copy of the patches just so you have them for your
> reference.
>
> Thanks,
> Mike
>
>
> > -----Original Message-----
> > From: Mike Snitzer <snitzer@xxxxxxxxxx>
> > Sent: Tuesday, 27 March 2018 4:07
> > To: Yael Chemla <yael.chemla@xxxxxxxxxxxx>
> > Cc: Alasdair Kergon <agk@xxxxxxxxxx>; dm-devel@xxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; ofir.drang@xxxxxxxxx; Yael Chemla
> > <yael.chemla@xxxxxxx>
> > Subject: Re: [PATCH 2/2] md: dm-verity: allow parallel processing of
> > bio blocks
> >
> > On Sun, Mar 25 2018 at 2:41pm -0400,
> > Yael Chemla <yael.chemla@xxxxxxxxxxxx> wrote:
> >
> > > Allow parallel processing of bio blocks by moving to async.
> > > completion handling. This allows for better resource utilization of
> > > both HW and software based hash tfm and therefore better
> > > performance in many cases, depending on the specific tfm in use.
> > >
> > > Tested on ARM32 (zynq board) and ARM64 (Juno board).
> > > Time of cat command was measured on a filesystem with various file sizes.
> > > 12% performance improvement when HW based hash was used (ccree
> driver).
> > > SW based hash showed less than 1% improvement.
> > > CPU utilization when HW based hash was used presented 10% less
> > > context switch, 4% less cycles and 7% less instructions. No
> > > difference in CPU utilization noticed with SW based hash.
> > >
> > > Signed-off-by: Yael Chemla <yael.chemla@xxxxxxxxxxxx>
> >
> > This one had various issues. I've fixed most of what I saw and staged in
> linux-next (purely for build test coverage purposes). I may drop this patch if
> others disagree with it (or my sg deallocation in the error path question isn't
> answered).
> >
> > I've staged the changes here (and in linux-next via 'for-next'):
> > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm
> > .git/log/?h=dm-4.17
> >
> > I switched all the new GFP_KERNEL uses to GFP_NOIO. The fact that you're
> doing allocations at all (per IO) is bad enough. Using GFP_KERNEL is a serious
> liability (risk of deadlock if dm-verity were to be used for something like..
> swap.. weird setup but possible).
> >
> > But the gfp flags aside, the need for additional memory and the expectation
> of scalable async parallel IO is potentially at odds with changes like this (that I
> just staged, and had to rebase your 2 patches ontop of):
> > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm
> > .git/commit/?h=dm-4.17&id=a89f6a2cfec86fba7a115642ff082cb4e9450ea6
> >
> > So I'm particulalry interested to hear from google folks to understand if they
> are OK with your proposed verity async crypto API use.
> >
> > Mike
> >