On Tue, Sep 10, 2013 at 06:11:22PM -0500, Ben Myers wrote:
> Hi Dave,
>
> On Wed, Aug 28, 2013 at 08:08:33AM +1000, Dave Chinner wrote:
> > On Tue, Aug 27, 2013 at 04:25:58PM -0500, Ben Myers wrote:
> > > On Tue, Aug 27, 2013 at 07:24:23AM +1000, Dave Chinner wrote:
> > > > On Mon, Aug 26, 2013 at 03:47:30PM -0500, Ben Myers wrote:
> > > > > Looks like in 48fde701 we removed the iput of the root inode in
> > > > > xfs_fs_fill_super for the error case. Add it back.
> > > > >
> > > > > Signed-off-by: Ben Myers <bpm@xxxxxxx>
> > > > >
> > > > > ---
> > > > > fs/xfs/xfs_super.c | 6 ++++--
> > > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > Index: b/fs/xfs/xfs_super.c
> > > > > ===================================================================
> > > > > --- a/fs/xfs/xfs_super.c 2013-08-26 15:36:09.170848579 -0500
> > > > > +++ b/fs/xfs/xfs_super.c 2013-08-26 15:40:19.450817933 -0500
> > > > > @@ -1493,12 +1493,12 @@ xfs_fs_fill_super(
> > > > > }
> > > > > if (is_bad_inode(root)) {
> > > > > error = EINVAL;
> > > > > - goto out_unmount;
> > > > > + goto out_iput;
> > > > > }
> > > > > sb->s_root = d_make_root(root);
> > > > > if (!sb->s_root) {
> > > > > error = ENOMEM;
> > > > > - goto out_unmount;
> > > > > + goto out_iput;
> > > > > }
> > > >
> > > > That's wrong. d_make_root() drops the reference to the inode on
> > > > failure itself, and so the change in 48fde701 is correct and valid.
> > > >
> > > > The leak on bad inodes (which, AFAICT, can never happen on XFS) has
> > > > been around a lot longer than Al's change - this commit introduced
> > > > it:
> > > >
> > > > 2bcf6e9 xfs: start periodic workers later
> > > >
> > > > with this hunk:
> > > >
> > > > if (is_bad_inode(root)) {
> > > > error = EINVAL;
> > > > - goto fail_vnrele;
> > > > + goto out_syncd_stop;
> > > > }
> > >
> > > Thanks Gents. Here's another try:
> > >
> > > xfs: don't leak root inode reference
> > >
> > > Looks like in 2bcf6e9 we removed the iput of the root inode in
> > > xfs_fs_fill_super for the is_bad_inode error case. Add it back.
> > >
> > > Signed-off-by: Ben Myers <bpm@xxxxxxx>
> >
> > I don't think this is right, either.
> >
> > As I said in my previous reply, I don't think that XFS can ever see
> > a bad inode. The fact is that we're grabbing mp->m_rootip, which is
> > we already have a reference to and is in cache and validated thanks
> > to an xfs_iget() call in xfs_mountfs(). If we fail validation when
> > reading the root inode into cache then xfs_mountfs() will fail and
> > we won't ever get to this check.
> >
> > Further, XFS never marks inodes bad - even on a failed lookup or a
> > shut down filesystem - and so AFAICT we cannot ever see the root
> > inode (or any other XFS inode) as a bad inode.
> >
> > Hence I think that the is_bad_inode(root) check should just go away.
>
> Ok. Lets try this. ;)
>
> xfs: remove usage of is_bad_inode
>
> XFS never calls mark_inode_bad or iget_failed, so it will never see a
> bad inode. Remove all checks for is_bad_inode because they are
> unnecessary.
>
> Signed-off-by: Ben Myers <bpm@xxxxxxx>
Applied this one.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
