linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Ira Weiny <ira.weiny@intel.com>
Cc: linux-kernel@vger.kernel.org,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Christoph Hellwig <hch@lst.de>,
	"Theodore Y. Ts'o" <tytso@mit.edu>, Jan Kara <jack@suse.cz>,
	Jeff Moyer <jmoyer@redhat.com>,
	linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH V6 4/8] fs/xfs: Make DAX mount option a tri-state
Date: Wed, 8 Apr 2020 10:48:01 +1000	[thread overview]
Message-ID: <20200408004801.GH24067@dread.disaster.area> (raw)
In-Reply-To: <20200408000903.GA569068@iweiny-DESK2.sc.intel.com>

On Tue, Apr 07, 2020 at 05:09:04PM -0700, Ira Weiny wrote:
> On Wed, Apr 08, 2020 at 09:59:09AM +1000, Dave Chinner wrote:
> > On Tue, Apr 07, 2020 at 11:29:54AM -0700, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > As agreed upon[1].  We make the dax mount option a tri-state.  '-o dax'
> > > continues to operate the same.  We add 'always', 'never', and 'iflag'
> > > (default).
> > > 
> > > [1] https://lore.kernel.org/lkml/20200405061945.GA94792@iweiny-DESK2.sc.intel.com/
> > > 
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > ---
> > > Changes from v5:
> > > 	New Patch
> > > ---
> > >  fs/xfs/xfs_iops.c  |  2 +-
> > >  fs/xfs/xfs_mount.h | 26 +++++++++++++++++++++++++-
> > >  fs/xfs/xfs_super.c | 34 +++++++++++++++++++++++++++++-----
> > >  3 files changed, 55 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > index 81f2f93caec0..1ec4a36917bd 100644
> > > --- a/fs/xfs/xfs_iops.c
> > > +++ b/fs/xfs/xfs_iops.c
> > > @@ -1248,7 +1248,7 @@ xfs_inode_supports_dax(
> > >  		return false;
> > >  
> > >  	/* DAX mount option or DAX iflag must be set. */
> > > -	if (!(mp->m_flags & XFS_MOUNT_DAX) &&
> > > +	if (xfs_mount_dax_mode(mp) != XFS_DAX_ALWAYS &&
> > >  	    !(ip->i_d.di_flags2 & XFS_DIFLAG2_DAX))
> > >  		return false;
> > >  
> > > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > > index 88ab09ed29e7..ce027ee06692 100644
> > > --- a/fs/xfs/xfs_mount.h
> > > +++ b/fs/xfs/xfs_mount.h
> > > @@ -233,7 +233,31 @@ typedef struct xfs_mount {
> > >  						   allocator */
> > >  #define XFS_MOUNT_NOATTR2	(1ULL << 25)	/* disable use of attr2 format */
> > >  
> > > -#define XFS_MOUNT_DAX		(1ULL << 62)	/* TEST ONLY! */
> > > +/* DAX flag is a 2 bit field representing a tri-state for dax
> > > + *      iflag, always, never
> > > + * We reserve/document the 2 bits using dax field/field2
> > > + */
> > > +#define XFS_DAX_FIELD_MASK 0x3ULL
> > > +#define XFS_DAX_FIELD_SHIFT 62
> > > +#define XFS_MOUNT_DAX_FIELD	(1ULL << 62)
> > > +#define XFS_MOUNT_DAX_FIELD2	(1ULL << 63)
> > > +
> > > +enum {
> > > +	XFS_DAX_IFLAG = 0,
> > > +	XFS_DAX_ALWAYS = 1,
> > > +	XFS_DAX_NEVER = 2,
> > > +};
> > > +
> > > +static inline void xfs_mount_set_dax(struct xfs_mount *mp, u32 val)
> > > +{
> > > +	mp->m_flags &= ~(XFS_DAX_FIELD_MASK << XFS_DAX_FIELD_SHIFT);
> > > +	mp->m_flags |= ((val & XFS_DAX_FIELD_MASK) << XFS_DAX_FIELD_SHIFT);
> > > +}
> > > +
> > > +static inline u32 xfs_mount_dax_mode(struct xfs_mount *mp)
> > > +{
> > > +	return (mp->m_flags >> XFS_DAX_FIELD_SHIFT) & XFS_DAX_FIELD_MASK;
> > > +}
> > 
> > This is overly complex. Just use 2 flags:
> 
> LOL...  I was afraid someone would say that.  At first I used 2 flags with
> fsparam_string, but then I realized Darrick suggested fsparam_enum:

Well, I'm not concerned about the fsparam enum, it's just that
encoding an integer into a flags bit field is just ... messy.
Especially when encoding that state can be done with just 2 flags.

If you want to keep the xfs_mount_dax_mode() wrapper, then:

static inline uint32_t xfs_mount_dax_mode(struct xfs_mount *mp)
{
	if (mp->m_flags & XFS_MOUNT_DAX_NEVER)
		return XFS_DAX_NEVER;
	if (mp->m_flags & XFS_MOUNT_DAX_ALWAYS)
		return XFS_DAX_ALWAYS;
	return XFS_DAX_IFLAG;
}

but once it's encoded in flags like this, the wrapper really isn't
necessary...

Also, while I think of it, can we change "iflag" to "inode". i.e.
the DAX state is held on the inode. Saying it comes from an "inode
flag" encodes the implementation into the user interface. i.e. it
could well be held in an xattr on the inode on another filesystem,
so we shouldn't mention "flag" in the user API....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2020-04-08  0:48 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-07 18:29 [PATCH V6 0/8] Enable per-file/per-directory DAX operations V6 ira.weiny
2020-04-07 18:29 ` [PATCH V6 1/8] fs/xfs: Remove unnecessary initialization of i_rwsem ira.weiny
2020-04-07 23:46   ` Dave Chinner
2020-04-07 18:29 ` [PATCH V6 2/8] fs: Remove unneeded IS_DAX() check ira.weiny
2020-04-09  7:31   ` Christoph Hellwig
2020-04-09 14:57     ` Ira Weiny
2020-04-07 18:29 ` [PATCH V6 3/8] fs/stat: Define DAX statx attribute ira.weiny
2020-04-07 23:47   ` Dave Chinner
2020-04-07 18:29 ` [PATCH V6 4/8] fs/xfs: Make DAX mount option a tri-state ira.weiny
2020-04-07 23:59   ` Dave Chinner
2020-04-08  0:09     ` Ira Weiny
2020-04-08  0:48       ` Dave Chinner [this message]
2020-04-09 15:03         ` Ira Weiny
2020-04-07 18:29 ` [PATCH V6 5/8] fs/xfs: Create function xfs_inode_enable_dax() ira.weiny
2020-04-08  0:05   ` Dave Chinner
2020-04-08  0:13     ` Ira Weiny
2020-04-07 18:29 ` [PATCH V6 6/8] fs/xfs: Combine xfs_diflags_to_linux() and xfs_diflags_to_iflags() ira.weiny
2020-04-08  2:08   ` Dave Chinner
2020-04-08 17:09     ` Ira Weiny
2020-04-08 21:02       ` Dave Chinner
2020-04-08 21:28         ` Dan Williams
2020-04-08 22:10           ` Ira Weiny
2020-04-08 23:58           ` Dave Chinner
2020-04-09  0:22             ` Ira Weiny
2020-04-09 12:41               ` Christoph Hellwig
2020-04-09 20:49                 ` Ira Weiny
2020-04-08 22:07         ` Ira Weiny
2020-04-08 23:21           ` Dave Chinner
2020-04-09  0:12             ` Ira Weiny
2020-04-09  0:30               ` Darrick J. Wong
2020-04-09 15:29                 ` Ira Weiny
2020-04-09 16:59                   ` Darrick J. Wong
2020-04-09 17:17                     ` Jan Kara
2020-04-09 20:54                     ` Ira Weiny
2020-04-09  0:49               ` Dave Chinner
2020-04-09 12:40                 ` Christoph Hellwig
2020-04-10  0:27                   ` Dave Chinner
2020-04-07 18:29 ` [PATCH V6 7/8] fs/xfs: Change xfs_ioctl_setattr_dax_invalidate() to xfs_ioctl_dax_check() ira.weiny
2020-04-08  2:23   ` Dave Chinner
2020-04-08  9:58     ` Jan Kara
2020-04-08 21:09       ` Dave Chinner
2020-04-08 22:26         ` Ira Weiny
2020-04-08 23:48           ` Dave Chinner
2020-04-09 12:28             ` Christoph Hellwig
2020-04-08 15:37   ` Darrick J. Wong
2020-04-08 18:13     ` Ira Weiny
2020-04-16  5:39   ` [fs/xfs] 857c9841f8: xfstests.xfs.046.fail kernel test robot
2020-04-07 18:29 ` [PATCH V6 8/8] Documentation/dax: Update Usage section ira.weiny

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200408004801.GH24067@dread.disaster.area \
    --to=david@fromorbit.com \
    --cc=dan.j.williams@intel.com \
    --cc=darrick.wong@oracle.com \
    --cc=hch@lst.de \
    --cc=ira.weiny@intel.com \
    --cc=jack@suse.cz \
    --cc=jmoyer@redhat.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=tytso@mit.edu \
    --subject='Re: [PATCH V6 4/8] fs/xfs: Make DAX mount option a tri-state' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).