Re: [PATCH] zram: clear disk io accounting when reset zram device

Re: [PATCH] zram: clear disk io accounting when reset zram device

[Minchan Kim]

Hello Sergey,

Separate as another thread.

On Sat, May 30, 2015 at 01:16:38PM +0900, Sergey Senozhatsky wrote:
> On (05/29/15 23:54), Minchan Kim wrote:
> > I think the problem is caused from weired feature "reset" of zram.
> 
> agree.
> 
> > Until a while ago, we didn't have hot_add/del feature so we should
> > use custom reset function but now we have hot/add feature.
> > So reset is logically same feature(ie, reset = hot_remove+hot_add
> > but remains same device id).
> > 
> 
> hm, sounds interesting, but I think it will end up being tricky.
> 
> zram_remove() will be called from device's sysfs node (now we call it from
> zram_control sysfs class node, makes a huge difference). sysfs locks the node
> until node's read/write handler returns back, so zram_remove() will be called
> with lock(s_active#XXX) being locked (we had a lockdep splat with these locks
> recently), while zram_remove()->sysfs_remove_group() will once again attempt
> to lock this node (the very same lock(s_active#XXX)). in other words, we cannot
> fully remove zram device from its sysfs attr. and I don't want to add any bool
> flags to zram_remove() and zram_add() indicating that this is a "partial" device
> remove: don't delete device's sysfs group in remove() and don't create it in add().
> 
> 
> doing reset from zram_control is easy, for sure:
> 	lock idr mutex,
> 	do zram_remove() and zram_add()
> 	unlock idr lock.
> 
> `echo ID > /sys/.../zram_control/reset`
> 
> no need to modify remove()/add() -- idr will pick up just released idx,
> so device_id will be preserved. but it'll be hard to drop the per-device
> `reset` attr and to make it a zram_control attr. things would have been
> much simpler if all of zram users were also zramctl users. zramctl, from
> this point of view, lets us change zram interfaces easily -- we merely need
> to teach/modify zramctl, the rest is transparent.

Thanks for the looking.
Fair enough.

So you mean you don't want to add any bool flags. Instead, you want to move
reset interface into /sys/.../zram_control/reset and it would be transparent
if everyone doesn't use raw interface.

Somethings I have in mind.

We should change old interface(ie, /sys/block/zram0/reset) by just
*implementation difficulty* which is just adding a bool flag?
IMO, it's not a good reason to change old interface.
I prefer adding a bool flag if it can meet our goal entirely.

Another thing I repeated several times is that we cannot guarantee
every users in the world will use zramctl forever so we should
be careful to change interface even though a userland tool becomes
popular.

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] zram: clear disk io accounting when reset zram device

[Sergey Senozhatsky]

On (06/04/15 10:53), Minchan Kim wrote:
> > hm, sounds interesting, but I think it will end up being tricky.
> > 
> > zram_remove() will be called from device's sysfs node (now we call it from
> > zram_control sysfs class node, makes a huge difference). sysfs locks the node
> > until node's read/write handler returns back, so zram_remove() will be called
> > with lock(s_active#XXX) being locked (we had a lockdep splat with these locks
> > recently), while zram_remove()->sysfs_remove_group() will once again attempt
> > to lock this node (the very same lock(s_active#XXX)). in other words, we cannot
> > fully remove zram device from its sysfs attr. and I don't want to add any bool
> > flags to zram_remove() and zram_add() indicating that this is a "partial" device
> > remove: don't delete device's sysfs group in remove() and don't create it in add().
> > 
> > 
> > doing reset from zram_control is easy, for sure:
> > 	lock idr mutex,
> > 	do zram_remove() and zram_add()
> > 	unlock idr lock.
> > 
> > `echo ID > /sys/.../zram_control/reset`
> > 
> > no need to modify remove()/add() -- idr will pick up just released idx,
> > so device_id will be preserved. but it'll be hard to drop the per-device
> > `reset` attr and to make it a zram_control attr. things would have been
> > much simpler if all of zram users were also zramctl users. zramctl, from
> > this point of view, lets us change zram interfaces easily -- we merely need
> > to teach/modify zramctl, the rest is transparent.
> 
> Thanks for the looking.
> Fair enough.
> 
> So you mean you don't want to add any bool flags. Instead, you want to move
> reset interface into /sys/.../zram_control/reset and it would be transparent
> if everyone doesn't use raw interface.

I just described the ideal case -- moving reset to zram_control. which
is very much unlikely to happen. even if zramX/reset will become a symlink
to zram_control/reset user still will have to supply a device_id. it's too
late to change this, unfortunately.


> Somethings I have in mind.
> 
> We should change old interface(ie, /sys/block/zram0/reset) by just
> *implementation difficulty* which is just adding a bool flag?
> IMO, it's not a good reason to change old interface.
> I prefer adding a bool flag if it can meet our goal entirely.

well, we can add it. but it's hacky and tricky.


having a clear
"zram_add(void)/zram_remove(void)" vs. "zram_add(bool partial)/zram_remove(bool partial)".

apart from that, zram_add() will introduce additional 4 places where we
can fail to re-create the device:
-- zram = kzalloc(sizeof(struct zram), GFP_KERNEL);
-- ret = idr_alloc(&zram_index_idr, zram, 0, 0, GFP_KERNEL);
-- queue = blk_alloc_queue(GFP_KERNEL);
-- zram->disk = alloc_disk(1);


so, we don't destroy and create zram's sysfs_group. which means that we
better not kfree() and kzalloc() zram pointer, otherise we still need to
set up &disk_to_dev(zram->disk)->kobj. so 'bool partial' flag will now
also make zram kfree()/kmalloc() optional. if we have kfree()/kmalloc()
optional, then we probably should keep idr allocation optional as well. iow,
optional idr_alloc/idr_remove().

which sort of turns zram_add()/zram_remove() into a hell.
I need to think about it more.


> Another thing I repeated several times is that we cannot guarantee
> every users in the world will use zramctl forever so we should
> be careful to change interface even though a userland tool becomes
> popular.

no, of course I'm not saying that everyone is using zramctl nor I count
on it, zram is simply ~4 years older than zramctl.

*things would have been much simpler if* ...


	-ss

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] zram: clear disk io accounting when reset zram device

[Weijie Yang]

This patch clears zram disk io accounting when reset the zram device,
if don't do this, the residual io accounting stat will affect the
diskstat in the next zram active cycle.

Signed-off-by: Weijie Yang <weijie.yang@samsung.com>
---
 drivers/block/zram/zram_drv.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 8dcbced..6e134f4 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -805,7 +805,9 @@ static void zram_reset_device(struct zram *zram)
 	memset(&zram->stats, 0, sizeof(zram->stats));
 	zram->disksize = 0;
 	zram->max_comp_streams = 1;
+
 	set_capacity(zram->disk, 0);
+	part_stat_set_all(&zram->disk->part0, 0);
 
 	up_write(&zram->init_lock);
 	/* I/O operation under all of CPU are done so let's free */
-- 
1.7.10.4


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] zram: clear disk io accounting when reset zram device

[Sergey Senozhatsky]

On (05/29/15 11:23), Weijie Yang wrote:
> This patch clears zram disk io accounting when reset the zram device,
> if don't do this, the residual io accounting stat will affect the
> diskstat in the next zram active cycle.
> 

thanks. my bad.

Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

> Signed-off-by: Weijie Yang <weijie.yang@samsung.com>
> ---
>  drivers/block/zram/zram_drv.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 8dcbced..6e134f4 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -805,7 +805,9 @@ static void zram_reset_device(struct zram *zram)
>  	memset(&zram->stats, 0, sizeof(zram->stats));
>  	zram->disksize = 0;
>  	zram->max_comp_streams = 1;
> +
>  	set_capacity(zram->disk, 0);
> +	part_stat_set_all(&zram->disk->part0, 0);
>  
>  	up_write(&zram->init_lock);
>  	/* I/O operation under all of CPU are done so let's free */
> -- 
> 1.7.10.4
> 
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] zram: clear disk io accounting when reset zram device

[Minchan Kim]

Hello guys,

On Fri, May 29, 2015 at 12:41:41PM +0900, Sergey Senozhatsky wrote:
> On (05/29/15 11:23), Weijie Yang wrote:
> > This patch clears zram disk io accounting when reset the zram device,
> > if don't do this, the residual io accounting stat will affect the
> > diskstat in the next zram active cycle.

