Re: [dm-devel] Fixes 6890e9b8c7d0a1062bbf4f854b6be3723836ad9a

From: Mike Snitzer <snitzer@redhat.com>
To: Nathan Huckleberry <nhuck@google.com>
Cc: Eric Biggers <ebiggers@kernel.org>,
	dm-devel@redhat.com, Mike Snitzer <snitzer@kernel.org>,
	Milan Broz <gmazyland@gmail.com>,
	Sami Tolvanen <samitolvanen@google.com>
Subject: Re: [dm-devel] Fixes 6890e9b8c7d0a1062bbf4f854b6be3723836ad9a
Date: Thu, 4 Aug 2022 16:22:22 -0400	[thread overview]
Message-ID: <Yuwqfmj4p6+pWV0i@redhat.com> (raw)
In-Reply-To: <20220803182905.3279728-1-nhuck@google.com>

On Wed, Aug 03 2022 at  2:29P -0400,
Nathan Huckleberry <nhuck@google.com> wrote:

> The previous patch had a bug when hash verification failed in the
> tasklet.  This happened because the tasklet has already advanced the
> bvec_iter when it falls back to the work-queue implementation.  When the
> work-queue job begins running, it attempts to restart verifiying at
> block i, but the bvec_iter is at block i+1.
> 
> This causes several failures, the most noticeable is that there are not
> enough bytes in the bvec_iter to finish verification.  Since there are
> not enough bytes to finish, the work queue gets stuck in an infinite
> loop in verity_for_io_block.
> 
> To fix this, we can make a local copy of the bvec_iter struct in
> verity_verify_io.  This requires that we restart verification from the
> first block when deferring to a work-queue rather than starting from the
> last block verified in the tasklet.
> 
> This patch also fixes what appears to be a memory leak when FEC is
> enabled.  The call to verity_fec_init_io should only be made if we are
> in a work-queue since a tasklet does not use FEC and is unable to free
> the memory.
> 
> Signed-off-by: Nathan Huckleberry <nhuck@google.com>

Thanks for the detailed header, helped me appreciate the details of
your incremental fixes. I folded your fixes in to the original commit
that adds the "try_verify_in_tasklet" feature. I also layered on some
verity_verify_io() optimizations in new commits. Lastly, I added the
use of WQ_HIGHPRI if "try_verify_in_tasklet" feature is used.

The end result passes Milan's testsuite with and without --use-tasklets.

I've staged the changes in linux-next, see:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=for-next

There is a chance I'll send another 6.0 pull request to Linus with
these changes tomorrow. We've done quite a bit of work here (_before_
the 6.0 merge window opened) so I do think it'd be best to get these
changes upstream sooner rather than later.

If anyone disagrees with sending these changes upstream now please
speak-up!

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel