[PATCH 0/7] dm-verity: FEC optimizations and fixes

[PATCH 0/7] dm-verity: FEC optimizations and fixes

[Eric Biggers]

Various optimizations and fixes for dm-verity's forward error
correction.  Targeting linux-dm/for-next and based on v6.19-rc1.

Eric Biggers (7):
  dm-verity: move dm_verity_fec_io to mempool
  dm-verity: make dm_verity_fec_io::bufs variable-length
  dm-verity: remove unnecessary condition for verity_fec_finish_io()
  dm-verity: remove unnecessary ifdef around verity_fec_decode()
  dm-verity: make verity_fec_is_enabled() an inline function
  dm-verity: correctly handle dm_bufio_client_create() failure
  dm-verity: allow REED_SOLOMON to be 'm' if DM_VERITY is 'm'

 drivers/md/Kconfig            |   4 +-
 drivers/md/dm-verity-fec.c    | 130 ++++++++++++++--------------------
 drivers/md/dm-verity-fec.h    |  34 ++++++---
 drivers/md/dm-verity-target.c |   5 +-
 drivers/md/dm-verity.h        |   4 ++
 5 files changed, 85 insertions(+), 92 deletions(-)


base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
-- 
2.52.0

[PATCH 1/7] dm-verity: move dm_verity_fec_io to mempool

[Eric Biggers]

Currently, struct dm_verity_fec_io is allocated in the front padding of
struct bio using dm_target::per_io_data_size.  Unfortunately, struct
dm_verity_fec_io is very large: 3096 bytes when CONFIG_64BIT=y &&
PAGE_SIZE == 4096, or 9240 bytes when CONFIG_64BIT=y && PAGE_SIZE ==
16384.  This makes the bio size very large.

Moreover, most of dm_verity_fec_io gets iterated over up to three times,
even on I/O requests that don't require any error correction:

1. To zero the memory on allocation, if init_on_alloc=1.  (This happens
   when the bio is allocated, not in dm-verity itself.)

2. To zero the buffers array in verity_fec_init_io().

3. To free the buffers in verity_fec_finish_io().

Fix all of these inefficiencies by moving dm_verity_fec_io to a mempool.
Replace the embedded dm_verity_fec_io with a pointer
dm_verity_io::fec_io.  verity_fec_init_io() initializes it to NULL,
verity_fec_decode() allocates it on the first call, and
verity_fec_finish_io() cleans it up.  The normal case is that the
pointer simply stays NULL, so the overhead becomes negligible.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 95 +++++++++++++++-----------------------
 drivers/md/dm-verity-fec.h | 14 +++++-
 drivers/md/dm-verity.h     |  4 ++
 3 files changed, 53 insertions(+), 60 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index c79de517afee..bf533ffa7d56 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -16,20 +16,10 @@
 bool verity_fec_is_enabled(struct dm_verity *v)
 {
 	return v->fec && v->fec->dev;
 }
 
