Re: [PATCH 21/45] xfs: embed the xlog_op_header in the unmount record

Darrick J. Wong
2021-03-09T00:16:22Z

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
>