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