Re: [PATCH RFCv2 05/10] dm-dedup: COW B-tree backend

[Vivek Goyal <vgoyal@redhat.com>]

On Tue, Feb 10, 2015 at 10:25:41AM -0500, Vivek Goyal wrote:
> On Thu, Aug 28, 2014 at 06:07:29PM -0400, Vasily Tarasov wrote:
> 
> [..]
> > +static struct metadata *init_meta_cowbtree(void *input_param, bool *unformatted)
> > +{
> > +	int ret;
> > +	struct metadata *md;
> > +	struct dm_block_manager *meta_bm;
> > +	struct dm_space_map *meta_sm;
> > +	struct dm_space_map *data_sm = NULL;
> > +	struct dm_transaction_manager *tm;
> > +	struct init_param_cowbtree *p =
> > +				(struct init_param_cowbtree *)input_param;
> > +
> > +	DMINFO("Initializing COWBTREE backend");
> > +
> > +	md = kzalloc(sizeof(*md), GFP_NOIO);
> > +	if (!md)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	meta_bm = dm_block_manager_create(p->metadata_bdev, METADATA_BSIZE,
> > +				METADATA_CACHESIZE, METADATA_MAXLOCKS);
> > +	if (IS_ERR(meta_bm)) {
> > +		md = (struct metadata *)meta_bm;
> > +		goto badbm;
> > +	}
> 
> I am wondering how is the error handling working in this function. Upon
> error, we are replacing the md pointer (which is pointing to a kzalloc
> memory) and later calling kfree(md).
> 
> md = kzalloc(sizeof(*md), GFP_NOIO);
> if (err)
>   md = struct metadata *)meta_bm;
> kfree(md).
> 
> What am I missing here.
> 
> [..]
> > +badwritesuper:
> > +	dm_sm_destroy(data_sm);
> > +badsm:
> > +	dm_tm_destroy(tm);
> > +	dm_sm_destroy(meta_sm);
> > +badtm:
> > +	dm_block_manager_destroy(meta_bm);
> > +badbm:
> > +	kfree(md);
> > +	return md;
> > +}
> 
> There are multiple such substituions in this function. I think these are
> bugs (until and unless I am missing something). I will fix these.
> 
> For now, I have pulled in dm-dedup-devel branch and will do code changes
> locally. I am planning to push my branch in following repo for everybody
> to have a look.
> 
> https://github.com/rhvgoyal/linux
> 
> I have yet to push a branch though.

Ok, I have fixed the error handling in init_meta_cowbtree() and also have
refactored the code into smaller functions so that it is much more
readable. (Primarily taken from dm-thin).

I have pushed the branch here.

https://github.com/rhvgoyal/linux/tree/dm-dedup-devel

I am planning to use this branch for committing my improvements and
cleanups. If you like you can send me patches which apply on top of
this branch and I will commit these after review.

So idea is to get dm-dedup merge ready on this branch and then ask
mike snitzer to pull it in.

Thanks
Vivek