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

[Matthew Wilcox]

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?

> dax: add IO completion callback for page faults
> 
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  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.

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

> +		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.

So, if it's OK with you, I'm going to fold this patch into version 11 and
add your Reviewed-by to it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/