Messages in this thread | | | Date | Mon, 4 May 2015 15:29:37 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [PATCHv3 9/9] zram: add dynamic device add/remove functionality |
| |
On (05/04/15 11:20), Minchan Kim wrote: > 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? >
thanks for looking! didn't have much time over the weekend to investigate. will take a look later today. at glance I think that may do the trick.
-ss
> 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 >
| |