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:

quoted

quoted

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

quoted

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

quoted

dax: add IO completion callback for page faults

From: Dave Chinner <redacted>

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

quoted

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

quoted

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