Messages in this thread | | | Subject | Re: [patch] inotify: locking | From | John McCutchan <> | Date | Wed, 22 Sep 2004 19:22:40 -0400 |
| |
On Wed, 2004-09-22 at 15:37, Robert Love wrote: > Hey, John. > > I went over the locking in drivers/char/inotify.c Looks right. > > I made two major changes: > > - In a couple places you used irq-safe locks, but in most places > you did not. It has to be all or never. We do not currently > need protection from interrupts, so I changed the few > irq-safe locks on dev->lock to normal spin_lock/spin_unlock > calls. > > - dev->event_count was an atomic_t, but it was never accessed > outside of dev->lock. I also did not see why ->event_count > was atomic but not ->nr_watches. So I made event_count an > unsigned int and removed the atomic operations. >
Okay, this is my first kernel project so I didn't know/follow all of the rules, I admit it is a bit of a mishmash.
> The rest of the (admittedly a bit large) patch is documenting the > locking rules. I tried to put the locking assumptions in comments at > the top of each function. I made some coding style cleanups as I went > along, too, but not too many (those come next). >
The patch and your previous patches look excellent. I have applied them to my tree and I will be making a new release this evening.
> I do have one remaining concern: create_watcher() is called without the > lock on dev, but it later obtains the lock, before it touches dev. So > it is safe in that regard, but what if dev is deallocated before it > grabs the lock? dev is passed in, so, for example, dev could be freed > (or otherwise manipulated) and then the dereference of dev->lock would > oops. A couple other functions do this. We probably need proper ref > counting on dev. BUT, are all of these call chains off of VFS functions > on the device? Perhaps so long as the device is open it is pinned? >
Yes, AFAIK the only places where we rely on the dev not going away are when we are handling a request from user space. As long as VFS operations are serialized I don't think we have to worry about that.
John - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |