Re: [PATCH V6 7/8] fs/xfs: Change xfs_ioctl_setattr_dax_invalidate() to xfs_ioctl_dax_check()

[Christoph Hellwig]

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@xxxxxx/
> > 
> > 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.