Re: [PATCH 2/2] xfs: quota: check result of register_shrinker()

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Dec 22, 2017 at 12:48:03AM +0300, Aliaksei Karaliou wrote:
> 
> 
> On 12/22/2017 12:21 AM, Darrick J. Wong wrote:
> >On Thu, Dec 21, 2017 at 11:17:43PM +0300, Aliaksei Karaliou wrote:
> >>xfs_qm_init_quotainfo() does not check result of register_shrinker()
> >>which was tagged as __must_check recently, reported by sparse.
> >>
> >>Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>
> >>---
> >>  fs/xfs/xfs_qm.c | 43 ++++++++++++++++++++++++++++---------------
> >>  1 file changed, 28 insertions(+), 15 deletions(-)
> >>
> >>diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> >>index d0053115427f..b27a019a844d 100644
> >>--- a/fs/xfs/xfs_qm.c
> >>+++ b/fs/xfs/xfs_qm.c
> >>@@ -48,7 +48,7 @@
> >>  STATIC int	xfs_qm_init_quotainos(xfs_mount_t *);
> >>  STATIC int	xfs_qm_init_quotainfo(xfs_mount_t *);
> >>-
> >>+STATIC void	xfs_qm_destroy_quotainos(xfs_quotainfo_t *qi);
> >>  STATIC void	xfs_qm_dqfree_one(struct xfs_dquot *dqp);
> >>  /*
> >>   * We use the batch lookup interface to iterate over the dquots as it
> >>@@ -695,9 +695,17 @@ xfs_qm_init_quotainfo(
> >>  	qinf->qi_shrinker.scan_objects = xfs_qm_shrink_scan;
> >>  	qinf->qi_shrinker.seeks = DEFAULT_SEEKS;
> >>  	qinf->qi_shrinker.flags = SHRINKER_NUMA_AWARE;
> >>-	register_shrinker(&qinf->qi_shrinker);
> >>+
> >>+	error = register_shrinker(&qinf->qi_shrinker);
> >>+	if (error)
> >>+		goto out_free_inos;
> >>+
> >>  	return 0;
> >>+out_free_inos:
> >>+	mutex_destroy(&qinf->qi_quotaofflock);
> >>+	mutex_destroy(&qinf->qi_tree_lock);
> >>+	xfs_qm_destroy_quotainos(qinf);
> >>  out_free_lru:
> >>  	list_lru_destroy(&qinf->qi_lru);
> >>  out_free_qinf:
> >>@@ -706,6 +714,23 @@ xfs_qm_init_quotainfo(
> >>  	return error;
> >>  }
> >>+STATIC void
> >>+xfs_qm_destroy_quotainos(
> >>+	xfs_quotainfo_t	*qi)
> >>+{
> >>+	if (qi->qi_uquotaip) {
> >>+		IRELE(qi->qi_uquotaip);
> >>+		qi->qi_uquotaip = NULL; /* paranoia */
> >>+	}
> >>+	if (qi->qi_gquotaip) {
> >>+		IRELE(qi->qi_gquotaip);
> >>+		qi->qi_gquotaip = NULL;
> >>+	}
> >>+	if (qi->qi_pquotaip) {
> >>+		IRELE(qi->qi_pquotaip);
> >>+		qi->qi_pquotaip = NULL;
> >>+	}
> >>+}
> >This ought to be nearby xfs_qm_init_quotainfo to keep the init/destroy
> >functions together, but otherwise looks ok, will test:
> >
> >Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> >
> >--D
> Yes, I also thought about this, but I'm not sure how to do better.
> On the one hand, I wanted to minimize code duplication and so didn't
> copy all this code to the error handling part.
> 
> On the other hand, the fact that function will not be inlined reduces
> the number of trace points created by IRELE (is it better or not ?).
> Plus, the same code exists in xfs_qm_unmount_quotas().

I might be misinterpreting you here, but I'm fine with having a separate
helper function, particularly since there's already a helper to grab the
inodes.  I was merely commenting on the placement, since there's already
a declaration at the top of the file.

> I'm ready to move this function closer to xfs_qm_init_quotainos() and
> resend patches.

Already took care of it, but thank you!

--D

> Best regards,
>     Aliaksei.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html