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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