Thanks for the fix.

> > 
> 
> thanks. my bad.
> 
> Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

I give my acked-by because it's surely fix so there is no reason to
hesitate.

In future, I hope to change it like below.

I think the problem is caused from weired feature "reset" of zram.
Until a while ago, we didn't have hot_add/del feature so we should
use custom reset function but now we have hot/add feature.
So reset is logically same feature(ie, reset = hot_remove+hot_add
but remains same device id).

If we reuse zram_remove/add for reset, finally it calls del_gendisk
which will do part_stat_set_all for us so we didn't have this kinds
of problems.

It needs more churns and some tweaks of zram_[remove|add] but
it's more clean and consistent between reset and hot_remove.

Just my two cents.

> 
> 	-ss
> 
> > Signed-off-by: Weijie Yang <weijie.yang@samsung.com>
> > ---
> >  drivers/block/zram/zram_drv.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 8dcbced..6e134f4 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -805,7 +805,9 @@ static void zram_reset_device(struct zram *zram)
> >  	memset(&zram->stats, 0, sizeof(zram->stats));
> >  	zram->disksize = 0;
> >  	zram->max_comp_streams = 1;
> > +
> >  	set_capacity(zram->disk, 0);
> > +	part_stat_set_all(&zram->disk->part0, 0);
> >  
> >  	up_write(&zram->init_lock);
> >  	/* I/O operation under all of CPU are done so let's free */
> > -- 
> > 1.7.10.4
> > 
> > 

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] zram: clear disk io accounting when reset zram device

[Sergey Senozhatsky]

On (05/29/15 23:54), Minchan Kim wrote:
> I think the problem is caused from weired feature "reset" of zram.

agree.

> Until a while ago, we didn't have hot_add/del feature so we should
> use custom reset function but now we have hot/add feature.
> So reset is logically same feature(ie, reset = hot_remove+hot_add
> but remains same device id).
> 

hm, sounds interesting, but I think it will end up being tricky.

zram_remove() will be called from device's sysfs node (now we call it from
zram_control sysfs class node, makes a huge difference). sysfs locks the node
until node's read/write handler returns back, so zram_remove() will be called
with lock(s_active#XXX) being locked (we had a lockdep splat with these locks
recently), while zram_remove()->sysfs_remove_group() will once again attempt
to lock this node (the very same lock(s_active#XXX)). in other words, we cannot
fully remove zram device from its sysfs attr. and I don't want to add any bool
flags to zram_remove() and zram_add() indicating that this is a "partial" device
remove: don't delete device's sysfs group in remove() and don't create it in add().


doing reset from zram_control is easy, for sure:
	lock idr mutex,
	do zram_remove() and zram_add()
	unlock idr lock.

`echo ID > /sys/.../zram_control/reset`

no need to modify remove()/add() -- idr will pick up just released idx,
so device_id will be preserved. but it'll be hard to drop the per-device
`reset` attr and to make it a zram_control attr. things would have been
much simpler if all of zram users were also zramctl users. zramctl, from
this point of view, lets us change zram interfaces easily -- we merely need
to teach/modify zramctl, the rest is transparent.

	-ss

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>