Re: zram: lockdep spew for zram->init_lock

[Andrew Morton]

On Fri, 28 Feb 2014 08:56:29 +0900 Minchan Kim <minchan@xxxxxxxxxx> wrote:

> Sasha reported following below lockdep spew of zram.
> 
> It was introduced by [1] in recent linux-next but it's false positive
> because zram_meta_alloc with down_write(init_lock) couldn't be called
> during zram is working as swap device so we could annotate the lock.
> 
> But I don't think it's worthy because it would make greate lockdep
> less effective. Instead, move zram_meta_alloc out of the lock as good
> old day so we could do unnecessary allocation/free of zram_meta for
> initialied device as Sergey claimed in [1] but it wouldn't be common
> and be harmful if someone might do it. Rather than, I'd like to respect
> lockdep which is great tool to prevent upcoming subtle bugs.
> 
> [1] zram: delete zram_init_device
>
> ...
>
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -537,26 +537,27 @@ static ssize_t disksize_store(struct device *dev,
>  		struct device_attribute *attr, const char *buf, size_t len)
>  {
>  	u64 disksize;
> +	struct zram_meta *meta;
>  	struct zram *zram = dev_to_zram(dev);
>  
>  	disksize = memparse(buf, NULL);
>  	if (!disksize)
>  		return -EINVAL;
>  
> +	disksize = PAGE_ALIGN(disksize);
> +	meta = zram_meta_alloc(disksize);
> +	if (!meta)
> +		return -ENOMEM;
> +
>  	down_write(&zram->init_lock);
>  	if (init_done(zram)) {
> +		zram_meta_free(meta);
>  		up_write(&zram->init_lock);
>  		pr_info("Cannot change disksize for initialized device\n");
>  		return -EBUSY;
>  	}
>  
> -	disksize = PAGE_ALIGN(disksize);
> -	zram->meta = zram_meta_alloc(disksize);
> -	if (!zram->meta) {
> -		up_write(&zram->init_lock);
> -		return -ENOMEM;
> -	}
> -
> +	zram->meta = meta;
>  	zram->disksize = disksize;
>  	set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
>  	up_write(&zram->init_lock);

When applying zram-use-zcomp-compressing-backends.patch on top of this
we get a bit of a mess, and simple conflict resolution results in a
leak.

disksize_store() was one of those nasty functions which does multiple
"return" statements after performing locking and resource allocation. 
As usual, this led to a resource leak.  Remember folks, "return" is a
goto in disguise.


Here's what I ended up with.  Please review.

static ssize_t disksize_store(struct device *dev,
		struct device_attribute *attr, const char *buf, size_t len)
{
	u64 disksize;
	struct zram_meta *meta;
	struct zram *zram = dev_to_zram(dev);
	int err;

	disksize = memparse(buf, NULL);
	if (!disksize)
		return -EINVAL;

	disksize = PAGE_ALIGN(disksize);
	meta = zram_meta_alloc(disksize);
	if (!meta)
		return -ENOMEM;

	down_write(&zram->init_lock);
	if (init_done(zram)) {
		pr_info("Cannot change disksize for initialized device\n");
		err = -EBUSY;
		goto out_free_meta;
	}

	zram->comp = zcomp_create(default_compressor);
	if (!zram->comp) {
		pr_info("Cannot initialise %s compressing backend\n",
				default_compressor);
		err = -EINVAL;
		goto out_free_meta;
	}

	zram->meta = meta;
	zram->disksize = disksize;
	set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
	up_write(&zram->init_lock);

	return len;

out_free_meta:
	up_write(&zram->init_lock);
	zram_meta_free(meta);
	return err;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/