diff mbox series

[v3,6/6] xfs: kill ialloced in xfs_dialloc()

Message ID 20201207001533.2702719-7-hsiangkao@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series xfs: some xfs_dialloc() cleanup | expand

Commit Message

Gao Xiang Dec. 7, 2020, 12:15 a.m. UTC 
It's enough to just use return code, and get rid of an argument.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/libxfs/xfs_ialloc.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

Comments

Christoph Hellwig Dec. 7, 2020, 1:57 p.m. UTC | #1 
> +		error = xfs_ialloc_ag_alloc(*tpp, agbp);
> +		if (error < 0) {
>  			xfs_trans_brelse(*tpp, agbp);
>  
>  			if (error == -ENOSPC)
>  				error = 0;
>  			break;
> +		} else if (error == 0) {

No need for the else after the break.
Gao Xiang Dec. 7, 2020, 2:24 p.m. UTC | #2 
On Mon, Dec 07, 2020 at 02:57:19PM +0100, Christoph Hellwig wrote:
> > +		error = xfs_ialloc_ag_alloc(*tpp, agbp);
> > +		if (error < 0) {
> >  			xfs_trans_brelse(*tpp, agbp);
> >  
> >  			if (error == -ENOSPC)
> >  				error = 0;
> >  			break;
> > +		} else if (error == 0) {
> 
> No need for the else after the break.

Personally, I'd like to save a line by using "} else if {"
for such case (and tell readers about these two judgments),
and for any cases, compilers will do their best.

Yet, if you like, I could add an extra line for this.
Will update in the next version.

Thanks,
Gao Xiang

>
Dave Chinner Dec. 7, 2020, 8:23 p.m. UTC | #3 
On Mon, Dec 07, 2020 at 10:24:48PM +0800, Gao Xiang wrote:
> On Mon, Dec 07, 2020 at 02:57:19PM +0100, Christoph Hellwig wrote:
> > > +		error = xfs_ialloc_ag_alloc(*tpp, agbp);
> > > +		if (error < 0) {
> > >  			xfs_trans_brelse(*tpp, agbp);
> > >  
> > >  			if (error == -ENOSPC)
> > >  				error = 0;
> > >  			break;
> > > +		} else if (error == 0) {
> > 
> > No need for the else after the break.
> 
> Personally, I'd like to save a line by using "} else if {"
> for such case (and tell readers about these two judgments),
> and for any cases, compilers will do their best.

And extra line is not an issue, and the convention we use everywhere
is to elide the "else" whereever possible. e.g. we do:

	if (foo)
		return false;
	if (!bar)
		return true;
	if (baz)
		return false;
	return true;

Rather than if() {} else if() {} else if() {} else {}. The elses in
these cases mainly obfuscate the actual logic flow...

Cheers,

Dave.
Gao Xiang Dec. 7, 2020, 10:03 p.m. UTC | #4 
On Tue, Dec 08, 2020 at 07:23:45AM +1100, Dave Chinner wrote:
> On Mon, Dec 07, 2020 at 10:24:48PM +0800, Gao Xiang wrote:
> > On Mon, Dec 07, 2020 at 02:57:19PM +0100, Christoph Hellwig wrote:
> > > > +		error = xfs_ialloc_ag_alloc(*tpp, agbp);
> > > > +		if (error < 0) {
> > > >  			xfs_trans_brelse(*tpp, agbp);
> > > >  
> > > >  			if (error == -ENOSPC)
> > > >  				error = 0;
> > > >  			break;
> > > > +		} else if (error == 0) {
> > > 
> > > No need for the else after the break.
> > 
> > Personally, I'd like to save a line by using "} else if {"
> > for such case (and tell readers about these two judgments),
> > and for any cases, compilers will do their best.
> 
> And extra line is not an issue, and the convention we use everywhere
> is to elide the "else" whereever possible. e.g. we do:
> 
> 	if (foo)
> 		return false;
> 	if (!bar)
> 		return true;
> 	if (baz)
> 		return false;
> 	return true;
> 
> Rather than if() {} else if() {} else if() {} else {}. The elses in
> these cases mainly obfuscate the actual logic flow...

(I mean no need to to use else if on irrelevant relationship as well)
Anyway, let me update it later...

Thanks,
Gao Xiang

> 
> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
>
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 527f17f09bd3..a0e6e333eea2 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -607,13 +607,13 @@  xfs_inobt_insert_sprec(
 
 /*
  * Allocate new inodes in the allocation group specified by agbp.
- * Return 0 for success, else error code.
+ * Returns 0 if inodes were allocated in this AG; 1 if there was no space
+ * in this AG; or the usual negative error code.
  */
 STATIC int
 xfs_ialloc_ag_alloc(
 	struct xfs_trans	*tp,
-	struct xfs_buf		*agbp,
-	int			*alloc)
+	struct xfs_buf		*agbp)
 {
 	struct xfs_agi		*agi;
 	struct xfs_alloc_arg	args;
@@ -795,10 +795,9 @@  xfs_ialloc_ag_alloc(
 		allocmask = (1 << (newlen / XFS_INODES_PER_HOLEMASK_BIT)) - 1;
 	}
 
-	if (args.fsbno == NULLFSBLOCK) {
-		*alloc = 0;
-		return 0;
-	}
+	if (args.fsbno == NULLFSBLOCK)
+		return 1;
+
 	ASSERT(args.len == args.minlen);
 
 	/*
@@ -903,7 +902,6 @@  xfs_ialloc_ag_alloc(
 	 */
 	xfs_trans_mod_sb(tp, XFS_TRANS_SB_ICOUNT, (long)newlen);
 	xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, (long)newlen);
-	*alloc = 1;
 	return 0;
 }
 
@@ -1749,7 +1747,6 @@  xfs_dialloc_select_ag(
 	struct xfs_buf		*agbp;
 	xfs_agnumber_t		agno;
 	int			error;
-	int			ialloced;
 	bool			noroom = false;
 	xfs_agnumber_t		start_agno;
 	struct xfs_perag	*pag;
@@ -1823,17 +1820,14 @@  xfs_dialloc_select_ag(
 		if (!okalloc)
 			goto nextag_relse_buffer;
 
-
-		error = xfs_ialloc_ag_alloc(*tpp, agbp, &ialloced);
-		if (error) {
+		error = xfs_ialloc_ag_alloc(*tpp, agbp);
+		if (error < 0) {
 			xfs_trans_brelse(*tpp, agbp);
 
 			if (error == -ENOSPC)
 				error = 0;
 			break;
-		}
-
-		if (ialloced) {
+		} else if (error == 0) {
 			/*
 			 * We successfully allocated some inodes, so roll the
 			 * transaction and return the locked AGI buffer to the