[dm-devel] [PATCH 6/8] [persistent-data] Add a transactional array.

[Alasdair G Kergon]

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