From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sandeen.net ([63.231.237.45]:37942 "EHLO sandeen.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726412AbeJRGr0 (ORCPT ); Thu, 18 Oct 2018 02:47:26 -0400 Subject: Re: [PATCH] xfs: reject per-inode dax flag on reflink filesystems References: <20181017223047.GR28243@magnolia> From: Eric Sandeen Message-ID: <7ea6bd8c-fcda-e1bc-0438-1d741a575e75@sandeen.net> Date: Wed, 17 Oct 2018 17:49:32 -0500 MIME-Version: 1.0 In-Reply-To: <20181017223047.GR28243@magnolia> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: "Darrick J. Wong" , Eric Sandeen Cc: linux-xfs , Jeff Moyer On 10/17/18 5:30 PM, Darrick J. Wong wrote: > On Wed, Oct 17, 2018 at 05:20:56PM -0500, Eric Sandeen wrote: >> Today we reject this flag on an already-reflinked inode, but we really >> need to reject it for any inode on a reflink-capable filesystem until >> reflink+dax is supported. >> >> Signed-off-by: Eric Sandeen >> --- >> >> (um, do we need to catch this when reading from disk as well? If we >> found a dax-flagged inode on a reflinked fs, then what would we do with it?) > > Ignore the DAX flag. See xfs_inode_supports_dax. Oh, ok. Hohum, by that measure shouldn't we allow mount -o dax on reflink filesystems, and just ignore/reject dax behavior on actually reflinked inodes? >> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >> index 0ef5ece5634c..63d579c652f2 100644 >> --- a/fs/xfs/xfs_ioctl.c >> +++ b/fs/xfs/xfs_ioctl.c >> @@ -1031,8 +1031,9 @@ xfs_ioctl_setattr_xflags( >> if ((fa->fsx_xflags & FS_XFLAG_REALTIME) && xfs_is_reflink_inode(ip)) >> ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK; >> >> - /* Don't allow us to set DAX mode for a reflinked file for now. */ >> - if ((fa->fsx_xflags & FS_XFLAG_DAX) && xfs_is_reflink_inode(ip)) >> + /* Don't allow us to set DAX mode on a reflink filesystem for now. */ >> + if ((fa->fsx_xflags & FS_XFLAG_DAX) && >> + xfs_sb_version_hasreflink(&ip->i_mount->m_sb)) >> return -EINVAL; > > Not sure we need this, since DAX always loses to REFLINK, whether it's > at inode loading time or if someone's trying to set it in the ioctl. loses to a reflinked /inode/ but not to a reflink-capable fs, like mount does. > Ofc it's hard to say what the behavior should be since the dax iflag > semantics are poorly defined (it's been more or less advisory this whole > time)... > > ...but I gather you're sending this patch because you don't like this > wishy washy "ok you change this flag but it doesn't tell you if that had > any effect and there's no way to find out either" behavior? :) Just aiming for some semblance of consistency. Today we have: mount -o dax + xfs_sb_version_hasreflink => ignore mount option, disable dax for all inodes regardless of reflinked status chattr +x + xfs_sb_version_hasreflink => inode is dax until it gets reflinked, then it's ignored right? (and what happens if we have a daxified inode that gets reflinked later?) -Eric