-/*
- * Return a pointer to dm_verity_fec_io after dm_verity_io and its variable
- * length fields.
- */
-static inline struct dm_verity_fec_io *fec_io(struct dm_verity_io *io)
-{
-	return (struct dm_verity_fec_io *)
-		((char *)io + io->v->ti->per_io_data_size - sizeof(struct dm_verity_fec_io));
-}
-
 /*
  * Return an interleaved offset for a byte in RS block.
  */
 static inline u64 fec_interleave(struct dm_verity *v, u64 offset)
 {
@@ -209,11 +199,11 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 {
 	bool is_zero;
 	int i, j, target_index = -1;
 	struct dm_buffer *buf;
 	struct dm_bufio_client *bufio;
-	struct dm_verity_fec_io *fio = fec_io(io);
+	struct dm_verity_fec_io *fio = io->fec_io;
 	u64 block, ileaved;
 	u8 *bbuf, *rs_block;
 	u8 want_digest[HASH_MAX_DIGESTSIZE];
 	unsigned int n, k;
 	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
@@ -305,43 +295,44 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 
 	return target_index;
 }
 
 /*
- * Allocate RS control structure and FEC buffers from preallocated mempools,
- * and attempt to allocate as many extra buffers as available.
+ * Allocate and initialize a struct dm_verity_fec_io to use for FEC for a bio.
+ * This runs the first time a block needs to be corrected for a bio.  In the
+ * common case where no block needs to be corrected, this code never runs.
+ *
+ * This always succeeds, as all required allocations are done from mempools.
+ * Additional buffers are also allocated opportunistically to improve error
+ * correction performance, but these aren't required to succeed.
  */
-static int fec_alloc_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio)
+static struct dm_verity_fec_io *fec_alloc_and_init_io(struct dm_verity *v)
 {
+	struct dm_verity_fec *f = v->fec;
+	struct dm_verity_fec_io *fio;
 	unsigned int n;
 
-	if (!fio->rs)
-		fio->rs = mempool_alloc(&v->fec->rs_pool, GFP_NOIO);
+	fio = mempool_alloc(&f->fio_pool, GFP_NOIO);
+	fio->rs = mempool_alloc(&f->rs_pool, GFP_NOIO);
 
-	fec_for_each_prealloc_buffer(n) {
-		if (fio->bufs[n])
-			continue;
+	memset(fio->bufs, 0, sizeof(fio->bufs));
 
-		fio->bufs[n] = mempool_alloc(&v->fec->prealloc_pool, GFP_NOIO);
-	}
+	fec_for_each_prealloc_buffer(n)
+		fio->bufs[n] = mempool_alloc(&f->prealloc_pool, GFP_NOIO);
 
 	/* try to allocate the maximum number of buffers */
 	fec_for_each_extra_buffer(fio, n) {
-		if (fio->bufs[n])
-			continue;
-
-		fio->bufs[n] = kmem_cache_alloc(v->fec->cache, GFP_NOWAIT);
+		fio->bufs[n] = kmem_cache_alloc(f->cache, GFP_NOWAIT);
 		/* we can manage with even one buffer if necessary */
 		if (unlikely(!fio->bufs[n]))
 			break;
 	}
 	fio->nbufs = n;
 
-	if (!fio->output)
-		fio->output = mempool_alloc(&v->fec->output_pool, GFP_NOIO);
-
-	return 0;
+	fio->output = mempool_alloc(&f->output_pool, GFP_NOIO);
+	fio->level = 0;
+	return fio;
 }
 
 /*
  * Initialize buffers and clear erasures. fec_read_bufs() assumes buffers are
  * zeroed before deinterleaving.
@@ -366,14 +357,10 @@ static int fec_decode_rsb(struct dm_verity *v, struct dm_verity_io *io,
 			  const u8 *want_digest, bool use_erasures)
 {
 	int r, neras = 0;
 	unsigned int pos;
 
-	r = fec_alloc_bufs(v, fio);
-	if (unlikely(r < 0))
-		return r;
-
 	for (pos = 0; pos < 1 << v->data_dev_block_bits; ) {
 		fec_init_bufs(v, fio);
 
 		r = fec_read_bufs(v, io, rsb, offset, pos,
 				  use_erasures ? &neras : NULL);
@@ -406,16 +393,20 @@ static int fec_decode_rsb(struct dm_verity *v, struct dm_verity_io *io,
 int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 		      enum verity_block_type type, const u8 *want_digest,
 		      sector_t block, u8 *dest)
 {
 	int r;
-	struct dm_verity_fec_io *fio = fec_io(io);
+	struct dm_verity_fec_io *fio;
 	u64 offset, res, rsb;
 
 	if (!verity_fec_is_enabled(v))
 		return -EOPNOTSUPP;
 
+	fio = io->fec_io;
+	if (!fio)
+		fio = io->fec_io = fec_alloc_and_init_io(v);
+
 	if (fio->level)
 		return -EIO;
 
 	fio->level++;
 
@@ -461,18 +452,15 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 }
 
 /*
  * Clean up per-bio data.
  */
-void verity_fec_finish_io(struct dm_verity_io *io)
+void __verity_fec_finish_io(struct dm_verity_io *io)
 {
 	unsigned int n;
 	struct dm_verity_fec *f = io->v->fec;
-	struct dm_verity_fec_io *fio = fec_io(io);
-
-	if (!verity_fec_is_enabled(io->v))
-		return;
+	struct dm_verity_fec_io *fio = io->fec_io;
 
 	mempool_free(fio->rs, &f->rs_pool);
 
 	fec_for_each_prealloc_buffer(n)
 		mempool_free(fio->bufs[n], &f->prealloc_pool);
@@ -480,27 +468,12 @@ void verity_fec_finish_io(struct dm_verity_io *io)
 	fec_for_each_extra_buffer(fio, n)
 		if (fio->bufs[n])
 			kmem_cache_free(f->cache, fio->bufs[n]);
 
 	mempool_free(fio->output, &f->output_pool);
-}
-
-/*
- * Initialize per-bio data.
- */
-void verity_fec_init_io(struct dm_verity_io *io)
-{
-	struct dm_verity_fec_io *fio = fec_io(io);
-
-	if (!verity_fec_is_enabled(io->v))
-		return;
 
-	fio->rs = NULL;
-	memset(fio->bufs, 0, sizeof(fio->bufs));
-	fio->nbufs = 0;
-	fio->output = NULL;
-	fio->level = 0;
+	mempool_free(fio, &f->fio_pool);
 }
 
 /*
  * Append feature arguments and values to the status table.
  */
