On Fri, Dec 04 2015 at 5:03P -0500, Sami Tolvanen <samitolvanen@xxxxxxxxxx> wrote: > On Thu, Dec 03, 2015 at 06:05:38PM -0500, Mike Snitzer wrote: > > If you're OK with those changes I'll fold that commit into your main FEC > > commit. > > Yes, these changes look fine. Thanks! OK, I reviewed the FEC code and have staged it in linux-next with the following fixes/tweaks/nits folded in. Please let me know if you see any problems with this (I'm obviously not an 80-columns zealot). I'm going to carry on reviewing aspects of memory allocations, table loads via ctr, etc.. but the code I've staged in linux-dm.git's 'for-next' is worth handing off to linux-next to see if it catches anything, see: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/log/?h=for-next I'd really appreciate it if you could do some regression testing, etc on your end to verify I didn't break anything while tweaking things. Thanks! From: Mike Snitzer <snitzer@xxxxxxxxxx> Date: Fri, 4 Dec 2015 15:49:58 -0500 Subject: [PATCH] dm verity fec: whitespace fixes and other small nits --- drivers/md/dm-verity-fec.c | 153 +++++++++++++++++++-------------------------- 1 file changed, 65 insertions(+), 88 deletions(-) diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c index a40ac01..e722ce5 100644 --- a/drivers/md/dm-verity-fec.c +++ b/drivers/md/dm-verity-fec.c @@ -69,18 +69,15 @@ static u8 *fec_read_parity(struct dm_verity *v, u64 rsb, int index, position = (index + rsb) * v->fec->roots; block = position >> v->data_dev_block_bits; - *offset = (unsigned)(position - (block << v->data_dev_block_bits)); res = dm_bufio_read(v->fec->bufio, v->fec->start + block, buf); - if (unlikely(IS_ERR(res))) { DMERR("%s: FEC %llu: parity read failed (block %llu): %ld", v->data_dev->name, (unsigned long long)rsb, (unsigned long long)(v->fec->start + block), PTR_ERR(res)); *buf = NULL; - return NULL; } return res; @@ -92,8 +89,7 @@ static u8 *fec_read_parity(struct dm_verity *v, u64 rsb, int index, /* 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++) + 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) \ @@ -132,14 +128,14 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio, u64 rsb, int byte_index, unsigned block_offset, int neras) { - int r = -1, corrected = 0, res; + int r, corrected = 0, res; struct dm_buffer *buf; unsigned n, i, offset; u8 *par, *block; par = fec_read_parity(v, rsb, block_offset, &offset, &buf); - if (unlikely(!par)) - return r; + if (IS_ERR(par)) + return PTR_ERR(par); /* * Decode the RS blocks we have in bufs. Each RS block results in @@ -148,9 +144,12 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio, fec_for_each_buffer_rs_block(fio, n, i) { block = fec_buffer_rs_block(v, fio, n, i); res = fec_decode_rs8(v, fio, block, &par[offset], neras); + if (res < 0) { + dm_bufio_release(buf); - if (res < 0) + r = res; goto error; + } corrected += res; fio->output[block_offset] = block[byte_index]; @@ -164,25 +163,20 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio, if (offset >= 1 << v->data_dev_block_bits) { dm_bufio_release(buf); - par = fec_read_parity(v, rsb, block_offset, &offset, - &buf); - if (unlikely(!par)) - return r; + par = fec_read_parity(v, rsb, block_offset, &offset, &buf); + if (unlikely(IS_ERR(par))) + return PTR_ERR(par); } } - done: r = corrected; error: - dm_bufio_release(buf); - if (r < 0 && neras) DMERR_LIMIT("%s: FEC %llu: failed to correct: %d", v->data_dev->name, (unsigned long long)rsb, r); else if (r > 0) DMWARN_LIMIT("%s: FEC %llu: corrected %d errors", - v->data_dev->name, (unsigned long long)rsb, - r); + v->data_dev->name, (unsigned long long)rsb, r); return r; } @@ -204,7 +198,7 @@ static int fec_is_erasure(struct dm_verity *v, struct dm_verity_io *io, /* * Read data blocks that are part of the RS block and deinterleave as much as - * fits into buffers. Check for erasure locations if neras is non-NULL. + * fits into buffers. Check for erasure locations if @neras is non-NULL. */ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io, u64 rsb, u64 target, unsigned block_offset, @@ -299,7 +293,6 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io, rs_block = fec_buffer_rs_block(v, fio, n, j); rs_block[i] = bbuf[k]; } - done: dm_bufio_release(buf); } @@ -317,7 +310,6 @@ static int fec_alloc_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio) if (!fio->rs) { fio->rs = mempool_alloc(v->fec->rs_pool, 0); - if (unlikely(!fio->rs)) { DMERR("failed to allocate RS"); return -ENOMEM; @@ -329,7 +321,6 @@ static int fec_alloc_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio) continue; fio->bufs[n] = mempool_alloc(v->fec->prealloc_pool, GFP_NOIO); - if (unlikely(!fio->bufs[n])) { DMERR("failed to allocate FEC buffer"); return -ENOMEM; @@ -342,12 +333,10 @@ static int fec_alloc_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio) continue; fio->bufs[n] = mempool_alloc(v->fec->extra_pool, GFP_NOIO); - /* we can manage with even one buffer if necessary */ if (unlikely(!fio->bufs[n])) break; } - fio->nbufs = n; if (!fio->output) { @@ -363,7 +352,7 @@ static int fec_alloc_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio) } /* - * Initialize buffers and clear erasures. fec_read_bufs assumes buffers are + * Initialize buffers and clear erasures. fec_read_bufs() assumes buffers are * zeroed before deinterleaving. */ static void fec_init_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio) @@ -371,27 +360,26 @@ static void fec_init_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio) unsigned n; fec_for_each_buffer(fio, n) - memset(fio->bufs[n], 0, - v->fec->rsn << DM_VERITY_FEC_BUF_RS_BITS); + memset(fio->bufs[n], 0, v->fec->rsn << DM_VERITY_FEC_BUF_RS_BITS); memset(fio->erasures, 0, sizeof(fio->erasures)); } /* * Decode all RS blocks in a single data block and return the target block - * (indicated by "offset") in fio->output. If use_erasures is non-zero, uses + * (indicated by @offset) in fio->output. If @use_erasures is non-zero, uses * hashes to locate erasures. */ static int fec_decode_rsb(struct dm_verity *v, struct dm_verity_io *io, struct dm_verity_fec_io *fio, u64 rsb, u64 offset, - int use_erasures) + bool use_erasures) { int r, neras = 0; unsigned pos; r = fec_alloc_bufs(v, fio); if (unlikely(r < 0)) - return -1; + return r; for (pos = 0; pos < 1 << v->data_dev_block_bits; ) { fec_init_bufs(v, fio); @@ -418,9 +406,8 @@ static int fec_decode_rsb(struct dm_verity *v, struct dm_verity_io *io, if (memcmp(verity_io_real_digest(v, io), verity_io_want_digest(v, io), v->digest_size)) { DMERR_LIMIT("%s: FEC %llu: failed to correct (%d erasures)", - v->data_dev->name, (unsigned long long)rsb, - neras); - return -1; + v->data_dev->name, (unsigned long long)rsb, neras); + return -EILSEQ; } return 0; @@ -445,12 +432,12 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io, enum verity_block_type type, sector_t block, u8 *dest, struct bvec_iter *iter) { - int r = -1; + int r; struct dm_verity_fec_io *fio = fec_io(io); u64 offset, res, rsb; if (!verity_fec_is_enabled(v)) - return -1; + return -EOPNOTSUPP; if (type == DM_VERITY_BLOCK_TYPE_METADATA) block += v->data_blocks; @@ -481,12 +468,12 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io, * them first. Do a second attempt with erasures if the corruption is * bad enough. */ - r = fec_decode_rsb(v, io, fio, rsb, offset, 0); - if (r < 0) - r = fec_decode_rsb(v, io, fio, rsb, offset, 1); - - if (r < 0) - return r; + r = fec_decode_rsb(v, io, fio, rsb, offset, false); + if (r < 0) { + r = fec_decode_rsb(v, io, fio, rsb, offset, true); + if (r < 0) + return r; + } if (dest) memcpy(dest, fio->output, 1 << v->data_dev_block_bits); @@ -577,7 +564,6 @@ void verity_fec_dtr(struct dm_verity *v) if (f->dev) dm_put_device(v->ti, f->dev); - out: kfree(f); v->fec = NULL; @@ -610,13 +596,14 @@ int verity_fec_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v, unsigned *argc, const char *arg_name) { int r; + struct dm_target *ti = v->ti; const char *arg_value; unsigned long long num_ll; unsigned char num_c; char dummy; if (!*argc) { - v->ti->error = "FEC feature arguments require a value"; + ti->error = "FEC feature arguments require a value"; return -EINVAL; } @@ -624,9 +611,9 @@ int verity_fec_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v, (*argc)--; if (!strcasecmp(arg_name, DM_VERITY_OPT_FEC_DEV)) { - r = dm_get_device(v->ti, arg_value, FMODE_READ, &v->fec->dev); + r = dm_get_device(ti, arg_value, FMODE_READ, &v->fec->dev); if (r) { - v->ti->error = "FEC device lookup failed"; + ti->error = "FEC device lookup failed"; return r; } @@ -634,7 +621,7 @@ int verity_fec_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v, if (sscanf(arg_value, "%llu%c", &num_ll, &dummy) != 1 || ((sector_t)(num_ll << (v->data_dev_block_bits - SECTOR_SHIFT)) >> (v->data_dev_block_bits - SECTOR_SHIFT) != num_ll)) { - v->ti->error = "Invalid " DM_VERITY_OPT_FEC_BLOCKS; + ti->error = "Invalid " DM_VERITY_OPT_FEC_BLOCKS; return -EINVAL; } v->fec->blocks = num_ll; @@ -643,7 +630,7 @@ int verity_fec_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v, if (sscanf(arg_value, "%llu%c", &num_ll, &dummy) != 1 || ((sector_t)(num_ll << (v->data_dev_block_bits - SECTOR_SHIFT)) >> (v->data_dev_block_bits - SECTOR_SHIFT) != num_ll)) { - v->ti->error = "Invalid " DM_VERITY_OPT_FEC_START; + ti->error = "Invalid " DM_VERITY_OPT_FEC_START; return -EINVAL; } v->fec->start = num_ll; @@ -652,13 +639,13 @@ int verity_fec_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v, if (sscanf(arg_value, "%hhu%c", &num_c, &dummy) != 1 || !num_c || num_c < (DM_VERITY_FEC_RSM - DM_VERITY_FEC_MAX_RSN) || num_c > (DM_VERITY_FEC_RSM - DM_VERITY_FEC_MIN_RSN)) { - v->ti->error = "Invalid " DM_VERITY_OPT_FEC_ROOTS; + ti->error = "Invalid " DM_VERITY_OPT_FEC_ROOTS; return -EINVAL; } v->fec->roots = num_c; } else { - v->ti->error = "Unrecognized verity FEC feature request"; + ti->error = "Unrecognized verity FEC feature request"; return -EINVAL; } @@ -677,8 +664,8 @@ int verity_fec_ctr_alloc(struct dm_verity *v) v->ti->error = "Cannot allocate FEC structure"; return -ENOMEM; } - v->fec = f; + return 0; } @@ -689,6 +676,7 @@ int verity_fec_ctr_alloc(struct dm_verity *v) int verity_fec_ctr(struct dm_verity *v) { struct dm_verity_fec *f = v->fec; + struct dm_target *ti = v->ti; u64 hash_blocks; if (!verity_fec_is_enabled(v)) { @@ -718,24 +706,22 @@ int verity_fec_ctr(struct dm_verity *v) * simplicity. */ if (v->data_dev_block_bits != v->hash_dev_block_bits) { - v->ti->error = "Block sizes must match to use FEC"; + ti->error = "Block sizes must match to use FEC"; return -EINVAL; } if (!f->roots) { - v->ti->error = "Missing " DM_VERITY_OPT_FEC_ROOTS; + ti->error = "Missing " DM_VERITY_OPT_FEC_ROOTS; return -EINVAL; } - f->rsn = DM_VERITY_FEC_RSM - f->roots; if (!f->blocks) { - v->ti->error = "Missing " DM_VERITY_OPT_FEC_BLOCKS; + ti->error = "Missing " DM_VERITY_OPT_FEC_BLOCKS; return -EINVAL; } f->rounds = f->blocks; - if (do_div(f->rounds, f->rsn)) f->rounds++; @@ -744,7 +730,7 @@ int verity_fec_ctr(struct dm_verity *v) * data_blocks and hash_blocks combined. */ if (f->blocks < v->data_blocks + hash_blocks || !f->rounds) { - v->ti->error = "Invalid " DM_VERITY_OPT_FEC_BLOCKS; + ti->error = "Invalid " DM_VERITY_OPT_FEC_BLOCKS; return -EINVAL; } @@ -753,89 +739,80 @@ int verity_fec_ctr(struct dm_verity *v) * it to be large enough. */ f->hash_blocks = f->blocks - v->data_blocks; - if (dm_bufio_get_device_size(v->bufio) < f->hash_blocks) { - v->ti->error = "Hash device is too small for " - DM_VERITY_OPT_FEC_BLOCKS; + ti->error = "Hash device is too small for " + DM_VERITY_OPT_FEC_BLOCKS; return -E2BIG; } f->bufio = dm_bufio_client_create(f->dev->bdev, - 1 << v->data_dev_block_bits, - 1, 0, NULL, NULL); - + 1 << v->data_dev_block_bits, + 1, 0, NULL, NULL); if (IS_ERR(f->bufio)) { - v->ti->error = "Cannot initialize dm-bufio"; + ti->error = "Cannot initialize FEC bufio client"; return PTR_ERR(f->bufio); } if (dm_bufio_get_device_size(f->bufio) < - (f->start + f->rounds * f->roots) - >> v->data_dev_block_bits) { - v->ti->error = "FEC device is too small"; + ((f->start + f->rounds * f->roots) >> v->data_dev_block_bits)) { + ti->error = "FEC device is too small"; return -E2BIG; } f->data_bufio = dm_bufio_client_create(v->data_dev->bdev, - 1 << v->data_dev_block_bits, - 1, 0, NULL, NULL); - + 1 << v->data_dev_block_bits, + 1, 0, NULL, NULL); if (IS_ERR(f->data_bufio)) { - v->ti->error = "Cannot initialize dm-bufio"; + ti->error = "Cannot initialize FEC data bufio client"; return PTR_ERR(f->data_bufio); } if (dm_bufio_get_device_size(f->data_bufio) < v->data_blocks) { - v->ti->error = "Data device is too small"; + ti->error = "Data device is too small"; return -E2BIG; } /* Preallocate an rs_control structure for each worker thread */ f->rs_pool = mempool_create(num_online_cpus(), fec_rs_alloc, - fec_rs_free, (void *) v); - + fec_rs_free, (void *) v); if (!f->rs_pool) { - v->ti->error = "Cannot allocate RS pool"; + ti->error = "Cannot allocate RS pool"; return -ENOMEM; } f->cache = kmem_cache_create("dm_verity_fec_buffers", - f->rsn << DM_VERITY_FEC_BUF_RS_BITS, - 0, 0, NULL); - + f->rsn << DM_VERITY_FEC_BUF_RS_BITS, + 0, 0, NULL); if (!f->cache) { - v->ti->error = "Cannot create FEC buffer cache"; + ti->error = "Cannot create FEC buffer cache"; return -ENOMEM; } /* Preallocate DM_VERITY_FEC_BUF_PREALLOC buffers for each thread */ f->prealloc_pool = mempool_create_slab_pool(num_online_cpus() * - DM_VERITY_FEC_BUF_PREALLOC, - f->cache); - + DM_VERITY_FEC_BUF_PREALLOC, + f->cache); if (!f->prealloc_pool) { - v->ti->error = "Cannot allocate FEC buffer prealloc pool"; + ti->error = "Cannot allocate FEC buffer prealloc pool"; return -ENOMEM; } f->extra_pool = mempool_create_slab_pool(0, f->cache); - if (!f->extra_pool) { - v->ti->error = "Cannot allocate FEC buffer extra pool"; + ti->error = "Cannot allocate FEC buffer extra pool"; return -ENOMEM; } /* Preallocate an output buffer for each thread */ f->output_pool = mempool_create_kmalloc_pool(num_online_cpus(), - 1 << v->data_dev_block_bits); - + 1 << v->data_dev_block_bits); if (!f->output_pool) { - v->ti->error = "Cannot allocate FEC output pool"; + ti->error = "Cannot allocate FEC output pool"; return -ENOMEM; } /* Reserve space for our per-bio data */ - v->ti->per_bio_data_size += sizeof(struct dm_verity_fec_io); + ti->per_bio_data_size += sizeof(struct dm_verity_fec_io); return 0; } -- 2.4.9 (Apple Git-60) -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
