Re: [PATCH 6/7] xfs: use generic inode iterator in xfs_qm_dqrele_all_inodes

Eric Sandeen
2009-06-05T20:11:51Z

Christoph Hellwig wrote:
> On Fri, Jun 05, 2009 at 02:15:38PM -0500, Eric Sandeen wrote:
>> Does this need the same error propagation treatment as 5/7 ?
>
> Yes, I've already fixed this up in my local copy:
>
> Subject: xfs: use generic inode iterator in xfs_qm_dqrele_all_inodes
> From: Christoph Hellwig <hch@lst.de>
>
> Use xfs_inode_ag_iterator instead of opencoding the inode walk in the
> quota code. Mark xfs_inode_ag_iterator and xfs_sync_inode_valid non-static
> to allow using them from the quota code.
>
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Josef 'Jeff' Sipek <jeffpc@josefsipek.net>

Ok, this looks fine to me.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

>
> Index: xfs/fs/xfs/quota/xfs_qm_syscalls.c
> ===================================================================
> --- xfs.orig/fs/xfs/quota/xfs_qm_syscalls.c 2009-06-04 12:47:30.742821242 +0200
> +++ xfs/fs/xfs/quota/xfs_qm_syscalls.c 2009-06-04 13:03:51.039832673 +0200
> @@ -847,105 +847,55 @@ xfs_qm_export_flags(
> }
>
>
> -/*
> - * Release all the dquots on the inodes in an AG.
> - */
> -STATIC void
> -xfs_qm_dqrele_inodes_ag(
> - xfs_mount_t *mp,
> - int ag,
> - uint flags)
> +STATIC int
> +xfs_dqrele_inode(
> + struct xfs_inode *ip,
> + struct xfs_perag *pag,
> + int flags)
> {
> - xfs_inode_t *ip = NULL;
> - xfs_perag_t *pag = &mp->m_perag[ag];
> - int first_index = 0;
> - int nr_found;
> -
> - do {
> - /*
> - * use a gang lookup to find the next inode in the tree
> - * as the tree is sparse and a gang lookup walks to find
> - * the number of objects requested.
> - */
> - read_lock(&pag->pag_ici_lock);
> - nr_found = radix_tree_gang_lookup(&pag->pag_ici_root,
> - (void**)&ip, first_index, 1);
> -
> - if (!nr_found) {
> - read_unlock(&pag->pag_ici_lock);
> - break;
> - }
> -
> - /*
> - * Update the index for the next lookup. Catch overflows
> - * into the next AG range which can occur if we have inodes
> - * in the last block of the AG and we are currently
> - * pointing to the last inode.
> - */
> - first_index = XFS_INO_TO_AGINO(mp, ip->i_ino + 1);
> - if (first_index < XFS_INO_TO_AGINO(mp, ip->i_ino)) {
> - read_unlock(&pag->pag_ici_lock);
> - break;
> - }
> -
> - /* skip quota inodes */
> - if (ip == XFS_QI_UQIP(mp) || ip == XFS_QI_GQIP(mp)) {
> - ASSERT(ip->i_udquot == NULL);
> - ASSERT(ip->i_gdquot == NULL);
> - read_unlock(&pag->pag_ici_lock);
> - continue;
> - }
> + int error;
>
> - /*
> - * If we can't get a reference on the inode, it must be
> - * in reclaim. Leave it for the reclaim code to flush.
> - */
> - if (!igrab(VFS_I(ip))) {
> - read_unlock(&pag->pag_ici_lock);
> - continue;
> - }
> + /* skip quota inodes */
> + if (ip == XFS_QI_UQIP(ip->i_mount) || ip == XFS_QI_GQIP(ip->i_mount)) {
> + ASSERT(ip->i_udquot == NULL);
> + ASSERT(ip->i_gdquot == NULL);
> read_unlock(&pag->pag_ici_lock);
> + return 0;
> + }
>
> - /* avoid new inodes though we shouldn't find any here */
> - if (xfs_iflags_test(ip, XFS_INEW)) {
> - IRELE(ip);
> - continue;
> - }
> + error = xfs_sync_inode_valid(ip, pag);
> + if (error)
> + return error;
>
> - xfs_ilock(ip, XFS_ILOCK_EXCL);
> - if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
> - xfs_qm_dqrele(ip->i_udquot);
> - ip->i_udquot = NULL;
> - }
> - if (flags & (XFS_PQUOTA_ACCT|XFS_GQUOTA_ACCT) &&
> - ip->i_gdquot) {
> - xfs_qm_dqrele(ip->i_gdquot);
> - ip->i_gdquot = NULL;
> - }
> - xfs_iput(ip, XFS_ILOCK_EXCL);
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> + if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
> + xfs_qm_dqrele(ip->i_udquot);
> + ip->i_udquot = NULL;
> + }
> + if (flags & (XFS_PQUOTA_ACCT|XFS_GQUOTA_ACCT) && ip->i_gdquot) {
> + xfs_qm_dqrele(ip->i_gdquot);
> + ip->i_gdquot = NULL;
> + }
> + xfs_iput(ip, XFS_ILOCK_EXCL);
> + IRELE(ip);
>
> - } while (nr_found);
> + return 0;
> }
>
> +
> /*
> * Go thru all the inodes in the file system, releasing their dquots.
> + *
> * Note that the mount structure gets modified to indicate that quotas are off
> - * AFTER this, in the case of quotaoff. This also gets called from
> - * xfs_rootumount.
> + * AFTER this, in the case of quotaoff.
> */
> void
> xfs_qm_dqrele_all_inodes(
> struct xfs_mount *mp,
> uint flags)
> {
> - int i;
> -
> ASSERT(mp->m_quotainfo);
> - for (i = 0; i < mp->m_sb.sb_agcount; i++) {
> - if (!mp->m_perag[i].pag_ici_init)
> - continue;
> - xfs_qm_dqrele_inodes_ag(mp, i, flags);
> - }
> + xfs_inode_ag_iterator(mp, xfs_dqrele_inode, flags, -1);
> }
>
> /*------------------------------------------------------------------------*/
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c 2009-06-04 13:01:29.388941822 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c 2009-06-04 13:02:26.249965001 +0200
> @@ -141,7 +141,7 @@ restart:
> return last_error;
> }
>
> -STATIC int
> +int
> xfs_inode_ag_iterator(
> struct xfs_mount *mp,
> int (*execute)(struct xfs_inode *ip,
> @@ -167,7 +167,7 @@ xfs_inode_ag_iterator(
> }
>
> /* must be called with pag_ici_lock held and releases it */
> -STATIC int
> +int
> xfs_sync_inode_valid(
> struct xfs_inode *ip,
> struct xfs_perag *pag)
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h 2009-06-04 12:47:30.804939977 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.h 2009-06-04 13:02:26.249965001 +0200
> @@ -54,4 +54,10 @@ void xfs_inode_set_reclaim_tag(struct xf
> void xfs_inode_clear_reclaim_tag(struct xfs_inode *ip);
> void __xfs_inode_clear_reclaim_tag(struct xfs_mount *mp, struct xfs_perag *pag,
> struct xfs_inode *ip);
> +
> +int xfs_sync_inode_valid(struct xfs_inode *ip, struct xfs_perag *pag);
> +int xfs_inode_ag_iterator(struct xfs_mount *mp,
> + int (*execute)(struct xfs_inode *ip, struct xfs_perag *pag, int flags),
> + int flags, int tag);
> +
> #endif
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs