| Message ID | 20200904082516.31205-3-hsiangkao@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: random patches on log recovery | expand |
On Fri, Sep 04, 2020 at 04:25:16PM +0800, Gao Xiang wrote: > Let's use DIV_ROUND_UP() to calculate log record header > blocks as what did in xlog_get_iclog_buffer_size() and > wrap up common helpers for log recovery. > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > --- > v1: https://lore.kernel.org/r/20200902140923.24392-1-hsiangkao@redhat.com > > changes since v1: > - add another helper xlog_logrec_hblks() for the cases with > xfs_sb_version_haslogv2(), and use xlog_logrecv2_hblks() > for the case of xlog_do_recovery_pass() since it has more > complex logic other than just calculate hblks... > > fs/xfs/xfs_log.c | 4 +-- > fs/xfs/xfs_log_recover.c | 53 ++++++++++++++++------------------------ > 2 files changed, 22 insertions(+), 35 deletions(-) > ... > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 28d952794bfa..c6163065f6e0 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -397,6 +397,23 @@ xlog_find_verify_cycle( > return error; > } > > +static inline int xlog_logrecv2_hblks(struct xlog_rec_header *rh) > +{ > + int h_size = be32_to_cpu(rh->h_size); > + > + if ((be32_to_cpu(rh->h_version) & XLOG_VERSION_2) && > + h_size > XLOG_HEADER_CYCLE_SIZE) > + return DIV_ROUND_UP(h_size, XLOG_HEADER_CYCLE_SIZE); > + return 1; > +} > + > +static inline int xlog_logrec_hblks(struct xlog *log, xlog_rec_header_t *rh) > +{ > + if (!xfs_sb_version_haslogv2(&log->l_mp->m_sb)) > + return 1; > + return xlog_logrecv2_hblks(rh); > +} > + h_version is assigned based on xfs_sb_version_haslogv2() in the first place so I'm not sure I see the need for multiple helpers like this, at least with the current code. I can't really speak to why some code checks the feature bit and/or the record header version and not the other way around, but perhaps there's some historical reason I'm not aware of. Regardless, is there ever a case where xfs_sb_version_haslogv2() == true and h_version != 2? That strikes me as more of a corruption scenario than anything.. Brian > /* > * Potentially backup over partial log record write. > * > @@ -489,15 +506,7 @@ xlog_find_verify_log_record( > * reset last_blk. Only when last_blk points in the middle of a log > * record do we update last_blk. > */ > - if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) { > - uint h_size = be32_to_cpu(head->h_size); > - > - xhdrs = h_size / XLOG_HEADER_CYCLE_SIZE; > - if (h_size % XLOG_HEADER_CYCLE_SIZE) > - xhdrs++; > - } else { > - xhdrs = 1; > - } > + xhdrs = xlog_logrec_hblks(log, head); > > if (*last_blk - i + extra_bblks != > BTOBB(be32_to_cpu(head->h_len)) + xhdrs) > @@ -1184,22 +1193,7 @@ xlog_check_unmount_rec( > * below. We won't want to clear the unmount record if there is one, so > * we pass the lsn of the unmount record rather than the block after it. > */ > - if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) { > - int h_size = be32_to_cpu(rhead->h_size); > - int h_version = be32_to_cpu(rhead->h_version); > - > - if ((h_version & XLOG_VERSION_2) && > - (h_size > XLOG_HEADER_CYCLE_SIZE)) { > - hblks = h_size / XLOG_HEADER_CYCLE_SIZE; > - if (h_size % XLOG_HEADER_CYCLE_SIZE) > - hblks++; > - } else { > - hblks = 1; > - } > - } else { > - hblks = 1; > - } > - > + hblks = xlog_logrec_hblks(log, rhead); > after_umount_blk = xlog_wrap_logbno(log, > rhead_blk + hblks + BTOBB(be32_to_cpu(rhead->h_len))); > > @@ -3024,15 +3018,10 @@ xlog_do_recovery_pass( > if (error) > goto bread_err1; > > - if ((be32_to_cpu(rhead->h_version) & XLOG_VERSION_2) && > - (h_size > XLOG_HEADER_CYCLE_SIZE)) { > - hblks = h_size / XLOG_HEADER_CYCLE_SIZE; > - if (h_size % XLOG_HEADER_CYCLE_SIZE) > - hblks++; > + hblks = xlog_logrecv2_hblks(rhead); > + if (hblks != 1) { > kmem_free(hbp); > hbp = xlog_alloc_buffer(log, hblks); > - } else { > - hblks = 1; > } > } else { > ASSERT(log->l_sectBBsize == 1); > -- > 2.18.1 >
Hi Brian, On Fri, Sep 04, 2020 at 07:25:48AM -0400, Brian Foster wrote: > On Fri, Sep 04, 2020 at 04:25:16PM +0800, Gao Xiang wrote: ... > > > > +static inline int xlog_logrecv2_hblks(struct xlog_rec_header *rh) > > +{ > > + int h_size = be32_to_cpu(rh->h_size); > > + > > + if ((be32_to_cpu(rh->h_version) & XLOG_VERSION_2) && > > + h_size > XLOG_HEADER_CYCLE_SIZE) > > + return DIV_ROUND_UP(h_size, XLOG_HEADER_CYCLE_SIZE); > > + return 1; > > +} > > + > > +static inline int xlog_logrec_hblks(struct xlog *log, xlog_rec_header_t *rh) > > +{ > > + if (!xfs_sb_version_haslogv2(&log->l_mp->m_sb)) > > + return 1; > > + return xlog_logrecv2_hblks(rh); > > +} > > + > > h_version is assigned based on xfs_sb_version_haslogv2() in the first > place so I'm not sure I see the need for multiple helpers like this, at > least with the current code. I can't really speak to why some code > checks the feature bit and/or the record header version and not the > other way around, but perhaps there's some historical reason I'm not > aware of. Regardless, is there ever a case where > xfs_sb_version_haslogv2() == true and h_version != 2? That strikes me as > more of a corruption scenario than anything.. Thanks for this. Honestly, I'm not quite sure if xfs_sb_version_haslogv2() == true and h_version != 2 is useful (or existed historially)... anyway, that is another seperate topic though... Could you kindly give me some code flow on your preferred way about this so I could update this patch proper (since we have a complex case in xlog_do_recovery_pass(), I'm not sure how the unique helper will be like because there are 3 cases below...) - for the first 2 cases, we already have rhead read in-memory, so the logic is like: .... log_bread (somewhere in advance) .... if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) { ... } else { ... } (so I folded this two cases in xlog_logrec_hblks()) - for xlog_do_recovery_pass, it behaves like if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) { log_bread (another extra bread to get h_size for allocated buffer and hblks). ... } else { ... } so in this case we don't have rhead until xfs_sb_version_haslogv2(&log->l_mp->m_sb) is true... Thanks in advance! Thanks, Gao Xiang > > Brian
On Fri, Sep 04, 2020 at 08:59:29PM +0800, Gao Xiang wrote: > Hi Brian, > > On Fri, Sep 04, 2020 at 07:25:48AM -0400, Brian Foster wrote: > > On Fri, Sep 04, 2020 at 04:25:16PM +0800, Gao Xiang wrote: > > ... > > > > > > > +static inline int xlog_logrecv2_hblks(struct xlog_rec_header *rh) > > > +{ > > > + int h_size = be32_to_cpu(rh->h_size); > > > + > > > + if ((be32_to_cpu(rh->h_version) & XLOG_VERSION_2) && > > > + h_size > XLOG_HEADER_CYCLE_SIZE) > > > + return DIV_ROUND_UP(h_size, XLOG_HEADER_CYCLE_SIZE); > > > + return 1; > > > +} > > > + > > > +static inline int xlog_logrec_hblks(struct xlog *log, xlog_rec_header_t *rh) > > > +{ > > > + if (!xfs_sb_version_haslogv2(&log->l_mp->m_sb)) > > > + return 1; > > > + return xlog_logrecv2_hblks(rh); > > > +} > > > + > > > > h_version is assigned based on xfs_sb_version_haslogv2() in the first > > place so I'm not sure I see the need for multiple helpers like this, at > > least with the current code. I can't really speak to why some code > > checks the feature bit and/or the record header version and not the > > other way around, but perhaps there's some historical reason I'm not > > aware of. Regardless, is there ever a case where > > xfs_sb_version_haslogv2() == true and h_version != 2? That strikes me as > > more of a corruption scenario than anything.. > > Thanks for this. > > Honestly, I'm not quite sure if xfs_sb_version_haslogv2() == true and > h_version != 2 is useful (or existed historially)... anyway, that is > another seperate topic though... > Indeed. > Could you kindly give me some code flow on your preferred way about > this so I could update this patch proper (since we have a complex > case in xlog_do_recovery_pass(), I'm not sure how the unique helper > will be like because there are 3 cases below...) > > - for the first 2 cases, we already have rhead read in-memory, > so the logic is like: > .... > log_bread (somewhere in advance) > .... > > if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) { > ... > } else { > ... > } > (so I folded this two cases in xlog_logrec_hblks()) > > - for xlog_do_recovery_pass, it behaves like > if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) { > log_bread (another extra bread to get h_size for > allocated buffer and hblks). > > ... > } else { > ... > } > so in this case we don't have rhead until > xfs_sb_version_haslogv2(&log->l_mp->m_sb) is true... > I'm not sure I'm following the problem... The current patch makes the following change in xlog_do_recovery_pass(): @@ -3024,15 +3018,10 @@ xlog_do_recovery_pass( if (error) goto bread_err1; - if ((be32_to_cpu(rhead->h_version) & XLOG_VERSION_2) && - (h_size > XLOG_HEADER_CYCLE_SIZE)) { - hblks = h_size / XLOG_HEADER_CYCLE_SIZE; - if (h_size % XLOG_HEADER_CYCLE_SIZE) - hblks++; + hblks = xlog_logrecv2_hblks(rhead); + if (hblks != 1) { kmem_free(hbp); hbp = xlog_alloc_buffer(log, hblks); - } else { - hblks = 1; } } else { ASSERT(log->l_sectBBsize == 1); My question is: why can't we replace the xlog_logrecv2_hblks() call here with xlog_logrec_hblks()? We already have rhead as the existing code is already looking at h_version. We're inside a _haslogv2() branch, so the check inside the helper is effectively a duplicate/no-op.. Hm? Brian > Thanks in advance! > > Thanks, > Gao Xiang > > > > > > Brian >
On Fri, Sep 04, 2020 at 09:37:21AM -0400, Brian Foster wrote: > On Fri, Sep 04, 2020 at 08:59:29PM +0800, Gao Xiang wrote: ... > > Could you kindly give me some code flow on your preferred way about > > this so I could update this patch proper (since we have a complex > > case in xlog_do_recovery_pass(), I'm not sure how the unique helper > > will be like because there are 3 cases below...) > > > > - for the first 2 cases, we already have rhead read in-memory, > > so the logic is like: > > .... > > log_bread (somewhere in advance) > > .... > > > > if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) { > > ... > > } else { > > ... > > } > > (so I folded this two cases in xlog_logrec_hblks()) > > > > - for xlog_do_recovery_pass, it behaves like > > if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) { > > log_bread (another extra bread to get h_size for > > allocated buffer and hblks). > > > > ... > > } else { > > ... > > } > > so in this case we don't have rhead until > > xfs_sb_version_haslogv2(&log->l_mp->m_sb) is true... > > > > I'm not sure I'm following the problem... > > The current patch makes the following change in xlog_do_recovery_pass(): > > @@ -3024,15 +3018,10 @@ xlog_do_recovery_pass( > if (error) > goto bread_err1; > > - if ((be32_to_cpu(rhead->h_version) & XLOG_VERSION_2) && > - (h_size > XLOG_HEADER_CYCLE_SIZE)) { > - hblks = h_size / XLOG_HEADER_CYCLE_SIZE; > - if (h_size % XLOG_HEADER_CYCLE_SIZE) > - hblks++; > + hblks = xlog_logrecv2_hblks(rhead); > + if (hblks != 1) { > kmem_free(hbp); > hbp = xlog_alloc_buffer(log, hblks); > - } else { > - hblks = 1; > } > } else { > ASSERT(log->l_sectBBsize == 1); > > My question is: why can't we replace the xlog_logrecv2_hblks() call here > with xlog_logrec_hblks()? We already have rhead as the existing code is > already looking at h_version. We're inside a _haslogv2() branch, so the > check inside the helper is effectively a duplicate/no-op.. Hm? Yeah, I get your point. That would introduce a duplicated check of _haslogv2() if we use xlog_logrec_hblks() here (IMHO compliers wouldn't treat the 2nd _haslogv2() check as no-op). I will go forward like this if no other concerns. Thank you! Thanks, Gao Xiang > > Brian > > > Thanks in advance! > > > > Thanks, > > Gao Xiang > > > > > > > > > > Brian > > >
On Fri, Sep 04, 2020 at 11:07:30PM +0800, Gao Xiang wrote: > On Fri, Sep 04, 2020 at 09:37:21AM -0400, Brian Foster wrote: > > On Fri, Sep 04, 2020 at 08:59:29PM +0800, Gao Xiang wrote: > ... > > > Could you kindly give me some code flow on your preferred way about > > > this so I could update this patch proper (since we have a complex > > > case in xlog_do_recovery_pass(), I'm not sure how the unique helper > > > will be like because there are 3 cases below...) > > > > > > - for the first 2 cases, we already have rhead read in-memory, > > > so the logic is like: > > > .... > > > log_bread (somewhere in advance) > > > .... > > > > > > if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) { > > > ... > > > } else { > > > ... > > > } > > > (so I folded this two cases in xlog_logrec_hblks()) > > > > > > - for xlog_do_recovery_pass, it behaves like > > > if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) { > > > log_bread (another extra bread to get h_size for > > > allocated buffer and hblks). > > > > > > ... > > > } else { > > > ... > > > } > > > so in this case we don't have rhead until > > > xfs_sb_version_haslogv2(&log->l_mp->m_sb) is true... > > > > > > > I'm not sure I'm following the problem... > > > > The current patch makes the following change in xlog_do_recovery_pass(): > > > > @@ -3024,15 +3018,10 @@ xlog_do_recovery_pass( > > if (error) > > goto bread_err1; > > > > - if ((be32_to_cpu(rhead->h_version) & XLOG_VERSION_2) && > > - (h_size > XLOG_HEADER_CYCLE_SIZE)) { > > - hblks = h_size / XLOG_HEADER_CYCLE_SIZE; > > - if (h_size % XLOG_HEADER_CYCLE_SIZE) > > - hblks++; > > + hblks = xlog_logrecv2_hblks(rhead); > > + if (hblks != 1) { > > kmem_free(hbp); > > hbp = xlog_alloc_buffer(log, hblks); > > - } else { > > - hblks = 1; > > } > > } else { > > ASSERT(log->l_sectBBsize == 1); > > > > My question is: why can't we replace the xlog_logrecv2_hblks() call here > > with xlog_logrec_hblks()? We already have rhead as the existing code is > > already looking at h_version. We're inside a _haslogv2() branch, so the > > check inside the helper is effectively a duplicate/no-op.. Hm? > > Yeah, I get your point. That would introduce a duplicated check of > _haslogv2() if we use xlog_logrec_hblks() here (IMHO compliers wouldn't > treat the 2nd _haslogv2() check as no-op). > Yeah, I meant it as more as a logical no-op. IOW, it wouldn't affect functionality. The check instructions might be duplicated, but I doubt that would measurably impact log recovery. > I will go forward like this if no other concerns. Thank you! > Thanks. Brian > Thanks, > Gao Xiang > > > > > Brian > > > > > Thanks in advance! > > > > > > Thanks, > > > Gao Xiang > > > > > > > > > > > > > > Brian > > > > > >
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index ad0c69ee8947..7a4ba408a3a2 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -1604,9 +1604,7 @@ xlog_cksum( int i; int xheads; - xheads = size / XLOG_HEADER_CYCLE_SIZE; - if (size % XLOG_HEADER_CYCLE_SIZE) - xheads++; + xheads = DIV_ROUND_UP(size, XLOG_HEADER_CYCLE_SIZE); for (i = 1; i < xheads; i++) { crc = crc32c(crc, &xhdr[i].hic_xheader, diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 28d952794bfa..c6163065f6e0 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -397,6 +397,23 @@ xlog_find_verify_cycle( return error; } +static inline int xlog_logrecv2_hblks(struct xlog_rec_header *rh) +{ + int h_size = be32_to_cpu(rh->h_size); + + if ((be32_to_cpu(rh->h_version) & XLOG_VERSION_2) && + h_size > XLOG_HEADER_CYCLE_SIZE) + return DIV_ROUND_UP(h_size, XLOG_HEADER_CYCLE_SIZE); + return 1; +} + +static inline int xlog_logrec_hblks(struct xlog *log, xlog_rec_header_t *rh) +{ + if (!xfs_sb_version_haslogv2(&log->l_mp->m_sb)) + return 1; + return xlog_logrecv2_hblks(rh); +} + /* * Potentially backup over partial log record write. * @@ -489,15 +506,7 @@ xlog_find_verify_log_record( * reset last_blk. Only when last_blk points in the middle of a log * record do we update last_blk. */ - if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) { - uint h_size = be32_to_cpu(head->h_size); - - xhdrs = h_size / XLOG_HEADER_CYCLE_SIZE; - if (h_size % XLOG_HEADER_CYCLE_SIZE) - xhdrs++; - } else { - xhdrs = 1; - } + xhdrs = xlog_logrec_hblks(log, head); if (*last_blk - i + extra_bblks != BTOBB(be32_to_cpu(head->h_len)) + xhdrs) @@ -1184,22 +1193,7 @@ xlog_check_unmount_rec( * below. We won't want to clear the unmount record if there is one, so * we pass the lsn of the unmount record rather than the block after it. */ - if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) { - int h_size = be32_to_cpu(rhead->h_size); - int h_version = be32_to_cpu(rhead->h_version); - - if ((h_version & XLOG_VERSION_2) && - (h_size > XLOG_HEADER_CYCLE_SIZE)) { - hblks = h_size / XLOG_HEADER_CYCLE_SIZE; - if (h_size % XLOG_HEADER_CYCLE_SIZE) - hblks++; - } else { - hblks = 1; - } - } else { - hblks = 1; - } - + hblks = xlog_logrec_hblks(log, rhead); after_umount_blk = xlog_wrap_logbno(log, rhead_blk + hblks + BTOBB(be32_to_cpu(rhead->h_len))); @@ -3024,15 +3018,10 @@ xlog_do_recovery_pass( if (error) goto bread_err1; - if ((be32_to_cpu(rhead->h_version) & XLOG_VERSION_2) && - (h_size > XLOG_HEADER_CYCLE_SIZE)) { - hblks = h_size / XLOG_HEADER_CYCLE_SIZE; - if (h_size % XLOG_HEADER_CYCLE_SIZE) - hblks++; + hblks = xlog_logrecv2_hblks(rhead); + if (hblks != 1) { kmem_free(hbp); hbp = xlog_alloc_buffer(log, hblks); - } else { - hblks = 1; } } else { ASSERT(log->l_sectBBsize == 1);
Let's use DIV_ROUND_UP() to calculate log record header blocks as what did in xlog_get_iclog_buffer_size() and wrap up common helpers for log recovery. Signed-off-by: Gao Xiang <hsiangkao@redhat.com> --- v1: https://lore.kernel.org/r/20200902140923.24392-1-hsiangkao@redhat.com changes since v1: - add another helper xlog_logrec_hblks() for the cases with xfs_sb_version_haslogv2(), and use xlog_logrecv2_hblks() for the case of xlog_do_recovery_pass() since it has more complex logic other than just calculate hblks... fs/xfs/xfs_log.c | 4 +-- fs/xfs/xfs_log_recover.c | 53 ++++++++++++++++------------------------ 2 files changed, 22 insertions(+), 35 deletions(-)