Re: [PATCHv2 10/10] zram: add dynamic device add/remove functionality

[Minchan Kim <minchan@kernel.org>]

On Thu, Apr 23, 2015 at 01:23:00PM +0900, Sergey Senozhatsky wrote:
> On (04/23/15 12:06), Minchan Kim wrote:
> > > +Example:
> > > +	cat /sys/class/zram-control/zram_add
> > 
> > Why do we put zram-contol there rather than /sys/block/zram
> 
> that's what clsss_register() does.
> 
> [..]
> 
> > > @@ -1168,8 +1172,15 @@ static int zram_add(int device_id)
> > 
> > Why do zram_add need device_id?
> > We decided to remove option user pass device_id.
> 
> will cleanup. it was simpler at that time to support both devices
> created by sysfs request and devices pre-crated for num_devices
> module param.
> 
> 
> > > +static struct zram *zram_lookup(int dev_id)
> > > +{
> > > +	struct zram *zram;
> > > +
> > > +	zram = idr_find(&zram_index_idr, dev_id);
> > > +	if (zram)
> > > +		return zram;
> > > +	return ERR_PTR(-ENODEV);
> > 
> > Just return NULL which is more simple and readable.
> > 
> 
> ok.
> 
> 
> [..]
> > > +	/*
> > > +	 * First, make ->disksize device attr RO, closing
> > > +	 * zram_remove() vs disksize_store() race window
> > 
> > Why don't you use zram->init_lock to protect the race?
> 
> zram_reset_device() takes this lock internally. but, it
> unlocks the device upon the return from zram_reset_device():
> 
> lock idr_lock
>  zram_reset_device()
>   lock bd_mutex
>     __zam_reset_device()
>         lock init_lock
>          reset
>         unlock init_lock     ---\
>   unlock bd_mutex                |
>                                  |<----- disksize_store() race window
>  zram_remove()               ---/
> unlock idr_lock
> 
> 
> until we call zram_remove() (which does sysfs_remove_group()) device has
> sysfs attrs and, thus, disksize_store() can arrive in the middle. the
> simplest things I came up with was that RO bit on sysfs disksize attrs.
> I can factor out another set of __foo function to handle it differently,
> not sure if this worth it.

I believe we should handle it with init_lock more elegantly rather than
RO trick. The init_lock was born to protect race between init and exit
although we didn't need it until now since we don't have dynamic device
feature. So, with refactoring, we should handle it with init_lock.

> 
> I'll revisit it.

Thanks. I believe you will find.

-- 
Kind regards,
Minchan Kim