linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Ira Weiny <ira.weiny@intel.com>
Cc: linux-kernel@vger.kernel.org, Jan Kara <jack@suse.cz>,
	Dan Williams <dan.j.williams@intel.com>,
	Dave Chinner <david@fromorbit.com>,
	Christoph Hellwig <hch@lst.de>,
	"Theodore Y. Ts'o" <tytso@mit.edu>,
	Jeff Moyer <jmoyer@redhat.com>,
	linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH RFC 4/8] fs/ext4: Introduce DAX inode flag
Date: Thu, 16 Apr 2020 18:57:31 -0700	[thread overview]
Message-ID: <20200417015731.GU6742@magnolia> (raw)
In-Reply-To: <20200417003719.GP2309605@iweiny-DESK2.sc.intel.com>

On Thu, Apr 16, 2020 at 05:37:19PM -0700, Ira Weiny wrote:
> On Thu, Apr 16, 2020 at 03:49:37PM -0700, Darrick J. Wong wrote:
> > On Thu, Apr 16, 2020 at 03:33:27PM -0700, Ira Weiny wrote:
> > > On Thu, Apr 16, 2020 at 09:25:04AM -0700, Darrick J. Wong wrote:
> > > > On Mon, Apr 13, 2020 at 09:00:26PM -0700, ira.weiny@intel.com wrote:
> > > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > > 
> > > > > Add a flag to preserve FS_XFLAG_DAX in the ext4 inode.
> > > > > 
> > > > > Set the flag to be user visible and changeable.  Set the flag to be
> > > > > inherited.  Allow applications to change the flag at any time.
> > > > > 
> > > > > Finally, on regular files, flag the inode to not be cached to facilitate
> > > > > changing S_DAX on the next creation of the inode.
> > > > > 
> > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > > > ---
> > > > >  fs/ext4/ext4.h  | 13 +++++++++----
> > > > >  fs/ext4/ioctl.c | 21 ++++++++++++++++++++-
> > > > >  2 files changed, 29 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > > > > index 61b37a052052..434021fcec88 100644
> > > > > --- a/fs/ext4/ext4.h
> > > > > +++ b/fs/ext4/ext4.h
> > > > > @@ -415,13 +415,16 @@ struct flex_groups {
> > > > >  #define EXT4_VERITY_FL			0x00100000 /* Verity protected inode */
> > > > >  #define EXT4_EA_INODE_FL	        0x00200000 /* Inode used for large EA */
> > > > >  #define EXT4_EOFBLOCKS_FL		0x00400000 /* Blocks allocated beyond EOF */
> > > > > +
> > > > > +#define EXT4_DAX_FL			0x00800000 /* Inode is DAX */
> > > > 
> > > > Sooo, fun fact about ext4 vs. the world--
> > > > 
> > > > The GETFLAGS/SETFLAGS ioctl, since it came from ext2, shares the same
> > > > flag values as the ondisk inode flags in ext*.  Therefore, each of these
> > > > EXT4_[whatever]_FL values are supposed to have a FS_[whatever]_FL
> > > > equivalent in include/uapi/linux/fs.h.
> > > 
> > > Interesting...
> > > 
> > > > 
> > > > (Note that the "[whatever]" is a straight translation since the same
> > > > uapi header also defines the FS_XFLAG_[xfswhatever] flag values; ignore
> > > > those.)
> > > > 
> > > > Evidently, FS_NOCOW_FL already took 0x800000, but ext4.h was never
> > > > updated to note that the value was taken.  I think Ted might be inclined
> > > > to reserve the ondisk inode bit just in case ext4 ever does support copy
> > > > on write, though that's his call. :)
> > > 
> > > Seems like I should change this...  And I did not realize I was inherently
> > > changing a bit definition which was exposed to other FS's...
> > 
> > <nod> This whole thing is a mess, particularly now that we have two vfs
> > ioctls to set per-fs inode attributes, both of which were inherited from
> > other filesystems... :(
> >
> 
> Ok I've changed it.
> 
> > 
> > > > 
> > > > Long story short - can you use 0x1000000 for this instead, and add the
> > > > corresponding value to the uapi fs.h?  I guess that also means that we
> > > > can change FS_XFLAG_DAX (in the form of FS_DAX_FL in FSSETFLAGS) after
> > > > that.
> > > 
> > > :-/
> > > 
> > > Are there any potential users of FS_XFLAG_DAX now?
> > 
> > Yes, it's in the userspace ABI so we can't get rid of it.
> > 
> > (FWIW there are several flags that exist in both FS_XFLAG_* and FS_*_FL
> > form.)
> > 
> > > From what it looks like, changing FS_XFLAG_DAX to FS_DAX_FL would be pretty
> > > straight forward.  Just to be sure, looks like XFS converts the FS_[xxx]_FL to
> > > FS_XFLAGS_[xxx] in xfs_merge_ioc_xflags()?  But it does not look like all the
> > > FS_[xxx]_FL flags are converted.  Is is that XFS does not support those
> > > options?  Or is it depending on the VFS layer for some of them?
> > 
> > XFS doesn't support most of the FS_*_FL flags.
> 
> If FS_XFLAG_DAX needs to continue to be user visible I think we need to keep
> that flag and we should not expose the EXT4_DAX_FL flag...
> 
> I think that works for XFS.
> 
> But for ext4 it looks like EXT4_FL_XFLAG_VISIBLE was intended to be used for
> [GET|SET]XATTR where EXT4_FL_USER_VISIBLE was intended to for [GET|SET]FLAGS...
> But if I don't add EXT4_DAX_FL in EXT4_FL_XFLAG_VISIBLE my test fails.
> 
> I've been playing with the flags and looking at the code and I _thought_ the
> following patch would ensure that FS_XFLAG_DAX is the only one visible but for
> some reason FS_XFLAG_DAX can't be set with this patch.  I still need the
> EXT4_FL_USER_VISIBLE mask altered...  Which I believe would expose EXT4_DAX_FL
> directly as well.
> 
> Jan, Ted?  Any ideas?  Or should we expose EXT4_DAX_FL and FS_XFLAG_DAX in
> ext4?

Both flags should be exposed through their respective ioctl interfaces
in both filesystems.  That way we don't have to add even more verbiage
to the documentation to instruct userspace programmers on how to special
case ext4 and XFS for the same piece of functionality.

--D

> Ira
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index fb7e66089a74..c3823f057755 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -423,7 +423,7 @@ struct flex_groups {
>  #define EXT4_CASEFOLD_FL               0x40000000 /* Casefolded file */
>  #define EXT4_RESERVED_FL               0x80000000 /* reserved for ext4 lib */
>  
> -#define EXT4_FL_USER_VISIBLE           0x715BDFFF /* User visible flags */
> +#define EXT4_FL_USER_VISIBLE           0x705BDFFF /* User visible flags */
>  #define EXT4_FL_USER_MODIFIABLE                0x614BC0FF /* User modifiable flags */
>  
>  /* Flags we can manipulate with through EXT4_IOC_FSSETXATTR */
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index b3c6e891185e..8bd0d3f9ca0b 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -744,8 +744,8 @@ static void ext4_fill_fsxattr(struct inode *inode, struct fsxattr *fa)
>  {
>         struct ext4_inode_info *ei = EXT4_I(inode);
>  
> -       simple_fill_fsxattr(fa, ext4_iflags_to_xflags(ei->i_flags &
> -                                                     EXT4_FL_USER_VISIBLE));
> +       simple_fill_fsxattr(fa, (ext4_iflags_to_xflags(ei->i_flags) &
> +                                                     EXT4_FL_XFLAG_VISIBLE));
>  
>         if (ext4_has_feature_project(inode->i_sb))
>                 fa->fsx_projid = from_kprojid(&init_user_ns, ei->i_projid);

  reply	other threads:[~2020-04-17  1:57 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-14  4:00 [PATCH RFC 0/8] Enable ext4 support for per-file/directory DAX operations ira.weiny
2020-04-14  4:00 ` [PATCH RFC 1/8] fs/ext4: Narrow scope of DAX check in setflags ira.weiny
2020-04-15 11:45   ` Jan Kara
2020-04-14  4:00 ` [PATCH RFC 2/8] fs/ext4: Disallow verity if inode is DAX ira.weiny
2020-04-15 11:58   ` Jan Kara
2020-04-15 12:00   ` Jan Kara
2020-04-15 15:55     ` Theodore Y. Ts'o
2020-04-15 19:21       ` Ira Weiny
2020-04-15 19:14     ` Ira Weiny
2020-04-16  1:29       ` Eric Biggers
2020-04-16  3:48         ` Ira Weiny
2020-04-15 20:34     ` Ira Weiny
2020-04-14  4:00 ` [PATCH RFC 3/8] fs/ext4: Disallow encryption " ira.weiny
2020-04-15 12:02   ` Jan Kara
2020-04-15 20:35     ` Ira Weiny
2020-04-15 16:03   ` Theodore Y. Ts'o
2020-04-15 17:27     ` Dan Williams
2020-04-15 19:54     ` Ira Weiny
2020-04-21 18:41       ` Ira Weiny
2020-04-21 18:56         ` Darrick J. Wong
2020-04-14  4:00 ` [PATCH RFC 4/8] fs/ext4: Introduce DAX inode flag ira.weiny
2020-04-15 12:08   ` Jan Kara
2020-04-15 20:39     ` Ira Weiny
2020-04-16 10:32       ` Jan Kara
2020-04-16 18:01       ` Theodore Y. Ts'o
2020-04-16 16:25   ` Darrick J. Wong
2020-04-16 22:33     ` Ira Weiny
2020-04-16 22:49       ` Darrick J. Wong
2020-04-17  0:37         ` Ira Weiny
2020-04-17  1:57           ` Darrick J. Wong [this message]
2020-04-17  2:20             ` Ira Weiny
2020-04-17  6:43               ` Andreas Dilger
2020-04-17 17:19                 ` Ira Weiny
2020-04-14  4:00 ` [PATCH RFC 5/8] fs/ext4: Make DAX mount option a tri-state ira.weiny
2020-04-15 12:51   ` Jan Kara
2020-04-14  4:00 ` [PATCH RFC 6/8] fs/ext4: Update ext4_should_use_dax() ira.weiny
2020-04-15 13:58   ` Jan Kara
2020-04-17 17:16     ` Ira Weiny
2020-04-14  4:00 ` [PATCH RFC 7/8] fs/ext4: Only change S_DAX on inode load ira.weiny
2020-04-15 14:03   ` Jan Kara
2020-04-17 17:18     ` Ira Weiny
2020-04-14  4:00 ` [PATCH RFC 8/8] Documentation/dax: Update DAX enablement for ext4 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=20200417015731.GU6742@magnolia \
    --to=darrick.wong@oracle.com \
    --cc=dan.j.williams@intel.com \
    --cc=david@fromorbit.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 \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).