zram: remove init_lock in zram_make_request

Quoted 24 lines · Sergey Senozhatsky

Hello Minchan,

Quoted 12 lines · Minchan Kim

Quoted 8 lines

Review

Quoted 4 lines

static inline int init_done(struct zram *zram)
{

(quoted diff) +1 -1

-	return zram->meta != NULL;
+	return zram->disksize != 0;

we don't set ->disksize to 0 when create device. and I think it's better to use refcount here, but set it to 0 during device creation. (see the patch below)

There was a reason I didn't use refcount there.
I should have written down it.

Quoted 8 lines

We need something to prevent further I/O handling on other CPUs. Otherwise, it's livelock. For example, new 'A' I/O rw path on CPU 1 can see non-zero refcount if another CPU is going on rw. Then, another new 'B' I/O rw path on CPU 2 can see non-zero refcount if A I/O is going on. Then, another new 'C' I/O rw path on CPU 3 can see non-zero refcount if B I/O is going on. Finally, 'A' IO is done on CPU 1 and next I/O 'D' on CPU 1 can see non-zero refcount because 'C' on CPU 3 is going on. Infinite loop.

Quoted 4 lines

sure, I did think about this. and I actually didn't find any reason not to use ->refcount there. if user wants to reset the device, he first should umount it to make bdev->bd_holders check happy. and that's where IOs will be failed. so it makes sense to switch to ->refcount there, IMHO.

If we use zram as block device itself(not a fs or swap) and open the block device as !FMODE_EXCL, bd_holders will be void.

Another topic: As I didn't see enough fs/block_dev.c bd_holders in zram would be mess. I guess we need to study hotplug of device and implement it for zram reset rather than strange own konb. It should go TODO. :(

Quoted 6 lines

If we use zram as block device itself(not a fs or swap) and open the block device as !FMODE_EXCL, bd_holders will be void.

Another topic: As I didn't see enough fs/block_dev.c bd_holders in zram would be mess. I guess we need to study hotplug of device and implement it for zram reset rather than strange own konb. It should go TODO. :(

Actually, I thought bd_mutex use from custom driver was terrible idea so we should walk around with device hotplug but as I look through another drivers, they have used the lock for a long time.
Maybe it's okay to use it in zram?
If so, Ganesh's patch is no problem to me although I didn't' review it in detail. One thing I want to point out is that it would be better to change bd_holders with bd_openers to filter out because dd test opens block device as !EXCL so bd_holders will be void.

What do you think about it?

Quoted 16 lines · Minchan Kim

Quoted 6 lines

If we use zram as block device itself(not a fs or swap) and open the block device as !FMODE_EXCL, bd_holders will be void.

Another topic: As I didn't see enough fs/block_dev.c bd_holders in zram would be mess. I guess we need to study hotplug of device and implement it for zram reset rather than strange own konb. It should go TODO. :(

Actually, I thought bd_mutex use from custom driver was terrible idea so we should walk around with device hotplug but as I look through another drivers, they have used the lock for a long time.
Maybe it's okay to use it in zram?
If so, Ganesh's patch is no problem to me although I didn't' review it in detail. One thing I want to point out is that it would be better to change bd_holders with bd_openers to filter out because dd test opens block device as !EXCL so bd_holders will be void.

Quoted 2 lines

What do you think about it?

a quick idea:
can we additionally move all bd flush and put work after zram_reset_device(zram, true) and, perhaps, replace ->bd_holders with something else?

zram_reset_device() will not return until we have active IOs, pending IOs will be invalidated by ->disksize != 0.

	-ss

Quoted 6 lines

a quick idea:
can we additionally move all bd flush and put work after zram_reset_device(zram, true) and, perhaps, replace ->bd_holders with something else?

zram_reset_device() will not return until we have active IOs, pending IOs will be invalidated by ->disksize != 0.

Sorry, I don't get it. Could you describe what you are concerning about active I/O? My concern is just race bd_holder/bd_openers and bd_holders of zram check. I don't think any simple solution without bd_mutex.
If we can close the race, anything could be a solution.
If we close the race, we should return -EBUSY if anyone is opening the zram device so bd_openers check would be better than bd_holders.

So, guys, how about doing it differently, in less lines of code, hopefully. Don't move reset_store()'s work to zram_reset_device(). Instead, move

	set_capacity(zram->disk, 0);
	revalidate_disk(zram->disk);

out from zram_reset_device() to reset_store(). this two function are executed only when called from reset_store() anyway. this also will let us drop `bool reset capacity' param from zram_reset_device().

so we will do in reset_store()

	mutex_lock(bdev->bd_mutex);

	fsync_bdev(bdev);
	zram_reset_device(zram);
	set_capacity(zram->disk, 0);

	mutex_unlock(&bdev->bd_mutex);

	revalidate_disk(zram->disk);
	bdput(bdev);

and change zram_reset_device(zram, false) call to simply zram_reset_device(zram) in __exit zram_exit(void).

Quoted 27 lines · Sergey Senozhatsky

So, guys, how about doing it differently, in less lines of code, hopefully. Don't move reset_store()'s work to zram_reset_device(). Instead, move

	set_capacity(zram->disk, 0);
	revalidate_disk(zram->disk);

out from zram_reset_device() to reset_store(). this two function are executed only when called from reset_store() anyway. this also will let us drop `bool reset capacity' param from zram_reset_device().

so we will do in reset_store()

	mutex_lock(bdev->bd_mutex);

	fsync_bdev(bdev);
	zram_reset_device(zram);
	set_capacity(zram->disk, 0);

	mutex_unlock(&bdev->bd_mutex);

	revalidate_disk(zram->disk);
	bdput(bdev);

Quoted 3 lines

and change zram_reset_device(zram, false) call to simply zram_reset_device(zram) in __exit zram_exit(void).

Hello,

Minchan, Ganesh, I sent a patch last night, with the above solution. looks ok to you?

Minchan, I think I'll send my small struct zram clean-up patch after your init_lock patch. what's your opinion?

Quoted 32 lines · Sergey Senozhatsky

Quoted 30 lines · Sergey Senozhatsky

So, guys, how about doing it differently, in less lines of code, hopefully. Don't move reset_store()'s work to zram_reset_device(). Instead, move

	set_capacity(zram->disk, 0);
	revalidate_disk(zram->disk);

out from zram_reset_device() to reset_store(). this two function are executed only when called from reset_store() anyway. this also will let us drop `bool reset capacity' param from zram_reset_device().

so we will do in reset_store()

	mutex_lock(bdev->bd_mutex);

	fsync_bdev(bdev);
	zram_reset_device(zram);
	set_capacity(zram->disk, 0);

	mutex_unlock(&bdev->bd_mutex);

	revalidate_disk(zram->disk);
	bdput(bdev);

and change zram_reset_device(zram, false) call to simply zram_reset_device(zram) in __exit zram_exit(void).

Quoted 4 lines

Hello,

Minchan, Ganesh, I sent a patch last night, with the above solution. looks ok to you?

Just I sent a feedback.

Quoted 3 lines

Minchan, I think I'll send my small struct zram clean-up patch after your init_lock patch. what's your opinion?

Good for me.

Thanks.

Signature · 2 lines

Kind regards,
Minchan Kim