@@ -527,10 +500,11 @@ void verity_fec_dtr(struct dm_verity *v)
 	struct dm_verity_fec *f = v->fec;
 
 	if (!verity_fec_is_enabled(v))
 		goto out;
 
+	mempool_exit(&f->fio_pool);
 	mempool_exit(&f->rs_pool);
 	mempool_exit(&f->prealloc_pool);
 	mempool_exit(&f->output_pool);
 	kmem_cache_destroy(f->cache);
 
@@ -756,10 +730,18 @@ int verity_fec_ctr(struct dm_verity *v)
 	if (dm_bufio_get_device_size(f->data_bufio) < v->data_blocks) {
 		ti->error = "Data device is too small";
 		return -E2BIG;
 	}
 
+	/* Preallocate some dm_verity_fec_io structures */
+	ret = mempool_init_kmalloc_pool(&f->fio_pool, num_online_cpus(),
+					sizeof(struct dm_verity_fec_io));
+	if (ret) {
+		ti->error = "Cannot allocate FEC IO pool";
+		return ret;
+	}
+
 	/* Preallocate an rs_control structure for each worker thread */
 	ret = mempool_init(&f->rs_pool, num_online_cpus(), fec_rs_alloc,
 			   fec_rs_free, (void *) v);
 	if (ret) {
 		ti->error = "Cannot allocate RS pool";
@@ -789,10 +771,7 @@ int verity_fec_ctr(struct dm_verity *v)
 	if (ret) {
 		ti->error = "Cannot allocate FEC output pool";
 		return ret;
 	}
 
-	/* Reserve space for our per-bio data */
-	ti->per_io_data_size += sizeof(struct dm_verity_fec_io);
-
 	return 0;
 }
diff --git a/drivers/md/dm-verity-fec.h b/drivers/md/dm-verity-fec.h
index 5fd267873812..b9488d1ddf14 100644
--- a/drivers/md/dm-verity-fec.h
+++ b/drivers/md/dm-verity-fec.h
@@ -38,10 +38,11 @@ struct dm_verity_fec {
 	sector_t blocks;	/* number of blocks covered */
 	sector_t rounds;	/* number of interleaving rounds */
 	sector_t hash_blocks;	/* blocks covered after v->hash_start */
 	unsigned char roots;	/* number of parity bytes, M-N of RS(M, N) */
 	unsigned char rsn;	/* N of RS(M, N) */
+	mempool_t fio_pool;	/* mempool for dm_verity_fec_io */
 	mempool_t rs_pool;	/* mempool for fio->rs */
 	mempool_t prealloc_pool;	/* mempool for preallocated buffers */
 	mempool_t output_pool;	/* mempool for output */
 	struct kmem_cache *cache;	/* cache for buffers */
 	atomic64_t corrected; /* corrected errors */
@@ -69,12 +70,21 @@ extern int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 			     sector_t block, u8 *dest);
 
 extern unsigned int verity_fec_status_table(struct dm_verity *v, unsigned int sz,
 					char *result, unsigned int maxlen);
 
