Re: [PATCH v10 09/21] Replace the XIP page fault handler with the DAX page fault handler
From: Matthew Wilcox <hidden>
Date: 2014-09-10 15:23:37
Also in:
linux-fsdevel, lkml
On Wed, Sep 03, 2014 at 05:47:24PM +1000, Dave Chinner wrote:
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?
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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, 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>