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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140910152337.GF27730@localhost.localdomain>
Date:	Wed, 10 Sep 2014 11:23:37 -0400
From:	Matthew Wilcox <willy@...ux.intel.com>
To:	Dave Chinner <david@...morbit.com>
Cc:	Matthew Wilcox <matthew.r.wilcox@...el.com>,
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, willy@...ux.intel.com
Subject: Re: [PATCH v10 09/21] Replace the XIP page fault handler with the
 DAX page fault handler

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

>   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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/