On Wed, May 08, 2019 at 01:10:33PM -0700, Darrick J. Wong wrote: > On Wed, May 08, 2019 at 02:28:09PM -0500, Eric Sandeen wrote: > > If there are no attributes on the inode, don't go through the > > cost of memory allocation and callling xfs_attr_get when we > > already know we'll just get -ENOATTR. > > > > Reported-by: David Valin <dvalin@xxxxxxxxxx> > > Suggested-by: Dave Chinner <david@xxxxxxxxxxxxx> > > Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx> > > --- > > > > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c > > index 8039e35147dd..b469b44e9e71 100644 > > --- a/fs/xfs/xfs_acl.c > > +++ b/fs/xfs/xfs_acl.c > > @@ -132,6 +132,9 @@ xfs_get_acl(struct inode *inode, int type) > > BUG(); > > } > > > > + if (!xfs_inode_hasattr(ip)) > > + return NULL; > > This isn't going to cause problems if someone's adding an ACL to the > inode at the same time, right? > > I'm assuming that's the case since we only would load inodes when > setting up a vfs inode but before any userspace can get its sticky > fingers all over the inode, but it sure would be nice to know that > for sure. :) > Hmm, that's a good question. At first I was thinking it wouldn't matter, but then I remembered the fairly recent issue around writing back an empty leaf buffer on format conversion a bit too early. That has me wondering if that would be an issue here as well. For example, suppose a non-empty local format attr fork is being converted to extent format due to a concurrent (and unrelated) xattr set. That involves xfs_attr_shortform_to_leaf() -> xfs_bmap_local_to_extents_empty(), which looks like it creates a transient empty fork state. Might xfs_inode_hasattr() catch that as a false negative here? If so, that would certainly be a problem if the existing xattr was the ACL the caller happens to be interested in. It might be prudent to surround this check with ILOCK_SHARED... Brian > --D > > > + > > /* > > * If we have a cached ACLs value just return it, not need to > > * go out to the disk. > >
