xfs
[Top] [All Lists]

Re: [PATCH v5 08/10] xfs: add quota id filtering to eofblocks scan

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v5 08/10] xfs: add quota id filtering to eofblocks scan
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Wed, 24 Oct 2012 12:18:58 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20121023014222.GL4291@dastard>
References: <1349446636-8611-1-git-send-email-bfoster@xxxxxxxxxx> <1349446636-8611-9-git-send-email-bfoster@xxxxxxxxxx> <20121023014222.GL4291@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120911 Thunderbird/15.0.1
On 10/22/2012 09:42 PM, Dave Chinner wrote:
> On Fri, Oct 05, 2012 at 10:17:14AM -0400, Brian Foster wrote:
>> Support quota ID filtering in the eofblocks scan. The caller must
>> set the XFS_EOF_FLAGS_QUOTA flags bit, quota ID and quota type. The
>> associated quota type must be enabled on the mounted filesystem.
> 
> I'm wondering if this even needs quota enabled to filter based on a
> uid, gid, or prid?
> 

Not really...

> The quota part of it seem irrelevant to the filtering that is being
> executed - we are only matching against the inode uid/gid/prid, not
> against dquots - and I can see situations where just being able to
> filter on a given uid/gid might be useful in a multi-user
> environment regardless of whether quotas are being used.
> 

This is how my rfc version was implemented (with a comment to the same
effect). I added the explicit checking in response to:

http://oss.sgi.com/archives/xfs/2012-09/msg00024.html

> Indeed, even testing is made much easier if we don't have to
> juggle quota mount options to test the filtering....
> 

I agree. I suppose the check can make a bit of sense if the current
group/proj mode of the mount conflicts with the eofb request, but as
you've outlined above, we're checking against the inode values so I
don't think it introduces a functional problem.

Regardless, could you confirm your reasoning here with regard to the
previous comments before I go and make this change? ;)

>> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
>> index 35efdda..b39970b 100644
>> --- a/fs/xfs/xfs_icache.c
>> +++ b/fs/xfs/xfs_icache.c
>> @@ -1171,6 +1171,22 @@ xfs_reclaim_inodes_count(
>>  }
>>  
>>  STATIC int
>> +xfs_inode_match_quota_id(
>> +    struct xfs_inode        *ip,
>> +    struct xfs_eofblocks    *eofb)
>> +{
>> +    unsigned int type = xfs_quota_type(eofb->eof_q_type);
>> +    if (type == XFS_DQ_USER)
>> +            return ip->i_d.di_uid == eofb->eof_q_id;
>> +    else if (type == XFS_DQ_GROUP)
>> +            return ip->i_d.di_gid == eofb->eof_q_id;
>> +    else if (type == XFS_DQ_PROJ)
>> +            return xfs_get_projid(ip) == eofb->eof_q_id;
>> +
>> +    return 0;
>> +}
> 
> Why do you need xfs_quota_type() here? why not just:
> 
>       switch (type) {
>       case XQM_USRQUOTA:
>               return ip->i_d.di_uid == eofb->eof_q_id;
>       case XQM_GRPQUOTA:
>               return ip->i_d.di_gid == eofb->eof_q_id;
>       case XQM_PRJQUOTA:
>               return xfs_get_projid(ip) == eofb->eof_q_id;
>       default:
>               break;
>       }
> 
>       return 0;
> 

Technically we don't really need xfs_quota_type(). I just thought it
cleaner to use the same mapping we use throughout XFS (i.e., I don't see
those definitions used elsewhere in XFS)...

>> @@ -1194,6 +1211,13 @@ xfs_inode_free_eofblocks(
>>          mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY))
>>              return 0;
>>  
>> +    if (eofb) {
>> +            /* filter by quota id */
>> +            if (eofb->eof_flags & XFS_EOF_FLAGS_QUOTA &&
>> +                !xfs_inode_match_quota_id(ip, eofb))
>> +                    return 0;
>> +    }
>> +
>>      ret = xfs_free_eofblocks(ip->i_mount, ip, true);
>>  
>>      /* don't revisit the inode if we're not waiting */
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index ad4352f..547363b 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -1613,6 +1613,14 @@ xfs_file_ioctl(
>>              if (eofb.eof_version != XFS_EOFBLOCKS_VERSION)
>>                      return -XFS_ERROR(EINVAL);
>>  
>> +            if (eofb.eof_flags & XFS_EOF_FLAGS_QUOTA) {
>> +                    unsigned int type = xfs_quota_type(eofb.eof_q_type);
>> +                    if ((type == XFS_DQ_USER && !XFS_IS_UQUOTA_ON(mp)) ||
>> +                        (type == XFS_DQ_GROUP && !XFS_IS_GQUOTA_ON(mp)) ||
>> +                        (type == XFS_DQ_PROJ && !XFS_IS_PQUOTA_ON(mp)))
>> +                            return -XFS_ERROR(EINVAL);
>> +            }
> 
> Again, I don't really see the reason for needing xfs_quota_type()
> here, and whether the quota check is relevant at all...
> 

Same comment as above... would you prefer I just use the XQM_
definitions and lose the xfs_quota_type() patch?

Brian

> Cheers,
> 
> Dave.
> 

<Prev in Thread] Current Thread [Next in Thread>