* [PATCH 1/2] xfs: quota: fix missed destroy of qi_tree_lock
@ 2017-12-21 20:17 Aliaksei Karaliou
2017-12-21 20:17 ` [PATCH 2/2] xfs: quota: check result of register_shrinker() Aliaksei Karaliou
2017-12-21 21:21 ` [PATCH 1/2] xfs: quota: fix missed destroy of qi_tree_lock Darrick J. Wong
0 siblings, 2 replies; 6+ messages in thread
From: Aliaksei Karaliou @ 2017-12-21 20:17 UTC (permalink / raw)
To: darrick.wong; +Cc: Aliaksei Karaliou, linux-xfs
xfs_qm_destroy_quotainfo() does not destroy quotainfo->qi_tree_lock
while destroys quotainfo->qi_quotaofflock.
Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>
---
fs/xfs/xfs_qm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index ec952dfad359..d0053115427f 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -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;
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] xfs: quota: check result of register_shrinker()
2017-12-21 20:17 [PATCH 1/2] xfs: quota: fix missed destroy of qi_tree_lock Aliaksei Karaliou
@ 2017-12-21 20:17 ` Aliaksei Karaliou
2017-12-21 21:21 ` Darrick J. Wong
2017-12-21 21:21 ` [PATCH 1/2] xfs: quota: fix missed destroy of qi_tree_lock Darrick J. Wong
1 sibling, 1 reply; 6+ messages in thread
From: Aliaksei Karaliou @ 2017-12-21 20:17 UTC (permalink / raw)
To: darrick.wong; +Cc: Aliaksei Karaliou, linux-xfs
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;
+ }
+}
/*
* 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);
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] xfs: quota: check result of register_shrinker()
2017-12-21 20:17 ` [PATCH 2/2] xfs: quota: check result of register_shrinker() Aliaksei Karaliou
@ 2017-12-21 21:21 ` Darrick J. Wong
2017-12-21 21:48 ` Aliaksei Karaliou
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2017-12-21 21:21 UTC (permalink / raw)
To: Aliaksei Karaliou; +Cc: linux-xfs
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
>
> /*
> * 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);
> --
> 2.11.0
>
> --
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] xfs: quota: fix missed destroy of qi_tree_lock
2017-12-21 20:17 [PATCH 1/2] xfs: quota: fix missed destroy of qi_tree_lock Aliaksei Karaliou
2017-12-21 20:17 ` [PATCH 2/2] xfs: quota: check result of register_shrinker() Aliaksei Karaliou
@ 2017-12-21 21:21 ` Darrick J. Wong
1 sibling, 0 replies; 6+ messages in thread
From: Darrick J. Wong @ 2017-12-21 21:21 UTC (permalink / raw)
To: Aliaksei Karaliou; +Cc: linux-xfs
On Thu, Dec 21, 2017 at 11:17:42PM +0300, Aliaksei Karaliou wrote:
> xfs_qm_destroy_quotainfo() does not destroy quotainfo->qi_tree_lock
> while destroys quotainfo->qi_quotaofflock.
>
> Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/xfs_qm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index ec952dfad359..d0053115427f 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -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;
> --
> 2.11.0
>
> --
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] xfs: quota: check result of register_shrinker()
2017-12-21 21:21 ` Darrick J. Wong
@ 2017-12-21 21:48 ` Aliaksei Karaliou
2017-12-21 21:59 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Aliaksei Karaliou @ 2017-12-21 21:48 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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'm ready to move this function closer to xfs_qm_init_quotainos() and
resend patches.
Best regards,
Aliaksei.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] xfs: quota: check result of register_shrinker()
2017-12-21 21:48 ` Aliaksei Karaliou
@ 2017-12-21 21:59 ` Darrick J. Wong
0 siblings, 0 replies; 6+ messages in thread
From: Darrick J. Wong @ 2017-12-21 21:59 UTC (permalink / raw)
To: Aliaksei Karaliou; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-21 22:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-21 20:17 [PATCH 1/2] xfs: quota: fix missed destroy of qi_tree_lock Aliaksei Karaliou
2017-12-21 20:17 ` [PATCH 2/2] xfs: quota: check result of register_shrinker() Aliaksei Karaliou
2017-12-21 21:21 ` Darrick J. Wong
2017-12-21 21:48 ` Aliaksei Karaliou
2017-12-21 21:59 ` Darrick J. Wong
2017-12-21 21:21 ` [PATCH 1/2] xfs: quota: fix missed destroy of qi_tree_lock Darrick J. Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).