Re: [PATCH 1/6] dax: don't abuse get_block mapping for endio callbacks

[Dave Chinner <david@fromorbit.com>]

On Wed, Mar 04, 2015 at 04:54:08PM +0100, Jan Kara wrote:
> On Wed 04-03-15 10:30:22, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > @@ -269,7 +269,8 @@ static int copy_user_bh(struct page *to, struct buffer_head *bh,
> >  }
> >  
> >  static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
> > -			struct vm_area_struct *vma, struct vm_fault *vmf)
> > +			struct vm_area_struct *vma, struct vm_fault *vmf,
> > +			dax_iodone_t complete_unwritten)
> >  {
> >  	struct address_space *mapping = inode->i_mapping;
> >  	sector_t sector = bh->b_blocknr << (inode->i_blkbits - 9);
> > @@ -310,14 +311,14 @@ static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
> >   out:
> >  	i_mmap_unlock_read(mapping);
> >  
> > -	if (bh->b_end_io)
> > -		bh->b_end_io(bh, 1);
> > +	if (buffer_unwritten(bh))
> > +		complete_unwritten(bh, 1);
> >  
> >  	return error;
> >  }
>   So frankly I don't see a big point in passing completion callback into
> dax_insert_mapping() only to call the function at the end of it. We could
> as well call the completion function from do_dax_fault() where it would
> seem more natural to me. But I don't feel too strongly about this.

On further review, I think the code is incorrect as is, even without
this change - we shouldn't be running unwritten extent conversion
if the block zeroing failed. So this needs fixing anyway. I'll pull
the completion back to do_dax_fault(), where it willonly be run if
there was no error inserting the mapping.

> Instead of the above I was also thinking about some way to pass information
> out of do_dax_fault() into filesystem so that it could just call completion
> handler itself but the completion callback is more standard interface I
> guess.

That seems unbalanced to me, as internal mapping state would need to
be leaked back out to the caller so they could run conversion. I
think it's cleaner to pass in the callback and leave all that
mapping state internal to do_dax_fault()....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs