On 6/26/19 1:12 PM, Darrick J. Wong wrote:
> 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@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...
>
> <shrug> But xfs_inode_hasattr checks forkoff > 0, so as long as the
It does do that ...
int
xfs_inode_hasattr(
struct xfs_inode *ip)
{
if (!XFS_IFORK_Q(ip) ||
> shortform to leaf conversion doesn't zero forkoff we'd be fine, I think.
> AFAICT it doesn't...?
but there's that pesky || part :
(ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
ip->i_d.di_anextents == 0))
return 0;
return 1;
}
and I think it's the latter state Brian was concerned about?
I can play with sandwiching it in a shared lock...
-Eric
