On Thu, Dec 13, 2012 at 08:19:14PM +0000, Joe Thornber wrote:
> diff --git a/drivers/md/persistent-data/dm-array.c b/drivers/md/persistent-data/dm-array.c
> new file mode 100644
> index 0000000..d762caf
> --- /dev/null
> +++ b/drivers/md/persistent-data/dm-array.c
> @@ -0,0 +1,818 @@
> +static int array_block_check(struct dm_block_validator *v,
> + struct dm_block *b,
> + size_t block_size)
Please rename block_size throughout to avoid any possible confusion
with the inline function of the same name.
> +{
> + struct array_block *bh_le = dm_block_data(b);
> + __le32 csum_disk;
> +
> + if (dm_block_location(b) != le64_to_cpu(bh_le->blocknr)) {
> + DMERR_LIMIT("array_block_check failed: blocknr %llu != wanted %llu",
> + le64_to_cpu(bh_le->blocknr), dm_block_location(b));
We generally use an explicit cast to (unsigned long long) to avoid warnings
on some archs. (Check the other places with format strings too.)
> +static uint32_t calc_max_entries(size_t value_size, size_t block_size)
> +{
> + return (block_size - sizeof(struct array_block)) / value_size;
> +}
: warning: conversion to ‘uint32_t’ from ‘long unsigned int’ may alter its value
Perhaps some of the implict casting could be tidied a bit, but I haven't spotted
any places where it causes real problems.
> +static int insert_full_ablocks(struct dm_array_info *info, size_t block_size,
> + unsigned begin_block, unsigned end_block,
> + unsigned max_entries, const void *value,
> + dm_block_t *root)
> +{
> + int r;
> + struct dm_block *block;
> + struct array_block *ab;
> +
> +
Extra blank line.
> + while (begin_block != end_block) {
> + r = alloc_ablock(info, block_size, &block, &ab);
> + if (r)
> + return r;
> +
> + fill_ablock(info, ab, value, le32_to_cpu(ab->max_entries));
max_entries function parameter is unused - which should it be?
> +static int grow(struct resize *resize)
> +{
> + int r;
> + struct dm_block *block;
> + struct array_block *ab;
> +
> + if (resize->new_nr_full_blocks > resize->old_nr_full_blocks) {
> + /*
> + * Pad the end of the old block?
> + */
> + if (resize->old_nr_entries_in_last_block > 0) {
> + r = shadow_ablock(resize->info, &resize->root,
> + resize->old_nr_full_blocks, &block, &ab);
> + if (r)
> + return r;
> +
> + fill_ablock(resize->info, ab, resize->value, resize->max_entries);
> + unlock_ablock(resize->info, block);
> + }
> +
> + /*
> + * Add the full blocks.
> + */
> + r = insert_full_ablocks(resize->info, resize->block_size,
> + resize->old_nr_full_blocks,
> + resize->new_nr_full_blocks,
> + resize->max_entries, resize->value,
> + &resize->root);
> + if (r)
> + return r;
> +
> + /*
> + * Add new tail block?
> + */
> + if (resize->new_nr_entries_in_last_block)
> + r = insert_partial_ablock(resize->info, resize->block_size,
> + resize->new_nr_full_blocks,
> + resize->new_nr_entries_in_last_block,
> + resize->value, &resize->root);
return directly here and drop the else (maybe inverting the if test) and
reducing the indentation?
> + } else {
> + if (!resize->old_nr_entries_in_last_block) {
> + r = insert_partial_ablock(resize->info, resize->block_size,
Redundant {}
> + resize->new_nr_full_blocks,
> + resize->new_nr_entries_in_last_block,
> + resize->value, &resize->root);
> + } else {
...
> + r = dm_tm_ref(info->btree_info.tm, b, &ref_count);
> + if (r) {
> + DMERR_LIMIT("couldn't get reference count");
> + return;
> + }
> +
> + if (ref_count == 1) {
> + /*
> + * We're about to drop the last reference to this ablock.
> + * So we need to decrement the ref count of the contents.
> + */
> + r = get_ablock(info, b, &block, &ab);
> + if (r) {
> + DMERR_LIMIT("couldn't get array block");
> + return;
> + }
Can we add more context to these error messages - e.g. the block number?
Alasdair
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
