linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dan Williams <dan.j.williams@intel.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Ira Weiny <ira.weiny@intel.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"Darrick J. Wong" <darrick.wong@oracle.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 <linux-ext4@vger.kernel.org>,
	linux-xfs <linux-xfs@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH V6 6/8] fs/xfs: Combine xfs_diflags_to_linux() and xfs_diflags_to_iflags()
Date: Wed, 8 Apr 2020 14:28:30 -0700	[thread overview]
Message-ID: <CAPcyv4gLvMSA9BypvWbYtv3xsK8o4+db3kvxBozUGAjr_sDDFQ@mail.gmail.com> (raw)
In-Reply-To: <20200408210236.GK24067@dread.disaster.area>

On Wed, Apr 8, 2020 at 2:02 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Wed, Apr 08, 2020 at 10:09:23AM -0700, Ira Weiny wrote:
> > On Wed, Apr 08, 2020 at 12:08:27PM +1000, Dave Chinner wrote:
> > > On Tue, Apr 07, 2020 at 11:29:56AM -0700, ira.weiny@intel.com wrote:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> >
> > [snip]
> >
> > > >
> > > > -STATIC void
> > > > -xfs_diflags_to_linux(
> > > > - struct xfs_inode        *ip)
> > > > -{
> > > > - struct inode            *inode = VFS_I(ip);
> > > > - unsigned int            xflags = xfs_ip2xflags(ip);
> > > > -
> > > > - if (xflags & FS_XFLAG_IMMUTABLE)
> > > > -         inode->i_flags |= S_IMMUTABLE;
> > > > - else
> > > > -         inode->i_flags &= ~S_IMMUTABLE;
> > > > - if (xflags & FS_XFLAG_APPEND)
> > > > -         inode->i_flags |= S_APPEND;
> > > > - else
> > > > -         inode->i_flags &= ~S_APPEND;
> > > > - if (xflags & FS_XFLAG_SYNC)
> > > > -         inode->i_flags |= S_SYNC;
> > > > - else
> > > > -         inode->i_flags &= ~S_SYNC;
> > > > - if (xflags & FS_XFLAG_NOATIME)
> > > > -         inode->i_flags |= S_NOATIME;
> > > > - else
> > > > -         inode->i_flags &= ~S_NOATIME;
> > > > -#if 0    /* disabled until the flag switching races are sorted out */
> > > > - if (xflags & FS_XFLAG_DAX)
> > > > -         inode->i_flags |= S_DAX;
> > > > - else
> > > > -         inode->i_flags &= ~S_DAX;
> > > > -#endif
> > >
> > > So this variant will set the flag in the inode if the disk inode
> > > flag is set, otherwise it will clear it.  It does it with if/else
> > > branches.
> > >
> > >
> > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > > index e07f7b641226..a4ac8568c8c7 100644
> > > > --- a/fs/xfs/xfs_iops.c
> > > > +++ b/fs/xfs/xfs_iops.c
> > > > @@ -1259,7 +1259,7 @@ xfs_inode_supports_dax(
> > > >   return xfs_inode_buftarg(ip)->bt_daxdev != NULL;
> > > >  }
> > > >
> > > > -STATIC bool
> > > > +static bool
> > > >  xfs_inode_enable_dax(
> > > >   struct xfs_inode *ip)
> > > >  {
> > >
> > > This belongs in the previous patch.
> >
> > Ah yea...  Sorry.
> >
> > Fixed in V7
> >
> > >
> > > > @@ -1272,26 +1272,38 @@ xfs_inode_enable_dax(
> > > >   return false;
> > > >  }
> > > >
> > > > -STATIC void
> > > > +void
> > > >  xfs_diflags_to_iflags(
> > > > - struct inode            *inode,
> > > > - struct xfs_inode        *ip)
> > > > + struct xfs_inode        *ip,
> > > > + bool init)
> > > >  {
> > > > - uint16_t                flags = ip->i_d.di_flags;
> > > > -
> > > > - inode->i_flags &= ~(S_IMMUTABLE | S_APPEND | S_SYNC |
> > > > -                     S_NOATIME | S_DAX);
> > >
> > > And this code cleared all the flags in the inode first, then
> > > set them if the disk inode flag is set. This does not require
> > > branches, resulting in more readable code and better code
> > > generation.
> > >
> > > > + struct inode            *inode = VFS_I(ip);
> > > > + uint                    diflags = xfs_ip2xflags(ip);
> > > >
> > > > - if (flags & XFS_DIFLAG_IMMUTABLE)
> > > > + if (diflags & FS_XFLAG_IMMUTABLE)
> > > >           inode->i_flags |= S_IMMUTABLE;
> > > > - if (flags & XFS_DIFLAG_APPEND)
> > > > + else
> > > > +         inode->i_flags &= ~S_IMMUTABLE;
> > >
> > > > + if (diflags & FS_XFLAG_APPEND)
> > > >           inode->i_flags |= S_APPEND;
> > > > - if (flags & XFS_DIFLAG_SYNC)
> > > > + else
> > > > +         inode->i_flags &= ~S_APPEND;
> > > > + if (diflags & FS_XFLAG_SYNC)
> > > >           inode->i_flags |= S_SYNC;
> > > > - if (flags & XFS_DIFLAG_NOATIME)
> > > > + else
> > > > +         inode->i_flags &= ~S_SYNC;
> > > > + if (diflags & FS_XFLAG_NOATIME)
> > > >           inode->i_flags |= S_NOATIME;
> > > > - if (xfs_inode_enable_dax(ip))
> > > > -         inode->i_flags |= S_DAX;
> > > > + else
> > > > +         inode->i_flags &= ~S_NOATIME;
> > > > +
> > > > + /* Only toggle the dax flag when initializing */
> > > > + if (init) {
> > > > +         if (xfs_inode_enable_dax(ip))
> > > > +                 inode->i_flags |= S_DAX;
> > > > +         else
> > > > +                 inode->i_flags &= ~S_DAX;
> > > > + }
> > > >  }
> > >
> > > IOWs, this:
> > >
> > >         struct inode            *inode = VFS_I(ip);
> > >         unsigned int            xflags = xfs_ip2xflags(ip);
> > >         unsigned int            flags = 0;
> > >
> > >         if (xflags & FS_XFLAG_IMMUTABLE)
> > >                 flags |= S_IMMUTABLE;
> > >         if (xflags & FS_XFLAG_APPEND)
> > >                 flags |= S_APPEND;
> > >         if (xflags & FS_XFLAG_SYNC)
> > >                 flags |= S_SYNC;
> > >         if (xflags & FS_XFLAG_NOATIME)
> > >                 flags |= S_NOATIME;
> > >     if ((xflags & FS_XFLAG_DAX) && init)
> > >             flags |= S_DAX;
> > >
> > >         inode->i_flags &= ~(S_IMMUTABLE | S_APPEND | S_SYNC | S_NOATIME);
> > >         inode->i_flags |= flags;
> > >
> > > ends up being much easier to read and results in better code
> > > generation. And we don't need to clear the S_DAX flag when "init" is
> > > set, because we are starting from an inode that has no flags set
> > > (because init!)...
> >
> > 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.
>
> 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.
>
> 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.

Makes sense, but is that sufficient? I recall you saying there might
be a multitude of other reasons that the inode is not evicted, not the
least of which is races [1]. Does this need another flag, lets call it
"dax toggle" to track the "I requested the inode to clear the flag,
but on cache-flush + restart the inode never got evicted" case. S_DAX
almost plays this role, but it loses the ability to audit which files
are pending an inode eviction event. So the dax-toggle flag indicates
to the kernel to xor the toggle value with the inode flag on inode
instantiation and the dax inode flag is never directly manipulated by
the ioctl path.

[1]: http://lore.kernel.org/r/20191025003603.GE4614@dread.disaster.area

  reply	other threads:[~2020-04-08 21:28 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
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 [this message]
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=CAPcyv4gLvMSA9BypvWbYtv3xsK8o4+db3kvxBozUGAjr_sDDFQ@mail.gmail.com \
    --to=dan.j.williams@intel.com \
    --cc=darrick.wong@oracle.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 \
    --subject='Re: [PATCH V6 6/8] fs/xfs: Combine xfs_diflags_to_linux() and xfs_diflags_to_iflags()' \
    /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).