Re: [PATCH] btrfs: preserve i_mode if __btrfs_set_acl() fails

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

On Sat, Jul 29, 2017 at 12:48:04AM +0000, Josef Bacik wrote:
> On Fri, Jul 28, 2017 at 09:26:29PM -0300, Ernesto A. Fernández wrote:
> > +	ret = __btrfs_set_acl(trans, inode, acl, type);
> > +	if (ret)
> > +		goto out;
> > +
> > +	inode->i_mode = mode;
> > +	inode_inc_iversion(inode);
> > +	inode->i_ctime = current_time(inode);
> > +	set_bit(BTRFS_INODE_COPY_EVERYTHING, &BTRFS_I(inode)->runtime_flags);
> 
> This only needs to be set if we actually set the xattr.  I'd fix setxattr to
> call it every time it's called.

I had not thought of that, thank you.

If I'm understanding this correctly the issue would be only when setting
a NULL default acl on an inode that is not a directory. In that case I
probably shouldn't be calling btrfs_update_inode either, but I can't move
that back to setxattr.

Perhaps __btrfs_set_acl could return an error in that case, like -ENOTDIR,
and then we can set ret back to 0 before returning from btrfs_set_acl.

> > +	ret = btrfs_update_inode(trans, root, inode);
> > +	BUG_ON(ret);
> 
> No BUG_ON, return the error.

The call to BUG_ON was already there before my patch, only inside the 
__btrfs_setxattr function. Since I didn't know the reason I thought it
was best not to change it. I'll do as you say in the next version.

Thank you for your review.