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: Jan Kara <jack@suse.cz>,
	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>,
	Jeff Moyer <jmoyer@redhat.com>,
	linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH V6 7/8] fs/xfs: Change xfs_ioctl_setattr_dax_invalidate() to xfs_ioctl_dax_check()
Date: Thu, 9 Apr 2020 09:48:17 +1000	[thread overview]
Message-ID: <20200408234817.GP24067@dread.disaster.area> (raw)
In-Reply-To: <20200408222636.GC664132@iweiny-DESK2.sc.intel.com>

On Wed, Apr 08, 2020 at 03:26:36PM -0700, Ira Weiny wrote:
> On Thu, Apr 09, 2020 at 07:09:50AM +1000, Dave Chinner wrote:
> > On Wed, Apr 08, 2020 at 11:58:03AM +0200, Jan Kara wrote:
> > I explained how we can safely remove the flag in the other branch of
> > this thread...
> > 
> > > But it seems like more understandable API than letting user clear
> > > the on-disk flag but the inode will still use DAX until kernel decides to
> > > evict the inode
> > 
> > Certainly doesn't seem that way to me. "stop app, clear flags, drop
> > caches, restart app" is a pretty simple, easy thing to do for an
> > admin.
> 
> I want to be clear here: I think this is reasonable.  However, I don't see
> consensus for that interface.
> 
> Christoph in particular said that a 'lazy change' is: "... straight from
> the playbook for arcane and confusing API designs."
> 
> 	"But returning an error and doing a lazy change anyway is straight from
> 	the playbook for arcane and confusing API designs."
> 
> 	-- https://lore.kernel.org/lkml/20200403072731.GA24176@lst.de/
> 
> Did I somehow misunderstand this?

Yes. Clearing the on-disk flag successfully should not return an
error.

What is wrong is having it clear the flag successfully and returning
an error because the operation doesn't take immediate effect, then
having the change take effect later after telling the application
there was an error.

That's what Christoph was saying is "straight from the playbook for
arcane and confusing API designs."

There's absolutely nothing wrong with setting/clearing the on-disk
flag and having the change take effect some time later depending on
some external context. We've done this sort of thing for a -long
time- and it's not XFS specific at all.

e.g.  changing the on-disk APPEND flag doesn't change the write
behaviour of currently open files - it only affects the behaviour of
future file opens. IOWs, we can have the flag set on disk, but we
can still write randomly to the inode as long as we have a file
descriptor that was opened before the APPEND on disk flag was set.

That's exactly the same class of behaviour as we are talking about
here for the on-disk DAX flag.

> > Especially compared to process that is effectively "stop app, backup
> > data set, delete data set, clear flags, restore data set, restart
> > app"
> > 
> > > - because that often means you need to restart the
> > > application using the file anyway for the flag change to have any effect.
> > 
> > That's a trivial requirement compared to the downtime and resource
> > cost of a data set backup/restore just to clear inode flags....
> 
> I agree but others do not.  This still provides a baby step forward and some

It's not a baby step forward. We can't expose a behaviour to
userspace and then decide to change it completely at some later
date.  We have to think through the entire admin model before
setting it in concrete.

If an admin operation can set an optional persistent feature flags
on a file, then there *must* be admin operations that can remove
that persistent feature flag from said files. This has *nothing to
do with DAX* - it's a fundamental principle of balanced system
design.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2020-04-08 23: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
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 [this message]
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=20200408234817.GP24067@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 7/8] fs/xfs: Change xfs_ioctl_setattr_dax_invalidate() to xfs_ioctl_dax_check()' \
    /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).