From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752079AbbEDCUZ (ORCPT ); Sun, 3 May 2015 22:20:25 -0400 Received: from mail-pa0-f51.google.com ([209.85.220.51]:36294 "EHLO mail-pa0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751537AbbEDCUR (ORCPT ); Sun, 3 May 2015 22:20:17 -0400 Date: Mon, 4 May 2015 11:20:08 +0900 From: Minchan Kim To: Sergey Senozhatsky Cc: Andrew Morton , Nitin Gupta , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCHv3 9/9] zram: add dynamic device add/remove functionality Message-ID: <20150504022008.GA14452@blaptop> References: <1430140911-7818-1-git-send-email-sergey.senozhatsky@gmail.com> <1430140911-7818-10-git-send-email-sergey.senozhatsky@gmail.com> <20150429001624.GA3917@swordfish> <20150429064858.GA5125@blaptop> <20150429070218.GA616@swordfish> <20150429072328.GA2987@swordfish> <20150430054702.GA21771@blaptop> <20150430063457.GA950@swordfish> <20150430064436.GB21771@blaptop> <20150430065111.GC950@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150430065111.GC950@swordfish> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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/reset -> kernfs->s_active(A) -> zram:reset_store->bd_mutex(B) CPU 1 echo > /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 --- 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