Re: [PATCH v2 2/2] dax: move writeback calls into the filesystems

[Jan Kara]

On Thu 11-02-16 07:22:00, Dan Williams wrote:
> On Thu, Feb 11, 2016 at 4:50 AM, Jan Kara <jack@xxxxxxx> wrote:
> > On Wed 10-02-16 15:43:40, Ross Zwisler wrote:
> >> On Thu, Feb 11, 2016 at 09:03:12AM +1100, Dave Chinner wrote:
> >> > On Wed, Feb 10, 2016 at 01:48:56PM -0700, Ross Zwisler wrote:
> >> > > Previously calls to dax_writeback_mapping_range() for all DAX filesystems
> >> > > (ext2, ext4 & xfs) were centralized in filemap_write_and_wait_range().
> >> > > dax_writeback_mapping_range() needs a struct block_device, and it used to
> >> > > get that from inode->i_sb->s_bdev.  This is correct for normal inodes
> >> > > mounted on ext2, ext4 and XFS filesystems, but is incorrect for DAX raw
> >> > > block devices and for XFS real-time files.
> >> > >
> >> > > Instead, call dax_writeback_mapping_range() directly from the filesystem
> >> > > ->writepages function so that it can supply us with a valid block
> >> > > device. This also fixes DAX code to properly flush caches in response to
> >> > > sync(2).
> >> > >
> >> > > Signed-off-by: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>
> >> > > Signed-off-by: Jan Kara <jack@xxxxxxx>
> >> > > ---
> >> > >  fs/block_dev.c      | 16 +++++++++++++++-
> >> > >  fs/dax.c            | 13 ++++++++-----
> >> > >  fs/ext2/inode.c     | 11 +++++++++++
> >> > >  fs/ext4/inode.c     |  7 +++++++
> >> > >  fs/xfs/xfs_aops.c   |  9 +++++++++
> >> > >  include/linux/dax.h |  6 ++++--
> >> > >  mm/filemap.c        | 12 ++++--------
> >> > >  7 files changed, 58 insertions(+), 16 deletions(-)
> >> > >
> >> > > diff --git a/fs/block_dev.c b/fs/block_dev.c
> >> > > index 39b3a17..fc01e43 100644
> >> > > --- a/fs/block_dev.c
> >> > > +++ b/fs/block_dev.c
> >> > > @@ -1693,13 +1693,27 @@ static int blkdev_releasepage(struct page *page, gfp_t wait)
> >> > >   return try_to_free_buffers(page);
> >> > >  }
> >> > >
> >> > > +static int blkdev_writepages(struct address_space *mapping,
> >> > > +                      struct writeback_control *wbc)
> >> > > +{
> >> > > + if (dax_mapping(mapping)) {
> >> > > +         struct block_device *bdev = I_BDEV(mapping->host);
> >> > > +         int error;
> >> > > +
> >> > > +         error = dax_writeback_mapping_range(mapping, bdev, wbc);
> >> > > +         if (error)
> >> > > +                 return error;
> >> > > + }
> >> > > + return generic_writepages(mapping, wbc);
> >> > > +}
> >> >
> >> > Can you remind of the reason for calling generic_writepages() on DAX
> >> > enabled address spaces?
> >>
> >> Sure.  The initial version of this patch didn't do this, and during testing I
> >> hit a bunch of xfstests failures.  In ext2 at least I believe these were
> >> happening because we were skipping the call into generic_writepages() for DAX
> >> inodes. Without a lot of data to back this up, my guess is that this is due
> >> to metadata inodes or something being marked as DAX (so dax_mapping(mapping)
> >> returns true), but having dirty page cache pages that need to be written back
> >> as part of the writeback.
> >>
> >> Changing this so we always call generic_writepages() even in the DAX case
> >> solved the xfstest failures.
> >>
> >> If this sounds incorrect, please let me know and I'll go and gather more data.
> >
> > So I think a more correct fix it to not set S_DAX for inodes that will have
> > any pagecache pages - e.g. don't set S_DAX for block device inodes when
> > filesystem is mounted on it (probably the easiest is to just refuse to
> > mount filesystem on block device which has S_DAX set).
> 
> I think we have a wider problem here.  See __blkdev_get, we set S_DAX
> on all block devices that have ->direct_access() and have a
> page-aligned starting address.  It seems to me we need to modify the
> metadata i/o paths to bypass the page cache

Heh, no way to do that easily. All the journalling machinery depends on
buffers and pages...

>, or teach the fsync code
> how to flush populated data pages out of the radix.

This might be doable but it will be difficult to avoid aliasing issues and
data corruption. And mainly I don't see the point: When you mount a
filesystem on top of block device, you do not want to mess with the block
device directly, even less using DAX. So we just have to find a way how to
set S_DAX for normal open but clear it from fs path. At worst, we could
clear S_DAX on the block device in mount_bdev() or something like that...

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR