xfs
[Top] [All Lists]

Re: [PATCH 2/2] xfs: ensure we mark all inodes in a freed cluster XFS_IS

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs: ensure we mark all inodes in a freed cluster XFS_ISTALE
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 30 Jul 2010 20:54:04 +1000
Cc: xfs@xxxxxxxxxxx, npiggin@xxxxxxxxx
In-reply-to: <20100730102746.GA10367@xxxxxxxxxxxxx>
References: <1280444146-14540-1-git-send-email-david@xxxxxxxxxxxxx> <1280444146-14540-3-git-send-email-david@xxxxxxxxxxxxx> <20100730102746.GA10367@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Fri, Jul 30, 2010 at 06:27:46AM -0400, Christoph Hellwig wrote:
> On Fri, Jul 30, 2010 at 08:55:46AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > Under heavy load parallel metadata loads (e.g. dbench), we can fail
> > to mark all the inodes in a cluster being freed as XFS_ISTALE as we
> > skip inodes we cannot get the XFS_ILOCK_EXCL or the flush lock on.
> > When this happens and the inode cluster buffer has already been
> > marked stale and freed, inode reclaim can try to write the inode out
> > as it is dirty and not marked stale. This can result in writing th
> > metadata to an freed extent, or in the case it has already
> > been overwritten trigger a magic number check failure and return an
> > EUCLEAN error such as:
> > 
> > Filesystem "ram0": inode 0x442ba1 background reclaim flush failed with 117
> > 
> > Fix this by ensuring that we hoover up all in memory inodes in the
> > cluster and mark them XFS_ISTALE when freeing the cluster.
> 
> Why do you move the loop over the log items around?  From all that
> I can see the original place is much better as we just have to loop
> over the items once.  Then once we look up the inodes in memory
> we skip over the inodes that already are stale, so the behaviour
> should be the same. 

You are right - it is doing the same as the old code where it is
marking them stale first. I rearranged some code when trying
a couple of crazy ideas, but forgot to move it back when I
had somethign that fixed the bug. I'll move it back - that shoul
dmake the diff lots smaller.

> Also instead of the i-- and continue for the
> lock failure an explicit goto retry would make it a lot more obvious.

Good point. I fix it up and test it again.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>