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. 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? Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx 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; } -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
