From: Dave Chinner <david@fromorbit.com>
To: Matthew Wilcox <matthew.r.wilcox@intel.com>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-mm@kvack.org, linux-ext4@vger.kernel.org
Subject: Re: [PATCH v5 00/22] Rewrite XIP code and add XIP support to ext4
Date: Fri, 31 Jan 2014 14:06:52 +1100 [thread overview]
Message-ID: <20140131030652.GK13997@dastard> (raw)
In-Reply-To: <20140130092537.GH13997@dastard>
On Thu, Jan 30, 2014 at 08:25:37PM +1100, Dave Chinner wrote:
> On Thu, Jan 30, 2014 at 05:42:30PM +1100, Dave Chinner wrote:
> > On Wed, Jan 15, 2014 at 08:24:18PM -0500, Matthew Wilcox wrote:
> > > This series of patches add support for XIP to ext4. Unfortunately,
> > > it turns out to be necessary to rewrite the existing XIP support code
> > > first due to races that are unfixable in the current design.
> > >
> > > Since v4 of this patchset, I've improved the documentation, fixed a
> > > couple of warnings that a newer version of gcc emitted, and fixed a
> > > bug where we would read/write the wrong address for I/Os that were not
> > > aligned to PAGE_SIZE.
> >
> > Looks like there's something fundamentally broken with the patch set
> > as it stands. I get this same data corruption on both ext4 and XFS
> > with XIP using fsx. It's as basic as it gets - the first read after
> > a mmapped write fails to see the data written by mmap:
> >
> > $ sudo mkfs.xfs -f /dev/ram0
> > meta-data=/dev/ram0 isize=256 agcount=4, agsize=256000 blks
> > = sectsz=512 attr=2, projid32bit=1
> > = crc=0
> > data = bsize=4096 blocks=1024000, imaxpct=25
> > = sunit=0 swidth=0 blks
> > naming =version 2 bsize=4096 ascii-ci=0 ftype=0
> > log =internal log bsize=4096 blocks=12800, version=2
> > = sectsz=512 sunit=0 blks, lazy-count=1
> > realtime =none extsz=4096 blocks=0, rtextents=0
> > $ sudo mount -o xip /dev/ram0 /mnt/scr
> > $ sudo chmod 777 /mnt/scr
> > $ ltp/fsx -d -N 1000 -S 0 /mnt/scr/fsx
> ....
> > operation# (mod 256) for the bad data unknown, check HOLE and EXTEND ops
> > LOG DUMP (9 total operations):
> > 1( 1 mod 256): MAPWRITE 0x3db39 thru 0x3ffff (0x24c7 bytes)
> > 2( 2 mod 256): MAPREAD 0x2e947 thru 0x33163 (0x481d bytes)
> > 3( 3 mod 256): READ 0x2e836 thru 0x3cba1 (0xe36c bytes)
> > 4( 4 mod 256): PUNCH 0x2e7 thru 0x5c42 (0x595c bytes)
> > 5( 5 mod 256): MAPWRITE 0xcaea thru 0x13ba9 (0x70c0 bytes) ******WWWW
> > 6( 6 mod 256): PUNCH 0x31645 thru 0x38d1c (0x76d8 bytes)
> > 7( 7 mod 256): FALLOC 0x24f92 thru 0x2f2b7 (0xa325 bytes) INTERIOR
> > 8( 8 mod 256): FALLOC 0xbcf1 thru 0x171ac (0xb4bb bytes) INTERIOR ******FFFF
> > 9( 9 mod 256): READ 0x126f thru 0x11136 (0xfec8 bytes) ***RRRR***
> > Correct content saved for comparison
> > (maybe hexdump "/mnt/scr/fsx" vs "/mnt/scr/fsx.fsxgood")
> >
> > XFS gives a good indication that we aren't doing something correctly
> > w.r.t. mapped XIP writes, as trying to fiemap the file ASSERT fails
> > with a delayed allocation extent somewhere inside the file after a
> > sync. I shall keep digging.
>
> Ok, I understand the XFS ASSERT failure, but I don't really
> understand the reason for the read failure. XFS assert failed
> because I was using the delayed allocation enabled xfs_get_blocks()
> to xip_fault/xip_mkwrite, so it was creating a delalloc extent
> rather than allocating blocks, and then not having any pages in the
> page cache to write back to convert the delalloc extent. This
> doesn't explain the zeros being read, though.
>
> So I changed to use the direct IO version, and that leaves me with
> an unwritten extent over the mapped write code. Why? Because there's
> no IO completion being run from either xip_fault() or xip_mkwrite()
> to zero the buffers and run IO completion to convert the extent to
> written....
>
> $ xfs_io -f -c "truncate 8k" -c "mmap 0 8k" -c "mwrite 0 4k" \
> > -c "bmap -vp" -c "pread -v 0 8k" -c "bmap -vp" /mnt/scr/foo
> ....
> /mnt/scr/foo:
> EXT: FILE-OFFSET BLOCK-RANGE AG AG-OFFSET TOTAL FLAGS
> 0: [0..7]: 224..231 0 (224..231) 8 10000
> 1: [8..15]: hole 8
> $
>
> We're trying to do something that the get_block callback has never
> supported. I note that you added zeroing to ext4_map_blocks() when
> an unwritten extent is found and call xip_clear_blocks() from there
> to try and handle this within the allocation context without
> actually making it obvious why it is necessary.
>
> Essentially what we need get_blocks(create = 1) to do here is this:
>
> if (hole)
> transactionally allocate and zero block in requested region
> if (unwritten)
> transactionally convert to written and zero block
> if (written)
> map blocks
>
> I think we can get away with this from a crash recovery perspective
> because the zeroing of the blocks is synchronous and within the
> allocation transaction. I'm implementing a new xfs_get_blocks_xip to
> do keep this new behaviour "separate" from the direct IO path
> semantics.
>
> I also got rid of the read block map followed by the "create" block
> map. Just a single call with create set appropriately for the caller
> context is all that is required - the getblock call will do the
> correct thing for allocation/conversion cases and if there's already
> a block there it will just return the mapping....
>
> <hack, hack>
>
> OK, I've fixed something. The above xfs_io test returns the correct
> data on read now, fsx still fails. I'll keep working on it in the
> morning, and when I have something that works I'll post it....
The read/write path is broken, Willy. We can't map arbitrary byte
ranges to the DIO subsystem. I'm now certain that the data
corruptions I'm seeing are in sub-sector regions from unaligned IOs
from userspace. We still need to use the buffered IO path for non
O_DIRECT IO to avoid these problems. I think I've worked out a way
to short-circuit page cache lookups for the buffered IO path, so
stay tuned....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2014-01-31 3:07 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-16 1:24 [PATCH v5 00/22] Rewrite XIP code and add XIP support to ext4 Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 01/22] Fix XIP fault vs truncate race Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 02/22] Allow page fault handlers to perform the COW Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 03/22] axonram: Fix bug in direct_access Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 04/22] Change direct_access calling convention Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 05/22] Introduce IS_XIP(inode) Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 06/22] Treat XIP like O_DIRECT Matthew Wilcox
2014-01-31 16:59 ` Jan Kara
2014-01-16 1:24 ` [PATCH v5 07/22] Rewrite XIP page fault handling Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 08/22] Change xip_truncate_page to take a get_block parameter Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 09/22] Remove mm/filemap_xip.c Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 10/22] Remove get_xip_mem Matthew Wilcox
2014-01-16 1:46 ` Randy Dunlap
2014-01-27 13:26 ` Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 11/22] Replace ext2_clear_xip_target with xip_clear_blocks Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 12/22] ext2: Remove ext2_xip_verify_sb() Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 13/22] ext2: Remove ext2_use_xip Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 14/22] ext2: Remove xip.c and xip.h Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 15/22] Remove CONFIG_EXT2_FS_XIP Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 16/22] ext2: Remove ext2_aops_xip Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 17/22] xip: Add xip_zero_page_range Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 18/22] ext4: Make ext4_block_zero_page_range static Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 19/22] ext4: Add XIP functionality Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 20/22] ext4: Fix typos Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 21/22] xip: Add reporting of major faults Matthew Wilcox
2014-01-16 1:24 ` [PATCH v5 22/22] XIP: Add support for unwritten extents Matthew Wilcox
[not found] ` <CEFDA737.22F87%matthew.r.wilcox@intel.com>
2014-01-17 0:00 ` [PATCH v5 19/22] ext4: Add XIP functionality Ross Zwisler
[not found] ` <CEFD7DAD.22F65%matthew.r.wilcox@intel.com>
2014-01-22 22:51 ` [PATCH v5 22/22] XIP: Add support for unwritten extents Ross Zwisler
2014-01-23 12:08 ` Matthew Wilcox
2014-01-23 19:13 ` Ross Zwisler
[not found] ` <CF0C370C.235F1%willy@linux.intel.com>
2014-01-27 23:32 ` Ross Zwisler
2014-01-28 3:49 ` Matthew Wilcox
2014-01-23 7:48 ` [PATCH v5 00/22] Rewrite XIP code and add XIP support to ext4 Dave Chinner
2014-01-23 7:53 ` Dave Chinner
2014-01-23 9:01 ` Dave Chinner
2014-01-23 12:12 ` Wilcox, Matthew R
2014-01-28 6:06 ` Dave Chinner
2014-01-30 6:42 ` Dave Chinner
2014-01-30 9:25 ` Dave Chinner
2014-01-31 3:06 ` Dave Chinner [this message]
2014-01-31 5:45 ` Ross Zwisler
2014-01-31 13:04 ` Dave Chinner
[not found] ` <CF1FF3EB.24114%matthew.r.wilcox@intel.com>
2014-02-11 23:12 ` [PATCH v5 19/22] ext4: Add XIP functionality Ross Zwisler
2014-02-13 0:00 ` Ross Zwisler
[not found] ` <CF215477.24281%matthew.r.wilcox@intel.com>
2014-02-12 23:53 ` [PATCH v5 06/22] Treat XIP like O_DIRECT Ross Zwisler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140131030652.GK13997@dastard \
--to=david@fromorbit.com \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=matthew.r.wilcox@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).