zram: clear disk io accounting when reset zram device

PATCH Minchan Kim 2 participants 2 messages Jun 4, 2015 Memory management

lore

Share & tools

lore

Minchan Kim

author

2015-06-04 01:53 UTC #0 lore raw

Hello Sergey,

Separate as another thread.

Quoted 29 lines · Sergey Senozhatsky

Quoted 1 line · Minchan Kim
I think the problem is caused from weired feature "reset" of zram.
agree.
Quoted 5 lines
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.

Quoted 8 lines

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.

Signature · 8 lines

Kind regards,
Minchan Kim

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

Sergey Senozhatsky

2015-06-04 02:32 UTC #1 lore raw

Quoted 27 lines · Minchan Kim

Quoted 18 lines
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.
Quoted 8 lines
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.

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>