From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?utf-8?B?SsO2cm4=?= Engel Subject: Re: [PATCH resend] introduce I_SYNC Date: Fri, 1 Jun 2007 13:56:44 +0200 Message-ID: <20070601115644.GA30328@lazybastard.org> References: <20070516170110.GC8113@lazybastard.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Andrew Morton , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Dave Kleikamp , David Chinner , Al Viro , Christoph Hellwig To: Anton Altaparmakov Return-path: Received: from lazybastard.de ([212.112.238.170]:38626 "EHLO longford.lazybastard.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751282AbXFAMBY (ORCPT ); Fri, 1 Jun 2007 08:01:24 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Fri, 1 June 2007 09:59:17 +0100, Anton Altaparmakov wrote: >=20 > I agree that your patch is a good idea. I reviewed the latest =20 > incarnation and it makes sense to me. And your comment concerning =20 Thanks. > the flags is a very welcome addition. Probably ought to find its way= =20 > into Documentation/filesystems/Locking or vfs.txt or somewhere like =20 > that also. Might make sense. Right now I would be more interested in getting the questions at the bottom answered. Possibly the right answer might be "here is a patch to fix it". > Note that once your patch is applied I think it would make sense to =20 > follow up with a second patch to remove the I_LOCK flag completely. = =20 > The only remaining uses are either together with I_NEW in which case = =20 > I_LOCK can be removed altogether or can be substituted with I_NEW =20 > when only I_LOCK is used. This is because no places remain where we = =20 > set I_LOCK by itself any more with your patch. The only place where = =20 > we set it is the place where a new inode gets created in memory and =20 > in that place we also set I_NEW at the same time as I_LOCK. =20 > wait_on_inode() can then be changed to wait on I_NEW instead of on =20 > I_LOCKED. That way we have one less confusing flag to worry about =20 > and things are much easier to understand. True. Waiting on I_NEW would be equivalent to waiting on I_LOCK. To some degree I still prefer the current method. I_NEW is a state, while I_LOCK is a lock or completion method. Having a confusing mix of state/lock/completion bits is bad enough. Having such a mix of uses fo= r a single bit could be even worse. > >I still suspect that NTFS has hit the same deadlock and its current = =20 > >"fix" will cause data corruption instead. >=20 > The NTFS "fix" will not cause data corruption at all. The usage in =20 > NTFS is very different... I am afraid your patch does not address =20 > the deadlock with NTFS or rather it only addresses the inode write =20 > deadlock and does not address the get_new_inode() deadlock that =20 > exists with ilookup5() and is avoided by ilookup5_nowait(). This =20 > deadlock is inherent to what NTFS does so you don't need to worry =20 > about it. (If you want I am happy to explain it but I would rather =20 > not waste my time explaining if no-one except me cares about it...) Two seperate deadlocks exist, we agree on this. I_SYNC only solves one of the two. LogFS solved the second deadlock by implementing its own destroy_inode() and drop_inode() methods. Any inodes that would cause the get_new_inode() deadlock get cached and logfs implements its own iget() method to return a cached inode from any deadlock-prone codepath= =2E It is not a pretty solution, but ilookup_nowait() would definitely caus= e data corruption for logfs, so that was not an option. Better ideas are very welcome. J=C3=B6rn --=20 The strong give up and move away, while the weak give up and stay. -- unknown - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html