From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f65.google.com ([209.85.215.65]:35505 "EHLO mail-lf0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752678AbdLUVsH (ORCPT ); Thu, 21 Dec 2017 16:48:07 -0500 Received: by mail-lf0-f65.google.com with SMTP id j124so29471244lfg.2 for ; Thu, 21 Dec 2017 13:48:06 -0800 (PST) Subject: Re: [PATCH 2/2] xfs: quota: check result of register_shrinker() References: <20171221201743.18016-1-akaraliou.dev@gmail.com> <20171221201743.18016-2-akaraliou.dev@gmail.com> <20171221212126.GX12613@magnolia> From: Aliaksei Karaliou Message-ID: Date: Fri, 22 Dec 2017 00:48:03 +0300 MIME-Version: 1.0 In-Reply-To: <20171221212126.GX12613@magnolia> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: "Darrick J. Wong" Cc: linux-xfs@vger.kernel.org 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 >> --- >> 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 > > --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.