From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-fx0-f49.google.com ([209.85.161.49]) by bombadil.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1OxUsJ-0003bs-KT for linux-mtd@lists.infradead.org; Mon, 20 Sep 2010 01:08:56 +0000 Received: by fxm4 with SMTP id 4so576636fxm.36 for ; Sun, 19 Sep 2010 18:08:54 -0700 (PDT) Subject: Re: [PATCH 1/3] MTD: blktrans: Final version of hotplug support From: Maxim Levitsky To: dedekind1@gmail.com In-Reply-To: <1284928536.4022.5.camel@maxim-laptop> References: <1284851558-1154-1-git-send-email-maximlevitsky@gmail.com> <1284851558-1154-2-git-send-email-maximlevitsky@gmail.com> <1284920908.1704.5.camel@brekeke> <1284928536.4022.5.camel@maxim-laptop> Content-Type: text/plain; charset="UTF-8" Date: Mon, 20 Sep 2010 03:08:48 +0200 Message-ID: <1284944928.27565.2.camel@maxim-laptop> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Cc: linux-mtd@lists.infradead.org, Andrew Morton , David Woodhouse , Maciej Rutecki List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Sun, 2010-09-19 at 22:35 +0200, Maxim Levitsky wrote: > On Sun, 2010-09-19 at 21:28 +0300, Artem Bityutskiy wrote: > > On Sun, 2010-09-19 at 01:12 +0200, Maxim Levitsky wrote: > > > 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, and I removed the BUG from deregister_mtd_blktrans > > > because the current mtd trans driver can still have 'dead' users upon removal. > > > These won't touch it again, so its safe to remove the module. > > > > > > Signed-off-by: Maxim Levitsky > > > > Too many changes in one patch - please, split it on smaller patches. Do > > one change per patch not many changes in one. This patch is not really > > reviewable. And since you are fixing regression, make sure you have > > first N _minimal_ patches which just fix the regression and nothing > > else, and to the rest in the following M patches. > Artem, this patch, really fixes just one regression (especially V2). > The move of blk_cleanup_queue back if you insist, I can skip, its just > one line. On the second thought, you are right, I dudn't notice few more changes I added because I was busy stabilizing it. Btw, no crashes yet despite many attempts. Best regards, Maxim Levitsky