| Message ID | 20210305051143.182133-22-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: consolidated log and optimisation changes | expand |
On Fri, Mar 05, 2021 at 04:11:19PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Remove another case where xlog_write() has to prepend an opheader to > a log transaction. The unmount record + ophdr is smaller than the > minimum amount of space guaranteed to be free in an iclog (2 * > sizeof(ophdr)) and so we don't have to care about an unmount record > being split across 2 iclogs. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_log.c | 35 ++++++++++++++++++++++++----------- > 1 file changed, 24 insertions(+), 11 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index b2f9fb1b4fed..94711b9ff007 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -798,12 +798,22 @@ xlog_write_unmount_record( > struct xlog *log, > struct xlog_ticket *ticket) > { > - struct xfs_unmount_log_format ulf = { > - .magic = XLOG_UNMOUNT_TYPE, > + struct { > + struct xlog_op_header ophdr; > + struct xfs_unmount_log_format ulf; > + } unmount_rec = { I wonder, should we have a BUILD_BUG_ON to confirm sizeof(umount_rec) just in case some weird architecture injects padding between these two? Prior to this code we formatted the op header and unmount record in separate incore buffers and wrote them to disk with no gap, right? --D > + .ophdr = { > + .oh_clientid = XFS_LOG, > + .oh_tid = cpu_to_be32(ticket->t_tid), > + .oh_flags = XLOG_UNMOUNT_TRANS, > + }, > + .ulf = { > + .magic = XLOG_UNMOUNT_TYPE, > + }, > }; > struct xfs_log_iovec reg = { > - .i_addr = &ulf, > - .i_len = sizeof(ulf), > + .i_addr = &unmount_rec, > + .i_len = sizeof(unmount_rec), > .i_type = XLOG_REG_TYPE_UNMOUNT, > }; > struct xfs_log_vec vec = { > @@ -812,7 +822,7 @@ xlog_write_unmount_record( > }; > > /* account for space used by record data */ > - ticket->t_curr_res -= sizeof(ulf); > + ticket->t_curr_res -= sizeof(unmount_rec); > > /* > * For external log devices, we need to flush the data device cache > @@ -2138,6 +2148,8 @@ xlog_write_calc_vec_length( > > /* Don't account for regions with embedded ophdrs */ > if (optype && headers > 0) { > + if (optype & XLOG_UNMOUNT_TRANS) > + headers--; > if (optype & XLOG_START_TRANS) { > ASSERT(headers >= 2); > headers -= 2; > @@ -2352,12 +2364,11 @@ xlog_write( > > /* > * If this is a commit or unmount transaction, we don't need a start > - * record to be written. We do, however, have to account for the > - * commit or unmount header that gets written. Hence we always have > - * to account for an extra xlog_op_header here for commit and unmount > - * records. > + * record to be written. We do, however, have to account for the commit > + * header that gets written. Hence we always have to account for an > + * extra xlog_op_header here for commit records. > */ > - if (optype & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) > + if (optype & XLOG_COMMIT_TRANS) > ticket->t_curr_res -= sizeof(struct xlog_op_header); > if (ticket->t_curr_res < 0) { > xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES, > @@ -2428,6 +2439,8 @@ xlog_write( > ophdr = reg->i_addr; > if (index) > optype &= ~XLOG_START_TRANS; > + } else if (optype & XLOG_UNMOUNT_TRANS) { > + ophdr = reg->i_addr; > } else { > ophdr = xlog_write_setup_ophdr(log, ptr, > ticket, optype); > @@ -2458,7 +2471,7 @@ xlog_write( > /* > * Copy region. > * > - * Commit and unmount records just log an opheader, so > + * Commit records just log an opheader, so > * we can have empty payloads with no data region to > * copy. Hence we only copy the payload if the vector > * says it has data to copy. > -- > 2.28.0 >
On Mon, Mar 08, 2021 at 04:15:23PM -0800, Darrick J. Wong wrote: > On Fri, Mar 05, 2021 at 04:11:19PM +1100, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > Remove another case where xlog_write() has to prepend an opheader to > > a log transaction. The unmount record + ophdr is smaller than the > > minimum amount of space guaranteed to be free in an iclog (2 * > > sizeof(ophdr)) and so we don't have to care about an unmount record > > being split across 2 iclogs. > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > --- > > fs/xfs/xfs_log.c | 35 ++++++++++++++++++++++++----------- > > 1 file changed, 24 insertions(+), 11 deletions(-) > > > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > > index b2f9fb1b4fed..94711b9ff007 100644 > > --- a/fs/xfs/xfs_log.c > > +++ b/fs/xfs/xfs_log.c > > @@ -798,12 +798,22 @@ xlog_write_unmount_record( > > struct xlog *log, > > struct xlog_ticket *ticket) > > { > > - struct xfs_unmount_log_format ulf = { > > - .magic = XLOG_UNMOUNT_TYPE, > > + struct { > > + struct xlog_op_header ophdr; > > + struct xfs_unmount_log_format ulf; > > + } unmount_rec = { > > I wonder, should we have a BUILD_BUG_ON to confirm sizeof(umount_rec) > just in case some weird architecture injects padding between these two? > Prior to this code we formatted the op header and unmount record in > separate incore buffers and wrote them to disk with no gap, right? Yup. Easy enough to add. Cheers, Dave.
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index b2f9fb1b4fed..94711b9ff007 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -798,12 +798,22 @@ xlog_write_unmount_record( struct xlog *log, struct xlog_ticket *ticket) { - struct xfs_unmount_log_format ulf = { - .magic = XLOG_UNMOUNT_TYPE, + struct { + struct xlog_op_header ophdr; + struct xfs_unmount_log_format ulf; + } unmount_rec = { + .ophdr = { + .oh_clientid = XFS_LOG, + .oh_tid = cpu_to_be32(ticket->t_tid), + .oh_flags = XLOG_UNMOUNT_TRANS, + }, + .ulf = { + .magic = XLOG_UNMOUNT_TYPE, + }, }; struct xfs_log_iovec reg = { - .i_addr = &ulf, - .i_len = sizeof(ulf), + .i_addr = &unmount_rec, + .i_len = sizeof(unmount_rec), .i_type = XLOG_REG_TYPE_UNMOUNT, }; struct xfs_log_vec vec = { @@ -812,7 +822,7 @@ xlog_write_unmount_record( }; /* account for space used by record data */ - ticket->t_curr_res -= sizeof(ulf); + ticket->t_curr_res -= sizeof(unmount_rec); /* * For external log devices, we need to flush the data device cache @@ -2138,6 +2148,8 @@ xlog_write_calc_vec_length( /* Don't account for regions with embedded ophdrs */ if (optype && headers > 0) { + if (optype & XLOG_UNMOUNT_TRANS) + headers--; if (optype & XLOG_START_TRANS) { ASSERT(headers >= 2); headers -= 2; @@ -2352,12 +2364,11 @@ xlog_write( /* * If this is a commit or unmount transaction, we don't need a start - * record to be written. We do, however, have to account for the - * commit or unmount header that gets written. Hence we always have - * to account for an extra xlog_op_header here for commit and unmount - * records. + * record to be written. We do, however, have to account for the commit + * header that gets written. Hence we always have to account for an + * extra xlog_op_header here for commit records. */ - if (optype & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) + if (optype & XLOG_COMMIT_TRANS) ticket->t_curr_res -= sizeof(struct xlog_op_header); if (ticket->t_curr_res < 0) { xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES, @@ -2428,6 +2439,8 @@ xlog_write( ophdr = reg->i_addr; if (index) optype &= ~XLOG_START_TRANS; + } else if (optype & XLOG_UNMOUNT_TRANS) { + ophdr = reg->i_addr; } else { ophdr = xlog_write_setup_ophdr(log, ptr, ticket, optype); @@ -2458,7 +2471,7 @@ xlog_write( /* * Copy region. * - * Commit and unmount records just log an opheader, so + * Commit records just log an opheader, so * we can have empty payloads with no data region to * copy. Hence we only copy the payload if the vector * says it has data to copy.