From: Guo Chao <yan@linux.vnet.ibm.com>
To: Dave Chinner <david@fromorbit.com>
Cc: viro@zeniv.linux.org.uk, dchinner@redhat.com, hch@infradead.org,
jack@suse.cz, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [RFC v4 Patch 0/4] fs/inode.c: optimization for inode lock usage
Date: Mon, 24 Sep 2012 10:42:21 +0800 [thread overview]
Message-ID: <20120924024221.GB7984@yanx> (raw)
In-Reply-To: <20120921224912.GA20960@dastard>
On Sat, Sep 22, 2012 at 08:49:12AM +1000, Dave Chinner wrote:
> On Fri, Sep 21, 2012 at 05:31:02PM +0800, Guo Chao wrote:
> > This patchset optimizes several places which take the per inode spin lock.
> > They have not been fully tested yet, thus they are marked as RFC.
>
> Inodes are RCU freed. The i_lock spinlock on the i_state field forms
> part of the memory barrier that allows the RCU read side to
> correctly detect a freed inode during a RCU protected cache lookup
> (hash list traversal for the VFS, or a radix tree traversal for XFS).
> The i_lock usage around the hahs list operations ensures the hash
> list operations are atomic with state changes so that such changes
> are correctly detected during RCU-protected traversals...
>
> IOWs, removing the i_lock from around the i_state transitions and
> inode hash insert/remove/traversal operations will cause races in
> the RCU lookups and result in incorrectly using freed inodes instead
> of failing the lookup and creating a new one.
>
> So I don't think this is a good idea at all...
>
Hello, Dave:
Thanks for your explanation.
Though I can't fully understand it, your concern seems to be that
RCU inode lookup will be bothered by this change. But we do not have
RCU inode lookup in VFS: inode lookup is done by rather a tranditional
way.
XFS gives me the impression that it implements its own inode cache.
There may be such thing there. I have little knowledge on XFS, but I
guess it's unlikely impacted by the change of code implementing VFS
inode cache.
As far as I can see, RCU inode free is for RCU dentry lookup, which
seems have nothing to do with 'detect a freed inode'. Taking i_lock in these
places looks like to me a result of following old lock scheme blindly when
breaking the big global inode lock. Of course, maybe they are there for
something. Could you speak more about the race this change (patch 1,2?) brings
up? Thank you.
Regards,
Guo Chao
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
next prev parent reply other threads:[~2012-09-24 2:42 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-21 9:31 [RFC v4 Patch 0/4] fs/inode.c: optimization for inode lock usage Guo Chao
2012-09-21 9:31 ` [PATCH 1/4] fs/inode.c: do not take i_lock on newly allocated inode Guo Chao
2012-09-21 9:31 ` [PATCH 2/4] fs/inode.c: do not take i_lock in __(insert|remove)_inode_hash Guo Chao
2012-09-21 9:31 ` [PATCH 3/4] fs/inode.c: do not take i_lock when identify an inode Guo Chao
2012-09-21 9:31 ` [PATCH 4/4] fs/inode.c: always take i_lock before calling filesystem's test() method Guo Chao
2012-09-21 12:17 ` [RFC v4 Patch 0/4] fs/inode.c: optimization for inode lock usage Matthew Wilcox
2012-09-21 22:49 ` Dave Chinner
2012-09-24 2:42 ` Guo Chao [this message]
2012-09-24 4:23 ` Dave Chinner
2012-09-24 6:12 ` Guo Chao
2012-09-24 6:28 ` Dave Chinner
2012-09-24 7:08 ` Guo Chao
2012-09-24 8:26 ` Dave Chinner
2012-09-25 8:59 ` Guo Chao
2012-09-26 0:54 ` Dave Chinner
2012-09-27 8:41 ` Guo Chao
2012-09-27 11:51 ` Dave Chinner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120924024221.GB7984@yanx \
--to=yan@linux.vnet.ibm.com \
--cc=david@fromorbit.com \
--cc=dchinner@redhat.com \
--cc=hch@infradead.org \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).