Messages in this thread Patch in this message | | | Date | Mon, 4 May 2015 11:20:08 +0900 | From | Minchan Kim <> | Subject | Re: [PATCHv3 9/9] zram: add dynamic device add/remove functionality |
| |
Hello Sergey,
On Thu, Apr 30, 2015 at 03:51:12PM +0900, Sergey Senozhatsky wrote: > On (04/30/15 15:44), Minchan Kim wrote: > > > > I think the problem of deadlock is that you are trying to remove sysfs file > > > > in sysfs handler. > > > > > > > > #> echo 1 > /sys/xxx/zram_remove > > > > > > > > kernfs_fop_write - hold s_active > > > > -> zram_remove_store > > > > -> zram_remove > > > > -> sysfs_remove_group - hold s_active *again* > > > > > > > > Right? > > > > > > > > > > are those same s_active locks? > > > > > > > > > we hold (s_active#163) and (&bdev->bd_mutex) and want to acquire (s_active#162) > > > > Thanks for sharing the message. > > You're right. It's another lock so it shouldn't be a reason. > > Okay, I will review it. Please give me time. > > > > sure, no problem and no rush. thanks!
I had a time to think over it.
I think your patch is rather tricky so someone cannot see sysfs although he already opened /dev/zram but after a while he can see sysfs. It's weired.
I want to fix it more generic way. Othewise, we might have trouble with locking problem sometime. We already have experieced it with init_lock although we finally fixed it.
I think we can fix it with below patch I hope it's more general and right approach. It's based on your [zram: return zram device_id from zram_add()]
What do you think about?
From e943df5407b880f9262ef959b270226fdc81bc9f Mon Sep 17 00:00:00 2001 From: Minchan Kim <minchan@kernel.org> Date: Mon, 4 May 2015 08:36:07 +0900 Subject: [PATCH 1/2] zram: close race by open overriding
[1] introduced bdev->bd_mutex to protect a race between mount and reset. At that time, we don't have dynamic zram-add/remove feature so it was okay.
However, as we introduce dynamic device feature, bd_mutex became trouble.
CPU 0
echo 1 > /sys/block/zram<id>/reset -> kernfs->s_active(A) -> zram:reset_store->bd_mutex(B)
CPU 1
echo <id> > /sys/class/zram/zram-remove ->zram:zram_remove: bd_mutex(B) -> sysfs_remove_group -> kernfs->s_active(A)
IOW, AB -> BA deadlock
The reason we are holding bd_mutex for zram_remove is to prevent any incoming open /dev/zram[0-9]. Otherwise, we could remove zram others already have opened. But it causes above deadlock problem.
To fix the problem, this patch overrides block_device.open and it returns -EBUSY if zram asserts he claims zram to reset so any incoming open will be failed so we don't need to hold bd_mutex for zram_remove ayn more.
This patch is to prepare for zram-add/remove feature.
[1] ba6b17: zram: fix umount-reset_store-mount race condition Signed-off-by: Minchan Kim <minchan@kernel.org> --- drivers/block/zram/zram_drv.c | 42 ++++++++++++++++++++++++++++++++---------- drivers/block/zram/zram_drv.h | 4 ++++ 2 files changed, 36 insertions(+), 10 deletions(-)
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 3df4394..7fb72dc 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -1074,13 +1074,6 @@ static ssize_t reset_store(struct device *dev, if (!bdev) return -ENOMEM; - mutex_lock(&bdev->bd_mutex); - /* Do not reset an active device! */ - if (bdev->bd_openers) { - ret = -EBUSY; - goto out; - } - ret = kstrtou16(buf, 10, &do_reset); if (ret) goto out; @@ -1090,23 +1083,52 @@ static ssize_t reset_store(struct device *dev, goto out; } + mutex_lock(&bdev->bd_mutex); + /* Do not reset an active device or claimed device */ + if (bdev->bd_openers || zram->claim) { + ret = -EBUSY; + mutex_unlock(&bdev->bd_mutex); + goto out; + } + + /* From now on, anyone can't open /dev/zram[0-9] */ + zram->claim = true; + mutex_unlock(&bdev->bd_mutex); + /* Make sure all pending I/O is finished */ fsync_bdev(bdev); zram_reset_device(zram); - mutex_unlock(&bdev->bd_mutex); revalidate_disk(zram->disk); bdput(bdev); - return len; + mutex_lock(&bdev->bd_mutex); + zram->claim = false; + mutex_unlock(&bdev->bd_mutex); + return len; out: - mutex_unlock(&bdev->bd_mutex); bdput(bdev); return ret; } +static int zram_open(struct block_device *bdev, fmode_t mode) +{ + int ret = 0; + struct zram *zram; + + WARN_ON(!mutex_is_locked(&bdev->bd_mutex)); + + zram = bdev->bd_disk->private_data; + /* zram was claimed to reset so open request fails */ + if (zram->claim) + ret = -EBUSY; + + return ret; +} + static const struct block_device_operations zram_devops = { + .open = zram_open, .swap_slot_free_notify = zram_slot_free_notify, .rw_page = zram_rw_page, .owner = THIS_MODULE diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h index 042994e..6dbe2df 100644 --- a/drivers/block/zram/zram_drv.h +++ b/drivers/block/zram/zram_drv.h @@ -115,5 +115,9 @@ struct zram { */ u64 disksize; /* bytes */ char compressor[10]; + /* + * zram is claimed so open request will be failed + */ + bool claim; /* Protected by bdev->bd_mutex */ }; #endif -- 1.9.3 -- Kind regards, Minchan Kim
| |