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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs