xfs: short circuit xfs_get_acl() if no acl is possible

Commit Message

Eric Sandeen May 8, 2019, 7:28 p.m. UTC

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 <[e-mail]>
Suggested-by: Dave Chinner <[e-mail]>
Signed-off-by: Eric Sandeen <[e-mail]>
---

Comments

Darrick J. Wong May 8, 2019, 8:10 p.m. UTC | #1

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 <[e-mail]>
> Suggested-by: Dave Chinner <[e-mail]>
> Signed-off-by: Eric Sandeen <[e-mail]>
> ---
> 
> 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. :)

--D

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

Brian Foster May 9, 2019, 1:05 p.m. UTC | #2

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 <[e-mail]>
> > Suggested-by: Dave Chinner <[e-mail]>
> > Signed-off-by: Eric Sandeen <[e-mail]>
> > ---
> > 
> > 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.
> >

Darrick J. Wong June 26, 2019, 6:12 p.m. UTC | #3

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 <[e-mail]>
> > > Suggested-by: Dave Chinner <[e-mail]>
> > > Signed-off-by: Eric Sandeen <[e-mail]>
> > > ---
> > > 
> > > 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.
> > >

Eric Sandeen June 26, 2019, 6:16 p.m. UTC | #4

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 <[e-mail]>
>>>> Suggested-by: Dave Chinner <[e-mail]>
>>>> Signed-off-by: Eric Sandeen <[e-mail]>
>>>> ---
>>>>
>>>> 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

Brian Foster July 1, 2019, 6:52 p.m. UTC | #5

On Wed, Jun 26, 2019 at 01:16:27PM -0500, Eric Sandeen wrote:
> 
> 
> 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 <[e-mail]>
> >>>> Suggested-by: Dave Chinner <[e-mail]>
> >>>> Signed-off-by: Eric Sandeen <[e-mail]>
> >>>> ---
> >>>>
> >>>> 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?
> 

Yep, pretty much.

Brian

> I can play with sandwiching it in a shared lock...
> 
> -Eric
>

Dave Chinner July 2, 2019, 10:31 p.m. UTC | #6

On Mon, Jul 01, 2019 at 02:52:07PM -0400, Brian Foster wrote:
> On Wed, Jun 26, 2019 at 01:16:27PM -0500, Eric Sandeen wrote:
> > 
> > 
> > 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 <[e-mail]>
> > >>>> Suggested-by: Dave Chinner <[e-mail]>
> > >>>> Signed-off-by: Eric Sandeen <[e-mail]>
> > >>>> ---
> > >>>>
> > >>>> 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?
> > 
> 
> Yep, pretty much.

/me needs to uncover the "drive allocation into attr code" patch he
wrote so this "noattr == no allocation" hack isn't necessary....

Cheers,

Dave.

Patch

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;
+
 	/*
 	 * If we have a cached ACLs value just return it, not need to
 	 * go out to the disk.