| Message ID | 20201124155130.40848-2-hsiangkao@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] xfs: convert noroom, okalloc in xfs_dialloc() to bool | expand |
On Tue, Nov 24, 2020 at 11:51:29PM +0800, Gao Xiang wrote: > It's enough to just use return code, and get rid of an argument. > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > --- > fs/xfs/libxfs/xfs_ialloc.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c > index 45cf7e55f5ee..5c8b0210aad3 100644 > --- a/fs/xfs/libxfs/xfs_ialloc.c > +++ b/fs/xfs/libxfs/xfs_ialloc.c > @@ -607,13 +607,14 @@ xfs_inobt_insert_sprec( > > /* > * Allocate new inodes in the allocation group specified by agbp. > - * Return 0 for success, else error code. > + * Return 0 for successfully allocating some inodes in this AG; > + * 1 for skipping to allocating in the next AG; s/for/when/ for both lines I think. > + * < 0 for error code. and < 0 for errors here maybe. But I'm not a native speaker either. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
Hi Christoph, On Tue, Dec 01, 2020 at 10:21:00AM +0000, Christoph Hellwig wrote: > On Tue, Nov 24, 2020 at 11:51:29PM +0800, Gao Xiang wrote: > > It's enough to just use return code, and get rid of an argument. > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > --- > > fs/xfs/libxfs/xfs_ialloc.c | 22 ++++++++++------------ > > 1 file changed, 10 insertions(+), 12 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c > > index 45cf7e55f5ee..5c8b0210aad3 100644 > > --- a/fs/xfs/libxfs/xfs_ialloc.c > > +++ b/fs/xfs/libxfs/xfs_ialloc.c > > @@ -607,13 +607,14 @@ xfs_inobt_insert_sprec( > > > > /* > > * Allocate new inodes in the allocation group specified by agbp. > > - * Return 0 for success, else error code. > > + * Return 0 for successfully allocating some inodes in this AG; > > + * 1 for skipping to allocating in the next AG; > > s/for/when/ for both lines I think. > > > + * < 0 for error code. > > and < 0 for errors here maybe. But I'm not a native speaker either. > > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Thanks for your review! I'm now spilting Dave's patch for better review as well (also already with some update since some lack of modification), will re-test and resend together with this series later. Thanks, Gao Xiang
On Tue, Dec 01, 2020 at 10:21:00AM +0000, Christoph Hellwig wrote: > On Tue, Nov 24, 2020 at 11:51:29PM +0800, Gao Xiang wrote: > > It's enough to just use return code, and get rid of an argument. > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > --- > > fs/xfs/libxfs/xfs_ialloc.c | 22 ++++++++++------------ > > 1 file changed, 10 insertions(+), 12 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c > > index 45cf7e55f5ee..5c8b0210aad3 100644 > > --- a/fs/xfs/libxfs/xfs_ialloc.c > > +++ b/fs/xfs/libxfs/xfs_ialloc.c > > @@ -607,13 +607,14 @@ xfs_inobt_insert_sprec( > > > > /* > > * Allocate new inodes in the allocation group specified by agbp. > > - * Return 0 for success, else error code. > > + * Return 0 for successfully allocating some inodes in this AG; > > + * 1 for skipping to allocating in the next AG; > > s/for/when/ for both lines I think. > > > + * < 0 for error code. > > and < 0 for errors here maybe. But I'm not a native speaker either. "Returns 0 if inodes were allocated in this AG; 1 if there was no space in this AG; or the usual negative error code." ? --D > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, Nov 24, 2020 at 11:51:29PM +0800, Gao Xiang wrote: > It's enough to just use return code, and get rid of an argument. > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > --- > fs/xfs/libxfs/xfs_ialloc.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c > index 45cf7e55f5ee..5c8b0210aad3 100644 > --- a/fs/xfs/libxfs/xfs_ialloc.c > +++ b/fs/xfs/libxfs/xfs_ialloc.c > @@ -607,13 +607,14 @@ xfs_inobt_insert_sprec( > > /* > * Allocate new inodes in the allocation group specified by agbp. > - * Return 0 for success, else error code. > + * Return 0 for successfully allocating some inodes in this AG; > + * 1 for skipping to allocating in the next AG; > + * < 0 for 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 +796,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 +903,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; > } > > @@ -1715,7 +1714,6 @@ xfs_dialloc( > struct xfs_buf *agbp; > xfs_agnumber_t agno; > int error; > - int ialloced; > bool noroom = false; > xfs_agnumber_t start_agno; > struct xfs_perag *pag; > @@ -1799,8 +1797,8 @@ xfs_dialloc( > goto nextag_relse_buffer; > > > - error = xfs_ialloc_ag_alloc(tp, agbp, &ialloced); > - if (error) { > + error = xfs_ialloc_ag_alloc(tp, agbp); > + if (error < 0) { > xfs_trans_brelse(tp, agbp); > > if (error != -ENOSPC) > @@ -1811,7 +1809,7 @@ xfs_dialloc( > return 0; > } > > - if (ialloced) { > + if (!error) { I wonder if this should be "if (error == 0)" because the comment for _ialloc_ag_alloc says that 0 and 1 have specific meanings? Otherwise looks fine to me. --D > /* > * We successfully allocated some inodes, return > * the current context to the caller so that it > -- > 2.18.4 >
Hi Darrick, On Tue, Dec 01, 2020 at 08:55:11AM -0800, Darrick J. Wong wrote: > On Tue, Dec 01, 2020 at 10:21:00AM +0000, Christoph Hellwig wrote: > > On Tue, Nov 24, 2020 at 11:51:29PM +0800, Gao Xiang wrote: > > > It's enough to just use return code, and get rid of an argument. > > > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > > --- > > > fs/xfs/libxfs/xfs_ialloc.c | 22 ++++++++++------------ > > > 1 file changed, 10 insertions(+), 12 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c > > > index 45cf7e55f5ee..5c8b0210aad3 100644 > > > --- a/fs/xfs/libxfs/xfs_ialloc.c > > > +++ b/fs/xfs/libxfs/xfs_ialloc.c > > > @@ -607,13 +607,14 @@ xfs_inobt_insert_sprec( > > > > > > /* > > > * Allocate new inodes in the allocation group specified by agbp. > > > - * Return 0 for success, else error code. > > > + * Return 0 for successfully allocating some inodes in this AG; > > > + * 1 for skipping to allocating in the next AG; > > > > s/for/when/ for both lines I think. > > > > > + * < 0 for error code. > > > > and < 0 for errors here maybe. But I'm not a native speaker either. > > "Returns 0 if inodes were allocated in this AG; 1 if there was no space > in this AG; or the usual negative error code." ? Okay, will update this in the next version. Thanks, Gao Xiang > > --D > > > Otherwise looks good: > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> >
On Tue, Dec 01, 2020 at 09:04:20AM -0800, Darrick J. Wong wrote: > On Tue, Nov 24, 2020 at 11:51:29PM +0800, Gao Xiang wrote: ... > > @@ -1799,8 +1797,8 @@ xfs_dialloc( > > goto nextag_relse_buffer; > > > > > > - error = xfs_ialloc_ag_alloc(tp, agbp, &ialloced); > > - if (error) { > > + error = xfs_ialloc_ag_alloc(tp, agbp); > > + if (error < 0) { > > xfs_trans_brelse(tp, agbp); > > > > if (error != -ENOSPC) > > @@ -1811,7 +1809,7 @@ xfs_dialloc( > > return 0; > > } > > > > - if (ialloced) { > > + if (!error) { > > I wonder if this should be "if (error == 0)" because the comment > for _ialloc_ag_alloc says that 0 and 1 have specific meanings? I'm fine with either way personally, it could make checkpatch.pl happy by using "if (!error)" though... (always follow such rule when I walked into the kernel community...) Thanks, Gao Xiang > > Otherwise looks fine to me. > > --D > > > /* > > * We successfully allocated some inodes, return > > * the current context to the caller so that it > > -- > > 2.18.4 > > >
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c index 45cf7e55f5ee..5c8b0210aad3 100644 --- a/fs/xfs/libxfs/xfs_ialloc.c +++ b/fs/xfs/libxfs/xfs_ialloc.c @@ -607,13 +607,14 @@ xfs_inobt_insert_sprec( /* * Allocate new inodes in the allocation group specified by agbp. - * Return 0 for success, else error code. + * Return 0 for successfully allocating some inodes in this AG; + * 1 for skipping to allocating in the next AG; + * < 0 for 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 +796,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 +903,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; } @@ -1715,7 +1714,6 @@ xfs_dialloc( struct xfs_buf *agbp; xfs_agnumber_t agno; int error; - int ialloced; bool noroom = false; xfs_agnumber_t start_agno; struct xfs_perag *pag; @@ -1799,8 +1797,8 @@ xfs_dialloc( goto nextag_relse_buffer; - error = xfs_ialloc_ag_alloc(tp, agbp, &ialloced); - if (error) { + error = xfs_ialloc_ag_alloc(tp, agbp); + if (error < 0) { xfs_trans_brelse(tp, agbp); if (error != -ENOSPC) @@ -1811,7 +1809,7 @@ xfs_dialloc( return 0; } - if (ialloced) { + if (!error) { /* * We successfully allocated some inodes, return * the current context to the caller so that it
It's enough to just use return code, and get rid of an argument. Signed-off-by: Gao Xiang <hsiangkao@redhat.com> --- fs/xfs/libxfs/xfs_ialloc.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)