Subject: Re: [patch] inotify: locking
From: John McCutchan <ttb@tentacle.dhs.org>
To: Robert Love <rml@novell.com>
Cc: linux-kernel@vger.kernel.org
In-Reply-To: <1095881861.5090.59.camel@betsy.boston.ximian.com>
References: <1095881861.5090.59.camel@betsy.boston.ximian.com>
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Message-Id: <1095895360.29226.17.camel@vertex>
Mime-Version: 1.0
Date: Wed, 22 Sep 2004 19:22:40 -0400
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2241
Lines: 51

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/