xfs
[Top] [All Lists]

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

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 5/8] xfs: add DAX file operations support
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 16 Apr 2015 18:29:09 +1000
Cc: xfs@xxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, willy@xxxxxxxxxxxxxxx, jack@xxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150406174900.GC58965@xxxxxxxxxxxxxxx>
References: <1427194266-2885-1-git-send-email-david@xxxxxxxxxxxxx> <1427194266-2885-6-git-send-email-david@xxxxxxxxxxxxx> <20150406174900.GC58965@xxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxx

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