Re: [PATCH v4 7/8] xfs: add enhanced filtering to EOFBLOCKS scan

[Brian Foster <bfoster@redhat.com>]

On 09/28/2012 03:53 AM, Dave Chinner wrote:
> On Thu, Sep 27, 2012 at 01:45:51PM -0400, Brian Foster wrote:
>> Support EOFBLOCKS scan filtering by quota ID or minimum file size.
>> Add the appropriate fields/flags to the xfs_eofblocks structure and
>> pass it down to xfs_inode_free_eofblocks() where filtering
>> functionality is implemented.
>>
>> A (user requested) quota ID based scan requires the associated
>> quota mode be enabled.
>>
>> Signed-off-by: Brian Foster <bfoster@redhat.com>
>> ---
>>  fs/xfs/xfs_fs.h    |    9 +++++++++
>>  fs/xfs/xfs_ioctl.c |   10 +++++++++-
>>  fs/xfs/xfs_sync.c  |   30 ++++++++++++++++++++++++++----
>>  fs/xfs/xfs_sync.h  |    2 +-
>>  4 files changed, 45 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
>> index 32bb2e8..54c0f39 100644
>> --- a/fs/xfs/xfs_fs.h
>> +++ b/fs/xfs/xfs_fs.h
>> @@ -343,12 +343,21 @@ typedef struct xfs_error_injection {
>>   */
>>  struct xfs_eofblocks {
>>  	__u32		eof_flags;
>> +	__u32		eof_id;
>> +	__u64		eof_min_file_size;
>>  	__s32		version;
>>  	unsigned char	pad[12];
>>  };
> 
> ACtually, looking at this the version needs to be the first field of
> the structure, so we can guarantee that it can be read by any kernel
> that supports the ioctl regardless of how the rest of the structure
> changes.
> 

Ah, right.

> 
>>  /* eof_flags values */
>>  #define XFS_EOF_FLAGS_FORCE		0x01	/* force/wait mode scan */
>> +#define XFS_EOF_FLAGS_USRID		0x02	/* filter by user id */
>> +#define XFS_EOF_FLAGS_GRPID		0x04	/* filter by group id */
>> +#define XFS_EOF_FLAGS_PROJID		0x08	/* filter by project id */
> 
> I'm wondering if it would be better to pass real quota fields (as
> per dqblk_xfs.h) than make up a new way to pass the same
> information). That way we might be able to use standard quota
> functions rather for checks and comparisons rather than duplicating
> them. That way if we ever add new quota types, we don't have to add
> flags and validation to this ioctl....
> 
> i.e. we have XFS_EOF_FLAGS_QUOTA to say "filter by quota fields",
> similar to the XFS_EOF_FLAGS_MINFILESIZE flag...
> 
> And it becomes much easier to convert to userns kqids that are not
> that far away:
> 
> https://lkml.org/lkml/2012/9/19/587
> 

I think that means I would replace the id (and associated id bits in
the flags field) in the structure to something like:

struct xfs_eofblocks {
	...
	__u32		eof_q_id;
	u8		eof_q_type;
	...
};

... to mirror the values passed through quotactl(). I would check these
only if the aforementioned XFS_EOF_FLAGS_QUOTA bit is set. These
implicitly convert to the qid_t and {USR,GRP,PRJ}QUOTA values used
currently and presumably will convert over nicely to kqid when that hits.

I don't think this changes the code too much. Perhaps I can open up and
make use of xfs_quota_type() as well, and it cleans up the duplication
you referred to in terms of all the quota definitions we have already.

