Re: [PATCH 6/8] [persistent-data] Add a transactional array.

[thornber@redhat.com]

Alasdair,

I've just pushed a series of patches to thin-dev, and all-caches that
address these.

I've refactored the grow() function, breaking it up to reduce
indentation.  However, I've left in 'else' clauses.  I find the
following says 'there are three options' ...

        if (resize->new_nr_full_blocks > resize->old_nr_full_blocks)
                return grow_needs_more_blocks(resize);

        else if (resize->old_nr_entries_in_last_block)
                return grow_extend_tail_block(resize, resize->new_nr_entries_in_last_block);

        else
                return grow_add_tail_block(resize);


... more clearly than ...

        if (resize->new_nr_full_blocks > resize->old_nr_full_blocks)
                return grow_needs_more_blocks(resize);

        else if (resize->old_nr_entries_in_last_block)
                return grow_extend_tail_block(resize, resize->new_nr_entries_in_last_block);

        return grow_add_tail_block(resize);


Will change if you're still having trouble with it though.

- Joe


On Fri, Jan 25, 2013 at 08:11:06PM +0000, Alasdair G Kergon wrote:
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel