[PATCH 1/3] MTD: blktrans: Final version of hotplug support

Maxim Levitsky maximlevitsky at gmail.com
Sun Sep 19 21:08:48 EDT 2010


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 <maximlevitsky at gmail.com>
> > 
> > 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




More information about the linux-mtd mailing list