On Thu, Apr 09, 2020 at 07:02:36AM +1000, Dave Chinner wrote:
> On Wed, Apr 08, 2020 at 10:09:23AM -0700, Ira Weiny wrote:
[snip]
> >
> > This sounds good but I think we need a slight modification to make the function equivalent in functionality.
> >
> > void
> > xfs_diflags_to_iflags(
> > struct xfs_inode *ip,
> > bool init)
> > {
> > struct inode *inode = VFS_I(ip);
> > unsigned int xflags = xfs_ip2xflags(ip);
> > unsigned int flags = 0;
> >
> > inode->i_flags &= ~(S_IMMUTABLE | S_APPEND | S_SYNC | S_NOATIME |
> > S_DAX);
>
> We don't want to clear the dax flag here, ever, if it is already
> set. That is an externally visible change and opens us up (again) to
> races where IS_DAX() changes half way through a fault path. IOWs, avoiding
> clearing the DAX flag was something I did explicitly in the above
> code fragment.
<sigh> yes... you are correct.
But I don't like depending on the caller to clear the S_DAX flag if
xfs_inode_enable_dax() is false. IMO this function should clear the flag in
that case for consistency...
This is part of the reason I used the if/else logic from xfs_diflags_to_linux()
originally. It is very explicit.
>
> And it makes the logic clearer by pre-calculating the new flags,
> then clearing and setting the inode flags together, rather than
> having the spearated at the top and bottom of the function.
But this will not clear the S_DAX flag even if init is true. To me that is a
potential for confusion down the road.
>
> THis leads to an obvious conclusion: if we never clear the in memory
> S_DAX flag, we can actually clear the on-disk flag safely, so that
> next time the inode cycles into memory it won't be using DAX. IOWs,
> admins can stop the applications, clear the DAX flag and drop
> caches. This should result in the inode being recycled and when the
> app is restarted it will run without DAX. No ned for deleting files,
> copying large data sets, etc just to turn off an inode flag.
We already discussed evicting the inode and it was determined to be too
confusing.[*]
Furthermore, if we did want an interface like that why not allow the on-disk
flag to be set as well as cleared?
IMO, this function should set all of the flags consistently including S_DAX.
Ira
[*] https://lore.kernel.org/lkml/20200403072731.GA24176@xxxxxx/
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
