xfs: quota: fix missed destroy of qi_tree_lock

PATCH In review Aliaksei Karaliou 2 participants 6 messages Dec 21, 2017 XFS filesystem

lore

Share & tools

lore

Aliaksei Karaliou

author

2017-12-21 20:17 UTC #0 lore raw

PATCH 1/2 xfs: quota: fix missed destroy of qi_tree_lock

xfs_qm_destroy_quotainfo() does not destroy quotainfo->qi_tree_lock while destroys quotainfo->qi_quotaofflock.

Signed-off-by Aliaksei Karaliou <[email protected]>

---
 fs/xfs/xfs_qm.c | 1 +
 1 file changed, 1 insertion(+)

fs/xfs/xfs_qm.c +1 -0

index ec952dfad359..d0053115427f 100644
@@ -736,6 +736,7 @@ xfs_qm_destroy_quotainfo(
 		IRELE(qi->qi_pquotaip);
 		qi->qi_pquotaip = NULL;
 	}
+	mutex_destroy(&qi->qi_tree_lock);
 	mutex_destroy(&qi->qi_quotaofflock);
 	kmem_free(qi);
 	mp->m_quotainfo = NULL;

Signature · 1 line

2.11.0

Aliaksei Karaliou

author

2017-12-21 20:18 UTC #1 lore raw

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

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 <[email protected]>

---
 fs/xfs/xfs_qm.c | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

fs/xfs/xfs_qm.c +28 ~~-15~~

index d0053115427f..b27a019a844d 100644
@@ -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;
+	}
+}
 
 /*
  * Gets called when unmounting a filesystem or when all quotas get

@@ -723,19 +748,7 @@ xfs_qm_destroy_quotainfo(
 
 	unregister_shrinker(&qi->qi_shrinker);
 	list_lru_destroy(&qi->qi_lru);
-
-	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;
-	}
+	xfs_qm_destroy_quotainos(qi);
 	mutex_destroy(&qi->qi_tree_lock);
 	mutex_destroy(&qi->qi_quotaofflock);
 	kmem_free(qi);

Signature · 1 line

2.11.0

Darrick J. Wong

Reviewed-by on v1 2/2

2017-12-21 21:26 UTC #2 lore raw

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

Quoted 51 lines · Aliaksei Karaliou

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 <[email protected]>

---
 fs/xfs/xfs_qm.c | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

fs/xfs/xfs_qm.c +17 -2

index d0053115427f..b27a019a844d 100644
@@ -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 */

fs/xfs/xfs_qm.c xfs_qm_init_quotainfo( · line 733

Quoted 10 lines

fs/xfs/xfs_qm.c +10 -0

+	}
+	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 <[email protected]>

--D

Quoted 31 lines

fs/xfs/xfs_qm.c +1 ~~-13~~

 /*
  * Gets called when unmounting a filesystem or when all quotas get

@@ -723,19 +748,7 @@ xfs_qm_destroy_quotainfo(
 
 	unregister_shrinker(&qi->qi_shrinker);
 	list_lru_destroy(&qi->qi_lru);
-
-	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;
-	}
+	xfs_qm_destroy_quotainos(qi);
 	mutex_destroy(&qi->qi_tree_lock);
 	mutex_destroy(&qi->qi_quotaofflock);
 	kmem_free(qi);

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Aliaksei Karaliou

author

2017-12-21 21:48 UTC #3 lore raw

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

Quoted 65 lines · Darrick J. Wong

Quoted 51 lines · Aliaksei Karaliou

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 <[email protected]>

---
  fs/xfs/xfs_qm.c | 43 ++++++++++++++++++++++++++++---------------
  1 file changed, 28 insertions(+), 15 deletions(-)

fs/xfs/xfs_qm.c +17 -2

index d0053115427f..b27a019a844d 100644
@@ -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 */

fs/xfs/xfs_qm.c xfs_qm_init_quotainfo( · line 733

Quoted 10 lines

fs/xfs/xfs_qm.c +10 -0

+	}
+	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:

Quoted 3 lines

Reviewed-by Darrick J. Wong <[email protected]>
--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'm ready to move this function closer to xfs_qm_init_quotainos() and resend patches.

Best regards,
Aliaksei.

Darrick J. Wong

2017-12-21 22:04 UTC #4 lore raw

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

Quoted 67 lines · Aliaksei Karaliou

Quoted 64 lines · Darrick J. Wong

Quoted 48 lines · Aliaksei Karaliou

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 <[email protected]>

---
 fs/xfs/xfs_qm.c | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

fs/xfs/xfs_qm.c +17 -2

index d0053115427f..b27a019a844d 100644
@@ -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 */

fs/xfs/xfs_qm.c xfs_qm_init_quotainfo( · line 732

Quoted 10 lines

fs/xfs/xfs_qm.c +10 -0

+	}
+	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 <[email protected]>

Quoted 8 lines

Quoted 1 line
--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.

Quoted 2 lines

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

Quoted 7 lines

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

Darrick J. Wong

Reviewed-by on v1 1/2

2017-12-21 21:21 UTC #5 lore raw

In reply to Aliaksei Karaliou

Quoted 4 lines · Aliaksei Karaliou

xfs_qm_destroy_quotainfo() does not destroy quotainfo->qi_tree_lock while destroys quotainfo->qi_quotaofflock.
Signed-off-by Aliaksei Karaliou <[email protected]>

Looks ok,

Reviewed-by Darrick J. Wong <[email protected]>

Quoted 23 lines

---
 fs/xfs/xfs_qm.c | 1 +
 1 file changed, 1 insertion(+)

fs/xfs/xfs_qm.c +1 -0

index ec952dfad359..d0053115427f 100644
@@ -736,6 +736,7 @@ xfs_qm_destroy_quotainfo(
 		IRELE(qi->qi_pquotaip);
 		qi->qi_pquotaip = NULL;
 	}
+	mutex_destroy(&qi->qi_tree_lock);
 	mutex_destroy(&qi->qi_quotaofflock);
 	kmem_free(qi);
 	mp->m_quotainfo = NULL;

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html