Messages in this thread | | | Date | Wed, 8 Apr 2020 15:07:35 -0700 | From | Ira Weiny <> | Subject | Re: [PATCH V6 6/8] fs/xfs: Combine xfs_diflags_to_linux() and xfs_diflags_to_iflags() |
| |
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@lst.de/
> > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
| |