On Wed, Jul 25, 2018 at 02:20:54PM -0700, Eric Sandeen wrote: > 742d842 xfs: disable per-inode DAX flag was, I think, intended > as a short-term workaround to avoid races when toggling DAX on > and off of active inodes until mm/ sorted that out. > > (It's also a confusing title, as it didn't really disable > per-inode DAX at all.) > > However, it has the surprising (to me, at least) result that while > the ioctl succeeds, no behavior changes until the inode is cycled > out of cache and re-read from disk at some unknown later time. > This seems to badly violate the principle of least surprise. > > Until said races are properly resolved, it seems most prudent to > disallow modification of the flag on regular files altogether. > We can still allow per-inode DAX flagging via directory inheritance. > > Since DAX is still flagged as experimental (in part due to these > concerns) I don't think it's a problem to (temporarily?) break > this interface further. > > Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx> > --- I'm not in tune with the latest state of dax, but if the situation is that we don't currently have a means to correctly switch the per-inode state for an active inode (and thus have simply skipped changing the online flag while carrying on with the on-disk flag, leading to this inode cache cycling requirement), then I think this makes sense. The current interface is essentially incomplete, I don't see any reason to allow unless/until it actually works sanely. BTW, what bits are actually missing to make that happen? Why is the flush/inval currently in this function not sufficient? Implementation wise, I'm a little curious why we're piling on hacks (such as the return short-circuit and the previous #if 0) as opposed to just removing the code. The code can always be restored directly from the git history, right? Brian > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 0ef5ece5634c..94e9e25883cc 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1102,6 +1102,16 @@ xfs_ioctl_setattr_dax_invalidate( > > if (S_ISDIR(inode->i_mode)) > return 0; > + /* > + * For now, don't allow changing DAX mode on files via ioctl at all. > + * See 742d842 xfs: disable per-inode DAX flag, which only disabled > + * switching it on "live" inodes, but still set it on disk for the > + * next read - which leads to changed behavior only after cycling > + * out of cache. Eliminate this surprising behavior. > + * We do still allow setting it as an inheritance flag on directories > + * (above) which will then affect newly-created files in the dir. > + */ > + return -EINVAL; > > /* lock, flush and invalidate mapping in preparation for flag change */ > xfs_ilock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL); > @@ -1359,6 +1369,9 @@ xfs_ioctl_setattr( > * or cancel time, so need to be passed through to > * xfs_ioctl_setattr_get_trans() so it can apply them to the join call > * appropriately. > + * > + * Note, changing DAX flags on regular files via ioctl is currently > + * disallowed by this function, see comments within. > */ > code = xfs_ioctl_setattr_dax_invalidate(ip, fa, &join_flags); > if (code) > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
