On Thu 21-08-14 15:09:09, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> generic/263 is failing fsx at this point with a page spanning
> EOF that cannot be invalidated. The operations are:
>
> 1190 mapwrite 0x52c00 thru 0x5e569 (0xb96a bytes)
> 1191 mapread 0x5c000 thru 0x5d636 (0x1637 bytes)
> 1192 write 0x5b600 thru 0x771ff (0x1bc00 bytes)
>
> where 1190 extents EOF from 0x54000 to 0x5e569. When the direct IO
> write attempts to invalidate the cached page over this range, it
> fails with -EBUSY and so we fire this assert:
>
> XFS: Assertion failed: ret < 0 || ret == count, file: fs/xfs/xfs_file.c, line: 676
>
> because the kernel is trying to fall back to buffered IO on the
> direct IO path (which XFS does not do).
>
> The real question is this: Why can't that page be invalidated after
> it has been written to disk an cleaned?
>
> Well, there's data on the first two buffers in the page (1k block
> size, 4k page), but the third buffer on the page (i.e. beyond EOF)
> is failing drop_buffers because it's bh->b_state == 0x3, which is
> BH_Uptodate | BH_Dirty. IOWs, there's dirty buffers beyond EOF. Say
> what?
>
> OK, set_buffer_dirty() is called on all buffers from
> __set_page_buffers_dirty(), regardless of whether the buffer is
> beyond EOF or not, which means that when we get to ->writepage,
> we have buffers marked dirty beyond EOF that we need to clean.
> So, we need to implement our own .set_page_dirty method that
> doesn't dirty buffers beyond EOF.
>
> This is messy because the buffer code is not meant to be shared
> and it has interesting locking issues on the buffer dirty bits.
> So just copy and paste it and then modify it to suit what we need.
Well, I'm not sure this is the cleanest way to fix your problem. The
thing is that inode size can change (decrease) after set_page_dirty() has
run and writeback can find the page before truncate_inode_pages() calls
do_invalidatepage() on the last partial page. Now I agree that given
truncate and direct IO are both synchronized using IOLOCK, this change
still fixes your problem. I just wanted to point out that your change
doesn't really make sure won't see dirty buffers in a tail page beyond
i_size.
As Anton has pointed out other filesystems solve the same issue by clearing
the dirty bits beyond EOF in their writepage() function. Also since we
zero-out the tail of the page in writepage() (even in XFS as I checked),
this kind of makes sense to me and has smaller overhead than special
set_page_dirty()...
Honza
> cc: <stable@xxxxxxxxxx>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_aops.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 11e9b4c..2a316ad 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -1753,11 +1753,69 @@ xfs_vm_readpages(
> return mpage_readpages(mapping, pages, nr_pages, xfs_get_blocks);
> }
>
> +/*
> + * This is basically a copy of __set_page_dirty_buffers() with one
> + * small tweak: buffers beyond EOF do not get marked dirty. If we mark them
> + * dirty, we'll never be able to clean them because we don't write buffers
> + * beyond EOF, and that means we can't invalidate pages that span EOF
> + * that have been marked dirty. Further, the dirty state can leak into
> + * the file interior if the file is extended, resulting in all sorts of
> + * bad things happening as the state does not match the unerlying data.
> + */
> +STATIC int
> +xfs_vm_set_page_dirty(
> + struct page *page)
> +{
> + struct address_space *mapping = page_mapping(page);
> + struct inode *inode = mapping->host;
> + loff_t end_offset;
> + loff_t offset;
> + int newly_dirty;
> +
> + if (unlikely(!mapping))
> + return !TestSetPageDirty(page);
> +
> + end_offset = i_size_read(inode);
> + offset = end_offset & PAGE_CACHE_MASK;
> +
> + spin_lock(&mapping->private_lock);
> + if (page_has_buffers(page)) {
> + struct buffer_head *head = page_buffers(page);
> + struct buffer_head *bh = head;
> +
> + do {
> + if (offset < end_offset)
> + set_buffer_dirty(bh);
> + bh = bh->b_this_page;
> + offset += 1 << inode->i_blkbits;
> + } while (bh != head);
> + }
> + newly_dirty = !TestSetPageDirty(page);
> + spin_unlock(&mapping->private_lock);
> +
> + if (newly_dirty) {
> + /* sigh - __set_page_dirty() is static, so copy it here, too */
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mapping->tree_lock, flags);
> + if (page->mapping) { /* Race with truncate? */
> + WARN_ON_ONCE(!PageUptodate(page));
> + account_page_dirtied(page, mapping);
> + radix_tree_tag_set(&mapping->page_tree,
> + page_index(page), PAGECACHE_TAG_DIRTY);
> + }
> + spin_unlock_irqrestore(&mapping->tree_lock, flags);
> + __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
> + }
> + return newly_dirty;
> +}
> +
> const struct address_space_operations xfs_address_space_operations = {
> .readpage = xfs_vm_readpage,
> .readpages = xfs_vm_readpages,
> .writepage = xfs_vm_writepage,
> .writepages = xfs_vm_writepages,
> + .set_page_dirty = xfs_vm_set_page_dirty,
> .releasepage = xfs_vm_releasepage,
> .invalidatepage = xfs_vm_invalidatepage,
> .write_begin = xfs_vm_write_begin,
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
