Message ID | 20210128044154.806715-2-david@fromorbit.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfs: various log stuff... | expand |
On Thu, Jan 28, 2021 at 03:41:50PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > We don't need to look at the xfs_mount and superblock every time we > need to do an iclog roundoff calculation. The property is fixed for > the life of the log, so store the roundoff in the log at mount time > and use that everywhere. > > On a debug build: > > $ size fs/xfs/xfs_log.o.* > text data bss dec hex filename > 27360 560 8 27928 6d18 fs/xfs/xfs_log.o.orig > 27219 560 8 27787 6c8b fs/xfs/xfs_log.o.patched > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_log_format.h | 3 -- > fs/xfs/xfs_log.c | 60 +++++++++++++++------------------- > fs/xfs/xfs_log_priv.h | 2 ++ > 3 files changed, 28 insertions(+), 37 deletions(-) > ... > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index 58699881c100..c5f507c24577 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -1400,6 +1400,12 @@ xlog_alloc_log( > xlog_assign_atomic_lsn(&log->l_last_sync_lsn, 1, 0); > log->l_curr_cycle = 1; /* 0 is bad since this is initial value */ > > + /* roundoff padding for transaction data and one for commit record */ I don't follow this comment. What do you mean by "... and one for commit record?" > + if (xfs_sb_version_haslogv2(&mp->m_sb) && mp->m_sb.sb_logsunit > 1) > + log->l_iclog_roundoff = mp->m_sb.sb_logsunit; > + else > + log->l_iclog_roundoff = BBSIZE; > + > xlog_grant_head_init(&log->l_reserve_head); > xlog_grant_head_init(&log->l_write_head); > ... > @@ -3485,18 +3475,20 @@ xfs_log_calc_unit_res( > /* for commit-rec LR header - note: padding will subsume the ophdr */ > unit_bytes += log->l_iclog_hsize; > > - /* for roundoff padding for transaction data and one for commit record */ > - if (xfs_sb_version_haslogv2(&mp->m_sb) && mp->m_sb.sb_logsunit > 1) { > - /* log su roundoff */ > - unit_bytes += 2 * mp->m_sb.sb_logsunit; > - } else { > - /* BB roundoff */ > - unit_bytes += 2 * BBSIZE; > - } > + /* roundoff padding for transaction data and one for commit record */ > + unit_bytes += log->l_iclog_roundoff; > > return unit_bytes; > } > > +int > +xfs_log_calc_unit_res( > + struct xfs_mount *mp, > + int unit_bytes) > +{ > + return xlog_calc_unit_res(mp->m_log, unit_bytes); > +} > + Could this be moved to the header as an inline? Otherwise looks reasonable. Brian > /* > * Allocate and initialise a new log ticket. > */ > @@ -3513,7 +3505,7 @@ xlog_ticket_alloc( > > tic = kmem_cache_zalloc(xfs_log_ticket_zone, GFP_NOFS | __GFP_NOFAIL); > > - unit_res = xfs_log_calc_unit_res(log->l_mp, unit_bytes); > + unit_res = xlog_calc_unit_res(log, unit_bytes); > > atomic_set(&tic->t_ref, 1); > tic->t_task = current; > diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h > index 1c6fdbf3d506..037950cf1061 100644 > --- a/fs/xfs/xfs_log_priv.h > +++ b/fs/xfs/xfs_log_priv.h > @@ -436,6 +436,8 @@ struct xlog { > #endif > /* log recovery lsn tracking (for buffer submission */ > xfs_lsn_t l_recovery_lsn; > + > + uint32_t l_iclog_roundoff;/* padding roundoff */ > }; > > #define XLOG_BUF_CANCEL_BUCKET(log, blkno) \ > -- > 2.28.0 >
On Thu, Jan 28, 2021 at 09:57:03AM -0500, Brian Foster wrote: > On Thu, Jan 28, 2021 at 03:41:50PM +1100, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > We don't need to look at the xfs_mount and superblock every time we > > need to do an iclog roundoff calculation. The property is fixed for > > the life of the log, so store the roundoff in the log at mount time > > and use that everywhere. > > > > On a debug build: > > > > $ size fs/xfs/xfs_log.o.* > > text data bss dec hex filename > > 27360 560 8 27928 6d18 fs/xfs/xfs_log.o.orig > > 27219 560 8 27787 6c8b fs/xfs/xfs_log.o.patched > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > --- > > fs/xfs/libxfs/xfs_log_format.h | 3 -- > > fs/xfs/xfs_log.c | 60 +++++++++++++++------------------- > > fs/xfs/xfs_log_priv.h | 2 ++ > > 3 files changed, 28 insertions(+), 37 deletions(-) > > > ... > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > > index 58699881c100..c5f507c24577 100644 > > --- a/fs/xfs/xfs_log.c > > +++ b/fs/xfs/xfs_log.c > > @@ -1400,6 +1400,12 @@ xlog_alloc_log( > > xlog_assign_atomic_lsn(&log->l_last_sync_lsn, 1, 0); > > log->l_curr_cycle = 1; /* 0 is bad since this is initial value */ > > > > + /* roundoff padding for transaction data and one for commit record */ > > I don't follow this comment. What do you mean by "... and one for commit > record?" I just copied the comment from the code I lifted it from. I will remove it. > > +int > > +xfs_log_calc_unit_res( > > + struct xfs_mount *mp, > > + int unit_bytes) > > +{ > > + return xlog_calc_unit_res(mp->m_log, unit_bytes); > > +} > > + > > Could this be moved to the header as an inline? Otherwise looks > reasonable. Not without making everything that includes the header file this is added to first including all the log header files. We really need to clean up the mess that are external log interfaces xfs_log_...(mp...) vs internal log interfaces xlog_...(log...) and the random distribution of such functions through xfs_log.c, xfs_log_recover.c, xfs_mount.c, xfs_log.h, xfs_log_priv.h, xfs_trans.h, and xfs_log_format.h.... Cheers, Dave.
On Thu, Jan 28, 2021 at 03:41:50PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > We don't need to look at the xfs_mount and superblock every time we > need to do an iclog roundoff calculation. The property is fixed for > the life of the log, so store the roundoff in the log at mount time > and use that everywhere. > > On a debug build: > > $ size fs/xfs/xfs_log.o.* > text data bss dec hex filename > 27360 560 8 27928 6d18 fs/xfs/xfs_log.o.orig > 27219 560 8 27787 6c8b fs/xfs/xfs_log.o.patched > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_log_format.h | 3 -- > fs/xfs/xfs_log.c | 60 +++++++++++++++------------------- > fs/xfs/xfs_log_priv.h | 2 ++ > 3 files changed, 28 insertions(+), 37 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h > index 8bd00da6d2a4..16587219549c 100644 > --- a/fs/xfs/libxfs/xfs_log_format.h > +++ b/fs/xfs/libxfs/xfs_log_format.h > @@ -34,9 +34,6 @@ typedef uint32_t xlog_tid_t; > #define XLOG_MIN_RECORD_BSHIFT 14 /* 16384 == 1 << 14 */ > #define XLOG_BIG_RECORD_BSHIFT 15 /* 32k == 1 << 15 */ > #define XLOG_MAX_RECORD_BSHIFT 18 /* 256k == 1 << 18 */ > -#define XLOG_BTOLSUNIT(log, b) (((b)+(log)->l_mp->m_sb.sb_logsunit-1) / \ > - (log)->l_mp->m_sb.sb_logsunit) > -#define XLOG_LSUNITTOB(log, su) ((su) * (log)->l_mp->m_sb.sb_logsunit) > > #define XLOG_HEADER_SIZE 512 > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index 58699881c100..c5f507c24577 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -1400,6 +1400,12 @@ xlog_alloc_log( > xlog_assign_atomic_lsn(&log->l_last_sync_lsn, 1, 0); > log->l_curr_cycle = 1; /* 0 is bad since this is initial value */ > > + /* roundoff padding for transaction data and one for commit record */ > + if (xfs_sb_version_haslogv2(&mp->m_sb) && mp->m_sb.sb_logsunit > 1) > + log->l_iclog_roundoff = mp->m_sb.sb_logsunit; > + else > + log->l_iclog_roundoff = BBSIZE; > + > xlog_grant_head_init(&log->l_reserve_head); > xlog_grant_head_init(&log->l_write_head); > > @@ -1852,29 +1858,15 @@ xlog_calc_iclog_size( > uint32_t *roundoff) > { > uint32_t count_init, count; > - bool use_lsunit; > - > - use_lsunit = xfs_sb_version_haslogv2(&log->l_mp->m_sb) && > - log->l_mp->m_sb.sb_logsunit > 1; > > /* Add for LR header */ > count_init = log->l_iclog_hsize + iclog->ic_offset; > + count = roundup(count_init, log->l_iclog_roundoff); > > - /* Round out the log write size */ > - if (use_lsunit) { > - /* we have a v2 stripe unit to use */ > - count = XLOG_LSUNITTOB(log, XLOG_BTOLSUNIT(log, count_init)); > - } else { > - count = BBTOB(BTOBB(count_init)); > - } > - > - ASSERT(count >= count_init); > *roundoff = count - count_init; > > - if (use_lsunit) > - ASSERT(*roundoff < log->l_mp->m_sb.sb_logsunit); > - else > - ASSERT(*roundoff < BBTOB(1)); > + ASSERT(count >= count_init); > + ASSERT(*roundoff < log->l_iclog_roundoff); > return count; > } > > @@ -3149,10 +3141,9 @@ xlog_state_switch_iclogs( > log->l_curr_block += BTOBB(eventual_size)+BTOBB(log->l_iclog_hsize); > > /* Round up to next log-sunit */ > - if (xfs_sb_version_haslogv2(&log->l_mp->m_sb) && > - log->l_mp->m_sb.sb_logsunit > 1) { > - uint32_t sunit_bb = BTOBB(log->l_mp->m_sb.sb_logsunit); > - log->l_curr_block = roundup(log->l_curr_block, sunit_bb); > + if (log->l_iclog_roundoff > BBSIZE) { > + log->l_curr_block = roundup(log->l_curr_block, > + BTOBB(log->l_iclog_roundoff)); > } > > if (log->l_curr_block >= log->l_logBBsize) { > @@ -3404,12 +3395,11 @@ xfs_log_ticket_get( > * Figure out the total log space unit (in bytes) that would be > * required for a log ticket. > */ > -int > -xfs_log_calc_unit_res( > - struct xfs_mount *mp, > +static int > +xlog_calc_unit_res( > + struct xlog *log, > int unit_bytes) > { > - struct xlog *log = mp->m_log; > int iclog_space; > uint num_headers; > > @@ -3485,18 +3475,20 @@ xfs_log_calc_unit_res( > /* for commit-rec LR header - note: padding will subsume the ophdr */ > unit_bytes += log->l_iclog_hsize; > > - /* for roundoff padding for transaction data and one for commit record */ > - if (xfs_sb_version_haslogv2(&mp->m_sb) && mp->m_sb.sb_logsunit > 1) { > - /* log su roundoff */ > - unit_bytes += 2 * mp->m_sb.sb_logsunit; > - } else { > - /* BB roundoff */ > - unit_bytes += 2 * BBSIZE; > - } > + /* roundoff padding for transaction data and one for commit record */ > + unit_bytes += log->l_iclog_roundoff; I don't understand why the "2 *" disappears here. It's not a part of the roundoff computation when we allocate the log, so AFAICT it's not just buried elsewhere? Was the old code saying that it added the roundoff factor twice because we needed to do so once for the transaction data and the second time for the commit record? --D > > return unit_bytes; > } > > +int > +xfs_log_calc_unit_res( > + struct xfs_mount *mp, > + int unit_bytes) > +{ > + return xlog_calc_unit_res(mp->m_log, unit_bytes); > +} > + > /* > * Allocate and initialise a new log ticket. > */ > @@ -3513,7 +3505,7 @@ xlog_ticket_alloc( > > tic = kmem_cache_zalloc(xfs_log_ticket_zone, GFP_NOFS | __GFP_NOFAIL); > > - unit_res = xfs_log_calc_unit_res(log->l_mp, unit_bytes); > + unit_res = xlog_calc_unit_res(log, unit_bytes); > > atomic_set(&tic->t_ref, 1); > tic->t_task = current; > diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h > index 1c6fdbf3d506..037950cf1061 100644 > --- a/fs/xfs/xfs_log_priv.h > +++ b/fs/xfs/xfs_log_priv.h > @@ -436,6 +436,8 @@ struct xlog { > #endif > /* log recovery lsn tracking (for buffer submission */ > xfs_lsn_t l_recovery_lsn; > + > + uint32_t l_iclog_roundoff;/* padding roundoff */ > }; > > #define XLOG_BUF_CANCEL_BUCKET(log, blkno) \ > -- > 2.28.0 >
On Thu, Jan 28, 2021 at 01:25:11PM -0800, Darrick J. Wong wrote: > On Thu, Jan 28, 2021 at 03:41:50PM +1100, Dave Chinner wrote: > > @@ -3404,12 +3395,11 @@ xfs_log_ticket_get( > > * Figure out the total log space unit (in bytes) that would be > > * required for a log ticket. > > */ > > -int > > -xfs_log_calc_unit_res( > > - struct xfs_mount *mp, > > +static int > > +xlog_calc_unit_res( > > + struct xlog *log, > > int unit_bytes) > > { > > - struct xlog *log = mp->m_log; > > int iclog_space; > > uint num_headers; > > > > @@ -3485,18 +3475,20 @@ xfs_log_calc_unit_res( > > /* for commit-rec LR header - note: padding will subsume the ophdr */ > > unit_bytes += log->l_iclog_hsize; > > > > - /* for roundoff padding for transaction data and one for commit record */ > > - if (xfs_sb_version_haslogv2(&mp->m_sb) && mp->m_sb.sb_logsunit > 1) { > > - /* log su roundoff */ > > - unit_bytes += 2 * mp->m_sb.sb_logsunit; > > - } else { > > - /* BB roundoff */ > > - unit_bytes += 2 * BBSIZE; > > - } > > + /* roundoff padding for transaction data and one for commit record */ > > + unit_bytes += log->l_iclog_roundoff; > > I don't understand why the "2 *" disappears here. It's not a part of > the roundoff computation when we allocate the log, so AFAICT it's not > just buried elsewhere? > > Was the old code saying that it added the roundoff factor twice because > we needed to do so once for the transaction data and the second time for > the commit record? Just a bug. I originally copying this entire chunk into the log init code (hence the comment), then I found another place it could be used but it didn't need the "* 2" value. SO i changed the init site, forgetting to put it back here. Will fix and resend. -Dave.
diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h index 8bd00da6d2a4..16587219549c 100644 --- a/fs/xfs/libxfs/xfs_log_format.h +++ b/fs/xfs/libxfs/xfs_log_format.h @@ -34,9 +34,6 @@ typedef uint32_t xlog_tid_t; #define XLOG_MIN_RECORD_BSHIFT 14 /* 16384 == 1 << 14 */ #define XLOG_BIG_RECORD_BSHIFT 15 /* 32k == 1 << 15 */ #define XLOG_MAX_RECORD_BSHIFT 18 /* 256k == 1 << 18 */ -#define XLOG_BTOLSUNIT(log, b) (((b)+(log)->l_mp->m_sb.sb_logsunit-1) / \ - (log)->l_mp->m_sb.sb_logsunit) -#define XLOG_LSUNITTOB(log, su) ((su) * (log)->l_mp->m_sb.sb_logsunit) #define XLOG_HEADER_SIZE 512 diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index 58699881c100..c5f507c24577 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -1400,6 +1400,12 @@ xlog_alloc_log( xlog_assign_atomic_lsn(&log->l_last_sync_lsn, 1, 0); log->l_curr_cycle = 1; /* 0 is bad since this is initial value */ + /* roundoff padding for transaction data and one for commit record */ + if (xfs_sb_version_haslogv2(&mp->m_sb) && mp->m_sb.sb_logsunit > 1) + log->l_iclog_roundoff = mp->m_sb.sb_logsunit; + else + log->l_iclog_roundoff = BBSIZE; + xlog_grant_head_init(&log->l_reserve_head); xlog_grant_head_init(&log->l_write_head); @@ -1852,29 +1858,15 @@ xlog_calc_iclog_size( uint32_t *roundoff) { uint32_t count_init, count; - bool use_lsunit; - - use_lsunit = xfs_sb_version_haslogv2(&log->l_mp->m_sb) && - log->l_mp->m_sb.sb_logsunit > 1; /* Add for LR header */ count_init = log->l_iclog_hsize + iclog->ic_offset; + count = roundup(count_init, log->l_iclog_roundoff); - /* Round out the log write size */ - if (use_lsunit) { - /* we have a v2 stripe unit to use */ - count = XLOG_LSUNITTOB(log, XLOG_BTOLSUNIT(log, count_init)); - } else { - count = BBTOB(BTOBB(count_init)); - } - - ASSERT(count >= count_init); *roundoff = count - count_init; - if (use_lsunit) - ASSERT(*roundoff < log->l_mp->m_sb.sb_logsunit); - else - ASSERT(*roundoff < BBTOB(1)); + ASSERT(count >= count_init); + ASSERT(*roundoff < log->l_iclog_roundoff); return count; } @@ -3149,10 +3141,9 @@ xlog_state_switch_iclogs( log->l_curr_block += BTOBB(eventual_size)+BTOBB(log->l_iclog_hsize); /* Round up to next log-sunit */ - if (xfs_sb_version_haslogv2(&log->l_mp->m_sb) && - log->l_mp->m_sb.sb_logsunit > 1) { - uint32_t sunit_bb = BTOBB(log->l_mp->m_sb.sb_logsunit); - log->l_curr_block = roundup(log->l_curr_block, sunit_bb); + if (log->l_iclog_roundoff > BBSIZE) { + log->l_curr_block = roundup(log->l_curr_block, + BTOBB(log->l_iclog_roundoff)); } if (log->l_curr_block >= log->l_logBBsize) { @@ -3404,12 +3395,11 @@ xfs_log_ticket_get( * Figure out the total log space unit (in bytes) that would be * required for a log ticket. */ -int -xfs_log_calc_unit_res( - struct xfs_mount *mp, +static int +xlog_calc_unit_res( + struct xlog *log, int unit_bytes) { - struct xlog *log = mp->m_log; int iclog_space; uint num_headers; @@ -3485,18 +3475,20 @@ xfs_log_calc_unit_res( /* for commit-rec LR header - note: padding will subsume the ophdr */ unit_bytes += log->l_iclog_hsize; - /* for roundoff padding for transaction data and one for commit record */ - if (xfs_sb_version_haslogv2(&mp->m_sb) && mp->m_sb.sb_logsunit > 1) { - /* log su roundoff */ - unit_bytes += 2 * mp->m_sb.sb_logsunit; - } else { - /* BB roundoff */ - unit_bytes += 2 * BBSIZE; - } + /* roundoff padding for transaction data and one for commit record */ + unit_bytes += log->l_iclog_roundoff; return unit_bytes; } +int +xfs_log_calc_unit_res( + struct xfs_mount *mp, + int unit_bytes) +{ + return xlog_calc_unit_res(mp->m_log, unit_bytes); +} + /* * Allocate and initialise a new log ticket. */ @@ -3513,7 +3505,7 @@ xlog_ticket_alloc( tic = kmem_cache_zalloc(xfs_log_ticket_zone, GFP_NOFS | __GFP_NOFAIL); - unit_res = xfs_log_calc_unit_res(log->l_mp, unit_bytes); + unit_res = xlog_calc_unit_res(log, unit_bytes); atomic_set(&tic->t_ref, 1); tic->t_task = current; diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h index 1c6fdbf3d506..037950cf1061 100644 --- a/fs/xfs/xfs_log_priv.h +++ b/fs/xfs/xfs_log_priv.h @@ -436,6 +436,8 @@ struct xlog { #endif /* log recovery lsn tracking (for buffer submission */ xfs_lsn_t l_recovery_lsn; + + uint32_t l_iclog_roundoff;/* padding roundoff */ }; #define XLOG_BUF_CANCEL_BUCKET(log, blkno) \