Re: [PATCH] xfs: short circuit xfs_get_acl() if no acl is possible

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, May 09, 2019 at 09:05:39AM -0400, Brian Foster wrote:
> 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@redhat.com>
> > > Suggested-by: Dave Chinner <david@fromorbit.com>
> > > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > > ---
> > > 
> > > 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...

<shrug> But xfs_inode_hasattr checks forkoff > 0, so as long as the
shortform to leaf conversion doesn't zero forkoff we'd be fine, I think.
AFAICT it doesn't...?

--D

> Brian
> 
> > --D
> > 
> > > +
> > >  	/*
> > >  	 * If we have a cached ACLs value just return it, not need to
> > >  	 * go out to the disk.
> > >