RE: Re: [PATCH v2] xfs: Don't flush stale inodes

Alex Elder
2010-01-09T16:32:07Z

Dave Chinner wrote:
> On Fri, Jan 08, 2010 at 05:14:25PM -0600, Alex Elder wrote:
>> Dave Chinner wrote:
>>> Because inodes remain in cache much longer than inode buffers do
>>> under memory pressure, we can get the situation where we have stale,
>>> dirty inodes being reclaimed but the backing storage has been freed.
>>> Hence we should never, ever flush XFS_ISTALE inodes to disk as
>>> there is no guarantee that the backing buffer is in cache and
>>> still marked stale when the flush occurs.
>>>
>>> Signed-off-by: Dave Chinner <david@fromorbit.com>
>>
>> Dave, I'm putting this patch in before your perag radix tree
>> patch series, so I had to port it. I am submitting here on
>> your behalf, but would like you to review it if possible.
>> It builds fine, and I've run it through a quick test.
>> I will retain all of your original commentary, etc. I did
>> *not* implement Christoph's recommendation that you change
>> the "for" loop in xfs_alloc_search_busy() to a more typical form.
>>
>> For reference, the original patch is here:
>> http://patchwork.xfs.org/patch/382/
>>
>> Signed-off-by: Alex Elder <aelder@sgi.com>
>
> Ah, which patch are you talking about? The title says stale inodes,
> the patch is searching busy extents.

Oops. I blame my Left-handed mouse clicking skills, which are
improving but are still unreliable. I'm working mostly without
my Right hand this week because of an injury...

> Assuming you meant the busy extent search fix, you've ported the
> wrong version of the busy extent search fix. I posted an updated
> version after Christoph reviewed that one (in my response to
> Christoph's review).

Yes, I see that now. I had marked your older one "Reviewed" and
didn't look further. I will re-do the port and will re-submit.
BTW I also plan to port the perag radix tree patch(es) over this
for you after I get this one published.

> Because it was a new patch, patchwork doesn't add the email to the
> original thread - it starts a new one. It's a real PITA, IMO, but
> that is the way patchwork does stuff. The patch that should be
> checked in is this one:
>
> http://patchwork.xfs.org/patch/387/

Yes, I agree. I find it helpful when updates are flagged with
something like "[PATCH v2]" because it somehow catches my eye
better *and* makes it explicit that the message contains a patch
that obsoletes a previous one.

-Alex