>> +#define XFS_EOF_FLAGS_MINFILESIZE	0x10	/* minimum file size */
>> +
>> +#define XFS_EOF_VALID_QUOTA	(XFS_EOF_FLAGS_USRID|XFS_EOF_FLAGS_GRPID| \
>> +				 XFS_EOF_FLAGS_PROJID)
>>  
>>  
>>  /*
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 216ca7a..a7bf847 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -1609,8 +1609,16 @@ xfs_file_ioctl(
>>  		if (copy_from_user(&eofb, arg, sizeof(eofb)))
>>  			return -XFS_ERROR(EFAULT);
>>  
>> +		if (((eofb.eof_flags & XFS_EOF_FLAGS_USRID) &&
>> +		     !XFS_IS_UQUOTA_ON(mp)) ||
>> +		    ((eofb.eof_flags & XFS_EOF_FLAGS_GRPID) &&
>> +		     !XFS_IS_GQUOTA_ON(mp)) ||
>> +		    ((eofb.eof_flags & XFS_EOF_FLAGS_PROJID) &&
>> +		     !XFS_IS_PQUOTA_ON(mp)))
>> +			return -XFS_ERROR(EINVAL);
>> +
>>  		flags = (eofb.eof_flags & XFS_EOF_FLAGS_FORCE) ? SYNC_WAIT : SYNC_TRYLOCK;
>> -		error = xfs_inodes_free_eofblocks(mp, flags);
>> +		error = xfs_inodes_free_eofblocks(mp, flags, &eofb);
> 
> You probably shoul djust pass the &eofb in the first patch, rather
> than having to change the implementation here again.
> 

I introduce the xfs_inodes_free_eofblocks() call in patch 5 and
xfs_eofblocks in patch 6 for the ioctl support. I could start passing
eofb down in patch 6, but it would be unused unless I replaced the flags
parameter and checked the scan mode based on the eof_flags instead. This
means the background scanner invocation would now pass an xfs_eofblocks
as well (as well as future callers). Thoughts?

If it's just a matter of ordering and patch succinctness, of course, I
can leave the function signature alone and just pass the structure down
unused. ;)

>>  		return -error;
>>  	}
>>  
>> diff --git a/fs/xfs/xfs_sync.c b/fs/xfs/xfs_sync.c
>> index 6854800..c9e1c16 100644
>> --- a/fs/xfs/xfs_sync.c
>> +++ b/fs/xfs/xfs_sync.c
>> @@ -1015,6 +1015,21 @@ xfs_reclaim_inodes_count(
>>  }
>>  
>>  STATIC int
>> +xfs_inode_match_quota_id(
>> +	struct xfs_inode	*ip,
>> +	struct xfs_eofblocks	*eofb)
>> +{
>> +	if (eofb->eof_flags & XFS_EOF_FLAGS_USRID)
>> +		return ip->i_d.di_uid == eofb->eof_id;
>> +	else if (eofb->eof_flags & XFS_EOF_FLAGS_GRPID)
>> +		return ip->i_d.di_gid == eofb->eof_id;
>> +	else if (eofb->eof_flags & XFS_EOF_FLAGS_PROJID)
>> +		return xfs_get_projid(ip) == eofb->eof_id;
>> +
>> +	return 0;
>> +}
>> +
>> +STATIC int
>>  xfs_inode_free_eofblocks(
>>  	struct xfs_inode	*ip,
>>  	struct xfs_perag	*pag,
>> @@ -1022,6 +1037,7 @@ xfs_inode_free_eofblocks(
>>  	void			*args)
>>  {
>>  	int ret;
>> +	struct xfs_eofblocks *eofb = args;
>>  	bool force = flags & SYNC_WAIT;
>>  
>>  	if (!xfs_can_free_eofblocks(ip, false)) {
>> @@ -1031,8 +1047,13 @@ xfs_inode_free_eofblocks(
>>  		return 0;
>>  	}
>>  
>> -	if (!force && mapping_tagged(VFS_I(ip)->i_mapping,
>> -				     PAGECACHE_TAG_DIRTY))
>> +	if ((eofb &&
>> +	     (((eofb->eof_flags & XFS_EOF_VALID_QUOTA) &&
>> +	       !xfs_inode_match_quota_id(ip, eofb)) || 
>> +	      ((eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE) &&
>> +	       (XFS_ISIZE(ip) < eofb->eof_min_file_size)))) ||
>> +	    (!force && mapping_tagged(VFS_I(ip)->i_mapping,
>> +				      PAGECACHE_TAG_DIRTY)))
>>  		return 0;
> 
> Break that up into multiple "if() return 0" statements so it is
> possible to read the logic. ;)
> 

Ok. ;)

Brian

> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs