| Message ID | 20210224063459.3436852-5-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: rewrite xlog_write() | expand |
On Wed, Feb 24, 2021 at 05:34:50PM +1100, Dave Chinner wrote: > /* Don't account for regions with embedded ophdrs */ > if (optype && headers > 0) { > + headers--; > if (optype & XLOG_START_TRANS) { > + ASSERT(headers >= 1); > + headers--; A more detailed comment on the magic for XLOG_START_TRANS might be useful here. > @@ -2518,14 +2516,13 @@ xlog_write( > /* > * The XLOG_START_TRANS has embedded ophdrs for the > * start record and transaction header. They will always > - * be the first two regions in the lv chain. > + * be the first two regions in the lv chain. Commit and > + * unmount records also have embedded ophdrs. > */ Maybe update this comment to cover the other special cases as well?
On Thu, Feb 25, 2021 at 10:38:34AM +0100, Christoph Hellwig wrote: > On Wed, Feb 24, 2021 at 05:34:50PM +1100, Dave Chinner wrote: > > /* Don't account for regions with embedded ophdrs */ > > if (optype && headers > 0) { > > + headers--; > > if (optype & XLOG_START_TRANS) { > > + ASSERT(headers >= 1); > > + headers--; > > A more detailed comment on the magic for XLOG_START_TRANS might be useful > here. > > > @@ -2518,14 +2516,13 @@ xlog_write( > > /* > > * The XLOG_START_TRANS has embedded ophdrs for the > > * start record and transaction header. They will always > > - * be the first two regions in the lv chain. > > + * be the first two regions in the lv chain. Commit and > > + * unmount records also have embedded ophdrs. > > */ > > Maybe update this comment to cover the other special cases as well? Again, these all go away later in the patchset, so I'm not going to spend any time prettifying this. It's there simply to avoid breaking the log and leaving bisect landmines... Cheers, Dave.
On Wed, Feb 24, 2021 at 05:34:50PM +1100, Dave Chinner wrote: > Subject: xfs: embed the xlog_op_header in the unmount record Uh... isn't this embedding the xlog op header in the *commit* record? (Just saying for my own lazy purposes because my scripts choke badly when a patchset has multiple patches with the same subject...) --D > Remove the final case where xlog_write() has to prepend an opheader > to a log transaction. Similar to the start record, the commit record > is just an empty opheader with a XLOG_COMMIT_TRANS type, so we can > just make this the payload for the region being passed to > xlog_write() and remove the special handling in xlog_write() for > the commit record. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_log.c | 33 +++++++++++++++------------------ > 1 file changed, 15 insertions(+), 18 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index f3cb7482dfea..78b9c11b585f 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -1596,9 +1596,14 @@ xlog_commit_record( > struct xlog_in_core **iclog, > xfs_lsn_t *lsn) > { > + struct xlog_op_header ophdr = { > + .oh_clientid = XFS_TRANSACTION, > + .oh_tid = cpu_to_be32(ticket->t_tid), > + .oh_flags = XLOG_COMMIT_TRANS, > + }; > struct xfs_log_iovec reg = { > - .i_addr = NULL, > - .i_len = 0, > + .i_addr = &ophdr, > + .i_len = sizeof(struct xlog_op_header), > .i_type = XLOG_REG_TYPE_COMMIT, > }; > struct xfs_log_vec vec = { > @@ -1610,6 +1615,8 @@ xlog_commit_record( > if (XLOG_FORCED_SHUTDOWN(log)) > return -EIO; > > + /* account for space used by record data */ > + ticket->t_curr_res -= reg.i_len; > error = xlog_write(log, &vec, ticket, lsn, iclog, XLOG_COMMIT_TRANS); > if (error) > xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR); > @@ -2233,11 +2240,10 @@ xlog_write_calc_vec_length( > > /* Don't account for regions with embedded ophdrs */ > if (optype && headers > 0) { > - if (optype & XLOG_UNMOUNT_TRANS) > - headers--; > + headers--; > if (optype & XLOG_START_TRANS) { > - ASSERT(headers >= 2); > - headers -= 2; > + ASSERT(headers >= 1); > + headers--; > } > } > > @@ -2447,14 +2453,6 @@ xlog_write( > int data_cnt = 0; > int error = 0; > > - /* > - * 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 > - * 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) > - ticket->t_curr_res -= sizeof(struct xlog_op_header); > if (ticket->t_curr_res < 0) { > xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES, > "ctx ticket reservation ran out. Need to up reservation"); > @@ -2518,14 +2516,13 @@ xlog_write( > /* > * The XLOG_START_TRANS has embedded ophdrs for the > * start record and transaction header. They will always > - * be the first two regions in the lv chain. > + * be the first two regions in the lv chain. Commit and > + * unmount records also have embedded ophdrs. > */ > - if (optype & XLOG_START_TRANS) { > + if (optype) { > 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); > -- > 2.28.0 >
On Thu, Feb 25, 2021 at 06:57:27PM -0800, Darrick J. Wong wrote: > On Wed, Feb 24, 2021 at 05:34:50PM +1100, Dave Chinner wrote: > > Subject: xfs: embed the xlog_op_header in the unmount record > > Uh... isn't this embedding the xlog op header in the *commit* record? > > (Just saying for my own lazy purposes because my scripts choke badly > when a patchset has multiple patches with the same subject...) Uh, yes. That's what I get for copy-pasta of the commit headers ;) Cheers, Dave.
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index f3cb7482dfea..78b9c11b585f 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -1596,9 +1596,14 @@ xlog_commit_record( struct xlog_in_core **iclog, xfs_lsn_t *lsn) { + struct xlog_op_header ophdr = { + .oh_clientid = XFS_TRANSACTION, + .oh_tid = cpu_to_be32(ticket->t_tid), + .oh_flags = XLOG_COMMIT_TRANS, + }; struct xfs_log_iovec reg = { - .i_addr = NULL, - .i_len = 0, + .i_addr = &ophdr, + .i_len = sizeof(struct xlog_op_header), .i_type = XLOG_REG_TYPE_COMMIT, }; struct xfs_log_vec vec = { @@ -1610,6 +1615,8 @@ xlog_commit_record( if (XLOG_FORCED_SHUTDOWN(log)) return -EIO; + /* account for space used by record data */ + ticket->t_curr_res -= reg.i_len; error = xlog_write(log, &vec, ticket, lsn, iclog, XLOG_COMMIT_TRANS); if (error) xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR); @@ -2233,11 +2240,10 @@ xlog_write_calc_vec_length( /* Don't account for regions with embedded ophdrs */ if (optype && headers > 0) { - if (optype & XLOG_UNMOUNT_TRANS) - headers--; + headers--; if (optype & XLOG_START_TRANS) { - ASSERT(headers >= 2); - headers -= 2; + ASSERT(headers >= 1); + headers--; } } @@ -2447,14 +2453,6 @@ xlog_write( int data_cnt = 0; int error = 0; - /* - * 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 - * 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) - ticket->t_curr_res -= sizeof(struct xlog_op_header); if (ticket->t_curr_res < 0) { xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES, "ctx ticket reservation ran out. Need to up reservation"); @@ -2518,14 +2516,13 @@ xlog_write( /* * The XLOG_START_TRANS has embedded ophdrs for the * start record and transaction header. They will always - * be the first two regions in the lv chain. + * be the first two regions in the lv chain. Commit and + * unmount records also have embedded ophdrs. */ - if (optype & XLOG_START_TRANS) { + if (optype) { 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);
Remove the final case where xlog_write() has to prepend an opheader to a log transaction. Similar to the start record, the commit record is just an empty opheader with a XLOG_COMMIT_TRANS type, so we can just make this the payload for the region being passed to xlog_write() and remove the special handling in xlog_write() for the commit record. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/xfs_log.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-)