xfs
[Top] [All Lists]

Re: [PATCH 1/2] xfs: xfs_check_page_type buffer checks need help

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 1/2] xfs: xfs_check_page_type buffer checks need help
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 6 Mar 2014 10:18:07 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140305220819.GC55736@xxxxxxxxxxxxxxx>
References: <1393981893-2497-1-git-send-email-david@xxxxxxxxxxxxx> <1393981893-2497-2-git-send-email-david@xxxxxxxxxxxxx> <20140305220819.GC55736@xxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Mar 05, 2014 at 05:08:20PM -0500, Brian Foster wrote:
> On Wed, Mar 05, 2014 at 12:11:32PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > xfs_aops_discard_page() was introduced in the following commit:
> > 
> >   xfs: truncate delalloc extents when IO fails in writeback
> > 
> > ... to clean up left over delalloc ranges after I/O failure in
> > ->writepage(). generic/224 tests for this scenario and occasionally
> > reproduces panics on sub-4k blocksize filesystems.
> > 
> > The cause of this is failure to clean up the delalloc range on a
> > page where the first buffer does not match one of the expected
> > states of xfs_check_page_type(). If a buffer is not unwritten,
> > delayed or dirty&mapped, xfs_check_page_type() stops and
> > immediately returns 0.
....
> > @@ -777,6 +795,7 @@ xfs_convert_page(
> >                     count++;
> >             } else {
> >                     done = 1;
> > +                   break;
> >             }
> >     } while (offset += len, (bh = bh->b_this_page) != head);
> > 
> 
> The next couple lines after the loop are:
> 
>       if (uptodate && bh == head)
>               SetPageUptodate(page);
> 
> Now that we can break out of the loop, the "bh == head" part of that
> check might not necessarily mean what it used to mean. The uptodate
> variable is initialized to 1 and we reset to 0 the moment we encounter a
> !uptodate buffer. Do you think it's possible to get here on the first
> buffer of the page, without having reset 'uptodate,'  and potentially
> incorrectly set the page uptodate?

Good question :)

I don't think this can happen because if the first buffer on the
page can't be written, xfs_check_page_type() will return false and
we won't get to the loop. By definition, buffer_unwritten() implies
buffer_uptodate(), as does buffer_delay() and buffer_dirty(). Hence
any of the types that will return acceptible will have the first
buffer uptodate.

As for the other breaks in the loop - the initial imap_valid check
ensures we have a map that covers the entire region of the page that
needs writing, and we know that offset < end_offset for the first
buffer on the page. Hence none of the loop breaks will trigger on
the first buffer, and so the above code should not trigger.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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