Re: [PATCH 5/8] xfs: add DAX file operations support

[Dave Chinner <david@fromorbit.com>]

On Mon, Apr 06, 2015 at 01:49:00PM -0400, Brian Foster wrote:
> On Tue, Mar 24, 2015 at 09:51:03PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Add the initial support for DAX file operations to XFS. This
> > includes the necessary block allocation and mmap page fault hooks
> > for DAX to function.
> > 
> > Note: we specifically have to disable splice_read/write from
> > occurring because they are dependent on usingthe page cache for
> > correct operation. We have no page cache for DAX, so we need to
> > disable them completely on DAX inodes.
> > 
> 
> Looks like Boaz already pointed out this required an update wrt to
> splice...
> 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_aops.c |  73 ++++++++++++++++++++++++++++++++--
> >  fs/xfs/xfs_aops.h |   7 +++-
> >  fs/xfs/xfs_file.c | 116 ++++++++++++++++++++++++++++++++----------------------
> >  3 files changed, 143 insertions(+), 53 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> > index 3a9b7a1..3fc5052 100644
> > --- a/fs/xfs/xfs_aops.c
> > +++ b/fs/xfs/xfs_aops.c
> > @@ -1233,13 +1233,64 @@ xfs_vm_releasepage(
> >  	return try_to_free_buffers(page);
> >  }
> >  
> > +/*
> > + * For DAX we need a mapping buffer callback for unwritten extent conversion
> > + * when page faults allocation blocks and then zero them.
> 
> s/allocation/allocate/
> 
> > + */
> > +#ifdef CONFIG_FS_DAX
> > +static struct xfs_ioend *
> > +xfs_dax_alloc_ioend(
> > +	struct inode	*inode,
> > +	xfs_off_t	offset,
> > +	ssize_t		size)
> > +{
> > +	struct xfs_ioend *ioend;
> > +
> > +	ASSERT(IS_DAX(inode));
> > +	ioend = xfs_alloc_ioend(inode, XFS_IO_UNWRITTEN);
> > +	ioend->io_offset = offset;
> > +	ioend->io_size = size;
> > +	return ioend;
> > +}
> > +
> > +void
> > +xfs_get_blocks_dax_complete(
> > +	struct buffer_head	*bh,
> > +	int			uptodate)
> > +{
> > +	struct xfs_ioend	*ioend = bh->b_private;
> > +	struct xfs_inode	*ip = XFS_I(ioend->io_inode);
> > +	int			error;
> > +
> > +	ASSERT(IS_DAX(ioend->io_inode));
> > +
> > +	/* if there was an error zeroing, then don't convert it */
> > +	if (!uptodate)
> > +		goto out_free;
> > +
> 
> Hmm, the error handling seems a bit off here. I'm new to the mmap paths
> so I could easily be missing something. Anyways, this uptodate val is
> hardcoded to 1 down in __dax_mkwrite(). This function is only called on
> !error, however, which seems to make this error handling superfluous. If
> I am following that correctly, who is going to free the ioend if an
> error does occur?

Right, the dax code needs fixing to unconditionally call the end IO
callback. I'll add that patch into the start of the series.

As it is, this patch needs significant rework after the DIO
write completion path rework. It greatly simplifies this because we
now have an ioend being allocated in __xfs_get_blocks....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com