Re: [PATCH v10 09/21] Replace the XIP page fault handler with the DAX page fault handler

From: Dave Chinner <david@fromorbit.com>
To: Matthew Wilcox <willy@linux.intel.com>
Cc: Matthew Wilcox <matthew.r.wilcox@intel.com>,
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v10 09/21] Replace the XIP page fault handler with the DAX page fault handler
Date: Thu, 25 Sep 2014 11:01:26 +1000	[thread overview]
Message-ID: <20140925010126.GC4945@dastard> (raw)
In-Reply-To: <20140924154307.GO27730@localhost.localdomain>

On Wed, Sep 24, 2014 at 11:43:07AM -0400, Matthew Wilcox wrote:
> On Thu, Sep 11, 2014 at 01:09:26PM +1000, Dave Chinner wrote:
> > On Wed, Sep 10, 2014 at 11:23:37AM -0400, Matthew Wilcox wrote:
> > > On Wed, Sep 03, 2014 at 05:47:24PM +1000, Dave Chinner wrote:
> > > > > +	error = get_block(inode, block, &bh, 0);
> > > > > +	if (!error && (bh.b_size < PAGE_SIZE))
> > > > > +		error = -EIO;
> > > > > +	if (error)
> > > > > +		goto unlock_page;
> > > > 
> > > > page fault into unwritten region, returns buffer_unwritten(bh) ==
> > > > true. Hence buffer_written(bh) is false, and we take this branch:
> > > > 
> > > > > +	if (!buffer_written(&bh) && !vmf->cow_page) {
> > > > > +		if (vmf->flags & FAULT_FLAG_WRITE) {
> > > > > +			error = get_block(inode, block, &bh, 1);
> > > > 
> > > > Exactly what are you expecting to happen here? We don't do
> > > > allocation because there are already unwritten blocks over this
> > > > extent, and so bh will be unchanged when returning. i.e. it will
> > > > still be mapping an unwritten extent.
> > > 
> > > I was expecting calling get_block() on an unwritten extent to convert it
> > > to a written extent.  Your suggestion below of using b_end_io() to do that
> > > is a better idea.
> > > 
> > > So this should be:
> > > 
> > > 	if (!buffer_mapped(&bh) && !vmf->cow_page) {
> > > 
> > > ... right?
> > 
> > Yes, that is the conclusion I reached as well. ;)
> 
> Now I know why I was expecting get_block() on an unwritten extent to
> convert it to a written extent.  That's the way ext4 behaves!

That seems wrong. Unwritten extent conversion should only occur
on IO completion...

> 
> [  236.660772] got bh ffffffffa06e3bd0 1000
> [  236.660814] got bh for write ffffffffa06e3bd0 60
> [  236.660821] calling end_io ffffffffa06e3bd0 60
> 
> (1000 is BH_Unwritten, 60 is BH_Mapped | BH_New)
> 
> The code producing this output:
> 
>         error = get_block(inode, block, &bh, 0);
> printk("got bh %p %lx\n", bh.b_end_io, bh.b_state);
>         if (!error && (bh.b_size < PAGE_SIZE))
>                 error = -EIO;
>         if (error)
>                 goto unlock_page;
> 
>         if (!buffer_mapped(&bh) && !vmf->cow_page) {
>                 if (vmf->flags & FAULT_FLAG_WRITE) {
>                         error = get_block(inode, block, &bh, 1);
> printk("got bh for write %p %lx\n", bh.b_end_io, bh.b_state);

%pF will do symbol decoding for you ;)

> 
> # xfs_io -f -c "truncate 20k" -c "fiemap -v" -c "falloc 0 20k" -c "fiemap -v" -c "mmap -w 0 20k" -c "fiemap -v" -c "mwrite 4k 4k" -c "fiemap -v" /mnt/ram0/b
> /mnt/ram0/b:
> /mnt/ram0/b:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..39]:         263176..263215      40 0x801
> /mnt/ram0/b:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..39]:         263176..263215      40 0x801
> /mnt/ram0/b:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..39]:         263176..263215      40   0x1
> 
> Actually, this looks wrong ... ext4 should only have converted one block
> of the extent to written, not all of it.  I think that means ext4 is
> exposing stale data :-(  I'll keep digging.

Check to see if ext4 has zeroed the entire extent - it does some
convoluted "hole filling" in certain siutations where it extends the
range of allocation operations by writing zeros around the range that
it was asked to allocate.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com