Re: [patch] inotify: locking

[John McCutchan <ttb@tentacle.dhs.org>]

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