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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
