Re: [PATCH 2/3] zram: support page-based parallel write

[Sergey Senozhatsky]

Hi Minchan,

On (10/24/16 13:47), Minchan Kim wrote:
> Hi Sergey,
> 
> > > +static void zram_unplug(struct blk_plug_cb *cb, bool from_schedule)
> > > +{
> > > +	spin_lock(&workers.req_lock);
> > > +	if (workers.nr_req)
> > > +		worker_wake_up();
> > > +	spin_unlock(&workers.req_lock);
> > > +	kfree(cb);
> > > +}
> > > +
> > > +static int zram_check_plugged(void)
> > > +{
> > > +	return !!blk_check_plugged(zram_unplug, NULL,
> > > +			sizeof(struct blk_plug_cb));
> > > +}
> > 
> > I'm having some troubles understanding the purpose of zram_check_plugged().
> > it's a global symbol, can you just use it directly? otherwise we are
> > doing additional kmalloc/kfree, spin_lock/unlock and so on.
> 
> I don't understnad it. Why does it that use zram_check_plugged directly reduce
> count things you mentioned?
> > 
> > what am I missing? current->plug? can it affect us? how?
> 
> Sorry. I can't understand your point.

I meant that every blk_check_plugged() is

struct blk_plug_cb *blk_check_plugged(blk_plug_cb_fn unplug, void *data,
				      int size)
{
	struct blk_plug *plug = current->plug;
	struct blk_plug_cb *cb;

	if (!plug)
		return NULL;

	list_for_each_entry(cb, &plug->cb_list, list)
		if (cb->callback == unplug && cb->data == data)
			return cb;

	/* Not currently on the callback list */
	BUG_ON(size < sizeof(*cb));
	cb = kzalloc(size, GFP_ATOMIC);
	if (cb) {
		cb->data = data;
		cb->callback = unplug;
		list_add(&cb->list, &plug->cb_list);
	}
	return cb;
}

which is extra kzalloc/kfree/etc. do we really need to do it all the time?
thus my question -- what am I missing (aka educate me)?


> > hm... no real objection, but exporing this sysfs attr can be very hacky
> > and difficult for people...
> 
> We have been used sysfs for tune the zram for a long time.
> Please suggest ideas if you have better. :)

yeah, but this one feels like a super-hacky knob. basically

"enable when you can't tweak your usage patterns. this will tweak the driver".

so I'd probably prefer to keep it hidden for now (may be eventually
we will come to some "out-of-zram" solution. but the opposition may
be "fix your usage pattern").

besides, you make this sysfs attr .config dependent

> +#ifdef CONFIG_ZRAM_ASYNC_IO
> +static DEVICE_ATTR_RW(use_aio);
> +#endif 
>
>  static struct attribute *zram_disk_attrs[] = {
>         &dev_attr_disksize.attr,
> @@ -1231,6 +1666,9 @@ static struct attribute *zram_disk_attrs[] = {
>         &dev_attr_mem_used_max.attr,
>         &dev_attr_max_comp_streams.attr,
>         &dev_attr_comp_algorithm.attr,
> +#ifdef CONFIG_ZRAM_ASYNC_IO
> +       &dev_attr_use_aio.attr,
> +#endif

so this knob is not even guaranteed to be there all the time.

I wish I could suggest any sound alternative, but I don't have one
at the moment. May be I'll have a chance to speak to block-dev people
next week.

	-ss