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:
> > > + 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?
Yes, that is the conclusion I reached as well. ;)
> > 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.
I missed that ;)
> > 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...
>
> > + 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@xxxxxxxxxxxxx
--
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
