Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails

["Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>]

On Wed, Aug 16, 2017 at 10:25:11AM +1000, Dave Chinner wrote:
> On Tue, Aug 15, 2017 at 04:29:39PM -0300, Ernesto A. Fernández wrote:
> > On Tue, Aug 15, 2017 at 06:44:30PM +1000, Dave Chinner wrote:
> > > On Tue, Aug 15, 2017 at 03:18:58AM -0300, Ernesto A. Fernández wrote:
> > > But I have to ask - why do we even need to modify the mode first?
> > > Why not change the ACL first, then modify the mode+timestamp? If
> > > setting the ACL fails, then we don't have anything to undo and all
> > > is good....
> > 
> > I intended for the mode to be committed as part of the same transaction
> > that sets or removes the ACL. In my mind making the changes later, as part
> > of a separate transaction, would have meant that a crash between the two
> > left the filesystem in an inconsistent state,
> 
> No, it will not leave the fileystem in an inconsistent state. It
> will leave the inode permissions in an /unwanted/ state, but there
> is no filesystem metadata inconsistency. 
> 
> > with a new ACL but without
> > the corresponding mode bits.
> 
> Yup, but that's no different from right now, where a crash after
> setting the mode bits could be applied but the ACL update is
> missing.
> 
> Either way is even rarely than "crash at the wrong time" implies,
> because we've also got to have a complete journal checkpoint occur
> between the two operations and then crash between the checkpoint and
> the second operation. Yes, it's possible, but in the entire time
> I've been working on XFS (almost 15 years now) I can count on one
> hand the number of times such a problem has occurred and been
> reported...
> 
> So, it's a rare problem, and one that will get solved in time
> because there's much more to solving the problem than just this
> case. e.g. I worte this in 2008:
> 
> http://xfs.org/index.php/Improving_Metadata_Performance_By_Reducing_Journal_Overhead#Atomic_Multi-Transaction_Operations
> 
> And we've really only got the infrastructure we could use to
> implement this in a widespread manner with the rmap/reflink
> functionality. But implementing it will require a large amount of
> re-organisation of filesystem operations, so it's something that
> will take time to roll out.

Alright, thanks for the explanation.
 
> With that in mind, here's waht I suggested above: set the mode after
> the xattr. I haven't tested it - can you check it solves the problem
> case you are testing?

It does. Of course the test still fails, as I said before, now claiming that
the filesystem is inconsistent. But that's a separate issue.

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> 
> xfs: don't change inode mode if ACL update fails
> 
> XXX: untested
> 
> ---
>  fs/xfs/xfs_acl.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index 7034e17535de..3354140de07e 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -247,6 +247,8 @@ xfs_set_mode(struct inode *inode, umode_t mode)
>  int
>  xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  {
> +	umode_t mode;
> +	bool set_mode = false;
>  	int error = 0;
>  
>  	if (!acl)
> @@ -257,16 +259,24 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  		return error;
>  
>  	if (type == ACL_TYPE_ACCESS) {
> -		umode_t mode;
> -
>  		error = posix_acl_update_mode(inode, &mode, &acl);
>  		if (error)
>  			return error;
> -		error = xfs_set_mode(inode, mode);
> -		if (error)
> -			return error;
> +		set_mode = true;
>  	}
>  
>   set_acl:
> -	return __xfs_set_acl(inode, acl, type);
> +	error =  __xfs_set_acl(inode, acl, type);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * We set the mode after successfully updating the ACL xattr because the
> +	 * xattr update can fail at ENOSPC and we don't want to change the mode
> +	 * if the ACL update hasn't been applied.
> +	 */
> +	if (set_mode)
> +		error = xfs_set_mode(inode, mode);
> +
> +	return error;
>  }