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

[Dan Williams <dan.j.williams@intel.com>]

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.