From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jamie Lokier Subject: Re: [PATCH] fs/locks.c Partially fixes leases issue Date: Thu, 4 Dec 2003 02:55:16 +0000 Sender: linux-fsdevel-owner@vger.kernel.org Message-ID: <20031204025516.GB1216@mail.shareable.org> References: <200312031748.37958.theman@josephdwagner.info> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: matthew@wil.cx, willy@debian.org, linux-fsdevel@vger.kernel.org Return-path: Received: from mail.jlokier.co.uk ([81.29.64.88]:20098 "EHLO mail.shareable.org") by vger.kernel.org with ESMTP id S261411AbTLDCzY (ORCPT ); Wed, 3 Dec 2003 21:55:24 -0500 To: "Joseph D. Wagner" Content-Disposition: inline In-Reply-To: <200312031748.37958.theman@josephdwagner.info> List-Id: linux-fsdevel.vger.kernel.org Joseph D. Wagner wrote: > Everything you need to know about the patch is documented in the patch > itself. Thanks. A couple of things about the patch: > + if ((arg == F_RDLCK) > + && ((atomic_read(&dentry->d_count) > 1) > + || (atomic_read(&inode->i_count) > 1))) { > [...] > + goto out_unlock; > if ((arg == F_WRLCK) > && ((atomic_read(&dentry->d_count) > 1) > || (atomic_read(&inode->i_count) > 1))) > goto out_unlock; These two are so similar they can be merged; just don't test "arg". The long comment which I wrote as "[...]" above is excessive: the code is clear without it. If every small patch had a comment that large, the kernel would be all comments and it paradoxically makes it harder to understand the code. (I used to write lots of comments in code myself, until I discovered colleages didn't read them because there were too many, so they skipped the really important technical notes. Now I follow the balance set by other programmers). A short FIXME comment indicating that the F_RDLCK case is not implemented properly is useful, though. > I hope you find it useful, and I hope that my poor first > impression will not prevent us from working together on filesystem > development in the future. No problem; welcome! I don't agree with this patch as it is because, as Sean Neakums pointed out, there is actually a field `inode->i_writecount' which is remarkeably appropriate. F_RDLCK should use `deny_write_access', and `get_write_access' should be changed to break any F_RDLCK leases. (Checking that locking for `break_lease' in the places where `get_write_access' is called is left as an exercise). F_WRLCK should use `get_write_accesss' because taking a write lease is equivalent to demanding write access - it should break any outstanding F_RDLCK leases. Alternatively, F_WRLCK should insist that `filp->f_mode & FMODE_WRITE' is set. -- Jamie