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

[Brian Foster <bfoster@redhat.com>]

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...

Brian

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