From: Christoph Hellwig <email@example.com> To: Dave Chinner <firstname.lastname@example.org> Cc: Ira Weiny <email@example.com>, Jan Kara <firstname.lastname@example.org>, email@example.com, "Darrick J. Wong" <firstname.lastname@example.org>, Dan Williams <email@example.com>, Christoph Hellwig <firstname.lastname@example.org>, "Theodore Y. Ts'o" <email@example.com>, Jeff Moyer <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org, email@example.com Subject: Re: [PATCH V6 7/8] fs/xfs: Change xfs_ioctl_setattr_dax_invalidate() to xfs_ioctl_dax_check() Date: Thu, 9 Apr 2020 14:28:56 +0200 [thread overview] Message-ID: <20200409122856.GA17929@lst.de> (raw) In-Reply-To: <20200408234817.GP24067@dread.disaster.area> On Thu, Apr 09, 2020 at 09:48:17AM +1000, Dave Chinner wrote: > > 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." Yes. > 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. Some people consider that a bug, though. But I don't think we can change that now. In general I don't think APIs that don't take immediate effect are all that great, but in some cases we can live with them if they are properly documented. But APIs that return an error, but actually take effect later anyway are just crazy.
next prev parent reply other threads:[~2020-04-09 12:29 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 2020-04-09 12:28 ` Christoph Hellwig [this message] 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=20200409122856.GA17929@lst.de \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --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).