Re: [PATCH v4 12/18] xfs: use DEFINE_FSDAX_AOPS

[Jan Kara <jack@suse.cz>]

On Tue 02-01-18 13:40:32, Dan Williams wrote:
> On Tue, Jan 2, 2018 at 1:15 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > On Sat, Dec 23, 2017 at 04:57:04PM -0800, Dan Williams wrote:
> >> In preparation for the dax implementation to start associating dax pages
> >> to inodes via page->mapping, we need to provide a 'struct
> >> address_space_operations' instance for dax. Otherwise, direct-I/O
> >> triggers incorrect page cache assumptions and warnings.
> >>
> >> Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
> >> Cc: linux-xfs@vger.kernel.org
> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >> ---
> >>  fs/xfs/xfs_aops.c |    2 ++
> >>  fs/xfs/xfs_aops.h |    1 +
> >>  fs/xfs/xfs_iops.c |    5 ++++-
> >>  3 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> >> index 21e2d70884e1..361915d53cef 100644
> >> --- a/fs/xfs/xfs_aops.c
> >> +++ b/fs/xfs/xfs_aops.c
> >> @@ -1492,3 +1492,5 @@ const struct address_space_operations xfs_address_space_operations = {
> >>       .is_partially_uptodate  = block_is_partially_uptodate,
> >>       .error_remove_page      = generic_error_remove_page,
> >>  };
> >> +
> >> +DEFINE_FSDAX_AOPS(xfs_dax_address_space_operations, xfs_vm_writepages);
> >
> > Hmm, if we ever re-enable changing the DAX flag on the fly, will
> > mapping->a_ops have to change dynamically too?
> >
> > How sure are we that we'll never have to set anything in the dax aops
> > other than ->writepages?
> >
> > (I also kinda wonder why not just make the callers savvy vs. a bunch of
> > dummy aops, but maybe that's been tried in a previous iteration?)
> 
> Matthew had similar feedback. I pushed back that I think more IS_DAX
> sprinkling increases the long term maintenance burden, but now that
> you've independently asked for the same thing I'm not opposed to
> changing my mind.
> 
> Either way this need to switch the address_space_operations, or
> synchronize against in-flight address_space_operations is going to
> complicate the "dynamic toggle the dax mode" feature.

ext4 already dynamically switches aops for journalled data vs
non-journalled data vs nodelalloc case. That being said I'm not quite sure
this is bug-free ;)

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR