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.
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).
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?
Attached patch is against your latest, plus the previous postings.
Thanks,
Robert Love
On Wed, 2004-09-22 at 15:37 -0400, Robert Love wrote:
> Attached patch is against your latest, plus the previous postings.
Evolution needs a heuristic to detect when I say that I attached
something, but when in reality I did not. I do this all the time -
sorry.
Patch attached for real this time.
Robert Love
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
On Wed, 2004-09-22 at 19:22 -0400, John McCutchan wrote:
> 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.
Heh, none of these issues were big, and everything else was fine.
> 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.
You can see what locks and serialization the VFS uses in
Documentation/filesystems/Locking
Robert Love
> Evolution needs a heuristic to detect when I say that I attached
> something, but when in reality I did not.
I've mostly stopped submitting patches using my email client, and
instead submit them using a variant of a 'patch bomb' script. This has
significantly improved the clerical accuracy of my patch submissions.
The variant I'm using is in pretty good shape - you're welcome to
give it a try. See the embedded Usage string for documentation.
http://www.speakeasy.org/~pj99/sgi/sendpatchset
It handles sending one or several related patches, to a list of email
addresses. You prepare a text directive file with the addresses,
subjects and pathnames to the files containing the message contents.
Then you send it all off with a single invocation of this 'sendpatchset'
script.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.650.933.1373