From patchwork Sun Sep 19 02:03:42 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Maxim Levitsky X-Patchwork-Id: 65145 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from bombadil.infradead.org (bombadil.infradead.org [18.85.46.34]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id CF7AFB6EE9 for ; Sun, 19 Sep 2010 14:20:38 +1000 (EST) Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1OxBMN-0004Ds-Gx; Sun, 19 Sep 2010 04:18:39 +0000 Received: from casper.infradead.org ([2001:770:15f::2]) by bombadil.infradead.org with esmtps (Exim 4.72 #1 (Red Hat Linux)) id 1OxBMK-00041d-HA for linux-mtd@bombadil.infradead.org; Sun, 19 Sep 2010 04:18:36 +0000 Received: from mail-fx0-f49.google.com ([209.85.161.49]) by casper.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1Ox9Gk-0001zk-SM for linux-mtd@lists.infradead.org; Sun, 19 Sep 2010 02:04:48 +0000 Received: by fxm4 with SMTP id 4so233741fxm.36 for ; Sat, 18 Sep 2010 19:03:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:from:to:cc:subject:date :message-id:x-mailer:in-reply-to:references; bh=HDSJy+xC9XZxsLutMvsBFGBgjpQ4vvf/ZQhxVLVp12A=; b=i6VrWVasxz08Ygm1p+XdongfDTK8GCtnm44mMvhjZJKgUq4oH7+5kcnTlWiiMEYRYt 9ZPwsoBHcFCLvFEijNd0v3VPk4g9y7gSt5ygvcqMlijXz39Vd6VW51T0UtZF9f7cy2RX 7p2A+TJ3yPqVfS7QOupD16CgCO1zc+6pVm9l0= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references; b=iBCZ7poHeraWKVSsgzkRG1aorbvBAbkl/qbslF9ZFOGAQ1NFtSLzht1JdTaNW6Kzh1 rCZCf9S73aqp4vTtrLyelaupnqt7nWCefvT5YfwSeweatLHlTQ39n+/as0W96ecUQ+pp iyDnbaoG4sR34dtfYTzA3Qs+jZGZfEaivjjRY= Received: by 10.223.120.201 with SMTP id e9mr3031978far.43.1284861833042; Sat, 18 Sep 2010 19:03:53 -0700 (PDT) Received: from localhost.localdomain (87.68.18.135.cable.012.net.il [87.68.18.135]) by mx.google.com with ESMTPS id a7sm2282978faa.45.2010.09.18.19.03.50 (version=SSLv3 cipher=RC4-MD5); Sat, 18 Sep 2010 19:03:51 -0700 (PDT) From: maximlevitsky@gmail.com To: David Woodhouse Subject: [PATCH V2] MTD: blktrans: Final version of hotplug support Date: Sun, 19 Sep 2010 04:03:42 +0200 Message-Id: <4c956f87.c706df0a.4bfd.5014@mx.google.com> X-Mailer: git-send-email 1.7.0.4 In-Reply-To: <1284851558-1154-2-git-send-email-maximlevitsky@gmail.com> References: <1284851558-1154-2-git-send-email-maximlevitsky@gmail.com> X-CRM114-Version: 20090807-BlameThorstenAndJenny ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20100919_030443_145086_671E8EFE X-CRM114-Status: GOOD ( 23.00 ) X-Spam-Score: -2.0 (--) X-Spam-Report: SpamAssassin version 3.3.1 on casper.infradead.org summary: Content analysis details: (-2.0 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.161.49 listed in list.dnswl.org] 0.0 FREEMAIL_FROM Sender email is freemail (maximlevitsky[at]gmail.com) -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature Cc: Andrew Morton , linux-mtd@lists.infradead.org, Maxim Levitsky , Maciej Rutecki X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: linux-mtd-bounces@lists.infradead.org Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org From: Maxim Levitsky Or at least this patch claims it to be so... Now it once again possible to remove mtdtrans and mtd drivers even if underlying mtd device is mounted. This time in addition to code review, I also made the code pass some torture tests like module reload in a loop + read in a loop + card insert/removal all at same time. The blktrans_open/blktrans_release don't take the mtd table lock because: While device is added (that includes execution of add_mtd_blktrans_dev) the lock is already taken Now suppose the device will never be removed. In this case even if we have changes in mtd table, the entry that we need will stay exactly the same. (Note that we don't look at table at all, just following private pointer of block device). Now suppose that someone tries to remove the mtd device. This will be propagated to trans driver which _ought_ to call del_mtd_blktrans_dev which will take the per device lock, release the mtd device and set trans->mtd = NULL. From this point on, following opens won't even be able to know anything about that mtd device (which at that point is likely not to exist) Also the same care is taken not to trip over NULL mtd pointer in blktrans_dev_release. In addition to that I moved the blk_cleanup_queue back to del_mtd_blktrans_dev because I now know that is ok. Signed-off-by: Maxim Levitsky --- drivers/mtd/mtd_blkdevs.c | 67 +++++++++++++++++++++------------------------ 1 files changed, 31 insertions(+), 36 deletions(-) diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c index 62e6870..c124cbe 100644 --- a/drivers/mtd/mtd_blkdevs.c +++ b/drivers/mtd/mtd_blkdevs.c @@ -47,7 +47,6 @@ void blktrans_dev_release(struct kref *kref) container_of(kref, struct mtd_blktrans_dev, ref); dev->disk->private_data = NULL; - blk_cleanup_queue(dev->rq); put_disk(dev->disk); list_del(&dev->list); kfree(dev); @@ -133,6 +132,10 @@ static int mtd_blktrans_thread(void *arg) if (!req && !(req = blk_fetch_request(rq))) { set_current_state(TASK_INTERRUPTIBLE); + + if (kthread_should_stop()) + break; + spin_unlock_irq(rq->queue_lock); schedule(); spin_lock_irq(rq->queue_lock); @@ -176,54 +179,53 @@ static void mtd_blktrans_request(struct request_queue *rq) static int blktrans_open(struct block_device *bdev, fmode_t mode) { struct mtd_blktrans_dev *dev = blktrans_dev_get(bdev->bd_disk); - int ret; + int ret = 0; if (!dev) - return -ERESTARTSYS; /* FIXME: busy loop! -arnd*/ + return -EIO; - lock_kernel(); mutex_lock(&dev->lock); - if (!dev->mtd) { - ret = -ENXIO; + if (dev->open++) goto unlock; - } - ret = !dev->open++ && dev->tr->open ? dev->tr->open(dev) : 0; + kref_get(&dev->ref); + __module_get(dev->tr->owner); + + if (dev->mtd) { + ret = dev->tr->open ? dev->tr->open(dev) : 0; + __get_mtd_device(dev->mtd); + } - /* Take another reference on the device so it won't go away till - last release */ - if (!ret) - kref_get(&dev->ref); unlock: mutex_unlock(&dev->lock); blktrans_dev_put(dev); - unlock_kernel(); return ret; } static int blktrans_release(struct gendisk *disk, fmode_t mode) { struct mtd_blktrans_dev *dev = blktrans_dev_get(disk); - int ret = -ENXIO; + int ret = 0; if (!dev) return ret; - lock_kernel(); mutex_lock(&dev->lock); - /* Release one reference, we sure its not the last one here*/ - kref_put(&dev->ref, blktrans_dev_release); - - if (!dev->mtd) + if (--dev->open) goto unlock; - ret = !--dev->open && dev->tr->release ? dev->tr->release(dev) : 0; + kref_put(&dev->ref, blktrans_dev_release); + module_put(dev->tr->owner); + + if (dev->mtd) { + ret = dev->tr->release ? dev->tr->release(dev) : 0; + __put_mtd_device(dev->mtd); + } unlock: mutex_unlock(&dev->lock); blktrans_dev_put(dev); - unlock_kernel(); return ret; } @@ -256,7 +258,6 @@ static int blktrans_ioctl(struct block_device *bdev, fmode_t mode, if (!dev) return ret; - lock_kernel(); mutex_lock(&dev->lock); if (!dev->mtd) @@ -271,7 +272,6 @@ static int blktrans_ioctl(struct block_device *bdev, fmode_t mode, } unlock: mutex_unlock(&dev->lock); - unlock_kernel(); blktrans_dev_put(dev); return ret; } @@ -385,9 +385,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new) gd->queue = new->rq; - __get_mtd_device(new->mtd); - __module_get(tr->owner); - /* Create processing thread */ /* TODO: workqueue ? */ new->thread = kthread_run(mtd_blktrans_thread, new, @@ -410,8 +407,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new) } return 0; error4: - module_put(tr->owner); - __put_mtd_device(new->mtd); blk_cleanup_queue(new->rq); error3: put_disk(new->disk); @@ -448,17 +443,17 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old) blk_start_queue(old->rq); spin_unlock_irqrestore(&old->queue_lock, flags); - /* Ask trans driver for release to the mtd device */ + blk_cleanup_queue(old->rq); + + /* If the device is currently open, tell trans driver to close it, + then put mtd device, and don't touch it again */ mutex_lock(&old->lock); - if (old->open && old->tr->release) { - old->tr->release(old); - old->open = 0; + if (old->open) { + if (old->tr->release) + old->tr->release(old); + __put_mtd_device(old->mtd); } - __put_mtd_device(old->mtd); - module_put(old->tr->owner); - - /* At that point, we don't touch the mtd anymore */ old->mtd = NULL; mutex_unlock(&old->lock);