[1651] in SIPB_Linux_Development
Re: Ted's Linux inode.c patch
daemon@ATHENA.MIT.EDU (Derek Atkins)
Thu Apr 10 14:43:11 1997
Date: Thu, 10 Apr 1997 14:42:57 -0400
From: Derek Atkins <warlord@MIT.EDU>
To: Erik Nygren <nygren@MIT.EDU>
Cc: linux-dev@MIT.EDU, tytso@MIT.EDU
In-Reply-To: "[1648] in SIPB_Linux_Development"
> In iput, i_lock is set (line 460) without any test for
> whether it is currently locked. This code was added since
This might be a problem...
> Shouldn't lock_inode be used instead of the explicit set of i_lock ?
> There is a wait_on_inode at the top of iput, so using lock_inode
> may not be necessary, but it looks like scheduling may happen
> after the wait_on_inode where the inode might get locked by
> something else.
You don't necessarily need to call lock_inode. So long as you can
be assured that the inode is unlocked, you can sometimes set i_lock
by hand. So, it is safe to execute this code:
wait_on_inode();
i_lock = 1;
> In clear_inode, your patch changes a wait_on_inode to a lock_inode,
> but there is no corresponding unlocking of the inode. Where does this
> happen? (Does the memset of the inode to 0 handle this? It make
> be clearer to use unlock_inode).
The memset unlocks the inode (since it sets i_lock to 0). If you
called an explicit unlock_inode() with would cause problems.
> write_inode also uses wait_on_inode instead of i_lock instead of
> lock_inode, but this is probably safe due to the wait_on_inode
> shortly before it.
I assume you means that it uses "wait_on_inode(); i_lock=1;", which,
as I said above, _is_ safe since the kernel is single-threaded.
> Also, it looks like __wait_on_inode doesn't return until i_lock is 0,
> so the changes to inode_lock shouldn't be necessary....
I think that there _is_ a race condition here, which Ted fixed.
As I recall (I don't have the sources in front of me), you used
to be able to have multiple kernel threads return from wait_on_inode
at the same time due to the way scheduling works:
two threads sitting in wait_on_inode.
inode gets unlocked, which schedules both threads to run
both threads leave wait_on_inode.
> I'll send out a variant of Ted's patch to try out in a bit...
> (Mostly the same thing, but with debugging printk's to log when
> a problem would have occurred. I'm also adding a "fix" to iput.
> Note that I really don't understand this, so Ted or someone should
> sanity check. ;)
I looked at the patch; I _think_ it looks ok. You should add
Ted's addendum as well and try it out.
-derek