-extern void verity_fec_finish_io(struct dm_verity_io *io);
-extern void verity_fec_init_io(struct dm_verity_io *io);
+extern void __verity_fec_finish_io(struct dm_verity_io *io);
+static inline void verity_fec_finish_io(struct dm_verity_io *io)
+{
+	if (unlikely(io->fec_io))
+		__verity_fec_finish_io(io);
+}
+
+static inline void verity_fec_init_io(struct dm_verity_io *io)
+{
+	io->fec_io = NULL;
+}
 
 extern bool verity_is_fec_opt_arg(const char *arg_name);
 extern int verity_fec_parse_opt_args(struct dm_arg_set *as,
 				     struct dm_verity *v, unsigned int *argc,
 				     const char *arg_name);
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index f975a9e5c5d6..4ad7ce3dae0a 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -102,10 +102,14 @@ struct dm_verity_io {
 	sector_t block;
 	unsigned int n_blocks;
 	bool in_bh;
 	bool had_mismatch;
 
+#ifdef CONFIG_DM_VERITY_FEC
+	struct dm_verity_fec_io *fec_io;
+#endif
+
 	struct work_struct work;
 	struct work_struct bh_work;
 
 	u8 tmp_digest[HASH_MAX_DIGESTSIZE];
 
-- 
2.52.0

[PATCH 2/7] dm-verity: make dm_verity_fec_io::bufs variable-length

[Eric Biggers]

When correcting a data block, the FEC code performs optimally when it
has enough buffers to hold all the needed RS blocks.  That number of
buffers is '1 << (v->data_dev_block_bits - DM_VERITY_FEC_BUF_RS_BITS)'.

However, since v->data_dev_block_bits isn't a compile-time constant, the
code actually used PAGE_SHIFT instead.

With the traditional PAGE_SIZE == data_block_size == 4096, this was
fine.  However, when PAGE_SIZE > data_block_size, this wastes space.
E.g., with data_block_size == 4096 && PAGE_SIZE == 16384, struct
dm_verity_fec_io is 9240 bytes, when in fact only 3096 bytes are needed.

Fix this by making dm_verity_fec_io::bufs a variable-length array.

This makes the macros DM_VERITY_FEC_BUF_MAX and
fec_for_each_extra_buffer() no longer apply, so remove them.  Also
remove the related macro fec_for_each_prealloc_buffer(), since
DM_VERITY_FEC_BUF_PREALLOC is fixed at 1 and was already assumed to be 1
(considering that mempool_alloc() shouldn't be called in a loop).

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 41 ++++++++++++++++++++------------------
 drivers/md/dm-verity-fec.h | 14 ++++++++-----
 2 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index bf533ffa7d56..7574e65c32ae 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -8,10 +8,22 @@
 #include "dm-verity-fec.h"
 #include <linux/math64.h>
 
 #define DM_MSG_PREFIX	"verity-fec"
 
+/*
+ * When correcting a data block, the FEC code performs optimally when it can
+ * collect all the associated RS blocks at the same time.  As each byte is part
+ * of a different RS block, there are '1 << data_dev_block_bits' RS blocks.
+ * There are '1 << DM_VERITY_FEC_BUF_RS_BITS' RS blocks per buffer, so that
+ * gives '1 << (data_dev_block_bits - DM_VERITY_FEC_BUF_RS_BITS)' buffers.
+ */
+static inline unsigned int fec_max_nbufs(struct dm_verity *v)
+{
+	return 1 << (v->data_dev_block_bits - DM_VERITY_FEC_BUF_RS_BITS);
+}
+
 /*
  * If error correction has been configured, returns true.
  */
 bool verity_fec_is_enabled(struct dm_verity *v)
 {
@@ -57,18 +69,10 @@ static u8 *fec_read_parity(struct dm_verity *v, u64 rsb, int index,
 	}
 
 	return res;
 }
 
-/* Loop over each preallocated buffer slot. */
-#define fec_for_each_prealloc_buffer(__i) \
-	for (__i = 0; __i < DM_VERITY_FEC_BUF_PREALLOC; __i++)
-
-/* Loop over each extra buffer slot. */
-#define fec_for_each_extra_buffer(io, __i) \
-	for (__i = DM_VERITY_FEC_BUF_PREALLOC; __i < DM_VERITY_FEC_BUF_MAX; __i++)
-
 /* Loop over each allocated buffer. */
 #define fec_for_each_buffer(io, __i) \
 	for (__i = 0; __i < (io)->nbufs; __i++)
 
 /* Loop over each RS block in each allocated buffer. */
@@ -305,24 +309,23 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
  * Additional buffers are also allocated opportunistically to improve error
  * correction performance, but these aren't required to succeed.
  */
 static struct dm_verity_fec_io *fec_alloc_and_init_io(struct dm_verity *v)
 {
+	const unsigned int max_nbufs = fec_max_nbufs(v);
 	struct dm_verity_fec *f = v->fec;
 	struct dm_verity_fec_io *fio;
 	unsigned int n;
 
 	fio = mempool_alloc(&f->fio_pool, GFP_NOIO);
 	fio->rs = mempool_alloc(&f->rs_pool, GFP_NOIO);
 
-	memset(fio->bufs, 0, sizeof(fio->bufs));
-
-	fec_for_each_prealloc_buffer(n)
-		fio->bufs[n] = mempool_alloc(&f->prealloc_pool, GFP_NOIO);
+	static_assert(DM_VERITY_FEC_BUF_PREALLOC == 1);
+	fio->bufs[0] = mempool_alloc(&f->prealloc_pool, GFP_NOIO);
 
 	/* try to allocate the maximum number of buffers */
-	fec_for_each_extra_buffer(fio, n) {
+	for (n = 1; n < max_nbufs; n++) {
 		fio->bufs[n] = kmem_cache_alloc(f->cache, GFP_NOWAIT);
 		/* we can manage with even one buffer if necessary */
 		if (unlikely(!fio->bufs[n]))
 			break;
 	}
@@ -460,16 +463,15 @@ void __verity_fec_finish_io(struct dm_verity_io *io)
 	struct dm_verity_fec *f = io->v->fec;
 	struct dm_verity_fec_io *fio = io->fec_io;
 
 	mempool_free(fio->rs, &f->rs_pool);
 
-	fec_for_each_prealloc_buffer(n)
-		mempool_free(fio->bufs[n], &f->prealloc_pool);
+	static_assert(DM_VERITY_FEC_BUF_PREALLOC == 1);
+	mempool_free(fio->bufs[0], &f->prealloc_pool);
 
-	fec_for_each_extra_buffer(fio, n)
-		if (fio->bufs[n])
-			kmem_cache_free(f->cache, fio->bufs[n]);
+	for (n = 1; n < fio->nbufs; n++)
+		kmem_cache_free(f->cache, fio->bufs[n]);
 
 	mempool_free(fio->output, &f->output_pool);
 
 	mempool_free(fio, &f->fio_pool);
 }
@@ -732,11 +734,12 @@ int verity_fec_ctr(struct dm_verity *v)
 		return -E2BIG;
 	}
 
 	/* Preallocate some dm_verity_fec_io structures */
 	ret = mempool_init_kmalloc_pool(&f->fio_pool, num_online_cpus(),
-					sizeof(struct dm_verity_fec_io));
+					struct_size((struct dm_verity_fec_io *)0,
+						    bufs, fec_max_nbufs(v)));
 	if (ret) {
 		ti->error = "Cannot allocate FEC IO pool";
 		return ret;
 	}
 
diff --git a/drivers/md/dm-verity-fec.h b/drivers/md/dm-verity-fec.h
index b9488d1ddf14..84f8299673ff 100644
--- a/drivers/md/dm-verity-fec.h
+++ b/drivers/md/dm-verity-fec.h
@@ -17,13 +17,10 @@
 #define DM_VERITY_FEC_MIN_RSN		231	/* ~10% space overhead */
 
 /* buffers for deinterleaving and decoding */
 #define DM_VERITY_FEC_BUF_PREALLOC	1	/* buffers to preallocate */
 #define DM_VERITY_FEC_BUF_RS_BITS	4	/* 1 << RS blocks per buffer */
-/* we need buffers for at most 1 << block size RS blocks */
-#define DM_VERITY_FEC_BUF_MAX \
-	(1 << (PAGE_SHIFT - DM_VERITY_FEC_BUF_RS_BITS))
 
 #define DM_VERITY_OPT_FEC_DEV		"use_fec_from_device"
 #define DM_VERITY_OPT_FEC_BLOCKS	"fec_blocks"
 #define DM_VERITY_OPT_FEC_START		"fec_start"
 #define DM_VERITY_OPT_FEC_ROOTS		"fec_roots"
@@ -50,14 +47,21 @@ struct dm_verity_fec {
 
 /* per-bio data */
 struct dm_verity_fec_io {
 	struct rs_control *rs;	/* Reed-Solomon state */
 	int erasures[DM_VERITY_FEC_MAX_RSN];	/* erasures for decode_rs8 */
-	u8 *bufs[DM_VERITY_FEC_BUF_MAX];	/* bufs for deinterleaving */
-	unsigned int nbufs;		/* number of buffers allocated */
 	u8 *output;		/* buffer for corrected output */
 	unsigned int level;		/* recursion level */
+	unsigned int nbufs;		/* number of buffers allocated */
+	/*
+	 * Buffers for deinterleaving RS blocks.  Each buffer has space for
+	 * the data bytes of (1 << DM_VERITY_FEC_BUF_RS_BITS) RS blocks.  The
+	 * array length is fec_max_nbufs(v), and we try to allocate that many
+	 * buffers.  However, in low-memory situations we may be unable to
+	 * allocate all buffers.  'nbufs' holds the number actually allocated.
+	 */
+	u8 *bufs[];
 };
 
 #ifdef CONFIG_DM_VERITY_FEC
 
 /* each feature parameter requires a value */
-- 
2.52.0

[PATCH 3/7] dm-verity: remove unnecessary condition for verity_fec_finish_io()

[Eric Biggers]

Make verity_finish_io() call verity_fec_finish_io() unconditionally,
instead of skipping it when 'in_bh' is true.

Although FEC can't have been done when 'in_bh' is true,
verity_fec_finish_io() is a no-op when FEC wasn't done.  An earlier
change also made verity_fec_finish_io() very lightweight when FEC wasn't
done.  So it should just be called unconditionally.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-target.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 5c17472d7896..c9f5602a42c6 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -617,12 +617,11 @@ static void verity_finish_io(struct dm_verity_io *io, blk_status_t status)
 	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
 
 	bio->bi_end_io = io->orig_bi_end_io;
 	bio->bi_status = status;
 
-	if (!static_branch_unlikely(&use_bh_wq_enabled) || !io->in_bh)
-		verity_fec_finish_io(io);
+	verity_fec_finish_io(io);
 
 	if (unlikely(status != BLK_STS_OK) &&
 	    unlikely(!(bio->bi_opf & REQ_RAHEAD)) &&
 	    !io->had_mismatch &&
 	    !verity_is_system_shutting_down()) {
-- 
2.52.0

[PATCH 4/7] dm-verity: remove unnecessary ifdef around verity_fec_decode()

[Eric Biggers]

Since verity_fec_decode() has a !CONFIG_DM_VERITY_FEC stub, it can just
be called unconditionally, similar to the other calls in the same file.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-target.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index c9f5602a42c6..777a0ebe8536 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -433,15 +433,13 @@ static int verity_handle_data_hash_mismatch(struct dm_verity *v,
 	if (verity_recheck(v, io, want_digest, blkno, data) == 0) {
 		if (v->validated_blocks)
 			set_bit(blkno, v->validated_blocks);
 		return 0;
 	}
-#if defined(CONFIG_DM_VERITY_FEC)
 	if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_DATA, want_digest,
 			      blkno, data) == 0)
 		return 0;
-#endif
 	if (bio->bi_status)
 		return -EIO; /* Error correction failed; Just return error */
 
 	if (verity_handle_err(v, DM_VERITY_BLOCK_TYPE_DATA, blkno)) {
 		io->had_mismatch = true;
-- 
2.52.0

[PATCH 5/7] dm-verity: make verity_fec_is_enabled() an inline function

[Eric Biggers]

verity_fec_is_enabled() is very short and is called in quite a few
places, so make it an inline function.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 8 --------
 drivers/md/dm-verity-fec.h | 6 +++++-
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 7574e65c32ae..9bf93280cf33 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -20,18 +20,10 @@
 static inline unsigned int fec_max_nbufs(struct dm_verity *v)
 {
 	return 1 << (v->data_dev_block_bits - DM_VERITY_FEC_BUF_RS_BITS);
 }
 
-/*
- * If error correction has been configured, returns true.
- */
-bool verity_fec_is_enabled(struct dm_verity *v)
-{
-	return v->fec && v->fec->dev;
-}
-
 /*
  * Return an interleaved offset for a byte in RS block.
  */
 static inline u64 fec_interleave(struct dm_verity *v, u64 offset)
 {
diff --git a/drivers/md/dm-verity-fec.h b/drivers/md/dm-verity-fec.h
index 84f8299673ff..a002cb5c96c5 100644
--- a/drivers/md/dm-verity-fec.h
+++ b/drivers/md/dm-verity-fec.h
@@ -65,11 +65,15 @@ struct dm_verity_fec_io {
 #ifdef CONFIG_DM_VERITY_FEC
 
 /* each feature parameter requires a value */
 #define DM_VERITY_OPTS_FEC	8
 
-extern bool verity_fec_is_enabled(struct dm_verity *v);
+/* Returns true if forward error correction is enabled. */
+static inline bool verity_fec_is_enabled(struct dm_verity *v)
+{
+	return v->fec && v->fec->dev;
+}
 
 extern int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 			     enum verity_block_type type, const u8 *want_digest,
 			     sector_t block, u8 *dest);
 
-- 
2.52.0

[PATCH 6/7] dm-verity: correctly handle dm_bufio_client_create() failure

[Eric Biggers]

If either of the calls to dm_bufio_client_create() in verity_fec_ctr()
fails, then dm_bufio_client_destroy() is later called with an ERR_PTR()
argument.  That causes a crash.  Fix this.

Fixes: a739ff3f543a ("dm verity: add support for forward error correction")
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 9bf93280cf33..e8f1913f5f12 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -500,13 +500,13 @@ void verity_fec_dtr(struct dm_verity *v)
 	mempool_exit(&f->rs_pool);
 	mempool_exit(&f->prealloc_pool);
 	mempool_exit(&f->output_pool);
 	kmem_cache_destroy(f->cache);
 
-	if (f->data_bufio)
+	if (!IS_ERR_OR_NULL(f->data_bufio))
 		dm_bufio_client_destroy(f->data_bufio);
-	if (f->bufio)
+	if (!IS_ERR_OR_NULL(f->bufio))
 		dm_bufio_client_destroy(f->bufio);
 
 	if (f->dev)
 		dm_put_device(v->ti, f->dev);
 out:
-- 
2.52.0

[PATCH 7/7] dm-verity: allow REED_SOLOMON to be 'm' if DM_VERITY is 'm'

[Eric Biggers]

The dm-verity kconfig options make the common mistake of selecting a
dependency from a bool "sub-option" rather than the main tristate
option.  This unnecessarily forces the dependency to built-in ('y').

Fix this by moving the selections of REED_SOLOMON and REED_SOLOMON_DEC8
into DM_VERITY, conditional on DM_VERITY_FEC.

This allows REED_SOLOMON to be 'm' if DM_VERITY is 'm'.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index 239c1744a926..c58a9a8ea54e 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -547,10 +547,12 @@ config DM_VERITY
 	depends on BLK_DEV_DM
 	select CRYPTO
 	select CRYPTO_HASH
 	select CRYPTO_LIB_SHA256
 	select DM_BUFIO
+	select REED_SOLOMON if DM_VERITY_FEC
+	select REED_SOLOMON_DEC8 if DM_VERITY_FEC
 	help
 	  This device-mapper target creates a read-only device that
 	  transparently validates the data on one underlying device against
 	  a pre-generated tree of cryptographic checksums stored on a second
 	  device.
@@ -596,12 +598,10 @@ config DM_VERITY_VERIFY_ROOTHASH_SIG_PLATFORM_KEYRING
 	  If unsure, say N.
 
 config DM_VERITY_FEC
 	bool "Verity forward error correction support"
 	depends on DM_VERITY
-	select REED_SOLOMON
-	select REED_SOLOMON_DEC8
 	help
 	  Add forward error correction support to dm-verity. This option
 	  makes it possible to use pre-generated error correction data to
 	  recover from corrupted blocks.
 
-- 
2.52.0

Re: [PATCH 1/7] dm-verity: move dm_verity_fec_io to mempool

[Sami Tolvanen]

On Tue, Dec 16, 2025 at 03:06:08PM -0800, Eric Biggers wrote:
> Currently, struct dm_verity_fec_io is allocated in the front padding of
> struct bio using dm_target::per_io_data_size.  Unfortunately, struct
> dm_verity_fec_io is very large: 3096 bytes when CONFIG_64BIT=y &&
> PAGE_SIZE == 4096, or 9240 bytes when CONFIG_64BIT=y && PAGE_SIZE ==
> 16384.  This makes the bio size very large.
> 
> Moreover, most of dm_verity_fec_io gets iterated over up to three times,
> even on I/O requests that don't require any error correction:
> 
> 1. To zero the memory on allocation, if init_on_alloc=1.  (This happens
>    when the bio is allocated, not in dm-verity itself.)
> 
> 2. To zero the buffers array in verity_fec_init_io().
> 
> 3. To free the buffers in verity_fec_finish_io().
> 
> Fix all of these inefficiencies by moving dm_verity_fec_io to a mempool.
> Replace the embedded dm_verity_fec_io with a pointer
> dm_verity_io::fec_io.  verity_fec_init_io() initializes it to NULL,
> verity_fec_decode() allocates it on the first call, and
> verity_fec_finish_io() cleans it up.  The normal case is that the
> pointer simply stays NULL, so the overhead becomes negligible.
> 
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH 2/7] dm-verity: make dm_verity_fec_io::bufs variable-length

[Sami Tolvanen]

On Tue, Dec 16, 2025 at 03:06:09PM -0800, Eric Biggers wrote:
> When correcting a data block, the FEC code performs optimally when it
> has enough buffers to hold all the needed RS blocks.  That number of
> buffers is '1 << (v->data_dev_block_bits - DM_VERITY_FEC_BUF_RS_BITS)'.
> 
> However, since v->data_dev_block_bits isn't a compile-time constant, the
> code actually used PAGE_SHIFT instead.
> 
> With the traditional PAGE_SIZE == data_block_size == 4096, this was
> fine.  However, when PAGE_SIZE > data_block_size, this wastes space.
> E.g., with data_block_size == 4096 && PAGE_SIZE == 16384, struct
> dm_verity_fec_io is 9240 bytes, when in fact only 3096 bytes are needed.
> 
> Fix this by making dm_verity_fec_io::bufs a variable-length array.
> 
> This makes the macros DM_VERITY_FEC_BUF_MAX and
> fec_for_each_extra_buffer() no longer apply, so remove them.  Also
> remove the related macro fec_for_each_prealloc_buffer(), since
> DM_VERITY_FEC_BUF_PREALLOC is fixed at 1 and was already assumed to be 1
> (considering that mempool_alloc() shouldn't be called in a loop).

> [..]

>  static struct dm_verity_fec_io *fec_alloc_and_init_io(struct dm_verity *v)
>  {
> +	const unsigned int max_nbufs = fec_max_nbufs(v);
>  	struct dm_verity_fec *f = v->fec;
>  	struct dm_verity_fec_io *fio;
>  	unsigned int n;
>  
>  	fio = mempool_alloc(&f->fio_pool, GFP_NOIO);
>  	fio->rs = mempool_alloc(&f->rs_pool, GFP_NOIO);
>  
> -	memset(fio->bufs, 0, sizeof(fio->bufs));
> -
> -	fec_for_each_prealloc_buffer(n)
> -		fio->bufs[n] = mempool_alloc(&f->prealloc_pool, GFP_NOIO);
> +	static_assert(DM_VERITY_FEC_BUF_PREALLOC == 1);
> +	fio->bufs[0] = mempool_alloc(&f->prealloc_pool, GFP_NOIO);

Can we just remove DM_VERITY_FEC_BUF_PREALLOC completely instead?

Sami

Re: [PATCH 3/7] dm-verity: remove unnecessary condition for verity_fec_finish_io()

[Sami Tolvanen]

On Tue, Dec 16, 2025 at 03:06:10PM -0800, Eric Biggers wrote:
> Make verity_finish_io() call verity_fec_finish_io() unconditionally,
> instead of skipping it when 'in_bh' is true.
> 
> Although FEC can't have been done when 'in_bh' is true,
> verity_fec_finish_io() is a no-op when FEC wasn't done.  An earlier
> change also made verity_fec_finish_io() very lightweight when FEC wasn't
> done.  So it should just be called unconditionally.
> 
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH 4/7] dm-verity: remove unnecessary ifdef around verity_fec_decode()

[Sami Tolvanen]

On Tue, Dec 16, 2025 at 03:06:11PM -0800, Eric Biggers wrote:
> Since verity_fec_decode() has a !CONFIG_DM_VERITY_FEC stub, it can just
> be called unconditionally, similar to the other calls in the same file.
> 
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH 5/7] dm-verity: make verity_fec_is_enabled() an inline function

[Sami Tolvanen]

On Tue, Dec 16, 2025 at 03:06:12PM -0800, Eric Biggers wrote:
> verity_fec_is_enabled() is very short and is called in quite a few
> places, so make it an inline function.
> 
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH 6/7] dm-verity: correctly handle dm_bufio_client_create() failure

[Sami Tolvanen]

On Tue, Dec 16, 2025 at 03:06:13PM -0800, Eric Biggers wrote:
> If either of the calls to dm_bufio_client_create() in verity_fec_ctr()
> fails, then dm_bufio_client_destroy() is later called with an ERR_PTR()
> argument.  That causes a crash.  Fix this.
> 
> Fixes: a739ff3f543a ("dm verity: add support for forward error correction")
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH 7/7] dm-verity: allow REED_SOLOMON to be 'm' if DM_VERITY is 'm'

[Sami Tolvanen]

On Tue, Dec 16, 2025 at 03:06:14PM -0800, Eric Biggers wrote:
> The dm-verity kconfig options make the common mistake of selecting a
> dependency from a bool "sub-option" rather than the main tristate
> option.  This unnecessarily forces the dependency to built-in ('y').
> 
> Fix this by moving the selections of REED_SOLOMON and REED_SOLOMON_DEC8
> into DM_VERITY, conditional on DM_VERITY_FEC.
> 
> This allows REED_SOLOMON to be 'm' if DM_VERITY is 'm'.
> 
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami