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

On Fri, Dec 22, 2017 at 12:48:03AM +0300, Aliaksei Karaliou wrote:

On 12/22/2017 12:21 AM, Darrick J. Wong wrote:

quoted

On Thu, Dec 21, 2017 at 11:17:43PM +0300, Aliaksei Karaliou wrote:

quoted

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 <redacted>
---
 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 <redacted>

--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