From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay.sgi.com (relay2.corp.sgi.com [137.38.102.29]) by oss.sgi.com (Postfix) with ESMTP id 938907F3F for ; Fri, 28 Feb 2014 15:21:16 -0600 (CST) Message-ID: <5310FDCA.70909@sgi.com> Date: Fri, 28 Feb 2014 15:21:14 -0600 From: Mark Tinguely MIME-Version: 1.0 Subject: Re: [PATCH] xfs: check all buffers in xfs_check_page_type() References: <1393615369-41882-1-git-send-email-bfoster@redhat.com> <5310EB67.5050404@sgi.com> <20140228203648.GC15562@laptop.bfoster> In-Reply-To: <20140228203648.GC15562@laptop.bfoster> List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: xfs-bounces@oss.sgi.com Sender: xfs-bounces@oss.sgi.com To: Brian Foster Cc: xfs@oss.sgi.com On 02/28/14 14:36, Brian Foster wrote: > On Fri, Feb 28, 2014 at 02:02:47PM -0600, Mark Tinguely wrote: >> On 02/28/14 13:22, Brian Foster wrote: >>> 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. >>> >>> The stress test of generic/224 creates a scenario where the first >>> several buffers of a page with delayed buffers are mapped&uptodate >>> and some subsequent buffer is delayed. If the ->writepage() happens >>> to fail for this page, xfs_aops_discard_page() incorrectly skips >>> the entire page. >>> >>> Modify xfs_aops_discard_page() to iterate all of the page buffers >>> to ensure a delayed buffer does not go undetected. >>> >>> Signed-off-by: Brian Foster >>> --- >>> >>> The only other caller to xfs_check_page_type() is xfs_convert_page(). I >>> think this is safe with respect to that codepath, given the additional >>> imap checks therein and whatnot, but thoughts appreciated. >>> >>> Brian >>> >>> fs/xfs/xfs_aops.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c >>> index db2cfb0..5962a9f 100644 >>> --- a/fs/xfs/xfs_aops.c >>> +++ b/fs/xfs/xfs_aops.c >>> @@ -655,8 +655,6 @@ xfs_check_page_type( >>> acceptable += (type == XFS_IO_DELALLOC); >>> else if (buffer_dirty(bh)&& buffer_mapped(bh)) >>> acceptable += (type == XFS_IO_OVERWRITE); >>> - else >>> - break; >>> } while ((bh = bh->b_this_page) != head); >>> >>> if (acceptable) >> >> Is there any reason to scan all the buffers when we all we want is >> an indication that at least one is acceptable? Maybe there are >> generally not may buffers to a page to make it worthwhile. >> > > Hi Mark, > > Good point. We could pull the 'if (acceptable)' check up into the loop > and exit as soon as we find something writeable. > > Alternatively, we could do something like the following and get rid of > 'acceptable' entirely (or continue to nest the type checks if there's a > performance concern): > > ... > if (buffer_unwritten(bh) && type == XFS_IO_UNWRITTEN) > return 1; > else if (buffer_delay(bh) && type == XFS_IO_DELALLOC) > return 1; > else if (buffer_dirty(bh) && buffer_mapped(bh)&& > type == XFS_IO_OVERWRITE) > return 1; > ... > > Brian Good catch BTW. Yes that is what I was thinking. --Mark. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs