Re: [PATCH v3 5/6] xfs: spilt xfs_dialloc() into 2 functions

From: Gao Xiang <hsiangkao@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	Dave Chinner <david@fromorbit.com>,
	Eric Sandeen <sandeen@sandeen.net>,
	Dave Chinner <dchinner@redhat.com>
Subject: Re: [PATCH v3 5/6] xfs: spilt xfs_dialloc() into 2 functions
Date: Mon, 7 Dec 2020 22:33:00 +0800	[thread overview]
Message-ID: <20201207143300.GE2817641@xiangao.remote.csb> (raw)
In-Reply-To: <20201207135642.GF29249@lst.de>

Hi Christoph,

On Mon, Dec 07, 2020 at 02:56:42PM +0100, Christoph Hellwig wrote:
> >  		if (pag->pagi_freecount) {
> >  			xfs_perag_put(pag);
> > +			*IO_agbp = agbp;
> > +			return 0;
> 
> I think assigning *IO_agbp would benefit from a little consolidation.
> Set it to NULL in the normal unsuccessful return, and add a found_ag
> label that assigns agbp and returns 0.

Just to confirm the main idea, I think it might be:

*IO_agbp = NULL;  at first,

and combine all such assignment
> > +			*IO_agbp = agbp;
> > +			return 0;
>

into a new found_ag lebel, and use goto found_ag; for such cases.
Do I understand correctly? If that is correct, will update
in the next version.

Thanks,
Gao Xiang

> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 