Re: [dm-devel] [PATCH 2/2] dm integrity: use init_completion instead of COMPLETION_INITIALIZER_ONSTACK

From: Mikulas Patocka
Date: Wed Aug 16 2017 - 12:13:07 EST

This patch is OK.

Acked-by: Mikulas Patocka <mpatocka@xxxxxxxxxx>

On Tue, 15 Aug 2017, Arnd Bergmann wrote:

> The new lockdep support for completions causeed the stack usage
> in dm-integrity to explode, in case of write_journal from 504 bytes
> to 1120 (using arm gcc-7.1.1):
>
> drivers/md/dm-integrity.c: In function 'write_journal':
> drivers/md/dm-integrity.c:827:1: error: the frame size of 1120 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> The problem is that not only the size of 'struct completion' grows
> significantly, but we end up having multiple copies of it on the stack
> when we assign it from a local variable after the initial declaration.
>
> COMPLETION_INITIALIZER_ONSTACK() is the right thing to use when we
> want to declare and initialize a completion on the stack. However,
> this driver doesn't do that and instead initializes the completion
> just before it is used.
>
> In this case, init_completion() does the same thing more efficiently,
> and drops the stack usage for the function above down to 496 bytes.
> While the other functions in this file are not bad enough to cause
> a warning, they benefit equally from the change, so I do the change
> across the entire file. In the one place where we reuse a completion,
> I picked the cheaper reinit_completion() over init_completion().
>
> Fixes: cd8084f91c02 ("locking/lockdep: Apply crossrelease to completions")
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
> The patch causing this is currently part of linux-next, scheduled for
> 4.14, so it would be good to have this in the same release.
> ---
> drivers/md/dm-integrity.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> index 293a19652d55..b16010bcbd17 100644
> --- a/drivers/md/dm-integrity.c
> +++ b/drivers/md/dm-integrity.c
> @@ -773,13 +773,13 @@ static void write_journal(struct dm_integrity_c *ic, unsigned commit_start, unsi
> unsigned i;
>
> io_comp.ic = ic;
> - io_comp.comp = COMPLETION_INITIALIZER_ONSTACK(io_comp.comp);
> + init_completion(&io_comp.comp);
>
> if (commit_start + commit_sections <= ic->journal_sections) {
> io_comp.in_flight = (atomic_t)ATOMIC_INIT(1);
> if (ic->journal_io) {
> crypt_comp_1.ic = ic;
> - crypt_comp_1.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp_1.comp);
> + init_completion(&crypt_comp_1.comp);
> crypt_comp_1.in_flight = (atomic_t)ATOMIC_INIT(0);
> encrypt_journal(ic, true, commit_start, commit_sections, &crypt_comp_1);
> wait_for_completion_io(&crypt_comp_1.comp);
> @@ -795,18 +795,18 @@ static void write_journal(struct dm_integrity_c *ic, unsigned commit_start, unsi
> to_end = ic->journal_sections - commit_start;
> if (ic->journal_io) {
> crypt_comp_1.ic = ic;
> - crypt_comp_1.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp_1.comp);
> + init_completion(&crypt_comp_1.comp);
> crypt_comp_1.in_flight = (atomic_t)ATOMIC_INIT(0);
> encrypt_journal(ic, true, commit_start, to_end, &crypt_comp_1);
> if (try_wait_for_completion(&crypt_comp_1.comp)) {
> rw_journal(ic, REQ_OP_WRITE, REQ_FUA, commit_start, to_end, &io_comp);
> - crypt_comp_1.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp_1.comp);
> + reinit_completion(&crypt_comp_1.comp);
> crypt_comp_1.in_flight = (atomic_t)ATOMIC_INIT(0);
> encrypt_journal(ic, true, 0, commit_sections - to_end, &crypt_comp_1);
> wait_for_completion_io(&crypt_comp_1.comp);
> } else {
> crypt_comp_2.ic = ic;
> - crypt_comp_2.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp_2.comp);
> + init_completion(&crypt_comp_2.comp);
> crypt_comp_2.in_flight = (atomic_t)ATOMIC_INIT(0);
> encrypt_journal(ic, true, 0, commit_sections - to_end, &crypt_comp_2);
> wait_for_completion_io(&crypt_comp_1.comp);
> @@ -1679,7 +1679,7 @@ static void dm_integrity_map_continue(struct dm_integrity_io *dio, bool from_map
> dio->in_flight = (atomic_t)ATOMIC_INIT(2);
>
> if (need_sync_io) {
> - read_comp = COMPLETION_INITIALIZER_ONSTACK(read_comp);
> + init_completion(&read_comp);
> dio->completion = &read_comp;
> } else
> dio->completion = NULL;
> @@ -1836,7 +1836,7 @@ static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start,
>
> comp.ic = ic;
> comp.in_flight = (atomic_t)ATOMIC_INIT(1);
> - comp.comp = COMPLETION_INITIALIZER_ONSTACK(comp.comp);
> + init_completion(&comp.comp);
>
> i = write_start;
> for (n = 0; n < write_sections; n++, i++, wraparound_section(ic, &i)) {
> @@ -2063,7 +2063,7 @@ static void replay_journal(struct dm_integrity_c *ic)
> if (ic->journal_io) {
> struct journal_completion crypt_comp;
> crypt_comp.ic = ic;
> - crypt_comp.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp.comp);
> + init_completion(&crypt_comp.comp);
> crypt_comp.in_flight = (atomic_t)ATOMIC_INIT(0);
> encrypt_journal(ic, false, 0, ic->journal_sections, &crypt_comp);
> wait_for_completion(&crypt_comp.comp);
> @@ -2636,7 +2636,7 @@ static int create_journal(struct dm_integrity_c *ic, char **error)
> memset(iv, 0x00, ivsize);
>
> skcipher_request_set_crypt(req, sg, sg, PAGE_SIZE * ic->journal_pages + sizeof ic->commit_ids, iv);
> - comp.comp = COMPLETION_INITIALIZER_ONSTACK(comp.comp);
> + init_completion(&comp.comp);
> comp.in_flight = (atomic_t)ATOMIC_INIT(1);
> if (do_crypt(true, req, &comp))
> wait_for_completion(&comp.comp);
> @@ -2693,7 +2693,7 @@ static int create_journal(struct dm_integrity_c *ic, char **error)
>
> sg_init_one(&sg, crypt_data, crypt_len);
> skcipher_request_set_crypt(req, &sg, &sg, crypt_len, iv);
> - comp.comp = COMPLETION_INITIALIZER_ONSTACK(comp.comp);
> + init_completion(&comp.comp);
> comp.in_flight = (atomic_t)ATOMIC_INIT(1);
> if (do_crypt(true, req, &comp))
> wait_for_completion(&comp.comp);
> --
> 2.9.0
>
> --
> dm-devel mailing list
> dm-devel@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/dm-devel
>