xfs
[Top] [All Lists]

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

To: Jan Kara <jack@xxxxxxx>
Subject: Re: [PATCH 1/6] dax: don't abuse get_block mapping for endio callbacks
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 5 Mar 2015 09:29:35 +1100
Cc: xfs@xxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, willy@xxxxxxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150304155408.GA2799@xxxxxxxxxxxxx>
References: <1425425427-16283-1-git-send-email-david@xxxxxxxxxxxxx> <1425425427-16283-2-git-send-email-david@xxxxxxxxxxxxx> <20150304155408.GA2799@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxx>
> > @@ -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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>