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

[Dave Chinner <david@fromorbit.com>]

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. ;)

> > dax: add IO completion callback for page faults
> > 
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > When a page fault drops into a hole, it needs to allocate an extent.
> > Filesystems may allocate unwritten extents so that the underlying
> > contents are not exposed until data is written to the extent. In
> > that case, we need an io completion callback to run once the blocks
> > have been zeroed to indicate that it is safe for the filesystem to
> > mark those blocks written without exposing stale data in the event
> > of a crash.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/dax.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 96c4fed..387ca78 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -306,6 +306,7 @@ static int do_dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
> >  	memset(&bh, 0, sizeof(bh));
> >  	block = (sector_t)vmf->pgoff << (PAGE_SHIFT - blkbits);
> >  	bh.b_size = PAGE_SIZE;
> > +	bh.b_end_io = NULL;
> 
> Given the above memset, I don't think we need to explicitly set b_end_io
> to NULL.

I missed that ;)

> >   repeat:
> >  	page = find_get_page(mapping, vmf->pgoff);
> > @@ -364,8 +365,12 @@ static int do_dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
> >  		return VM_FAULT_LOCKED;
> >  	}
> >  
> > -	if (buffer_unwritten(&bh) || buffer_new(&bh))
> > +	if (buffer_unwritten(&bh) || buffer_new(&bh)) {
> > +		/* XXX: errors zeroing the blocks are propagated how? */
> >  		dax_clear_blocks(inode, bh.b_blocknr, bh.b_size);
> 
> That's a great question.  I think we need to segfault here.

I suspect there are other cases where we need to do similar "trigger
segv" error handling rather than ignoring errors altogether...

> 
> > +		if (bh.b_end_io)
> > +			bh.b_end_io(&bh, 1);
> > +	}
> 
> I think ext4 is going to need to set b_end_io too.  Right now, it uses the
> dio_iodone_t to convert unwritten extents to written extents, but we don't
> have (and I don't think we should have) a kiocb for page faults.

Yes, ext4 is going to need this as well. After I got XFS running
without problems, I then went back and ran xfstests on ext4 and it
failed many of the tests that do operations into unwritten regions.

> So, if it's OK with you, I'm going to fold this patch into version 11 and
> add your Reviewed-by to it.

Fold it in, I'll review the result ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>