Re: zram: lockdep spew for zram->init_lock

[Minchan Kim <minchan@kernel.org>]

Hello Andrew,

I'm not in office now and I would be off in this week, maybe
so I don't have source code on top of Sergey's recent change
but it seems below code has same problem.

Pz, Sergey or Jerome Could you confirm it instead of me?

On Fri, Feb 28, 2014 at 04:32:06PM -0800, Andrew Morton wrote:
> On Fri, 28 Feb 2014 08:56:29 +0900 Minchan Kim <minchan@kernel.org> 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);

AFAIR, zcomp_create try to allocate memory with GFP_KERNEL so it could
make a same problem. If so, let's allocate it out of the lock.

Please check it.

> 	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;
> }
>