xfs: clean up log tickets and record writes v3

xfs: clean up log tickets and record writes v3

[Christoph Hellwig]

This series follows up on conversions about relogging infrastructure
and the way xfs_log_done() does two things but only one of several
callers uses both of those functions. It also pointed out that
xfs_trans_commit() never writes to the log anymore, so only
checkpoints pass a ticket to xlog_write() with this flag set and
no transaction makes multiple calls to xlog_write() calls on the
same ticket. Hence there's no real need for XLOG_TIC_INITED to track
whether a ticket has written a start record to the log anymore.

A lot of further cleanups fell out of this. Once we no longer use
XLOG_TIC_INITED to carry state inside the write loop, the logic
can be simplified in both xlog_write and xfs_log_done. xfs_log_done
can be split up, and then the call chain can be flattened because
xlog_write_done() and xlog_commit_record() are basically the same.

This then leads to cleanups writing both commit and unmount records.

Finally, to complete what started all this, the XLOG_TIC_INITED flag
is removed.

A git tree is avaiblable here:

    git://git.infradead.org/users/hch/xfs.git xlog-ticket-cleanup.3

Gitweb:

    http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xlog-ticket-cleanup.3

Changes since v2:
 - fix a commit message typo
 - move the XLOG_TIC_INITED earlier, and move another hunk to this patch
 - keep checking for XLOG_FORCED_SHUTDOWN and skip regrants
 - merge the two patches for refactoring and renaming the log unmount
   record handling

Changes since v1:
 - taking this over from Dave (for now) as he is still injured, an it
   interacts closely with my log error handling bits
 - rebased on top of for-next + the "more log cleanups" series
 - fix an accounting error in xlog_write
 - use a bool for the ticket header in xlog_write
 - add a new patch to split xlog_ticket_done

[PATCH 1/8] xfs: don't try to write a start record into every iclog

[Christoph Hellwig]

From: Dave Chinner <dchinner@redhat.com>

The xlog_write() function iterates over iclogs until it completes
writing all the log vectors passed in. The ticket tracks whether
a start record has been written or not, so only the first iclog gets
a start record. We only ever pass single use tickets to
xlog_write() so we only ever need to write a start record once per
xlog_write() call.

Hence we don't need to store whether we should write a start record
in the ticket as the callers provide all the information we need to
determine if a start record should be written. For the moment, we
have to ensure that we clear the XLOG_TIC_INITED appropriately so
the code in xfs_log_done() still works correctly for committing
transactions.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
[hch: use an need_start_rec bool]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 63 ++++++++++++++++++++++++------------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 2a90a483c2d6..bf071552094a 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2112,23 +2112,21 @@ xlog_print_trans(
 }
 
 /*
- * Calculate the potential space needed by the log vector.  Each region gets
- * its own xlog_op_header_t and may need to be double word aligned.
+ * Calculate the potential space needed by the log vector.  We may need a start
+ * record, and each region gets its own struct xlog_op_header and may need to be
+ * double word aligned.
  */
 static int
 xlog_write_calc_vec_length(
 	struct xlog_ticket	*ticket,
-	struct xfs_log_vec	*log_vector)
+	struct xfs_log_vec	*log_vector,
+	bool			need_start_rec)
 {
 	struct xfs_log_vec	*lv;
-	int			headers = 0;
+	int			headers = need_start_rec ? 1 : 0;
 	int			len = 0;
 	int			i;
 
-	/* acct for start rec of xact */
-	if (ticket->t_flags & XLOG_TIC_INITED)
-		headers++;
-
 	for (lv = log_vector; lv; lv = lv->lv_next) {
 		/* we don't write ordered log vectors */
 		if (lv->lv_buf_len == XFS_LOG_VEC_ORDERED)
@@ -2150,27 +2148,16 @@ xlog_write_calc_vec_length(
 	return len;
 }
 
-/*
- * If first write for transaction, insert start record  We can't be trying to
- * commit if we are inited.  We can't have any "partial_copy" if we are inited.
- */
-static int
+static void
 xlog_write_start_rec(
 	struct xlog_op_header	*ophdr,
 	struct xlog_ticket	*ticket)
 {
-	if (!(ticket->t_flags & XLOG_TIC_INITED))
-		return 0;
-
 	ophdr->oh_tid	= cpu_to_be32(ticket->t_tid);
 	ophdr->oh_clientid = ticket->t_clientid;
 	ophdr->oh_len = 0;
 	ophdr->oh_flags = XLOG_START_TRANS;
 	ophdr->oh_res2 = 0;
-
-	ticket->t_flags &= ~XLOG_TIC_INITED;
-
-	return sizeof(struct xlog_op_header);
 }
 
 static xlog_op_header_t *
@@ -2372,25 +2359,29 @@ xlog_write(
 	int			record_cnt = 0;
 	int			data_cnt = 0;
 	int			error = 0;
+	bool			need_start_rec = true;
 
 	*start_lsn = 0;
 
-	len = xlog_write_calc_vec_length(ticket, log_vector);
 
 	/*
 	 * Region headers and bytes are already accounted for.
 	 * We only need to take into account start records and
 	 * split regions in this function.
 	 */
-	if (ticket->t_flags & XLOG_TIC_INITED)
-		ticket->t_curr_res -= sizeof(xlog_op_header_t);
+	if (ticket->t_flags & XLOG_TIC_INITED) {
+		ticket->t_curr_res -= sizeof(struct xlog_op_header);
+		ticket->t_flags &= ~XLOG_TIC_INITED;
+	}
 
 	/*
 	 * Commit record headers need to be accounted for. These
 	 * come in as separate writes so are easy to detect.
 	 */
-	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
-		ticket->t_curr_res -= sizeof(xlog_op_header_t);
+	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
+		ticket->t_curr_res -= sizeof(struct xlog_op_header);
+		need_start_rec = false;
+	}
 
 	if (ticket->t_curr_res < 0) {
 		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
@@ -2399,6 +2390,8 @@ xlog_write(
 		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
 	}
 
+	len = xlog_write_calc_vec_length(ticket, log_vector, need_start_rec);
+
 	index = 0;
 	lv = log_vector;
 	vecp = lv->lv_iovecp;
@@ -2425,7 +2418,6 @@ xlog_write(
 		while (lv && (!lv->lv_niovecs || index < lv->lv_niovecs)) {
 			struct xfs_log_iovec	*reg;
 			struct xlog_op_header	*ophdr;
-			int			start_rec_copy;
 			int			copy_len;
 			int			copy_off;
 			bool			ordered = false;
@@ -2441,11 +2433,15 @@ xlog_write(
 			ASSERT(reg->i_len % sizeof(int32_t) == 0);
 			ASSERT((unsigned long)ptr % sizeof(int32_t) == 0);
 
-			start_rec_copy = xlog_write_start_rec(ptr, ticket);
-			if (start_rec_copy) {
-				record_cnt++;
+			/*
+			 * Before we start formatting log vectors, we need to
+			 * write a start record. Only do this for the first
+			 * iclog we write to.
+			 */
+			if (need_start_rec) {
+				xlog_write_start_rec(ptr, ticket);
 				xlog_write_adv_cnt(&ptr, &len, &log_offset,
-						   start_rec_copy);
+						sizeof(struct xlog_op_header));
 			}
 
 			ophdr = xlog_write_setup_ophdr(log, ptr, ticket, flags);
@@ -2477,8 +2473,13 @@ xlog_write(
 				xlog_write_adv_cnt(&ptr, &len, &log_offset,
 						   copy_len);
 			}
-			copy_len += start_rec_copy + sizeof(xlog_op_header_t);
+			copy_len += sizeof(struct xlog_op_header);
 			record_cnt++;
+			if (need_start_rec) {
+				copy_len += sizeof(struct xlog_op_header);
+				record_cnt++;
+				need_start_rec = false;
+			}
 			data_cnt += contwr ? copy_len : 0;
 
 			error = xlog_write_copy_finish(log, iclog, flags,
-- 
2.25.1

Re: [PATCH 1/8] xfs: don't try to write a start record into every iclog

[Darrick J. Wong]

On Tue, Mar 24, 2020 at 06:44:52PM +0100, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The xlog_write() function iterates over iclogs until it completes
> writing all the log vectors passed in. The ticket tracks whether
> a start record has been written or not, so only the first iclog gets
> a start record. We only ever pass single use tickets to
> xlog_write() so we only ever need to write a start record once per
> xlog_write() call.
> 
> Hence we don't need to store whether we should write a start record
> in the ticket as the callers provide all the information we need to
> determine if a start record should be written. For the moment, we
> have to ensure that we clear the XLOG_TIC_INITED appropriately so
> the code in xfs_log_done() still works correctly for committing
> transactions.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> [hch: use an need_start_rec bool]
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log.c | 63 ++++++++++++++++++++++++------------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 2a90a483c2d6..bf071552094a 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2112,23 +2112,21 @@ xlog_print_trans(
>  }
>  
>  /*
> - * Calculate the potential space needed by the log vector.  Each region gets
> - * its own xlog_op_header_t and may need to be double word aligned.
> + * Calculate the potential space needed by the log vector.  We may need a start
> + * record, and each region gets its own struct xlog_op_header and may need to be
> + * double word aligned.
>   */
>  static int
>  xlog_write_calc_vec_length(
>  	struct xlog_ticket	*ticket,
> -	struct xfs_log_vec	*log_vector)
> +	struct xfs_log_vec	*log_vector,
> +	bool			need_start_rec)
>  {
>  	struct xfs_log_vec	*lv;
> -	int			headers = 0;
> +	int			headers = need_start_rec ? 1 : 0;
>  	int			len = 0;
>  	int			i;
>  
> -	/* acct for start rec of xact */
> -	if (ticket->t_flags & XLOG_TIC_INITED)
> -		headers++;
> -
>  	for (lv = log_vector; lv; lv = lv->lv_next) {
>  		/* we don't write ordered log vectors */
>  		if (lv->lv_buf_len == XFS_LOG_VEC_ORDERED)
> @@ -2150,27 +2148,16 @@ xlog_write_calc_vec_length(
>  	return len;
>  }
>  
> -/*
> - * If first write for transaction, insert start record  We can't be trying to
> - * commit if we are inited.  We can't have any "partial_copy" if we are inited.
> - */
> -static int
> +static void
>  xlog_write_start_rec(
>  	struct xlog_op_header	*ophdr,
>  	struct xlog_ticket	*ticket)
>  {
> -	if (!(ticket->t_flags & XLOG_TIC_INITED))
> -		return 0;
> -
>  	ophdr->oh_tid	= cpu_to_be32(ticket->t_tid);
>  	ophdr->oh_clientid = ticket->t_clientid;
>  	ophdr->oh_len = 0;
>  	ophdr->oh_flags = XLOG_START_TRANS;
>  	ophdr->oh_res2 = 0;
> -
> -	ticket->t_flags &= ~XLOG_TIC_INITED;
> -
> -	return sizeof(struct xlog_op_header);
>  }
>  
>  static xlog_op_header_t *
> @@ -2372,25 +2359,29 @@ xlog_write(
>  	int			record_cnt = 0;
>  	int			data_cnt = 0;
>  	int			error = 0;
> +	bool			need_start_rec = true;
>  
>  	*start_lsn = 0;
>  
> -	len = xlog_write_calc_vec_length(ticket, log_vector);
>  
>  	/*
>  	 * Region headers and bytes are already accounted for.
>  	 * We only need to take into account start records and
>  	 * split regions in this function.
>  	 */
> -	if (ticket->t_flags & XLOG_TIC_INITED)
> -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> +	if (ticket->t_flags & XLOG_TIC_INITED) {
> +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> +		ticket->t_flags &= ~XLOG_TIC_INITED;
> +	}
>  
>  	/*
>  	 * Commit record headers need to be accounted for. These
>  	 * come in as separate writes so are easy to detect.
>  	 */
> -	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
> -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> +	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
> +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> +		need_start_rec = false;
> +	}
>  
>  	if (ticket->t_curr_res < 0) {
>  		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
> @@ -2399,6 +2390,8 @@ xlog_write(
>  		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
>  	}
>  
> +	len = xlog_write_calc_vec_length(ticket, log_vector, need_start_rec);
> +
>  	index = 0;
>  	lv = log_vector;
>  	vecp = lv->lv_iovecp;
> @@ -2425,7 +2418,6 @@ xlog_write(
>  		while (lv && (!lv->lv_niovecs || index < lv->lv_niovecs)) {
>  			struct xfs_log_iovec	*reg;
>  			struct xlog_op_header	*ophdr;
> -			int			start_rec_copy;
>  			int			copy_len;
>  			int			copy_off;
>  			bool			ordered = false;
> @@ -2441,11 +2433,15 @@ xlog_write(
>  			ASSERT(reg->i_len % sizeof(int32_t) == 0);
>  			ASSERT((unsigned long)ptr % sizeof(int32_t) == 0);
>  
> -			start_rec_copy = xlog_write_start_rec(ptr, ticket);
> -			if (start_rec_copy) {
> -				record_cnt++;
> +			/*
> +			 * Before we start formatting log vectors, we need to
> +			 * write a start record. Only do this for the first
> +			 * iclog we write to.
> +			 */
> +			if (need_start_rec) {
> +				xlog_write_start_rec(ptr, ticket);
>  				xlog_write_adv_cnt(&ptr, &len, &log_offset,
> -						   start_rec_copy);
> +						sizeof(struct xlog_op_header));
>  			}
>  
>  			ophdr = xlog_write_setup_ophdr(log, ptr, ticket, flags);
> @@ -2477,8 +2473,13 @@ xlog_write(
>  				xlog_write_adv_cnt(&ptr, &len, &log_offset,
>  						   copy_len);
>  			}
> -			copy_len += start_rec_copy + sizeof(xlog_op_header_t);
> +			copy_len += sizeof(struct xlog_op_header);
>  			record_cnt++;
> +			if (need_start_rec) {
> +				copy_len += sizeof(struct xlog_op_header);
> +				record_cnt++;
> +				need_start_rec = false;
> +			}
>  			data_cnt += contwr ? copy_len : 0;
>  
>  			error = xlog_write_copy_finish(log, iclog, flags,
> -- 
> 2.25.1
> 

Re: [PATCH 1/8] xfs: don't try to write a start record into every iclog

[Brian Foster]

On Tue, Mar 24, 2020 at 06:44:52PM +0100, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The xlog_write() function iterates over iclogs until it completes
> writing all the log vectors passed in. The ticket tracks whether
> a start record has been written or not, so only the first iclog gets
> a start record. We only ever pass single use tickets to
> xlog_write() so we only ever need to write a start record once per
> xlog_write() call.
> 
> Hence we don't need to store whether we should write a start record
> in the ticket as the callers provide all the information we need to
> determine if a start record should be written. For the moment, we
> have to ensure that we clear the XLOG_TIC_INITED appropriately so
> the code in xfs_log_done() still works correctly for committing
> transactions.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> [hch: use an need_start_rec bool]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 63 ++++++++++++++++++++++++------------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 2a90a483c2d6..bf071552094a 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
...
> @@ -2372,25 +2359,29 @@ xlog_write(
>  	int			record_cnt = 0;
>  	int			data_cnt = 0;
>  	int			error = 0;
> +	bool			need_start_rec = true;
>  
>  	*start_lsn = 0;
>  
> -	len = xlog_write_calc_vec_length(ticket, log_vector);
>  
>  	/*
>  	 * Region headers and bytes are already accounted for.
>  	 * We only need to take into account start records and
>  	 * split regions in this function.
>  	 */
> -	if (ticket->t_flags & XLOG_TIC_INITED)
> -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> +	if (ticket->t_flags & XLOG_TIC_INITED) {
> +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> +		ticket->t_flags &= ~XLOG_TIC_INITED;
> +	}
>  
>  	/*
>  	 * Commit record headers need to be accounted for. These
>  	 * come in as separate writes so are easy to detect.
>  	 */
> -	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
> -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> +	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
> +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> +		need_start_rec = false;
> +	}

Hmm.. I was asking for a comment update in v1 for this logic change.
Looking through it again, what happens here if
xfs_log_write_unmount_record() clears the UNMOUNT_TRANS flag for that
summary counter check? That looks like a potential behavior change wrt
to the start record..

Brian

>  
>  	if (ticket->t_curr_res < 0) {
>  		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
> @@ -2399,6 +2390,8 @@ xlog_write(
>  		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
>  	}
>  
> +	len = xlog_write_calc_vec_length(ticket, log_vector, need_start_rec);
> +
>  	index = 0;
>  	lv = log_vector;
>  	vecp = lv->lv_iovecp;
> @@ -2425,7 +2418,6 @@ xlog_write(
>  		while (lv && (!lv->lv_niovecs || index < lv->lv_niovecs)) {
>  			struct xfs_log_iovec	*reg;
>  			struct xlog_op_header	*ophdr;
> -			int			start_rec_copy;
>  			int			copy_len;
>  			int			copy_off;
>  			bool			ordered = false;
> @@ -2441,11 +2433,15 @@ xlog_write(
>  			ASSERT(reg->i_len % sizeof(int32_t) == 0);
>  			ASSERT((unsigned long)ptr % sizeof(int32_t) == 0);
>  
> -			start_rec_copy = xlog_write_start_rec(ptr, ticket);
> -			if (start_rec_copy) {
> -				record_cnt++;
> +			/*
> +			 * Before we start formatting log vectors, we need to
> +			 * write a start record. Only do this for the first
> +			 * iclog we write to.
> +			 */
> +			if (need_start_rec) {
> +				xlog_write_start_rec(ptr, ticket);
>  				xlog_write_adv_cnt(&ptr, &len, &log_offset,
> -						   start_rec_copy);
> +						sizeof(struct xlog_op_header));
>  			}
>  
>  			ophdr = xlog_write_setup_ophdr(log, ptr, ticket, flags);
> @@ -2477,8 +2473,13 @@ xlog_write(
>  				xlog_write_adv_cnt(&ptr, &len, &log_offset,
>  						   copy_len);
>  			}
> -			copy_len += start_rec_copy + sizeof(xlog_op_header_t);
> +			copy_len += sizeof(struct xlog_op_header);
>  			record_cnt++;
> +			if (need_start_rec) {
> +				copy_len += sizeof(struct xlog_op_header);
> +				record_cnt++;
> +				need_start_rec = false;
> +			}
>  			data_cnt += contwr ? copy_len : 0;
>  
>  			error = xlog_write_copy_finish(log, iclog, flags,
> -- 
> 2.25.1
> 

Re: [PATCH 1/8] xfs: don't try to write a start record into every iclog

[Dave Chinner]

On Wed, Mar 25, 2020 at 08:33:14AM -0400, Brian Foster wrote:
> On Tue, Mar 24, 2020 at 06:44:52PM +0100, Christoph Hellwig wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The xlog_write() function iterates over iclogs until it completes
> > writing all the log vectors passed in. The ticket tracks whether
> > a start record has been written or not, so only the first iclog gets
> > a start record. We only ever pass single use tickets to
> > xlog_write() so we only ever need to write a start record once per
> > xlog_write() call.
> > 
> > Hence we don't need to store whether we should write a start record
> > in the ticket as the callers provide all the information we need to
> > determine if a start record should be written. For the moment, we
> > have to ensure that we clear the XLOG_TIC_INITED appropriately so
> > the code in xfs_log_done() still works correctly for committing
> > transactions.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > [hch: use an need_start_rec bool]
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/xfs/xfs_log.c | 63 ++++++++++++++++++++++++------------------------
> >  1 file changed, 32 insertions(+), 31 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > index 2a90a483c2d6..bf071552094a 100644
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> ...
> > @@ -2372,25 +2359,29 @@ xlog_write(
> >  	int			record_cnt = 0;
> >  	int			data_cnt = 0;
> >  	int			error = 0;
> > +	bool			need_start_rec = true;
> >  
> >  	*start_lsn = 0;
> >  
> > -	len = xlog_write_calc_vec_length(ticket, log_vector);
> >  
> >  	/*
> >  	 * Region headers and bytes are already accounted for.
> >  	 * We only need to take into account start records and
> >  	 * split regions in this function.
> >  	 */
> > -	if (ticket->t_flags & XLOG_TIC_INITED)
> > -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > +	if (ticket->t_flags & XLOG_TIC_INITED) {
> > +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> > +		ticket->t_flags &= ~XLOG_TIC_INITED;
> > +	}
> >  
> >  	/*
> >  	 * Commit record headers need to be accounted for. These
> >  	 * come in as separate writes so are easy to detect.
> >  	 */
> > -	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
> > -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > +	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
> > +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> > +		need_start_rec = false;
> > +	}
> 
> Hmm.. I was asking for a comment update in v1 for this logic change.
> Looking through it again, what happens here if
> xfs_log_write_unmount_record() clears the UNMOUNT_TRANS flag for that
> summary counter check? That looks like a potential behavior change wrt
> to the start record..

xfs_log_write_unmount_record() clears the XLOG_TIC_INITED
flag before calling xlog_write(), so the current code never writes
out a start record for the unmount record. i.e. the unmount
record is a single region with the unmount log item in it, and
AFAICT this code does not change the behaviour of the unmount record
write at all.

FWIW, that error injection code looks dodgy - it turns the unmount
record into an XFS_LOG transaction type with an invalid log item
type (0). That probably should be flagged as corruption, not be
silently ignored by recovery. IOWs, I think the error injection code
was wrong to begin with - if we want to ensure the log is dirty at
unmount, we should just skip writing the unmount record altogether.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/8] xfs: don't try to write a start record into every iclog

[Brian Foster]

On Thu, Mar 26, 2020 at 10:25:00AM +1100, Dave Chinner wrote:
> On Wed, Mar 25, 2020 at 08:33:14AM -0400, Brian Foster wrote:
> > On Tue, Mar 24, 2020 at 06:44:52PM +0100, Christoph Hellwig wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > The xlog_write() function iterates over iclogs until it completes
> > > writing all the log vectors passed in. The ticket tracks whether
> > > a start record has been written or not, so only the first iclog gets
> > > a start record. We only ever pass single use tickets to
> > > xlog_write() so we only ever need to write a start record once per
> > > xlog_write() call.
> > > 
> > > Hence we don't need to store whether we should write a start record
> > > in the ticket as the callers provide all the information we need to
> > > determine if a start record should be written. For the moment, we
> > > have to ensure that we clear the XLOG_TIC_INITED appropriately so
> > > the code in xfs_log_done() still works correctly for committing
> > > transactions.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > [hch: use an need_start_rec bool]
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  fs/xfs/xfs_log.c | 63 ++++++++++++++++++++++++------------------------
> > >  1 file changed, 32 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > > index 2a90a483c2d6..bf071552094a 100644
> > > --- a/fs/xfs/xfs_log.c
> > > +++ b/fs/xfs/xfs_log.c
> > ...
> > > @@ -2372,25 +2359,29 @@ xlog_write(
> > >  	int			record_cnt = 0;
> > >  	int			data_cnt = 0;
> > >  	int			error = 0;
> > > +	bool			need_start_rec = true;
> > >  
> > >  	*start_lsn = 0;
> > >  
> > > -	len = xlog_write_calc_vec_length(ticket, log_vector);
> > >  
> > >  	/*
> > >  	 * Region headers and bytes are already accounted for.
> > >  	 * We only need to take into account start records and
> > >  	 * split regions in this function.
> > >  	 */
> > > -	if (ticket->t_flags & XLOG_TIC_INITED)
> > > -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > > +	if (ticket->t_flags & XLOG_TIC_INITED) {
> > > +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> > > +		ticket->t_flags &= ~XLOG_TIC_INITED;
> > > +	}
> > >  
> > >  	/*
> > >  	 * Commit record headers need to be accounted for. These
> > >  	 * come in as separate writes so are easy to detect.
> > >  	 */
> > > -	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
> > > -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > > +	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
> > > +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> > > +		need_start_rec = false;
> > > +	}
> > 
> > Hmm.. I was asking for a comment update in v1 for this logic change.
> > Looking through it again, what happens here if
> > xfs_log_write_unmount_record() clears the UNMOUNT_TRANS flag for that
> > summary counter check? That looks like a potential behavior change wrt
> > to the start record..
> 
> xfs_log_write_unmount_record() clears the XLOG_TIC_INITED
> flag before calling xlog_write(), so the current code never writes
> out a start record for the unmount record. i.e. the unmount
> record is a single region with the unmount log item in it, and
> AFAICT this code does not change the behaviour of the unmount record
> write at all.
> 

I'm referring to the UNMOUNT_TRANS flag, not t_flags. With this patch,
we actually would write a start record in some cases because
need_start_rec is toggled based on the flags parameter and the summary
counter check zeroes it.

> FWIW, that error injection code looks dodgy - it turns the unmount
> record into an XFS_LOG transaction type with an invalid log item
> type (0). That probably should be flagged as corruption, not be
> silently ignored by recovery. IOWs, I think the error injection code
> was wrong to begin with - if we want to ensure the log is dirty at
> unmount, we should just skip writing the unmount record altogether.
> 

That may be true... TBH I wasn't totally clear on what that logic was
for (it isn't purely error injection). From the commit (f467cad95f5e3)
log, the intent appears to be to "skip writing the unmount record," but
that doesn't quite describe the behavior. Darrick might want to
comment..? If we do revisit this, I'm mainly curious on whether there's
a change in recovery behavior between having this specially crafted
record vs. just writing nothing. For example, does recovery still set
the head/tail based on this record even though we don't mark the log
clean? If so, do we care..?

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH 1/8] xfs: don't try to write a start record into every iclog

[Darrick J. Wong]

On Thu, Mar 26, 2020 at 07:08:28AM -0400, Brian Foster wrote:
> On Thu, Mar 26, 2020 at 10:25:00AM +1100, Dave Chinner wrote:
> > On Wed, Mar 25, 2020 at 08:33:14AM -0400, Brian Foster wrote:
> > > On Tue, Mar 24, 2020 at 06:44:52PM +0100, Christoph Hellwig wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > > 
> > > > The xlog_write() function iterates over iclogs until it completes
> > > > writing all the log vectors passed in. The ticket tracks whether
> > > > a start record has been written or not, so only the first iclog gets
> > > > a start record. We only ever pass single use tickets to
> > > > xlog_write() so we only ever need to write a start record once per
> > > > xlog_write() call.
> > > > 
> > > > Hence we don't need to store whether we should write a start record
> > > > in the ticket as the callers provide all the information we need to
> > > > determine if a start record should be written. For the moment, we
> > > > have to ensure that we clear the XLOG_TIC_INITED appropriately so
> > > > the code in xfs_log_done() still works correctly for committing
> > > > transactions.
> > > > 
> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > [hch: use an need_start_rec bool]
> > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > ---
> > > >  fs/xfs/xfs_log.c | 63 ++++++++++++++++++++++++------------------------
> > > >  1 file changed, 32 insertions(+), 31 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > > > index 2a90a483c2d6..bf071552094a 100644
> > > > --- a/fs/xfs/xfs_log.c
> > > > +++ b/fs/xfs/xfs_log.c
> > > ...
> > > > @@ -2372,25 +2359,29 @@ xlog_write(
> > > >  	int			record_cnt = 0;
> > > >  	int			data_cnt = 0;
> > > >  	int			error = 0;
> > > > +	bool			need_start_rec = true;
> > > >  
> > > >  	*start_lsn = 0;
> > > >  
> > > > -	len = xlog_write_calc_vec_length(ticket, log_vector);
> > > >  
> > > >  	/*
> > > >  	 * Region headers and bytes are already accounted for.
> > > >  	 * We only need to take into account start records and
> > > >  	 * split regions in this function.
> > > >  	 */
> > > > -	if (ticket->t_flags & XLOG_TIC_INITED)
> > > > -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > > > +	if (ticket->t_flags & XLOG_TIC_INITED) {
> > > > +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> > > > +		ticket->t_flags &= ~XLOG_TIC_INITED;
> > > > +	}
> > > >  
> > > >  	/*
> > > >  	 * Commit record headers need to be accounted for. These
> > > >  	 * come in as separate writes so are easy to detect.
> > > >  	 */
> > > > -	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
> > > > -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > > > +	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
> > > > +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> > > > +		need_start_rec = false;
> > > > +	}
> > > 
> > > Hmm.. I was asking for a comment update in v1 for this logic change.
> > > Looking through it again, what happens here if
> > > xfs_log_write_unmount_record() clears the UNMOUNT_TRANS flag for that
> > > summary counter check? That looks like a potential behavior change wrt
> > > to the start record..
> > 
> > xfs_log_write_unmount_record() clears the XLOG_TIC_INITED
> > flag before calling xlog_write(), so the current code never writes
> > out a start record for the unmount record. i.e. the unmount
> > record is a single region with the unmount log item in it, and
> > AFAICT this code does not change the behaviour of the unmount record
> > write at all.
> > 
> 
> I'm referring to the UNMOUNT_TRANS flag, not t_flags. With this patch,
> we actually would write a start record in some cases because
> need_start_rec is toggled based on the flags parameter and the summary
> counter check zeroes it.
> 
> > FWIW, that error injection code looks dodgy - it turns the unmount
> > record into an XFS_LOG transaction type with an invalid log item
> > type (0). That probably should be flagged as corruption, not be
> > silently ignored by recovery. IOWs, I think the error injection code
> > was wrong to begin with - if we want to ensure the log is dirty at
> > unmount, we should just skip writing the unmount record altogether.
> > 
> 
> That may be true... TBH I wasn't totally clear on what that logic was
> for (it isn't purely error injection). From the commit (f467cad95f5e3)
> log, the intent appears to be to "skip writing the unmount record," but
> that doesn't quite describe the behavior. Darrick might want to
> comment..? If we do revisit this, I'm mainly curious on whether there's
> a change in recovery behavior between having this specially crafted
> record vs. just writing nothing. For example, does recovery still set
> the head/tail based on this record even though we don't mark the log
> clean? If so, do we care..?

I'll have a look later today, but I think Dave is correct that the
summary recalc force code should bail out of xlog_unmount_write without
writing anything.

It's curious that recovery doesn't complain about this, but at this
point we've potentially been writing out logs with oh_flags == 0...

...though now that I look at xfs_log_recover.c, we don't ever actually
look for XLOG_UNMOUNT_TYPE, do we... heh.

--D

> 
> Brian
> 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 
> 

[PATCH 2/8] xfs: re-order initial space accounting checks in xlog_write

[Christoph Hellwig]

From: Dave Chinner <dchinner@redhat.com>

Commit and unmount records records do not need start records to be
written, so rearrange the logic in xlog_write() to remove the need
to check for XLOG_TIC_INITED to determine if we should account for
the space used by a start record.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_log.c | 38 ++++++++++++--------------------------
 1 file changed, 12 insertions(+), 26 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index bf071552094a..116f59b16b04 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2349,39 +2349,27 @@ xlog_write(
 	uint			flags)
 {
 	struct xlog_in_core	*iclog = NULL;
-	struct xfs_log_iovec	*vecp;
-	struct xfs_log_vec	*lv;
+	struct xfs_log_vec	*lv = log_vector;
+	struct xfs_log_iovec	*vecp = lv->lv_iovecp;
+	int			index = 0;
 	int			len;
-	int			index;
 	int			partial_copy = 0;
 	int			partial_copy_len = 0;
 	int			contwr = 0;
 	int			record_cnt = 0;
 	int			data_cnt = 0;
 	int			error = 0;
-	bool			need_start_rec = true;
-
-	*start_lsn = 0;
-
+	bool			need_start_rec;
 
 	/*
-	 * Region headers and bytes are already accounted for.
-	 * We only need to take into account start records and
-	 * split regions in this function.
+	 * 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.
 	 */
-	if (ticket->t_flags & XLOG_TIC_INITED) {
-		ticket->t_curr_res -= sizeof(struct xlog_op_header);
+	ticket->t_curr_res -= sizeof(struct xlog_op_header);
+	if (ticket->t_flags & XLOG_TIC_INITED)
 		ticket->t_flags &= ~XLOG_TIC_INITED;
-	}
-
-	/*
-	 * Commit record headers need to be accounted for. These
-	 * come in as separate writes so are easy to detect.
-	 */
-	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
-		ticket->t_curr_res -= sizeof(struct xlog_op_header);
-		need_start_rec = false;
-	}
 
 	if (ticket->t_curr_res < 0) {
 		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
@@ -2390,11 +2378,9 @@ xlog_write(
 		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
 	}
 
+	need_start_rec = !(flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS));
 	len = xlog_write_calc_vec_length(ticket, log_vector, need_start_rec);
-
-	index = 0;
-	lv = log_vector;
-	vecp = lv->lv_iovecp;
+	*start_lsn = 0;
 	while (lv && (!lv->lv_niovecs || index < lv->lv_niovecs)) {
 		void		*ptr;
 		int		log_offset;
-- 
2.25.1

Re: [PATCH 2/8] xfs: re-order initial space accounting checks in xlog_write

[Darrick J. Wong]

On Tue, Mar 24, 2020 at 06:44:53PM +0100, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Commit and unmount records records do not need start records to be
> written, so rearrange the logic in xlog_write() to remove the need
> to check for XLOG_TIC_INITED to determine if we should account for
> the space used by a start record.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Looks pretty straightforward,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log.c | 38 ++++++++++++--------------------------
>  1 file changed, 12 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index bf071552094a..116f59b16b04 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2349,39 +2349,27 @@ xlog_write(
>  	uint			flags)
>  {
>  	struct xlog_in_core	*iclog = NULL;
> -	struct xfs_log_iovec	*vecp;
> -	struct xfs_log_vec	*lv;
> +	struct xfs_log_vec	*lv = log_vector;
> +	struct xfs_log_iovec	*vecp = lv->lv_iovecp;
> +	int			index = 0;
>  	int			len;
> -	int			index;
>  	int			partial_copy = 0;
>  	int			partial_copy_len = 0;
>  	int			contwr = 0;
>  	int			record_cnt = 0;
>  	int			data_cnt = 0;
>  	int			error = 0;
> -	bool			need_start_rec = true;
> -
> -	*start_lsn = 0;
> -
> +	bool			need_start_rec;
>  
>  	/*
> -	 * Region headers and bytes are already accounted for.
> -	 * We only need to take into account start records and
> -	 * split regions in this function.
> +	 * 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.
>  	 */
> -	if (ticket->t_flags & XLOG_TIC_INITED) {
> -		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> +	ticket->t_curr_res -= sizeof(struct xlog_op_header);
> +	if (ticket->t_flags & XLOG_TIC_INITED)
>  		ticket->t_flags &= ~XLOG_TIC_INITED;
> -	}
> -
> -	/*
> -	 * Commit record headers need to be accounted for. These
> -	 * come in as separate writes so are easy to detect.
> -	 */
> -	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
> -		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> -		need_start_rec = false;
> -	}
>  
>  	if (ticket->t_curr_res < 0) {
>  		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
> @@ -2390,11 +2378,9 @@ xlog_write(
>  		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
>  	}
>  
> +	need_start_rec = !(flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS));
>  	len = xlog_write_calc_vec_length(ticket, log_vector, need_start_rec);
> -
> -	index = 0;
> -	lv = log_vector;
> -	vecp = lv->lv_iovecp;
> +	*start_lsn = 0;
>  	while (lv && (!lv->lv_niovecs || index < lv->lv_niovecs)) {
>  		void		*ptr;
>  		int		log_offset;
> -- 
> 2.25.1
> 

[PATCH 3/8] xfs: refactor and split xfs_log_done()

[Christoph Hellwig]

From: Dave Chinner <dchinner@redhat.com>

xfs_log_done() does two separate things. Firstly, it triggers commit
records to be written for permanent transactions, and secondly it
releases or regrants transaction reservation space.

Since delayed logging was introduced, transactions no longer write
directly to the log, hence they never have the XLOG_TIC_INITED flag
cleared on them. Hence transactions never write commit records to
the log and only need to modify reservation space.

Split up xfs_log_done into two parts, and only call the parts of the
operation needed for the context xfs_log_done() is currently being
called from.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c      | 64 ++++++++++++++-----------------------------
 fs/xfs/xfs_log.h      |  4 ---
 fs/xfs/xfs_log_cil.c  | 13 +++++----
 fs/xfs/xfs_log_priv.h | 16 +++++------
 fs/xfs/xfs_trans.c    | 24 ++++++++--------
 5 files changed, 48 insertions(+), 73 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 116f59b16b04..528ace7a6bb9 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -487,62 +487,40 @@ xfs_log_reserve(
  */
 
 /*
- * This routine is called when a user of a log manager ticket is done with
- * the reservation.  If the ticket was ever used, then a commit record for
- * the associated transaction is written out as a log operation header with
- * no data.  The flag XLOG_TIC_INITED is set when the first write occurs with
- * a given ticket.  If the ticket was one with a permanent reservation, then
- * a few operations are done differently.  Permanent reservation tickets by
- * default don't release the reservation.  They just commit the current
- * transaction with the belief that the reservation is still needed.  A flag
- * must be passed in before permanent reservations are actually released.
- * When these type of tickets are not released, they need to be set into
- * the inited state again.  By doing this, a start record will be written
- * out when the next write occurs.
+ * Write a commit record to the log to close off a running log write.
  */
-xfs_lsn_t
-xfs_log_done(
-	struct xfs_mount	*mp,
+int
+xlog_write_done(
+	struct xlog		*log,
 	struct xlog_ticket	*ticket,
 	struct xlog_in_core	**iclog,
-	bool			regrant)
+	xfs_lsn_t		*lsn)
 {
-	struct xlog		*log = mp->m_log;
-	xfs_lsn_t		lsn = 0;
-
-	if (XLOG_FORCED_SHUTDOWN(log) ||
-	    /*
-	     * If nothing was ever written, don't write out commit record.
-	     * If we get an error, just continue and give back the log ticket.
-	     */
-	    (((ticket->t_flags & XLOG_TIC_INITED) == 0) &&
-	     (xlog_commit_record(log, ticket, iclog, &lsn)))) {
-		lsn = (xfs_lsn_t) -1;
-		regrant = false;
-	}
+	if (XLOG_FORCED_SHUTDOWN(log))
+		return -EIO;
 
+	return xlog_commit_record(log, ticket, iclog, lsn);
+}
 
-	if (!regrant) {
+/*
+ * Release or regrant the ticket reservation now the transaction is done with
+ * it depending on caller context. Rolling transactions need the ticket
+ * regranted, otherwise we release it completely.
+ */
+void
+xlog_ticket_done(
+	struct xlog		*log,
+	struct xlog_ticket	*ticket,
+	bool			regrant)
+{
+	if (!regrant || XLOG_FORCED_SHUTDOWN(log)) {
 		trace_xfs_log_done_nonperm(log, ticket);
-
-		/*
-		 * Release ticket if not permanent reservation or a specific
-		 * request has been made to release a permanent reservation.
-		 */
 		xlog_ungrant_log_space(log, ticket);
 	} else {
 		trace_xfs_log_done_perm(log, ticket);
-
 		xlog_regrant_reserve_log_space(log, ticket);
-		/* If this ticket was a permanent reservation and we aren't
-		 * trying to release it, reset the inited flags; so next time
-		 * we write, a start record will be written out.
-		 */
-		ticket->t_flags |= XLOG_TIC_INITED;
 	}
-
 	xfs_log_ticket_put(ticket);
-	return lsn;
 }
 
 static bool
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index cc77cc36560a..1412d6993f1e 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -105,10 +105,6 @@ struct xfs_log_item;
 struct xfs_item_ops;
 struct xfs_trans;
 
-xfs_lsn_t xfs_log_done(struct xfs_mount *mp,
-		       struct xlog_ticket *ticket,
-		       struct xlog_in_core **iclog,
-		       bool regrant);
 int	  xfs_log_force(struct xfs_mount *mp, uint flags);
 int	  xfs_log_force_lsn(struct xfs_mount *mp, xfs_lsn_t lsn, uint flags,
 		int *log_forced);
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 64cc0bf2ab3b..880de1aa4288 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -839,10 +839,11 @@ xlog_cil_push_work(
 	}
 	spin_unlock(&cil->xc_push_lock);
 
-	/* xfs_log_done always frees the ticket on error. */
-	commit_lsn = xfs_log_done(log->l_mp, tic, &commit_iclog, false);
-	if (commit_lsn == -1)
-		goto out_abort;
+	error = xlog_write_done(log, tic, &commit_iclog, &commit_lsn);
+	if (error)
+		goto out_abort_free_ticket;
+
+	xlog_ticket_done(log, tic, false);
 
 	spin_lock(&commit_iclog->ic_callback_lock);
 	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
@@ -875,7 +876,7 @@ xlog_cil_push_work(
 	return;
 
 out_abort_free_ticket:
-	xfs_log_ticket_put(tic);
+	xlog_ticket_done(log, tic, false);
 out_abort:
 	ASSERT(XLOG_FORCED_SHUTDOWN(log));
 	xlog_cil_committed(ctx);
@@ -1007,7 +1008,7 @@ xfs_log_commit_cil(
 	if (commit_lsn)
 		*commit_lsn = xc_commit_lsn;
 
-	xfs_log_done(mp, tp->t_ticket, NULL, regrant);
+	xlog_ticket_done(log, tp->t_ticket, regrant);
 	tp->t_ticket = NULL;
 	xfs_trans_unreserve_and_mod_sb(tp);
 
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 2b0aec37e73e..32bb6856e69d 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -439,14 +439,14 @@ xlog_write_adv_cnt(void **ptr, int *len, int *off, size_t bytes)
 
 void	xlog_print_tic_res(struct xfs_mount *mp, struct xlog_ticket *ticket);
 void	xlog_print_trans(struct xfs_trans *);
-int
-xlog_write(
-	struct xlog		*log,
-	struct xfs_log_vec	*log_vector,
-	struct xlog_ticket	*tic,
-	xfs_lsn_t		*start_lsn,
-	struct xlog_in_core	**commit_iclog,
-	uint			flags);
+
+int xlog_write(struct xlog *log, struct xfs_log_vec *log_vector,
+			struct xlog_ticket *tic, xfs_lsn_t *start_lsn,
+			struct xlog_in_core **commit_iclog, uint flags);
+int xlog_write_done(struct xlog *log, struct xlog_ticket *ticket,
+			struct xlog_in_core **iclog, xfs_lsn_t *lsn);
+void xlog_ticket_done(struct xlog *log, struct xlog_ticket *ticket,
+			bool regrant);
 
 /*
  * When we crack an atomic LSN, we sample it first so that the value will not
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 73c534093f09..123ecc8435f6 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -9,6 +9,7 @@
 #include "xfs_shared.h"
 #include "xfs_format.h"
 #include "xfs_log_format.h"
+#include "xfs_log_priv.h"
 #include "xfs_trans_resv.h"
 #include "xfs_mount.h"
 #include "xfs_extent_busy.h"
@@ -150,8 +151,9 @@ xfs_trans_reserve(
 	uint			blocks,
 	uint			rtextents)
 {
-	int		error = 0;
-	bool		rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0;
+	struct xfs_mount	*mp = tp->t_mountp;
+	int			error = 0;
+	bool			rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0;
 
 	/* Mark this thread as being in a transaction */
 	current_set_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
@@ -162,7 +164,7 @@ xfs_trans_reserve(
 	 * fail if the count would go below zero.
 	 */
 	if (blocks > 0) {
-		error = xfs_mod_fdblocks(tp->t_mountp, -((int64_t)blocks), rsvd);
+		error = xfs_mod_fdblocks(mp, -((int64_t)blocks), rsvd);
 		if (error != 0) {
 			current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
 			return -ENOSPC;
@@ -191,9 +193,9 @@ xfs_trans_reserve(
 
 		if (tp->t_ticket != NULL) {
 			ASSERT(resp->tr_logflags & XFS_TRANS_PERM_LOG_RES);
-			error = xfs_log_regrant(tp->t_mountp, tp->t_ticket);
+			error = xfs_log_regrant(mp, tp->t_ticket);
 		} else {
-			error = xfs_log_reserve(tp->t_mountp,
+			error = xfs_log_reserve(mp,
 						resp->tr_logres,
 						resp->tr_logcount,
 						&tp->t_ticket, XFS_TRANSACTION,
@@ -213,7 +215,7 @@ xfs_trans_reserve(
 	 * fail if the count would go below zero.
 	 */
 	if (rtextents > 0) {
-		error = xfs_mod_frextents(tp->t_mountp, -((int64_t)rtextents));
+		error = xfs_mod_frextents(mp, -((int64_t)rtextents));
 		if (error) {
 			error = -ENOSPC;
 			goto undo_log;
@@ -229,7 +231,7 @@ xfs_trans_reserve(
 	 */
 undo_log:
 	if (resp->tr_logres > 0) {
-		xfs_log_done(tp->t_mountp, tp->t_ticket, NULL, false);
+		xlog_ticket_done(mp->m_log, tp->t_ticket, false);
 		tp->t_ticket = NULL;
 		tp->t_log_res = 0;
 		tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
@@ -237,7 +239,7 @@ xfs_trans_reserve(
 
 undo_blocks:
 	if (blocks > 0) {
-		xfs_mod_fdblocks(tp->t_mountp, (int64_t)blocks, rsvd);
+		xfs_mod_fdblocks(mp, (int64_t)blocks, rsvd);
 		tp->t_blk_res = 0;
 	}
 
@@ -999,9 +1001,7 @@ __xfs_trans_commit(
 	 */
 	xfs_trans_unreserve_and_mod_dquots(tp);
 	if (tp->t_ticket) {
-		commit_lsn = xfs_log_done(mp, tp->t_ticket, NULL, regrant);
-		if (commit_lsn == -1 && !error)
-			error = -EIO;
+		xlog_ticket_done(mp->m_log, tp->t_ticket, regrant);
 		tp->t_ticket = NULL;
 	}
 	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
@@ -1060,7 +1060,7 @@ xfs_trans_cancel(
 	xfs_trans_unreserve_and_mod_dquots(tp);
 
 	if (tp->t_ticket) {
-		xfs_log_done(mp, tp->t_ticket, NULL, false);
+		xlog_ticket_done(mp->m_log, tp->t_ticket, false);
 		tp->t_ticket = NULL;
 	}
 
-- 
2.25.1

Re: [PATCH 3/8] xfs: refactor and split xfs_log_done()

[Darrick J. Wong]

On Tue, Mar 24, 2020 at 06:44:54PM +0100, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xfs_log_done() does two separate things. Firstly, it triggers commit
> records to be written for permanent transactions, and secondly it
> releases or regrants transaction reservation space.
> 
> Since delayed logging was introduced, transactions no longer write
> directly to the log, hence they never have the XLOG_TIC_INITED flag
> cleared on them. Hence transactions never write commit records to
> the log and only need to modify reservation space.
> 
> Split up xfs_log_done into two parts, and only call the parts of the
> operation needed for the context xfs_log_done() is currently being
> called from.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Seems like a pretty straightforward split,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log.c      | 64 ++++++++++++++-----------------------------
>  fs/xfs/xfs_log.h      |  4 ---
>  fs/xfs/xfs_log_cil.c  | 13 +++++----
>  fs/xfs/xfs_log_priv.h | 16 +++++------
>  fs/xfs/xfs_trans.c    | 24 ++++++++--------
>  5 files changed, 48 insertions(+), 73 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 116f59b16b04..528ace7a6bb9 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -487,62 +487,40 @@ xfs_log_reserve(
>   */
>  
>  /*
> - * This routine is called when a user of a log manager ticket is done with
> - * the reservation.  If the ticket was ever used, then a commit record for
> - * the associated transaction is written out as a log operation header with
> - * no data.  The flag XLOG_TIC_INITED is set when the first write occurs with
> - * a given ticket.  If the ticket was one with a permanent reservation, then
> - * a few operations are done differently.  Permanent reservation tickets by
> - * default don't release the reservation.  They just commit the current
> - * transaction with the belief that the reservation is still needed.  A flag
> - * must be passed in before permanent reservations are actually released.
> - * When these type of tickets are not released, they need to be set into
> - * the inited state again.  By doing this, a start record will be written
> - * out when the next write occurs.
> + * Write a commit record to the log to close off a running log write.
>   */
> -xfs_lsn_t
> -xfs_log_done(
> -	struct xfs_mount	*mp,
> +int
> +xlog_write_done(
> +	struct xlog		*log,
>  	struct xlog_ticket	*ticket,
>  	struct xlog_in_core	**iclog,
> -	bool			regrant)
> +	xfs_lsn_t		*lsn)
>  {
> -	struct xlog		*log = mp->m_log;
> -	xfs_lsn_t		lsn = 0;
> -
> -	if (XLOG_FORCED_SHUTDOWN(log) ||
> -	    /*
> -	     * If nothing was ever written, don't write out commit record.
> -	     * If we get an error, just continue and give back the log ticket.
> -	     */
> -	    (((ticket->t_flags & XLOG_TIC_INITED) == 0) &&
> -	     (xlog_commit_record(log, ticket, iclog, &lsn)))) {
> -		lsn = (xfs_lsn_t) -1;
> -		regrant = false;
> -	}
> +	if (XLOG_FORCED_SHUTDOWN(log))
> +		return -EIO;
>  
> +	return xlog_commit_record(log, ticket, iclog, lsn);
> +}
>  
> -	if (!regrant) {
> +/*
> + * Release or regrant the ticket reservation now the transaction is done with
> + * it depending on caller context. Rolling transactions need the ticket
> + * regranted, otherwise we release it completely.
> + */
> +void
> +xlog_ticket_done(
> +	struct xlog		*log,
> +	struct xlog_ticket	*ticket,
> +	bool			regrant)
> +{
> +	if (!regrant || XLOG_FORCED_SHUTDOWN(log)) {
>  		trace_xfs_log_done_nonperm(log, ticket);
> -
> -		/*
> -		 * Release ticket if not permanent reservation or a specific
> -		 * request has been made to release a permanent reservation.
> -		 */
>  		xlog_ungrant_log_space(log, ticket);
>  	} else {
>  		trace_xfs_log_done_perm(log, ticket);
> -
>  		xlog_regrant_reserve_log_space(log, ticket);
> -		/* If this ticket was a permanent reservation and we aren't
> -		 * trying to release it, reset the inited flags; so next time
> -		 * we write, a start record will be written out.
> -		 */
> -		ticket->t_flags |= XLOG_TIC_INITED;
>  	}
> -
>  	xfs_log_ticket_put(ticket);
> -	return lsn;
>  }
>  
>  static bool
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index cc77cc36560a..1412d6993f1e 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -105,10 +105,6 @@ struct xfs_log_item;
>  struct xfs_item_ops;
>  struct xfs_trans;
>  
> -xfs_lsn_t xfs_log_done(struct xfs_mount *mp,
> -		       struct xlog_ticket *ticket,
> -		       struct xlog_in_core **iclog,
> -		       bool regrant);
>  int	  xfs_log_force(struct xfs_mount *mp, uint flags);
>  int	  xfs_log_force_lsn(struct xfs_mount *mp, xfs_lsn_t lsn, uint flags,
>  		int *log_forced);
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 64cc0bf2ab3b..880de1aa4288 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -839,10 +839,11 @@ xlog_cil_push_work(
>  	}
>  	spin_unlock(&cil->xc_push_lock);
>  
> -	/* xfs_log_done always frees the ticket on error. */
> -	commit_lsn = xfs_log_done(log->l_mp, tic, &commit_iclog, false);
> -	if (commit_lsn == -1)
> -		goto out_abort;
> +	error = xlog_write_done(log, tic, &commit_iclog, &commit_lsn);
> +	if (error)
> +		goto out_abort_free_ticket;
> +
> +	xlog_ticket_done(log, tic, false);
>  
>  	spin_lock(&commit_iclog->ic_callback_lock);
>  	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
> @@ -875,7 +876,7 @@ xlog_cil_push_work(
>  	return;
>  
>  out_abort_free_ticket:
> -	xfs_log_ticket_put(tic);
> +	xlog_ticket_done(log, tic, false);
>  out_abort:
>  	ASSERT(XLOG_FORCED_SHUTDOWN(log));
>  	xlog_cil_committed(ctx);
> @@ -1007,7 +1008,7 @@ xfs_log_commit_cil(
>  	if (commit_lsn)
>  		*commit_lsn = xc_commit_lsn;
>  
> -	xfs_log_done(mp, tp->t_ticket, NULL, regrant);
> +	xlog_ticket_done(log, tp->t_ticket, regrant);
>  	tp->t_ticket = NULL;
>  	xfs_trans_unreserve_and_mod_sb(tp);
>  
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 2b0aec37e73e..32bb6856e69d 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -439,14 +439,14 @@ xlog_write_adv_cnt(void **ptr, int *len, int *off, size_t bytes)
>  
>  void	xlog_print_tic_res(struct xfs_mount *mp, struct xlog_ticket *ticket);
>  void	xlog_print_trans(struct xfs_trans *);
> -int
> -xlog_write(
> -	struct xlog		*log,
> -	struct xfs_log_vec	*log_vector,
> -	struct xlog_ticket	*tic,
> -	xfs_lsn_t		*start_lsn,
> -	struct xlog_in_core	**commit_iclog,
> -	uint			flags);
> +
> +int xlog_write(struct xlog *log, struct xfs_log_vec *log_vector,
> +			struct xlog_ticket *tic, xfs_lsn_t *start_lsn,
> +			struct xlog_in_core **commit_iclog, uint flags);
> +int xlog_write_done(struct xlog *log, struct xlog_ticket *ticket,
> +			struct xlog_in_core **iclog, xfs_lsn_t *lsn);
> +void xlog_ticket_done(struct xlog *log, struct xlog_ticket *ticket,
> +			bool regrant);
>  
>  /*
>   * When we crack an atomic LSN, we sample it first so that the value will not
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 73c534093f09..123ecc8435f6 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -9,6 +9,7 @@
>  #include "xfs_shared.h"
>  #include "xfs_format.h"
>  #include "xfs_log_format.h"
> +#include "xfs_log_priv.h"
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
>  #include "xfs_extent_busy.h"
> @@ -150,8 +151,9 @@ xfs_trans_reserve(
>  	uint			blocks,
>  	uint			rtextents)
>  {
> -	int		error = 0;
> -	bool		rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0;
> +	struct xfs_mount	*mp = tp->t_mountp;
> +	int			error = 0;
> +	bool			rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0;
>  
>  	/* Mark this thread as being in a transaction */
>  	current_set_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> @@ -162,7 +164,7 @@ xfs_trans_reserve(
>  	 * fail if the count would go below zero.
>  	 */
>  	if (blocks > 0) {
> -		error = xfs_mod_fdblocks(tp->t_mountp, -((int64_t)blocks), rsvd);
> +		error = xfs_mod_fdblocks(mp, -((int64_t)blocks), rsvd);
>  		if (error != 0) {
>  			current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
>  			return -ENOSPC;
> @@ -191,9 +193,9 @@ xfs_trans_reserve(
>  
>  		if (tp->t_ticket != NULL) {
>  			ASSERT(resp->tr_logflags & XFS_TRANS_PERM_LOG_RES);
> -			error = xfs_log_regrant(tp->t_mountp, tp->t_ticket);
> +			error = xfs_log_regrant(mp, tp->t_ticket);
>  		} else {
> -			error = xfs_log_reserve(tp->t_mountp,
> +			error = xfs_log_reserve(mp,
>  						resp->tr_logres,
>  						resp->tr_logcount,
>  						&tp->t_ticket, XFS_TRANSACTION,
> @@ -213,7 +215,7 @@ xfs_trans_reserve(
>  	 * fail if the count would go below zero.
>  	 */
>  	if (rtextents > 0) {
> -		error = xfs_mod_frextents(tp->t_mountp, -((int64_t)rtextents));
> +		error = xfs_mod_frextents(mp, -((int64_t)rtextents));
>  		if (error) {
>  			error = -ENOSPC;
>  			goto undo_log;
> @@ -229,7 +231,7 @@ xfs_trans_reserve(
>  	 */
>  undo_log:
>  	if (resp->tr_logres > 0) {
> -		xfs_log_done(tp->t_mountp, tp->t_ticket, NULL, false);
> +		xlog_ticket_done(mp->m_log, tp->t_ticket, false);
>  		tp->t_ticket = NULL;
>  		tp->t_log_res = 0;
>  		tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
> @@ -237,7 +239,7 @@ xfs_trans_reserve(
>  
>  undo_blocks:
>  	if (blocks > 0) {
> -		xfs_mod_fdblocks(tp->t_mountp, (int64_t)blocks, rsvd);
> +		xfs_mod_fdblocks(mp, (int64_t)blocks, rsvd);
>  		tp->t_blk_res = 0;
>  	}
>  
> @@ -999,9 +1001,7 @@ __xfs_trans_commit(
>  	 */
>  	xfs_trans_unreserve_and_mod_dquots(tp);
>  	if (tp->t_ticket) {
> -		commit_lsn = xfs_log_done(mp, tp->t_ticket, NULL, regrant);
> -		if (commit_lsn == -1 && !error)
> -			error = -EIO;
> +		xlog_ticket_done(mp->m_log, tp->t_ticket, regrant);
>  		tp->t_ticket = NULL;
>  	}
>  	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> @@ -1060,7 +1060,7 @@ xfs_trans_cancel(
>  	xfs_trans_unreserve_and_mod_dquots(tp);
>  
>  	if (tp->t_ticket) {
> -		xfs_log_done(mp, tp->t_ticket, NULL, false);
> +		xlog_ticket_done(mp->m_log, tp->t_ticket, false);
>  		tp->t_ticket = NULL;
>  	}
>  
> -- 
> 2.25.1
> 

Re: [PATCH 3/8] xfs: refactor and split xfs_log_done()

[Brian Foster]

On Tue, Mar 24, 2020 at 06:44:54PM +0100, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xfs_log_done() does two separate things. Firstly, it triggers commit
> records to be written for permanent transactions, and secondly it
> releases or regrants transaction reservation space.
> 
> Since delayed logging was introduced, transactions no longer write
> directly to the log, hence they never have the XLOG_TIC_INITED flag
> cleared on them. Hence transactions never write commit records to
> the log and only need to modify reservation space.
> 
> Split up xfs_log_done into two parts, and only call the parts of the
> operation needed for the context xfs_log_done() is currently being
> called from.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c      | 64 ++++++++++++++-----------------------------
>  fs/xfs/xfs_log.h      |  4 ---
>  fs/xfs/xfs_log_cil.c  | 13 +++++----
>  fs/xfs/xfs_log_priv.h | 16 +++++------
>  fs/xfs/xfs_trans.c    | 24 ++++++++--------
>  5 files changed, 48 insertions(+), 73 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 116f59b16b04..528ace7a6bb9 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -487,62 +487,40 @@ xfs_log_reserve(
>   */
>  
>  /*
> - * This routine is called when a user of a log manager ticket is done with
> - * the reservation.  If the ticket was ever used, then a commit record for
> - * the associated transaction is written out as a log operation header with
> - * no data.  The flag XLOG_TIC_INITED is set when the first write occurs with
> - * a given ticket.  If the ticket was one with a permanent reservation, then
> - * a few operations are done differently.  Permanent reservation tickets by
> - * default don't release the reservation.  They just commit the current
> - * transaction with the belief that the reservation is still needed.  A flag
> - * must be passed in before permanent reservations are actually released.
> - * When these type of tickets are not released, they need to be set into
> - * the inited state again.  By doing this, a start record will be written
> - * out when the next write occurs.
> + * Write a commit record to the log to close off a running log write.
>   */
> -xfs_lsn_t
> -xfs_log_done(
> -	struct xfs_mount	*mp,
> +int
> +xlog_write_done(
> +	struct xlog		*log,
>  	struct xlog_ticket	*ticket,
>  	struct xlog_in_core	**iclog,
> -	bool			regrant)
> +	xfs_lsn_t		*lsn)
>  {
> -	struct xlog		*log = mp->m_log;
> -	xfs_lsn_t		lsn = 0;
> -
> -	if (XLOG_FORCED_SHUTDOWN(log) ||
> -	    /*
> -	     * If nothing was ever written, don't write out commit record.
> -	     * If we get an error, just continue and give back the log ticket.
> -	     */
> -	    (((ticket->t_flags & XLOG_TIC_INITED) == 0) &&
> -	     (xlog_commit_record(log, ticket, iclog, &lsn)))) {
> -		lsn = (xfs_lsn_t) -1;
> -		regrant = false;
> -	}
> +	if (XLOG_FORCED_SHUTDOWN(log))
> +		return -EIO;
>  
> +	return xlog_commit_record(log, ticket, iclog, lsn);
> +}
>  
> -	if (!regrant) {
> +/*
> + * Release or regrant the ticket reservation now the transaction is done with
> + * it depending on caller context. Rolling transactions need the ticket
> + * regranted, otherwise we release it completely.
> + */
> +void
> +xlog_ticket_done(
> +	struct xlog		*log,
> +	struct xlog_ticket	*ticket,
> +	bool			regrant)
> +{
> +	if (!regrant || XLOG_FORCED_SHUTDOWN(log)) {
>  		trace_xfs_log_done_nonperm(log, ticket);
> -
> -		/*
> -		 * Release ticket if not permanent reservation or a specific
> -		 * request has been made to release a permanent reservation.
> -		 */
>  		xlog_ungrant_log_space(log, ticket);
>  	} else {
>  		trace_xfs_log_done_perm(log, ticket);
> -
>  		xlog_regrant_reserve_log_space(log, ticket);
> -		/* If this ticket was a permanent reservation and we aren't
> -		 * trying to release it, reset the inited flags; so next time
> -		 * we write, a start record will be written out.
> -		 */
> -		ticket->t_flags |= XLOG_TIC_INITED;
>  	}
> -
>  	xfs_log_ticket_put(ticket);
> -	return lsn;
>  }
>  
>  static bool
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index cc77cc36560a..1412d6993f1e 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -105,10 +105,6 @@ struct xfs_log_item;
>  struct xfs_item_ops;
>  struct xfs_trans;
>  
> -xfs_lsn_t xfs_log_done(struct xfs_mount *mp,
> -		       struct xlog_ticket *ticket,
> -		       struct xlog_in_core **iclog,
> -		       bool regrant);
>  int	  xfs_log_force(struct xfs_mount *mp, uint flags);
>  int	  xfs_log_force_lsn(struct xfs_mount *mp, xfs_lsn_t lsn, uint flags,
>  		int *log_forced);
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 64cc0bf2ab3b..880de1aa4288 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -839,10 +839,11 @@ xlog_cil_push_work(
>  	}
>  	spin_unlock(&cil->xc_push_lock);
>  
> -	/* xfs_log_done always frees the ticket on error. */
> -	commit_lsn = xfs_log_done(log->l_mp, tic, &commit_iclog, false);
> -	if (commit_lsn == -1)
> -		goto out_abort;
> +	error = xlog_write_done(log, tic, &commit_iclog, &commit_lsn);
> +	if (error)
> +		goto out_abort_free_ticket;
> +
> +	xlog_ticket_done(log, tic, false);
>  
>  	spin_lock(&commit_iclog->ic_callback_lock);
>  	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
> @@ -875,7 +876,7 @@ xlog_cil_push_work(
>  	return;
>  
>  out_abort_free_ticket:
> -	xfs_log_ticket_put(tic);
> +	xlog_ticket_done(log, tic, false);
>  out_abort:
>  	ASSERT(XLOG_FORCED_SHUTDOWN(log));
>  	xlog_cil_committed(ctx);
> @@ -1007,7 +1008,7 @@ xfs_log_commit_cil(
>  	if (commit_lsn)
>  		*commit_lsn = xc_commit_lsn;
>  
> -	xfs_log_done(mp, tp->t_ticket, NULL, regrant);
> +	xlog_ticket_done(log, tp->t_ticket, regrant);
>  	tp->t_ticket = NULL;
>  	xfs_trans_unreserve_and_mod_sb(tp);
>  
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 2b0aec37e73e..32bb6856e69d 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -439,14 +439,14 @@ xlog_write_adv_cnt(void **ptr, int *len, int *off, size_t bytes)
>  
>  void	xlog_print_tic_res(struct xfs_mount *mp, struct xlog_ticket *ticket);
>  void	xlog_print_trans(struct xfs_trans *);
> -int
> -xlog_write(
> -	struct xlog		*log,
> -	struct xfs_log_vec	*log_vector,
> -	struct xlog_ticket	*tic,
> -	xfs_lsn_t		*start_lsn,
> -	struct xlog_in_core	**commit_iclog,
> -	uint			flags);
> +
> +int xlog_write(struct xlog *log, struct xfs_log_vec *log_vector,
> +			struct xlog_ticket *tic, xfs_lsn_t *start_lsn,
> +			struct xlog_in_core **commit_iclog, uint flags);
> +int xlog_write_done(struct xlog *log, struct xlog_ticket *ticket,
> +			struct xlog_in_core **iclog, xfs_lsn_t *lsn);
> +void xlog_ticket_done(struct xlog *log, struct xlog_ticket *ticket,
> +			bool regrant);
>  
>  /*
>   * When we crack an atomic LSN, we sample it first so that the value will not
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 73c534093f09..123ecc8435f6 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -9,6 +9,7 @@
>  #include "xfs_shared.h"
>  #include "xfs_format.h"
>  #include "xfs_log_format.h"
> +#include "xfs_log_priv.h"
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
>  #include "xfs_extent_busy.h"
> @@ -150,8 +151,9 @@ xfs_trans_reserve(
>  	uint			blocks,
>  	uint			rtextents)
>  {
> -	int		error = 0;
> -	bool		rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0;
> +	struct xfs_mount	*mp = tp->t_mountp;
> +	int			error = 0;
> +	bool			rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0;
>  
>  	/* Mark this thread as being in a transaction */
>  	current_set_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> @@ -162,7 +164,7 @@ xfs_trans_reserve(
>  	 * fail if the count would go below zero.
>  	 */
>  	if (blocks > 0) {
> -		error = xfs_mod_fdblocks(tp->t_mountp, -((int64_t)blocks), rsvd);
> +		error = xfs_mod_fdblocks(mp, -((int64_t)blocks), rsvd);
>  		if (error != 0) {
>  			current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
>  			return -ENOSPC;
> @@ -191,9 +193,9 @@ xfs_trans_reserve(
>  
>  		if (tp->t_ticket != NULL) {
>  			ASSERT(resp->tr_logflags & XFS_TRANS_PERM_LOG_RES);
> -			error = xfs_log_regrant(tp->t_mountp, tp->t_ticket);
> +			error = xfs_log_regrant(mp, tp->t_ticket);
>  		} else {
> -			error = xfs_log_reserve(tp->t_mountp,
> +			error = xfs_log_reserve(mp,
>  						resp->tr_logres,
>  						resp->tr_logcount,
>  						&tp->t_ticket, XFS_TRANSACTION,
> @@ -213,7 +215,7 @@ xfs_trans_reserve(
>  	 * fail if the count would go below zero.
>  	 */
>  	if (rtextents > 0) {
> -		error = xfs_mod_frextents(tp->t_mountp, -((int64_t)rtextents));
> +		error = xfs_mod_frextents(mp, -((int64_t)rtextents));
>  		if (error) {
>  			error = -ENOSPC;
>  			goto undo_log;
> @@ -229,7 +231,7 @@ xfs_trans_reserve(
>  	 */
>  undo_log:
>  	if (resp->tr_logres > 0) {
> -		xfs_log_done(tp->t_mountp, tp->t_ticket, NULL, false);
> +		xlog_ticket_done(mp->m_log, tp->t_ticket, false);
>  		tp->t_ticket = NULL;
>  		tp->t_log_res = 0;
>  		tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
> @@ -237,7 +239,7 @@ xfs_trans_reserve(
>  
>  undo_blocks:
>  	if (blocks > 0) {
> -		xfs_mod_fdblocks(tp->t_mountp, (int64_t)blocks, rsvd);
> +		xfs_mod_fdblocks(mp, (int64_t)blocks, rsvd);
>  		tp->t_blk_res = 0;
>  	}
>  
> @@ -999,9 +1001,7 @@ __xfs_trans_commit(
>  	 */
>  	xfs_trans_unreserve_and_mod_dquots(tp);
>  	if (tp->t_ticket) {
> -		commit_lsn = xfs_log_done(mp, tp->t_ticket, NULL, regrant);
> -		if (commit_lsn == -1 && !error)
> -			error = -EIO;
> +		xlog_ticket_done(mp->m_log, tp->t_ticket, regrant);
>  		tp->t_ticket = NULL;
>  	}
>  	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> @@ -1060,7 +1060,7 @@ xfs_trans_cancel(
>  	xfs_trans_unreserve_and_mod_dquots(tp);
>  
>  	if (tp->t_ticket) {
> -		xfs_log_done(mp, tp->t_ticket, NULL, false);
> +		xlog_ticket_done(mp->m_log, tp->t_ticket, false);
>  		tp->t_ticket = NULL;
>  	}
>  
> -- 
> 2.25.1
> 

[PATCH 4/8] xfs: kill XLOG_TIC_INITED

[Christoph Hellwig]

From: Dave Chinner <dchinner@redhat.com>

It is not longer used or checked by anything, so remove the last
traces from the log ticket code.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_log.c      | 4 ----
 fs/xfs/xfs_log_priv.h | 6 ++----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 528ace7a6bb9..b30bb6452494 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2346,9 +2346,6 @@ xlog_write(
 	 * to account for an extra xlog_op_header here.
 	 */
 	ticket->t_curr_res -= sizeof(struct xlog_op_header);
-	if (ticket->t_flags & XLOG_TIC_INITED)
-		ticket->t_flags &= ~XLOG_TIC_INITED;
-
 	if (ticket->t_curr_res < 0) {
 		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
 		     "ctx ticket reservation ran out. Need to up reservation");
@@ -3488,7 +3485,6 @@ xlog_ticket_alloc(
 	tic->t_ocnt		= cnt;
 	tic->t_tid		= prandom_u32();
 	tic->t_clientid		= client;
-	tic->t_flags		= XLOG_TIC_INITED;
 	if (permanent)
 		tic->t_flags |= XLOG_TIC_PERM_RESERV;
 
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 32bb6856e69d..cfe5295ef4e3 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -51,13 +51,11 @@ enum xlog_iclog_state {
 };
 
 /*
- * Flags to log ticket
+ * Log ticket flags
  */
-#define XLOG_TIC_INITED		0x1	/* has been initialized */
-#define XLOG_TIC_PERM_RESERV	0x2	/* permanent reservation */
+#define XLOG_TIC_PERM_RESERV	0x1	/* permanent reservation */
 
 #define XLOG_TIC_FLAGS \
-	{ XLOG_TIC_INITED,	"XLOG_TIC_INITED" }, \
 	{ XLOG_TIC_PERM_RESERV,	"XLOG_TIC_PERM_RESERV" }
 
 /*
-- 
2.25.1

Re: [PATCH 4/8] xfs: kill XLOG_TIC_INITED

[Darrick J. Wong]

On Tue, Mar 24, 2020 at 06:44:55PM +0100, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> It is not longer used or checked by anything, so remove the last
> traces from the log ticket code.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log.c      | 4 ----
>  fs/xfs/xfs_log_priv.h | 6 ++----
>  2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 528ace7a6bb9..b30bb6452494 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2346,9 +2346,6 @@ xlog_write(
>  	 * to account for an extra xlog_op_header here.
>  	 */
>  	ticket->t_curr_res -= sizeof(struct xlog_op_header);
> -	if (ticket->t_flags & XLOG_TIC_INITED)
> -		ticket->t_flags &= ~XLOG_TIC_INITED;
> -
>  	if (ticket->t_curr_res < 0) {
>  		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
>  		     "ctx ticket reservation ran out. Need to up reservation");
> @@ -3488,7 +3485,6 @@ xlog_ticket_alloc(
>  	tic->t_ocnt		= cnt;
>  	tic->t_tid		= prandom_u32();
>  	tic->t_clientid		= client;
> -	tic->t_flags		= XLOG_TIC_INITED;
>  	if (permanent)
>  		tic->t_flags |= XLOG_TIC_PERM_RESERV;
>  
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 32bb6856e69d..cfe5295ef4e3 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -51,13 +51,11 @@ enum xlog_iclog_state {
>  };
>  
>  /*
> - * Flags to log ticket
> + * Log ticket flags
>   */
> -#define XLOG_TIC_INITED		0x1	/* has been initialized */
> -#define XLOG_TIC_PERM_RESERV	0x2	/* permanent reservation */
> +#define XLOG_TIC_PERM_RESERV	0x1	/* permanent reservation */
>  
>  #define XLOG_TIC_FLAGS \
> -	{ XLOG_TIC_INITED,	"XLOG_TIC_INITED" }, \
>  	{ XLOG_TIC_PERM_RESERV,	"XLOG_TIC_PERM_RESERV" }
>  
>  /*
> -- 
> 2.25.1
> 

[PATCH 5/8] xfs: split xlog_ticket_done

[Christoph Hellwig]

Split the regrant case out of xlog_ticket_done and into a new
xlog_ticket_regrant helper.  Merge both functions with the low-level
functions implementing the actual functionality and adjust the
tracepoints.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log.c      | 84 ++++++++++++++-----------------------------
 fs/xfs/xfs_log_cil.c  |  9 +++--
 fs/xfs/xfs_log_priv.h |  4 +--
 fs/xfs/xfs_trace.h    | 14 ++++----
 fs/xfs/xfs_trans.c    |  9 +++--
 5 files changed, 47 insertions(+), 73 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index b30bb6452494..9a26ee8db238 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -66,14 +66,6 @@ xlog_grant_push_ail(
 	struct xlog		*log,
 	int			need_bytes);
 STATIC void
-xlog_regrant_reserve_log_space(
-	struct xlog		*log,
-	struct xlog_ticket	*ticket);
-STATIC void
-xlog_ungrant_log_space(
-	struct xlog		*log,
-	struct xlog_ticket	*ticket);
-STATIC void
 xlog_sync(
 	struct xlog		*log,
 	struct xlog_in_core	*iclog);
@@ -502,27 +494,6 @@ xlog_write_done(
 	return xlog_commit_record(log, ticket, iclog, lsn);
 }
 
-/*
- * Release or regrant the ticket reservation now the transaction is done with
- * it depending on caller context. Rolling transactions need the ticket
- * regranted, otherwise we release it completely.
- */
-void
-xlog_ticket_done(
-	struct xlog		*log,
-	struct xlog_ticket	*ticket,
-	bool			regrant)
-{
-	if (!regrant || XLOG_FORCED_SHUTDOWN(log)) {
-		trace_xfs_log_done_nonperm(log, ticket);
-		xlog_ungrant_log_space(log, ticket);
-	} else {
-		trace_xfs_log_done_perm(log, ticket);
-		xlog_regrant_reserve_log_space(log, ticket);
-	}
-	xfs_log_ticket_put(ticket);
-}
-
 static bool
 __xlog_state_release_iclog(
 	struct xlog		*log,
@@ -921,8 +892,7 @@ xfs_log_write_unmount_record(
 
 	if (tic) {
 		trace_xfs_log_umount_write(log, tic);
-		xlog_ungrant_log_space(log, tic);
-		xfs_log_ticket_put(tic);
+		xlog_ticket_done(log, tic);
 	}
 }
 
@@ -2987,19 +2957,18 @@ xlog_state_get_iclog_space(
 	return 0;
 }	/* xlog_state_get_iclog_space */
 
-/* The first cnt-1 times through here we don't need to
- * move the grant write head because the permanent
- * reservation has reserved cnt times the unit amount.
- * Release part of current permanent unit reservation and
- * reset current reservation to be one units worth.  Also
- * move grant reservation head forward.
+/*
+ * The first cnt-1 times through here we don't need to move the grant write head
+ * because the permanent reservation has reserved cnt times the unit amount.
+ * Release part of current permanent unit reservation and reset current
+ * reservation to be one units worth.  Also move grant reservation head forward.
  */
-STATIC void
-xlog_regrant_reserve_log_space(
+void
+xlog_ticket_regrant(
 	struct xlog		*log,
 	struct xlog_ticket	*ticket)
 {
-	trace_xfs_log_regrant_reserve_enter(log, ticket);
+	trace_xfs_log_ticket_regrant(log, ticket);
 
 	if (ticket->t_cnt > 0)
 		ticket->t_cnt--;
@@ -3011,21 +2980,20 @@ xlog_regrant_reserve_log_space(
 	ticket->t_curr_res = ticket->t_unit_res;
 	xlog_tic_reset_res(ticket);
 
-	trace_xfs_log_regrant_reserve_sub(log, ticket);
+	trace_xfs_log_ticket_regrant_sub(log, ticket);
 
 	/* just return if we still have some of the pre-reserved space */
-	if (ticket->t_cnt > 0)
-		return;
+	if (!ticket->t_cnt) {
+		xlog_grant_add_space(log, &log->l_reserve_head.grant,
+				     ticket->t_unit_res);
+		trace_xfs_log_ticket_regrant_exit(log, ticket);
 
-	xlog_grant_add_space(log, &log->l_reserve_head.grant,
-					ticket->t_unit_res);
-
-	trace_xfs_log_regrant_reserve_exit(log, ticket);
-
-	ticket->t_curr_res = ticket->t_unit_res;
-	xlog_tic_reset_res(ticket);
-}	/* xlog_regrant_reserve_log_space */
+		ticket->t_curr_res = ticket->t_unit_res;
+		xlog_tic_reset_res(ticket);
+	}
 
+	xfs_log_ticket_put(ticket);
+}
 
 /*
  * Give back the space left from a reservation.
@@ -3041,18 +3009,19 @@ xlog_regrant_reserve_log_space(
  * space, the count will stay at zero and the only space remaining will be
  * in the current reservation field.
  */
-STATIC void
-xlog_ungrant_log_space(
+void
+xlog_ticket_done(
 	struct xlog		*log,
 	struct xlog_ticket	*ticket)
 {
-	int	bytes;
+	int			bytes;
+
+	trace_xfs_log_ticket_done(log, ticket);
 
 	if (ticket->t_cnt > 0)
 		ticket->t_cnt--;
 
-	trace_xfs_log_ungrant_enter(log, ticket);
-	trace_xfs_log_ungrant_sub(log, ticket);
+	trace_xfs_log_ticket_done_sub(log, ticket);
 
 	/*
 	 * If this is a permanent reservation ticket, we may be able to free
@@ -3067,9 +3036,10 @@ xlog_ungrant_log_space(
 	xlog_grant_sub_space(log, &log->l_reserve_head.grant, bytes);
 	xlog_grant_sub_space(log, &log->l_write_head.grant, bytes);
 
-	trace_xfs_log_ungrant_exit(log, ticket);
+	trace_xfs_log_ticket_done_exit(log, ticket);
 
 	xfs_log_space_wake(log->l_mp);
+	xfs_log_ticket_put(ticket);
 }
 
 /*
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 880de1aa4288..1189c8cfa525 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -843,7 +843,7 @@ xlog_cil_push_work(
 	if (error)
 		goto out_abort_free_ticket;
 
-	xlog_ticket_done(log, tic, false);
+	xlog_ticket_done(log, tic);
 
 	spin_lock(&commit_iclog->ic_callback_lock);
 	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
@@ -876,7 +876,7 @@ xlog_cil_push_work(
 	return;
 
 out_abort_free_ticket:
-	xlog_ticket_done(log, tic, false);
+	xlog_ticket_done(log, tic);
 out_abort:
 	ASSERT(XLOG_FORCED_SHUTDOWN(log));
 	xlog_cil_committed(ctx);
@@ -1008,7 +1008,10 @@ xfs_log_commit_cil(
 	if (commit_lsn)
 		*commit_lsn = xc_commit_lsn;
 
-	xlog_ticket_done(log, tp->t_ticket, regrant);
+	if (regrant && !XLOG_FORCED_SHUTDOWN(log))
+		xlog_ticket_regrant(log, tp->t_ticket);
+	else
+		xlog_ticket_done(log, tp->t_ticket);
 	tp->t_ticket = NULL;
 	xfs_trans_unreserve_and_mod_sb(tp);
 
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index cfe5295ef4e3..cfcf3f02e30a 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -443,8 +443,8 @@ int xlog_write(struct xlog *log, struct xfs_log_vec *log_vector,
 			struct xlog_in_core **commit_iclog, uint flags);
 int xlog_write_done(struct xlog *log, struct xlog_ticket *ticket,
 			struct xlog_in_core **iclog, xfs_lsn_t *lsn);
-void xlog_ticket_done(struct xlog *log, struct xlog_ticket *ticket,
-			bool regrant);
+void xlog_ticket_done(struct xlog *log, struct xlog_ticket *ticket);
+void xlog_ticket_regrant(struct xlog *log, struct xlog_ticket *ticket);
 
 /*
  * When we crack an atomic LSN, we sample it first so that the value will not
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index efc7751550d9..fbfdd9cf160d 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -1001,8 +1001,6 @@ DECLARE_EVENT_CLASS(xfs_loggrant_class,
 DEFINE_EVENT(xfs_loggrant_class, name, \
 	TP_PROTO(struct xlog *log, struct xlog_ticket *tic), \
 	TP_ARGS(log, tic))
-DEFINE_LOGGRANT_EVENT(xfs_log_done_nonperm);
-DEFINE_LOGGRANT_EVENT(xfs_log_done_perm);
 DEFINE_LOGGRANT_EVENT(xfs_log_umount_write);
 DEFINE_LOGGRANT_EVENT(xfs_log_grant_sleep);
 DEFINE_LOGGRANT_EVENT(xfs_log_grant_wake);
@@ -1011,12 +1009,12 @@ DEFINE_LOGGRANT_EVENT(xfs_log_reserve);
 DEFINE_LOGGRANT_EVENT(xfs_log_reserve_exit);
 DEFINE_LOGGRANT_EVENT(xfs_log_regrant);
 DEFINE_LOGGRANT_EVENT(xfs_log_regrant_exit);
-DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_enter);
-DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_exit);
-DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_sub);
-DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_enter);
-DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_exit);
-DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_sub);
+DEFINE_LOGGRANT_EVENT(xfs_log_ticket_regrant);
+DEFINE_LOGGRANT_EVENT(xfs_log_ticket_regrant_exit);
+DEFINE_LOGGRANT_EVENT(xfs_log_ticket_regrant_sub);
+DEFINE_LOGGRANT_EVENT(xfs_log_ticket_done);
+DEFINE_LOGGRANT_EVENT(xfs_log_ticket_done_sub);
+DEFINE_LOGGRANT_EVENT(xfs_log_ticket_done_exit);
 
 DECLARE_EVENT_CLASS(xfs_log_item_class,
 	TP_PROTO(struct xfs_log_item *lip),
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 123ecc8435f6..d7c66c3331ec 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -231,7 +231,7 @@ xfs_trans_reserve(
 	 */
 undo_log:
 	if (resp->tr_logres > 0) {
-		xlog_ticket_done(mp->m_log, tp->t_ticket, false);
+		xlog_ticket_done(mp->m_log, tp->t_ticket);
 		tp->t_ticket = NULL;
 		tp->t_log_res = 0;
 		tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
@@ -1001,7 +1001,10 @@ __xfs_trans_commit(
 	 */
 	xfs_trans_unreserve_and_mod_dquots(tp);
 	if (tp->t_ticket) {
-		xlog_ticket_done(mp->m_log, tp->t_ticket, regrant);
+		if (regrant && !XLOG_FORCED_SHUTDOWN(mp->m_log))
+			xlog_ticket_regrant(mp->m_log, tp->t_ticket);
+		else
+			xlog_ticket_done(mp->m_log, tp->t_ticket);
 		tp->t_ticket = NULL;
 	}
 	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
@@ -1060,7 +1063,7 @@ xfs_trans_cancel(
 	xfs_trans_unreserve_and_mod_dquots(tp);
 
 	if (tp->t_ticket) {
-		xlog_ticket_done(mp->m_log, tp->t_ticket, false);
+		xlog_ticket_done(mp->m_log, tp->t_ticket);
 		tp->t_ticket = NULL;
 	}
 
-- 
2.25.1

Re: [PATCH 5/8] xfs: split xlog_ticket_done

[Darrick J. Wong]

On Tue, Mar 24, 2020 at 06:44:56PM +0100, Christoph Hellwig wrote:
> Split the regrant case out of xlog_ticket_done and into a new
> xlog_ticket_regrant helper.  Merge both functions with the low-level
> functions implementing the actual functionality and adjust the
> tracepoints.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

More cohesive functions FTW,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log.c      | 84 ++++++++++++++-----------------------------
>  fs/xfs/xfs_log_cil.c  |  9 +++--
>  fs/xfs/xfs_log_priv.h |  4 +--
>  fs/xfs/xfs_trace.h    | 14 ++++----
>  fs/xfs/xfs_trans.c    |  9 +++--
>  5 files changed, 47 insertions(+), 73 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index b30bb6452494..9a26ee8db238 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -66,14 +66,6 @@ xlog_grant_push_ail(
>  	struct xlog		*log,
>  	int			need_bytes);
>  STATIC void
> -xlog_regrant_reserve_log_space(
> -	struct xlog		*log,
> -	struct xlog_ticket	*ticket);
> -STATIC void
> -xlog_ungrant_log_space(
> -	struct xlog		*log,
> -	struct xlog_ticket	*ticket);
> -STATIC void
>  xlog_sync(
>  	struct xlog		*log,
>  	struct xlog_in_core	*iclog);
> @@ -502,27 +494,6 @@ xlog_write_done(
>  	return xlog_commit_record(log, ticket, iclog, lsn);
>  }
>  
> -/*
> - * Release or regrant the ticket reservation now the transaction is done with
> - * it depending on caller context. Rolling transactions need the ticket
> - * regranted, otherwise we release it completely.
> - */
> -void
> -xlog_ticket_done(
> -	struct xlog		*log,
> -	struct xlog_ticket	*ticket,
> -	bool			regrant)
> -{
> -	if (!regrant || XLOG_FORCED_SHUTDOWN(log)) {
> -		trace_xfs_log_done_nonperm(log, ticket);
> -		xlog_ungrant_log_space(log, ticket);
> -	} else {
> -		trace_xfs_log_done_perm(log, ticket);
> -		xlog_regrant_reserve_log_space(log, ticket);
> -	}
> -	xfs_log_ticket_put(ticket);
> -}
> -
>  static bool
>  __xlog_state_release_iclog(
>  	struct xlog		*log,
> @@ -921,8 +892,7 @@ xfs_log_write_unmount_record(
>  
>  	if (tic) {
>  		trace_xfs_log_umount_write(log, tic);
> -		xlog_ungrant_log_space(log, tic);
> -		xfs_log_ticket_put(tic);
> +		xlog_ticket_done(log, tic);
>  	}
>  }
>  
> @@ -2987,19 +2957,18 @@ xlog_state_get_iclog_space(
>  	return 0;
>  }	/* xlog_state_get_iclog_space */
>  
> -/* The first cnt-1 times through here we don't need to
> - * move the grant write head because the permanent
> - * reservation has reserved cnt times the unit amount.
> - * Release part of current permanent unit reservation and
> - * reset current reservation to be one units worth.  Also
> - * move grant reservation head forward.
> +/*
> + * The first cnt-1 times through here we don't need to move the grant write head
> + * because the permanent reservation has reserved cnt times the unit amount.
> + * Release part of current permanent unit reservation and reset current
> + * reservation to be one units worth.  Also move grant reservation head forward.
>   */
> -STATIC void
> -xlog_regrant_reserve_log_space(
> +void
> +xlog_ticket_regrant(
>  	struct xlog		*log,
>  	struct xlog_ticket	*ticket)
>  {
> -	trace_xfs_log_regrant_reserve_enter(log, ticket);
> +	trace_xfs_log_ticket_regrant(log, ticket);
>  
>  	if (ticket->t_cnt > 0)
>  		ticket->t_cnt--;
> @@ -3011,21 +2980,20 @@ xlog_regrant_reserve_log_space(
>  	ticket->t_curr_res = ticket->t_unit_res;
>  	xlog_tic_reset_res(ticket);
>  
> -	trace_xfs_log_regrant_reserve_sub(log, ticket);
> +	trace_xfs_log_ticket_regrant_sub(log, ticket);
>  
>  	/* just return if we still have some of the pre-reserved space */
> -	if (ticket->t_cnt > 0)
> -		return;
> +	if (!ticket->t_cnt) {
> +		xlog_grant_add_space(log, &log->l_reserve_head.grant,
> +				     ticket->t_unit_res);
> +		trace_xfs_log_ticket_regrant_exit(log, ticket);
>  
> -	xlog_grant_add_space(log, &log->l_reserve_head.grant,
> -					ticket->t_unit_res);
> -
> -	trace_xfs_log_regrant_reserve_exit(log, ticket);
> -
> -	ticket->t_curr_res = ticket->t_unit_res;
> -	xlog_tic_reset_res(ticket);
> -}	/* xlog_regrant_reserve_log_space */
> +		ticket->t_curr_res = ticket->t_unit_res;
> +		xlog_tic_reset_res(ticket);
> +	}
>  
> +	xfs_log_ticket_put(ticket);
> +}
>  
>  /*
>   * Give back the space left from a reservation.
> @@ -3041,18 +3009,19 @@ xlog_regrant_reserve_log_space(
>   * space, the count will stay at zero and the only space remaining will be
>   * in the current reservation field.
>   */
> -STATIC void
> -xlog_ungrant_log_space(
> +void
> +xlog_ticket_done(
>  	struct xlog		*log,
>  	struct xlog_ticket	*ticket)
>  {
> -	int	bytes;
> +	int			bytes;
> +
> +	trace_xfs_log_ticket_done(log, ticket);
>  
>  	if (ticket->t_cnt > 0)
>  		ticket->t_cnt--;
>  
> -	trace_xfs_log_ungrant_enter(log, ticket);
> -	trace_xfs_log_ungrant_sub(log, ticket);
> +	trace_xfs_log_ticket_done_sub(log, ticket);
>  
>  	/*
>  	 * If this is a permanent reservation ticket, we may be able to free
> @@ -3067,9 +3036,10 @@ xlog_ungrant_log_space(
>  	xlog_grant_sub_space(log, &log->l_reserve_head.grant, bytes);
>  	xlog_grant_sub_space(log, &log->l_write_head.grant, bytes);
>  
> -	trace_xfs_log_ungrant_exit(log, ticket);
> +	trace_xfs_log_ticket_done_exit(log, ticket);
>  
>  	xfs_log_space_wake(log->l_mp);
> +	xfs_log_ticket_put(ticket);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 880de1aa4288..1189c8cfa525 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -843,7 +843,7 @@ xlog_cil_push_work(
>  	if (error)
>  		goto out_abort_free_ticket;
>  
> -	xlog_ticket_done(log, tic, false);
> +	xlog_ticket_done(log, tic);
>  
>  	spin_lock(&commit_iclog->ic_callback_lock);
>  	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
> @@ -876,7 +876,7 @@ xlog_cil_push_work(
>  	return;
>  
>  out_abort_free_ticket:
> -	xlog_ticket_done(log, tic, false);
> +	xlog_ticket_done(log, tic);
>  out_abort:
>  	ASSERT(XLOG_FORCED_SHUTDOWN(log));
>  	xlog_cil_committed(ctx);
> @@ -1008,7 +1008,10 @@ xfs_log_commit_cil(
>  	if (commit_lsn)
>  		*commit_lsn = xc_commit_lsn;
>  
> -	xlog_ticket_done(log, tp->t_ticket, regrant);
> +	if (regrant && !XLOG_FORCED_SHUTDOWN(log))
> +		xlog_ticket_regrant(log, tp->t_ticket);
> +	else
> +		xlog_ticket_done(log, tp->t_ticket);
>  	tp->t_ticket = NULL;
>  	xfs_trans_unreserve_and_mod_sb(tp);
>  
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index cfe5295ef4e3..cfcf3f02e30a 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -443,8 +443,8 @@ int xlog_write(struct xlog *log, struct xfs_log_vec *log_vector,
>  			struct xlog_in_core **commit_iclog, uint flags);
>  int xlog_write_done(struct xlog *log, struct xlog_ticket *ticket,
>  			struct xlog_in_core **iclog, xfs_lsn_t *lsn);
> -void xlog_ticket_done(struct xlog *log, struct xlog_ticket *ticket,
> -			bool regrant);
> +void xlog_ticket_done(struct xlog *log, struct xlog_ticket *ticket);
> +void xlog_ticket_regrant(struct xlog *log, struct xlog_ticket *ticket);
>  
>  /*
>   * When we crack an atomic LSN, we sample it first so that the value will not
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index efc7751550d9..fbfdd9cf160d 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -1001,8 +1001,6 @@ DECLARE_EVENT_CLASS(xfs_loggrant_class,
>  DEFINE_EVENT(xfs_loggrant_class, name, \
>  	TP_PROTO(struct xlog *log, struct xlog_ticket *tic), \
>  	TP_ARGS(log, tic))
> -DEFINE_LOGGRANT_EVENT(xfs_log_done_nonperm);
> -DEFINE_LOGGRANT_EVENT(xfs_log_done_perm);
>  DEFINE_LOGGRANT_EVENT(xfs_log_umount_write);
>  DEFINE_LOGGRANT_EVENT(xfs_log_grant_sleep);
>  DEFINE_LOGGRANT_EVENT(xfs_log_grant_wake);
> @@ -1011,12 +1009,12 @@ DEFINE_LOGGRANT_EVENT(xfs_log_reserve);
>  DEFINE_LOGGRANT_EVENT(xfs_log_reserve_exit);
>  DEFINE_LOGGRANT_EVENT(xfs_log_regrant);
>  DEFINE_LOGGRANT_EVENT(xfs_log_regrant_exit);
> -DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_enter);
> -DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_exit);
> -DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_sub);
> -DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_enter);
> -DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_exit);
> -DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_sub);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_regrant);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_regrant_exit);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_regrant_sub);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_done);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_done_sub);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_done_exit);
>  
>  DECLARE_EVENT_CLASS(xfs_log_item_class,
>  	TP_PROTO(struct xfs_log_item *lip),
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 123ecc8435f6..d7c66c3331ec 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -231,7 +231,7 @@ xfs_trans_reserve(
>  	 */
>  undo_log:
>  	if (resp->tr_logres > 0) {
> -		xlog_ticket_done(mp->m_log, tp->t_ticket, false);
> +		xlog_ticket_done(mp->m_log, tp->t_ticket);
>  		tp->t_ticket = NULL;
>  		tp->t_log_res = 0;
>  		tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
> @@ -1001,7 +1001,10 @@ __xfs_trans_commit(
>  	 */
>  	xfs_trans_unreserve_and_mod_dquots(tp);
>  	if (tp->t_ticket) {
> -		xlog_ticket_done(mp->m_log, tp->t_ticket, regrant);
> +		if (regrant && !XLOG_FORCED_SHUTDOWN(mp->m_log))
> +			xlog_ticket_regrant(mp->m_log, tp->t_ticket);
> +		else
> +			xlog_ticket_done(mp->m_log, tp->t_ticket);
>  		tp->t_ticket = NULL;
>  	}
>  	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> @@ -1060,7 +1063,7 @@ xfs_trans_cancel(
>  	xfs_trans_unreserve_and_mod_dquots(tp);
>  
>  	if (tp->t_ticket) {
> -		xlog_ticket_done(mp->m_log, tp->t_ticket, false);
> +		xlog_ticket_done(mp->m_log, tp->t_ticket);
>  		tp->t_ticket = NULL;
>  	}
>  
> -- 
> 2.25.1
> 

Re: [PATCH 5/8] xfs: split xlog_ticket_done

[Brian Foster]

On Tue, Mar 24, 2020 at 06:44:56PM +0100, Christoph Hellwig wrote:
> Split the regrant case out of xlog_ticket_done and into a new
> xlog_ticket_regrant helper.  Merge both functions with the low-level
> functions implementing the actual functionality and adjust the
> tracepoints.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log.c      | 84 ++++++++++++++-----------------------------
>  fs/xfs/xfs_log_cil.c  |  9 +++--
>  fs/xfs/xfs_log_priv.h |  4 +--
>  fs/xfs/xfs_trace.h    | 14 ++++----
>  fs/xfs/xfs_trans.c    |  9 +++--
>  5 files changed, 47 insertions(+), 73 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index efc7751550d9..fbfdd9cf160d 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
...
> @@ -1011,12 +1009,12 @@ DEFINE_LOGGRANT_EVENT(xfs_log_reserve);
>  DEFINE_LOGGRANT_EVENT(xfs_log_reserve_exit);
>  DEFINE_LOGGRANT_EVENT(xfs_log_regrant);
>  DEFINE_LOGGRANT_EVENT(xfs_log_regrant_exit);
> -DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_enter);
> -DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_exit);
> -DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_sub);
> -DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_enter);
> -DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_exit);
> -DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_sub);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_regrant);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_regrant_exit);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_regrant_sub);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_done);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_done_sub);
> +DEFINE_LOGGRANT_EVENT(xfs_log_ticket_done_exit);
>  

Any reason we carry over the vague 'done' naming to the lower level
functions? xlog_ticket_[re|un]grant() seems more consistent and explicit
to me, but either way:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  DECLARE_EVENT_CLASS(xfs_log_item_class,
>  	TP_PROTO(struct xfs_log_item *lip),
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 123ecc8435f6..d7c66c3331ec 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -231,7 +231,7 @@ xfs_trans_reserve(
>  	 */
>  undo_log:
>  	if (resp->tr_logres > 0) {
> -		xlog_ticket_done(mp->m_log, tp->t_ticket, false);
> +		xlog_ticket_done(mp->m_log, tp->t_ticket);
>  		tp->t_ticket = NULL;
>  		tp->t_log_res = 0;
>  		tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
> @@ -1001,7 +1001,10 @@ __xfs_trans_commit(
>  	 */
>  	xfs_trans_unreserve_and_mod_dquots(tp);
>  	if (tp->t_ticket) {
> -		xlog_ticket_done(mp->m_log, tp->t_ticket, regrant);
> +		if (regrant && !XLOG_FORCED_SHUTDOWN(mp->m_log))
> +			xlog_ticket_regrant(mp->m_log, tp->t_ticket);
> +		else
> +			xlog_ticket_done(mp->m_log, tp->t_ticket);
>  		tp->t_ticket = NULL;
>  	}
>  	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> @@ -1060,7 +1063,7 @@ xfs_trans_cancel(
>  	xfs_trans_unreserve_and_mod_dquots(tp);
>  
>  	if (tp->t_ticket) {
> -		xlog_ticket_done(mp->m_log, tp->t_ticket, false);
> +		xlog_ticket_done(mp->m_log, tp->t_ticket);
>  		tp->t_ticket = NULL;
>  	}
>  
> -- 
> 2.25.1
> 

[PATCH 6/8] xfs: merge xlog_commit_record with xlog_write_done()

[Christoph Hellwig]

From: Dave Chinner <dchinner@redhat.com>

xlog_write_done() is just a thin wrapper around
xlog_commit_record(), so they can be merged together easily. Convert
all the xlog_commit_record() callers to use xlog_write_done() and
merge the implementations.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_log.c | 44 +++++++++++---------------------------------
 1 file changed, 11 insertions(+), 33 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 9a26ee8db238..a173b5925d1b 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -24,13 +24,6 @@
 kmem_zone_t	*xfs_log_ticket_zone;
 
 /* Local miscellaneous function prototypes */
-STATIC int
-xlog_commit_record(
-	struct xlog		*log,
-	struct xlog_ticket	*ticket,
-	struct xlog_in_core	**iclog,
-	xfs_lsn_t		*commitlsnp);
-
 STATIC struct xlog *
 xlog_alloc_log(
 	struct xfs_mount	*mp,
@@ -478,22 +471,6 @@ xfs_log_reserve(
  *		marked as with WANT_SYNC.
  */
 
-/*
- * Write a commit record to the log to close off a running log write.
- */
-int
-xlog_write_done(
-	struct xlog		*log,
-	struct xlog_ticket	*ticket,
-	struct xlog_in_core	**iclog,
-	xfs_lsn_t		*lsn)
-{
-	if (XLOG_FORCED_SHUTDOWN(log))
-		return -EIO;
-
-	return xlog_commit_record(log, ticket, iclog, lsn);
-}
-
 static bool
 __xlog_state_release_iclog(
 	struct xlog		*log,
@@ -1463,20 +1440,17 @@ xlog_alloc_log(
 	return ERR_PTR(error);
 }	/* xlog_alloc_log */
 
-
 /*
  * Write out the commit record of a transaction associated with the given
- * ticket.  Return the lsn of the commit record.
+ * ticket to close off a running log write. Return the lsn of the commit record.
  */
-STATIC int
-xlog_commit_record(
+int
+xlog_write_done(
 	struct xlog		*log,
 	struct xlog_ticket	*ticket,
 	struct xlog_in_core	**iclog,
-	xfs_lsn_t		*commitlsnp)
+	xfs_lsn_t		*lsn)
 {
-	struct xfs_mount *mp = log->l_mp;
-	int	error;
 	struct xfs_log_iovec reg = {
 		.i_addr = NULL,
 		.i_len = 0,
@@ -1486,12 +1460,16 @@ xlog_commit_record(
 		.lv_niovecs = 1,
 		.lv_iovecp = &reg,
 	};
+	int	error;
 
 	ASSERT_ALWAYS(iclog);
-	error = xlog_write(log, &vec, ticket, commitlsnp, iclog,
-					XLOG_COMMIT_TRANS);
+
+	if (XLOG_FORCED_SHUTDOWN(log))
+		return -EIO;
+
+	error = xlog_write(log, &vec, ticket, lsn, iclog, XLOG_COMMIT_TRANS);
 	if (error)
-		xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
+		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
 	return error;
 }
 
-- 
2.25.1

Re: [PATCH 6/8] xfs: merge xlog_commit_record with xlog_write_done()

[Darrick J. Wong]

On Tue, Mar 24, 2020 at 06:44:57PM +0100, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xlog_write_done() is just a thin wrapper around
> xlog_commit_record(), so they can be merged together easily. Convert
> all the xlog_commit_record() callers to use xlog_write_done() and
> merge the implementations.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log.c | 44 +++++++++++---------------------------------
>  1 file changed, 11 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 9a26ee8db238..a173b5925d1b 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -24,13 +24,6 @@
>  kmem_zone_t	*xfs_log_ticket_zone;
>  
>  /* Local miscellaneous function prototypes */
> -STATIC int
> -xlog_commit_record(
> -	struct xlog		*log,
> -	struct xlog_ticket	*ticket,
> -	struct xlog_in_core	**iclog,
> -	xfs_lsn_t		*commitlsnp);
> -
>  STATIC struct xlog *
>  xlog_alloc_log(
>  	struct xfs_mount	*mp,
> @@ -478,22 +471,6 @@ xfs_log_reserve(
>   *		marked as with WANT_SYNC.
>   */
>  
> -/*
> - * Write a commit record to the log to close off a running log write.
> - */
> -int
> -xlog_write_done(
> -	struct xlog		*log,
> -	struct xlog_ticket	*ticket,
> -	struct xlog_in_core	**iclog,
> -	xfs_lsn_t		*lsn)
> -{
> -	if (XLOG_FORCED_SHUTDOWN(log))
> -		return -EIO;
> -
> -	return xlog_commit_record(log, ticket, iclog, lsn);
> -}
> -
>  static bool
>  __xlog_state_release_iclog(
>  	struct xlog		*log,
> @@ -1463,20 +1440,17 @@ xlog_alloc_log(
>  	return ERR_PTR(error);
>  }	/* xlog_alloc_log */
>  
> -
>  /*
>   * Write out the commit record of a transaction associated with the given
> - * ticket.  Return the lsn of the commit record.
> + * ticket to close off a running log write. Return the lsn of the commit record.
>   */
> -STATIC int
> -xlog_commit_record(
> +int
> +xlog_write_done(
>  	struct xlog		*log,
>  	struct xlog_ticket	*ticket,
>  	struct xlog_in_core	**iclog,
> -	xfs_lsn_t		*commitlsnp)
> +	xfs_lsn_t		*lsn)
>  {
> -	struct xfs_mount *mp = log->l_mp;
> -	int	error;
>  	struct xfs_log_iovec reg = {
>  		.i_addr = NULL,
>  		.i_len = 0,
> @@ -1486,12 +1460,16 @@ xlog_commit_record(
>  		.lv_niovecs = 1,
>  		.lv_iovecp = &reg,
>  	};
> +	int	error;
>  
>  	ASSERT_ALWAYS(iclog);
> -	error = xlog_write(log, &vec, ticket, commitlsnp, iclog,
> -					XLOG_COMMIT_TRANS);
> +
> +	if (XLOG_FORCED_SHUTDOWN(log))
> +		return -EIO;
> +
> +	error = xlog_write(log, &vec, ticket, lsn, iclog, XLOG_COMMIT_TRANS);
>  	if (error)
> -		xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
> +		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
>  	return error;
>  }
>  
> -- 
> 2.25.1
> 

[PATCH 7/8] xfs: refactor unmount record writing

[Christoph Hellwig]

From: Dave Chinner <dchinner@redhat.com>

Separate out the unmount record writing from the rest of the
ticket and log state futzing necessary to make it work. This is
a no-op, just makes the code cleaner and places the unmount record
formatting and writing alongside the commit record formatting and
writing code.

We can also get rid of the ticket flag clearing before the
xlog_write() call because it no longer cares about the state of
XLOG_TIC_INITED.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_log.c | 65 +++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a173b5925d1b..1d6ed696f717 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -471,6 +471,36 @@ xfs_log_reserve(
  *		marked as with WANT_SYNC.
  */
 
+/*
+ * Write out an unmount record using the ticket provided. We have to account for
+ * the data space used in the unmount ticket as this write is not done from a
+ * transaction context that has already done the accounting for us.
+ */
+static int
+xlog_write_unmount_record(
+	struct xlog		*log,
+	struct xlog_ticket	*ticket,
+	xfs_lsn_t		*lsn,
+	uint			flags)
+{
+	struct xfs_unmount_log_format ulf = {
+		.magic = XLOG_UNMOUNT_TYPE,
+	};
+	struct xfs_log_iovec reg = {
+		.i_addr = &ulf,
+		.i_len = sizeof(ulf),
+		.i_type = XLOG_REG_TYPE_UNMOUNT,
+	};
+	struct xfs_log_vec vec = {
+		.lv_niovecs = 1,
+		.lv_iovecp = &reg,
+	};
+
+	/* account for space used by record data */
+	ticket->t_curr_res -= sizeof(ulf);
+	return xlog_write(log, &vec, ticket, lsn, NULL, flags);
+}
+
 static bool
 __xlog_state_release_iclog(
 	struct xlog		*log,
@@ -795,32 +825,14 @@ xlog_wait_on_iclog(
 }
 
 /*
- * Final log writes as part of unmount.
- *
- * Mark the filesystem clean as unmount happens.  Note that during relocation
- * this routine needs to be executed as part of source-bag while the
- * deallocation must not be done until source-end.
+ * Mark the filesystem clean by writing an unmount record to the head of the
+ * log.
  */
-
-/* Actually write the unmount record to disk. */
 static void
-xfs_log_write_unmount_record(
-	struct xfs_mount	*mp)
+xlog_unmount_write(
+	struct xlog		*log)
 {
-	/* the data section must be 32 bit size aligned */
-	struct xfs_unmount_log_format magic = {
-		.magic = XLOG_UNMOUNT_TYPE,
-	};
-	struct xfs_log_iovec reg = {
-		.i_addr = &magic,
-		.i_len = sizeof(magic),
-		.i_type = XLOG_REG_TYPE_UNMOUNT,
-	};
-	struct xfs_log_vec vec = {
-		.lv_niovecs = 1,
-		.lv_iovecp = &reg,
-	};
-	struct xlog		*log = mp->m_log;
+	struct xfs_mount	*mp = log->l_mp;
 	struct xlog_in_core	*iclog;
 	struct xlog_ticket	*tic = NULL;
 	xfs_lsn_t		lsn;
@@ -844,10 +856,7 @@ xfs_log_write_unmount_record(
 		flags &= ~XLOG_UNMOUNT_TRANS;
 	}
 
-	/* remove inited flag, and account for space used */
-	tic->t_flags = 0;
-	tic->t_curr_res -= sizeof(magic);
-	error = xlog_write(log, &vec, tic, &lsn, NULL, flags);
+	error = xlog_write_unmount_record(log, tic, &lsn, flags);
 	/*
 	 * At this point, we're umounting anyway, so there's no point in
 	 * transitioning log state to IOERROR. Just continue...
@@ -913,7 +922,7 @@ xfs_log_unmount_write(
 	if (XLOG_FORCED_SHUTDOWN(log))
 		return;
 	xfs_log_unmount_verify_iclog(log);
-	xfs_log_write_unmount_record(mp);
+	xlog_unmount_write(log);
 }
 
 /*
-- 
2.25.1

Re: [PATCH 7/8] xfs: refactor unmount record writing

[Darrick J. Wong]

On Tue, Mar 24, 2020 at 06:44:58PM +0100, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Separate out the unmount record writing from the rest of the
> ticket and log state futzing necessary to make it work. This is
> a no-op, just makes the code cleaner and places the unmount record
> formatting and writing alongside the commit record formatting and
> writing code.
> 
> We can also get rid of the ticket flag clearing before the
> xlog_write() call because it no longer cares about the state of
> XLOG_TIC_INITED.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log.c | 65 +++++++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index a173b5925d1b..1d6ed696f717 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -471,6 +471,36 @@ xfs_log_reserve(
>   *		marked as with WANT_SYNC.
>   */
>  
> +/*
> + * Write out an unmount record using the ticket provided. We have to account for
> + * the data space used in the unmount ticket as this write is not done from a
> + * transaction context that has already done the accounting for us.
> + */
> +static int
> +xlog_write_unmount_record(
> +	struct xlog		*log,
> +	struct xlog_ticket	*ticket,
> +	xfs_lsn_t		*lsn,
> +	uint			flags)
> +{
> +	struct xfs_unmount_log_format ulf = {
> +		.magic = XLOG_UNMOUNT_TYPE,
> +	};
> +	struct xfs_log_iovec reg = {
> +		.i_addr = &ulf,
> +		.i_len = sizeof(ulf),
> +		.i_type = XLOG_REG_TYPE_UNMOUNT,
> +	};
> +	struct xfs_log_vec vec = {
> +		.lv_niovecs = 1,
> +		.lv_iovecp = &reg,
> +	};
> +
> +	/* account for space used by record data */
> +	ticket->t_curr_res -= sizeof(ulf);
> +	return xlog_write(log, &vec, ticket, lsn, NULL, flags);
> +}
> +
>  static bool
>  __xlog_state_release_iclog(
>  	struct xlog		*log,
> @@ -795,32 +825,14 @@ xlog_wait_on_iclog(
>  }
>  
>  /*
> - * Final log writes as part of unmount.
> - *
> - * Mark the filesystem clean as unmount happens.  Note that during relocation
> - * this routine needs to be executed as part of source-bag while the
> - * deallocation must not be done until source-end.
> + * Mark the filesystem clean by writing an unmount record to the head of the
> + * log.
>   */
> -
> -/* Actually write the unmount record to disk. */
>  static void
> -xfs_log_write_unmount_record(
> -	struct xfs_mount	*mp)
> +xlog_unmount_write(
> +	struct xlog		*log)
>  {
> -	/* the data section must be 32 bit size aligned */
> -	struct xfs_unmount_log_format magic = {
> -		.magic = XLOG_UNMOUNT_TYPE,
> -	};
> -	struct xfs_log_iovec reg = {
> -		.i_addr = &magic,
> -		.i_len = sizeof(magic),
> -		.i_type = XLOG_REG_TYPE_UNMOUNT,
> -	};
> -	struct xfs_log_vec vec = {
> -		.lv_niovecs = 1,
> -		.lv_iovecp = &reg,
> -	};
> -	struct xlog		*log = mp->m_log;
> +	struct xfs_mount	*mp = log->l_mp;
>  	struct xlog_in_core	*iclog;
>  	struct xlog_ticket	*tic = NULL;
>  	xfs_lsn_t		lsn;
> @@ -844,10 +856,7 @@ xfs_log_write_unmount_record(
>  		flags &= ~XLOG_UNMOUNT_TRANS;
>  	}
>  
> -	/* remove inited flag, and account for space used */
> -	tic->t_flags = 0;
> -	tic->t_curr_res -= sizeof(magic);
> -	error = xlog_write(log, &vec, tic, &lsn, NULL, flags);
> +	error = xlog_write_unmount_record(log, tic, &lsn, flags);
>  	/*
>  	 * At this point, we're umounting anyway, so there's no point in
>  	 * transitioning log state to IOERROR. Just continue...
> @@ -913,7 +922,7 @@ xfs_log_unmount_write(
>  	if (XLOG_FORCED_SHUTDOWN(log))
>  		return;
>  	xfs_log_unmount_verify_iclog(log);
> -	xfs_log_write_unmount_record(mp);
> +	xlog_unmount_write(log);
>  }
>  
>  /*
> -- 
> 2.25.1
> 

[PATCH 8/8] xfs: remove some stale comments from the log code

[Christoph Hellwig]

From: Dave Chinner <dchinner@redhat.com>

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_log.c | 59 +++++++++++-------------------------------------
 1 file changed, 13 insertions(+), 46 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 1d6ed696f717..521fe77e3aaa 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -463,14 +463,6 @@ xfs_log_reserve(
 	return error;
 }
 
-
-/*
- * NOTES:
- *
- *	1. currblock field gets updated at startup and after in-core logs
- *		marked as with WANT_SYNC.
- */
-
 /*
  * Write out an unmount record using the ticket provided. We have to account for
  * the data space used in the unmount ticket as this write is not done from a
@@ -1910,7 +1902,7 @@ xlog_dealloc_log(
 	log->l_mp->m_log = NULL;
 	destroy_workqueue(log->l_ioend_workqueue);
 	kmem_free(log);
-}	/* xlog_dealloc_log */
+}
 
 /*
  * Update counters atomically now that memcpy is done.
@@ -2454,14 +2446,6 @@ xlog_write(
 	return error;
 }
 
-
-/*****************************************************************************
- *
- *		State Machine functions
- *
- *****************************************************************************
- */
-
 static void
 xlog_state_activate_iclog(
 	struct xlog_in_core	*iclog,
@@ -2822,7 +2806,7 @@ xlog_state_done_syncing(
 	 */
 	wake_up_all(&iclog->ic_write_wait);
 	spin_unlock(&log->l_icloglock);
-	xlog_state_do_callback(log);	/* also cleans log */
+	xlog_state_do_callback(log);
 }
 
 /*
@@ -2942,13 +2926,14 @@ xlog_state_get_iclog_space(
 
 	*logoffsetp = log_offset;
 	return 0;
-}	/* xlog_state_get_iclog_space */
+}
 
 /*
- * The first cnt-1 times through here we don't need to move the grant write head
- * because the permanent reservation has reserved cnt times the unit amount.
- * Release part of current permanent unit reservation and reset current
- * reservation to be one units worth.  Also move grant reservation head forward.
+ * The first cnt-1 times a ticket goes through here we don't need to move the
+ * grant write head because the permanent reservation has reserved cnt times the
+ * unit amount.  Release part of current permanent unit reservation and reset
+ * current reservation to be one units worth.  Also move grant reservation head
+ * forward.
  */
 void
 xlog_ticket_regrant(
@@ -3030,12 +3015,8 @@ xlog_ticket_done(
 }
 
 /*
- * Mark the current iclog in the ring as WANT_SYNC and move the current iclog
- * pointer to the next iclog in the ring.
- *
- * When called from xlog_state_get_iclog_space(), the exact size of the iclog
- * has not yet been determined, all we know is that we have run out of space in
- * the current iclog.
+ * This routine will mark the current iclog in the ring as WANT_SYNC and move
+ * the current iclog pointer to the next iclog in the ring.
  */
 STATIC void
 xlog_state_switch_iclogs(
@@ -3080,7 +3061,7 @@ xlog_state_switch_iclogs(
 	}
 	ASSERT(iclog == log->l_iclog);
 	log->l_iclog = iclog->ic_next;
-}	/* xlog_state_switch_iclogs */
+}
 
 /*
  * Write out all data in the in-core log as of this exact moment in time.
@@ -3287,13 +3268,6 @@ xfs_log_force_lsn(
 	return ret;
 }
 
-/*****************************************************************************
- *
- *		TICKET functions
- *
- *****************************************************************************
- */
-
 /*
  * Free a used ticket when its refcount falls to zero.
  */
@@ -3450,13 +3424,6 @@ xlog_ticket_alloc(
 	return tic;
 }
 
-
-/******************************************************************************
- *
- *		Log debug routines
- *
- ******************************************************************************
- */
 #if defined(DEBUG)
 /*
  * Make sure that the destination ptr is within the valid data region of
@@ -3542,7 +3509,7 @@ xlog_verify_tail_lsn(
 	if (blocks < BTOBB(iclog->ic_offset) + 1)
 		xfs_emerg(log->l_mp, "%s: ran out of log space", __func__);
     }
-}	/* xlog_verify_tail_lsn */
+}
 
 /*
  * Perform a number of checks on the iclog before writing to disk.
@@ -3645,7 +3612,7 @@ xlog_verify_iclog(
 		}
 		ptr += sizeof(xlog_op_header_t) + op_len;
 	}
-}	/* xlog_verify_iclog */
+}
 #endif
 
 /*
-- 
2.25.1

Re: [PATCH 8/8] xfs: remove some stale comments from the log code

[Darrick J. Wong]

On Tue, Mar 24, 2020 at 06:44:59PM +0100, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Looks good,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log.c | 59 +++++++++++-------------------------------------
>  1 file changed, 13 insertions(+), 46 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 1d6ed696f717..521fe77e3aaa 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -463,14 +463,6 @@ xfs_log_reserve(
>  	return error;
>  }
>  
> -
> -/*
> - * NOTES:
> - *
> - *	1. currblock field gets updated at startup and after in-core logs
> - *		marked as with WANT_SYNC.
> - */
> -
>  /*
>   * Write out an unmount record using the ticket provided. We have to account for
>   * the data space used in the unmount ticket as this write is not done from a
> @@ -1910,7 +1902,7 @@ xlog_dealloc_log(
>  	log->l_mp->m_log = NULL;
>  	destroy_workqueue(log->l_ioend_workqueue);
>  	kmem_free(log);
> -}	/* xlog_dealloc_log */
> +}
>  
>  /*
>   * Update counters atomically now that memcpy is done.
> @@ -2454,14 +2446,6 @@ xlog_write(
>  	return error;
>  }
>  
> -
> -/*****************************************************************************
> - *
> - *		State Machine functions
> - *
> - *****************************************************************************
> - */
> -
>  static void
>  xlog_state_activate_iclog(
>  	struct xlog_in_core	*iclog,
> @@ -2822,7 +2806,7 @@ xlog_state_done_syncing(
>  	 */
>  	wake_up_all(&iclog->ic_write_wait);
>  	spin_unlock(&log->l_icloglock);
> -	xlog_state_do_callback(log);	/* also cleans log */
> +	xlog_state_do_callback(log);
>  }
>  
>  /*
> @@ -2942,13 +2926,14 @@ xlog_state_get_iclog_space(
>  
>  	*logoffsetp = log_offset;
>  	return 0;
> -}	/* xlog_state_get_iclog_space */
> +}
>  
>  /*
> - * The first cnt-1 times through here we don't need to move the grant write head
> - * because the permanent reservation has reserved cnt times the unit amount.
> - * Release part of current permanent unit reservation and reset current
> - * reservation to be one units worth.  Also move grant reservation head forward.
> + * The first cnt-1 times a ticket goes through here we don't need to move the
> + * grant write head because the permanent reservation has reserved cnt times the
> + * unit amount.  Release part of current permanent unit reservation and reset
> + * current reservation to be one units worth.  Also move grant reservation head
> + * forward.
>   */
>  void
>  xlog_ticket_regrant(
> @@ -3030,12 +3015,8 @@ xlog_ticket_done(
>  }
>  
>  /*
> - * Mark the current iclog in the ring as WANT_SYNC and move the current iclog
> - * pointer to the next iclog in the ring.
> - *
> - * When called from xlog_state_get_iclog_space(), the exact size of the iclog
> - * has not yet been determined, all we know is that we have run out of space in
> - * the current iclog.
> + * This routine will mark the current iclog in the ring as WANT_SYNC and move
> + * the current iclog pointer to the next iclog in the ring.
>   */
>  STATIC void
>  xlog_state_switch_iclogs(
> @@ -3080,7 +3061,7 @@ xlog_state_switch_iclogs(
>  	}
>  	ASSERT(iclog == log->l_iclog);
>  	log->l_iclog = iclog->ic_next;
> -}	/* xlog_state_switch_iclogs */
> +}
>  
>  /*
>   * Write out all data in the in-core log as of this exact moment in time.
> @@ -3287,13 +3268,6 @@ xfs_log_force_lsn(
>  	return ret;
>  }
>  
> -/*****************************************************************************
> - *
> - *		TICKET functions
> - *
> - *****************************************************************************
> - */
> -
>  /*
>   * Free a used ticket when its refcount falls to zero.
>   */
> @@ -3450,13 +3424,6 @@ xlog_ticket_alloc(
>  	return tic;
>  }
>  
> -
> -/******************************************************************************
> - *
> - *		Log debug routines
> - *
> - ******************************************************************************
> - */
>  #if defined(DEBUG)
>  /*
>   * Make sure that the destination ptr is within the valid data region of
> @@ -3542,7 +3509,7 @@ xlog_verify_tail_lsn(
>  	if (blocks < BTOBB(iclog->ic_offset) + 1)
>  		xfs_emerg(log->l_mp, "%s: ran out of log space", __func__);
>      }
> -}	/* xlog_verify_tail_lsn */
> +}
>  
>  /*
>   * Perform a number of checks on the iclog before writing to disk.
> @@ -3645,7 +3612,7 @@ xlog_verify_iclog(
>  		}
>  		ptr += sizeof(xlog_op_header_t) + op_len;
>  	}
> -}	/* xlog_verify_iclog */
> +}
>  #endif
>  
>  /*
> -- 
> 2.25.1
> 

xfs: clean up log tickets and record writes v4

[Christoph Hellwig]

This series follows up on conversions about relogging infrastructure
and the way xfs_log_done() does two things but only one of several
callers uses both of those functions. It also pointed out that
xfs_trans_commit() never writes to the log anymore, so only
checkpoints pass a ticket to xlog_write() with this flag set and
no transaction makes multiple calls to xlog_write() calls on the
same ticket. Hence there's no real need for XLOG_TIC_INITED to track
whether a ticket has written a start record to the log anymore.

A lot of further cleanups fell out of this. Once we no longer use
XLOG_TIC_INITED to carry state inside the write loop, the logic
can be simplified in both xlog_write and xfs_log_done. xfs_log_done
can be split up, and then the call chain can be flattened because
xlog_write_done() and xlog_commit_record() are basically the same.

This then leads to cleanups writing both commit and unmount records.

Finally, to complete what started all this, the XLOG_TIC_INITED flag
is removed.

A git tree is avaiblable here:

    git://git.infradead.org/users/hch/xfs.git xlog-ticket-cleanup.4

Gitweb:

    http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xlog-ticket-cleanup.4

Changes since v3:
 - pass an explicit need_start_rec to xlog_write()
 - rename xlog_ticket_done to xfs_log_ticket_ungrant and
   xlog_ticket_regrant to xfs_log_ticket_regrant
 - rename xlog_write_done to xlog_commit_record

Changes since v2:
 - fix a commit message typo
 - move the XLOG_TIC_INITED earlier, and move another hunk to this patch
 - keep checking for XLOG_FORCED_SHUTDOWN and skip regrants
 - merge the two patches for refactoring and renaming the log unmount
   record handling

Changes since v1:
 - taking this over from Dave (for now) as he is still injured, an it
   interacts closely with my log error handling bits
 - rebased on top of for-next + the "more log cleanups" series
 - fix an accounting error in xlog_write
 - use a bool for the ticket header in xlog_write
 - add a new patch to split xlog_ticket_done

[PATCH 1/8] xfs: don't try to write a start record into every iclog

[Christoph Hellwig]

From: Dave Chinner <dchinner@redhat.com>

The xlog_write() function iterates over iclogs until it completes
writing all the log vectors passed in. The ticket tracks whether
a start record has been written or not, so only the first iclog gets
a start record. We only ever pass single use tickets to
xlog_write() so we only ever need to write a start record once per
xlog_write() call.

Hence we don't need to store whether we should write a start record
in the ticket as the callers provide all the information we need to
determine if a start record should be written. For the moment, we
have to ensure that we clear the XLOG_TIC_INITED appropriately so
the code in xfs_log_done() still works correctly for committing
transactions.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
[hch: pass an explicit need_start_rec argument]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c      | 77 +++++++++++++++++++++----------------------
 fs/xfs/xfs_log_cil.c  |  2 +-
 fs/xfs/xfs_log_priv.h | 12 +++----
 3 files changed, 42 insertions(+), 49 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 2a90a483c2d6..617b393272de 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -921,7 +921,7 @@ xfs_log_write_unmount_record(
 	/* remove inited flag, and account for space used */
 	tic->t_flags = 0;
 	tic->t_curr_res -= sizeof(magic);
-	error = xlog_write(log, &vec, tic, &lsn, NULL, flags);
+	error = xlog_write(log, &vec, tic, &lsn, NULL, flags, false);
 	/*
 	 * At this point, we're umounting anyway, so there's no point in
 	 * transitioning log state to IOERROR. Just continue...
@@ -1541,7 +1541,7 @@ xlog_commit_record(
 
 	ASSERT_ALWAYS(iclog);
 	error = xlog_write(log, &vec, ticket, commitlsnp, iclog,
-					XLOG_COMMIT_TRANS);
+					XLOG_COMMIT_TRANS, false);
 	if (error)
 		xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
 	return error;
@@ -2112,23 +2112,21 @@ xlog_print_trans(
 }
 
 /*
- * Calculate the potential space needed by the log vector.  Each region gets
- * its own xlog_op_header_t and may need to be double word aligned.
+ * Calculate the potential space needed by the log vector.  We may need a start
+ * record, and each region gets its own struct xlog_op_header and may need to be
+ * double word aligned.
  */
 static int
 xlog_write_calc_vec_length(
 	struct xlog_ticket	*ticket,
-	struct xfs_log_vec	*log_vector)
+	struct xfs_log_vec	*log_vector,
+	bool			need_start_rec)
 {
 	struct xfs_log_vec	*lv;
-	int			headers = 0;
+	int			headers = need_start_rec ? 1 : 0;
 	int			len = 0;
 	int			i;
 
-	/* acct for start rec of xact */
-	if (ticket->t_flags & XLOG_TIC_INITED)
-		headers++;
-
 	for (lv = log_vector; lv; lv = lv->lv_next) {
 		/* we don't write ordered log vectors */
 		if (lv->lv_buf_len == XFS_LOG_VEC_ORDERED)
@@ -2150,27 +2148,16 @@ xlog_write_calc_vec_length(
 	return len;
 }
 
-/*
- * If first write for transaction, insert start record  We can't be trying to
- * commit if we are inited.  We can't have any "partial_copy" if we are inited.
- */
-static int
+static void
 xlog_write_start_rec(
 	struct xlog_op_header	*ophdr,
 	struct xlog_ticket	*ticket)
 {
-	if (!(ticket->t_flags & XLOG_TIC_INITED))
-		return 0;
-
 	ophdr->oh_tid	= cpu_to_be32(ticket->t_tid);
 	ophdr->oh_clientid = ticket->t_clientid;
 	ophdr->oh_len = 0;
 	ophdr->oh_flags = XLOG_START_TRANS;
 	ophdr->oh_res2 = 0;
-
-	ticket->t_flags &= ~XLOG_TIC_INITED;
-
-	return sizeof(struct xlog_op_header);
 }
 
 static xlog_op_header_t *
@@ -2359,7 +2346,8 @@ xlog_write(
 	struct xlog_ticket	*ticket,
 	xfs_lsn_t		*start_lsn,
 	struct xlog_in_core	**commit_iclog,
-	uint			flags)
+	uint			flags,
+	bool			need_start_rec)
 {
 	struct xlog_in_core	*iclog = NULL;
 	struct xfs_log_iovec	*vecp;
@@ -2375,23 +2363,22 @@ xlog_write(
 
 	*start_lsn = 0;
 
-	len = xlog_write_calc_vec_length(ticket, log_vector);
 
 	/*
-	 * Region headers and bytes are already accounted for.
-	 * We only need to take into account start records and
-	 * split regions in this function.
+	 * Region headers and bytes are already accounted for.  We only need to
+	 * take into account start records and split regions in this function.
 	 */
-	if (ticket->t_flags & XLOG_TIC_INITED)
-		ticket->t_curr_res -= sizeof(xlog_op_header_t);
+	if (ticket->t_flags & XLOG_TIC_INITED) {
+		ticket->t_curr_res -= sizeof(struct xlog_op_header);
+		ticket->t_flags &= ~XLOG_TIC_INITED;
+	}
 
 	/*
-	 * Commit record headers need to be accounted for. These
-	 * come in as separate writes so are easy to detect.
+	 * Commit record headers and unmount records need to be accounted for.
+	 * These come in as separate writes so are easy to detect.
 	 */
-	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
-		ticket->t_curr_res -= sizeof(xlog_op_header_t);
-
+	if (!need_start_rec)
+		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");
@@ -2399,6 +2386,8 @@ xlog_write(
 		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
 	}
 
+	len = xlog_write_calc_vec_length(ticket, log_vector, need_start_rec);
+
 	index = 0;
 	lv = log_vector;
 	vecp = lv->lv_iovecp;
@@ -2425,7 +2414,6 @@ xlog_write(
 		while (lv && (!lv->lv_niovecs || index < lv->lv_niovecs)) {
 			struct xfs_log_iovec	*reg;
 			struct xlog_op_header	*ophdr;
-			int			start_rec_copy;
 			int			copy_len;
 			int			copy_off;
 			bool			ordered = false;
@@ -2441,11 +2429,15 @@ xlog_write(
 			ASSERT(reg->i_len % sizeof(int32_t) == 0);
 			ASSERT((unsigned long)ptr % sizeof(int32_t) == 0);
 
-			start_rec_copy = xlog_write_start_rec(ptr, ticket);
-			if (start_rec_copy) {
-				record_cnt++;
+			/*
+			 * Before we start formatting log vectors, we need to
+			 * write a start record. Only do this for the first
+			 * iclog we write to.
+			 */
+			if (need_start_rec) {
+				xlog_write_start_rec(ptr, ticket);
 				xlog_write_adv_cnt(&ptr, &len, &log_offset,
-						   start_rec_copy);
+						sizeof(struct xlog_op_header));
 			}
 
 			ophdr = xlog_write_setup_ophdr(log, ptr, ticket, flags);
@@ -2477,8 +2469,13 @@ xlog_write(
 				xlog_write_adv_cnt(&ptr, &len, &log_offset,
 						   copy_len);
 			}
-			copy_len += start_rec_copy + sizeof(xlog_op_header_t);
+			copy_len += sizeof(struct xlog_op_header);
 			record_cnt++;
+			if (need_start_rec) {
+				copy_len += sizeof(struct xlog_op_header);
+				record_cnt++;
+				need_start_rec = false;
+			}
 			data_cnt += contwr ? copy_len : 0;
 
 			error = xlog_write_copy_finish(log, iclog, flags,
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 64cc0bf2ab3b..e0aeb316ce6c 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -801,7 +801,7 @@ xlog_cil_push_work(
 	lvhdr.lv_iovecp = &lhdr;
 	lvhdr.lv_next = ctx->lv_chain;
 
-	error = xlog_write(log, &lvhdr, tic, &ctx->start_lsn, NULL, 0);
+	error = xlog_write(log, &lvhdr, tic, &ctx->start_lsn, NULL, 0, true);
 	if (error)
 		goto out_abort_free_ticket;
 
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 2b0aec37e73e..b895e16460ee 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -439,14 +439,10 @@ xlog_write_adv_cnt(void **ptr, int *len, int *off, size_t bytes)
 
 void	xlog_print_tic_res(struct xfs_mount *mp, struct xlog_ticket *ticket);
 void	xlog_print_trans(struct xfs_trans *);
-int
-xlog_write(
-	struct xlog		*log,
-	struct xfs_log_vec	*log_vector,
-	struct xlog_ticket	*tic,
-	xfs_lsn_t		*start_lsn,
-	struct xlog_in_core	**commit_iclog,
-	uint			flags);
+int	xlog_write(struct xlog *log, struct xfs_log_vec *log_vector,
+		struct xlog_ticket *tic, xfs_lsn_t *start_lsn,
+		struct xlog_in_core **commit_iclog, uint flags,
+		bool need_start_rec);
 
 /*
  * When we crack an atomic LSN, we sample it first so that the value will not
-- 
2.25.1

Re: [PATCH 1/8] xfs: don't try to write a start record into every iclog

[Darrick J. Wong]

On Wed, Mar 25, 2020 at 07:42:58PM +0100, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The xlog_write() function iterates over iclogs until it completes
> writing all the log vectors passed in. The ticket tracks whether
> a start record has been written or not, so only the first iclog gets
> a start record. We only ever pass single use tickets to
> xlog_write() so we only ever need to write a start record once per
> xlog_write() call.
> 
> Hence we don't need to store whether we should write a start record
> in the ticket as the callers provide all the information we need to
> determine if a start record should be written. For the moment, we
> have to ensure that we clear the XLOG_TIC_INITED appropriately so
> the code in xfs_log_done() still works correctly for committing
> transactions.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> [hch: pass an explicit need_start_rec argument]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c      | 77 +++++++++++++++++++++----------------------
>  fs/xfs/xfs_log_cil.c  |  2 +-
>  fs/xfs/xfs_log_priv.h | 12 +++----
>  3 files changed, 42 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 2a90a483c2d6..617b393272de 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -921,7 +921,7 @@ xfs_log_write_unmount_record(
>  	/* remove inited flag, and account for space used */
>  	tic->t_flags = 0;
>  	tic->t_curr_res -= sizeof(magic);
> -	error = xlog_write(log, &vec, tic, &lsn, NULL, flags);
> +	error = xlog_write(log, &vec, tic, &lsn, NULL, flags, false);

/me vaguely doesn't like the look of flags and a boolean, but teaching
this mess to have separate flags namespaces and mask them out
appropriately seems like a much more confusing mess...

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  	/*
>  	 * At this point, we're umounting anyway, so there's no point in
>  	 * transitioning log state to IOERROR. Just continue...
> @@ -1541,7 +1541,7 @@ xlog_commit_record(
>  
>  	ASSERT_ALWAYS(iclog);
>  	error = xlog_write(log, &vec, ticket, commitlsnp, iclog,
> -					XLOG_COMMIT_TRANS);
> +					XLOG_COMMIT_TRANS, false);
>  	if (error)
>  		xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
>  	return error;
> @@ -2112,23 +2112,21 @@ xlog_print_trans(
>  }
>  
>  /*
> - * Calculate the potential space needed by the log vector.  Each region gets
> - * its own xlog_op_header_t and may need to be double word aligned.
> + * Calculate the potential space needed by the log vector.  We may need a start
> + * record, and each region gets its own struct xlog_op_header and may need to be
> + * double word aligned.
>   */
>  static int
>  xlog_write_calc_vec_length(
>  	struct xlog_ticket	*ticket,
> -	struct xfs_log_vec	*log_vector)
> +	struct xfs_log_vec	*log_vector,
> +	bool			need_start_rec)
>  {
>  	struct xfs_log_vec	*lv;
> -	int			headers = 0;
> +	int			headers = need_start_rec ? 1 : 0;
>  	int			len = 0;
>  	int			i;
>  
> -	/* acct for start rec of xact */
> -	if (ticket->t_flags & XLOG_TIC_INITED)
> -		headers++;
> -
>  	for (lv = log_vector; lv; lv = lv->lv_next) {
>  		/* we don't write ordered log vectors */
>  		if (lv->lv_buf_len == XFS_LOG_VEC_ORDERED)
> @@ -2150,27 +2148,16 @@ xlog_write_calc_vec_length(
>  	return len;
>  }
>  
> -/*
> - * If first write for transaction, insert start record  We can't be trying to
> - * commit if we are inited.  We can't have any "partial_copy" if we are inited.
> - */
> -static int
> +static void
>  xlog_write_start_rec(
>  	struct xlog_op_header	*ophdr,
>  	struct xlog_ticket	*ticket)
>  {
> -	if (!(ticket->t_flags & XLOG_TIC_INITED))
> -		return 0;
> -
>  	ophdr->oh_tid	= cpu_to_be32(ticket->t_tid);
>  	ophdr->oh_clientid = ticket->t_clientid;
>  	ophdr->oh_len = 0;
>  	ophdr->oh_flags = XLOG_START_TRANS;
>  	ophdr->oh_res2 = 0;
> -
> -	ticket->t_flags &= ~XLOG_TIC_INITED;
> -
> -	return sizeof(struct xlog_op_header);
>  }
>  
>  static xlog_op_header_t *
> @@ -2359,7 +2346,8 @@ xlog_write(
>  	struct xlog_ticket	*ticket,
>  	xfs_lsn_t		*start_lsn,
>  	struct xlog_in_core	**commit_iclog,
> -	uint			flags)
> +	uint			flags,
> +	bool			need_start_rec)
>  {
>  	struct xlog_in_core	*iclog = NULL;
>  	struct xfs_log_iovec	*vecp;
> @@ -2375,23 +2363,22 @@ xlog_write(
>  
>  	*start_lsn = 0;
>  
> -	len = xlog_write_calc_vec_length(ticket, log_vector);
>  
>  	/*
> -	 * Region headers and bytes are already accounted for.
> -	 * We only need to take into account start records and
> -	 * split regions in this function.
> +	 * Region headers and bytes are already accounted for.  We only need to
> +	 * take into account start records and split regions in this function.
>  	 */
> -	if (ticket->t_flags & XLOG_TIC_INITED)
> -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> +	if (ticket->t_flags & XLOG_TIC_INITED) {
> +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> +		ticket->t_flags &= ~XLOG_TIC_INITED;
> +	}
>  
>  	/*
> -	 * Commit record headers need to be accounted for. These
> -	 * come in as separate writes so are easy to detect.
> +	 * Commit record headers and unmount records need to be accounted for.
> +	 * These come in as separate writes so are easy to detect.
>  	 */
> -	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
> -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> -
> +	if (!need_start_rec)
> +		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");
> @@ -2399,6 +2386,8 @@ xlog_write(
>  		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
>  	}
>  
> +	len = xlog_write_calc_vec_length(ticket, log_vector, need_start_rec);
> +
>  	index = 0;
>  	lv = log_vector;
>  	vecp = lv->lv_iovecp;
> @@ -2425,7 +2414,6 @@ xlog_write(
>  		while (lv && (!lv->lv_niovecs || index < lv->lv_niovecs)) {
>  			struct xfs_log_iovec	*reg;
>  			struct xlog_op_header	*ophdr;
> -			int			start_rec_copy;
>  			int			copy_len;
>  			int			copy_off;
>  			bool			ordered = false;
> @@ -2441,11 +2429,15 @@ xlog_write(
>  			ASSERT(reg->i_len % sizeof(int32_t) == 0);
>  			ASSERT((unsigned long)ptr % sizeof(int32_t) == 0);
>  
> -			start_rec_copy = xlog_write_start_rec(ptr, ticket);
> -			if (start_rec_copy) {
> -				record_cnt++;
> +			/*
> +			 * Before we start formatting log vectors, we need to
> +			 * write a start record. Only do this for the first
> +			 * iclog we write to.
> +			 */
> +			if (need_start_rec) {
> +				xlog_write_start_rec(ptr, ticket);
>  				xlog_write_adv_cnt(&ptr, &len, &log_offset,
> -						   start_rec_copy);
> +						sizeof(struct xlog_op_header));
>  			}
>  
>  			ophdr = xlog_write_setup_ophdr(log, ptr, ticket, flags);
> @@ -2477,8 +2469,13 @@ xlog_write(
>  				xlog_write_adv_cnt(&ptr, &len, &log_offset,
>  						   copy_len);
>  			}
> -			copy_len += start_rec_copy + sizeof(xlog_op_header_t);
> +			copy_len += sizeof(struct xlog_op_header);
>  			record_cnt++;
> +			if (need_start_rec) {
> +				copy_len += sizeof(struct xlog_op_header);
> +				record_cnt++;
> +				need_start_rec = false;
> +			}
>  			data_cnt += contwr ? copy_len : 0;
>  
>  			error = xlog_write_copy_finish(log, iclog, flags,
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 64cc0bf2ab3b..e0aeb316ce6c 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -801,7 +801,7 @@ xlog_cil_push_work(
>  	lvhdr.lv_iovecp = &lhdr;
>  	lvhdr.lv_next = ctx->lv_chain;
>  
> -	error = xlog_write(log, &lvhdr, tic, &ctx->start_lsn, NULL, 0);
> +	error = xlog_write(log, &lvhdr, tic, &ctx->start_lsn, NULL, 0, true);
>  	if (error)
>  		goto out_abort_free_ticket;
>  
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 2b0aec37e73e..b895e16460ee 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -439,14 +439,10 @@ xlog_write_adv_cnt(void **ptr, int *len, int *off, size_t bytes)
>  
>  void	xlog_print_tic_res(struct xfs_mount *mp, struct xlog_ticket *ticket);
>  void	xlog_print_trans(struct xfs_trans *);
> -int
> -xlog_write(
> -	struct xlog		*log,
> -	struct xfs_log_vec	*log_vector,
> -	struct xlog_ticket	*tic,
> -	xfs_lsn_t		*start_lsn,
> -	struct xlog_in_core	**commit_iclog,
> -	uint			flags);
> +int	xlog_write(struct xlog *log, struct xfs_log_vec *log_vector,
> +		struct xlog_ticket *tic, xfs_lsn_t *start_lsn,
> +		struct xlog_in_core **commit_iclog, uint flags,
> +		bool need_start_rec);
>  
>  /*
>   * When we crack an atomic LSN, we sample it first so that the value will not
> -- 
> 2.25.1
> 

Re: [PATCH 1/8] xfs: don't try to write a start record into every iclog

[Brian Foster]

On Wed, Mar 25, 2020 at 07:42:58PM +0100, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The xlog_write() function iterates over iclogs until it completes
> writing all the log vectors passed in. The ticket tracks whether
> a start record has been written or not, so only the first iclog gets
> a start record. We only ever pass single use tickets to
> xlog_write() so we only ever need to write a start record once per
> xlog_write() call.
> 
> Hence we don't need to store whether we should write a start record
> in the ticket as the callers provide all the information we need to
> determine if a start record should be written. For the moment, we
> have to ensure that we clear the XLOG_TIC_INITED appropriately so
> the code in xfs_log_done() still works correctly for committing
> transactions.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> [hch: pass an explicit need_start_rec argument]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c      | 77 +++++++++++++++++++++----------------------
>  fs/xfs/xfs_log_cil.c  |  2 +-
>  fs/xfs/xfs_log_priv.h | 12 +++----
>  3 files changed, 42 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 2a90a483c2d6..617b393272de 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
...
> @@ -2375,23 +2363,22 @@ xlog_write(
>  
>  	*start_lsn = 0;
>  
> -	len = xlog_write_calc_vec_length(ticket, log_vector);
>  
>  	/*
> -	 * Region headers and bytes are already accounted for.
> -	 * We only need to take into account start records and
> -	 * split regions in this function.
> +	 * Region headers and bytes are already accounted for.  We only need to
> +	 * take into account start records and split regions in this function.
>  	 */
> -	if (ticket->t_flags & XLOG_TIC_INITED)
> -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> +	if (ticket->t_flags & XLOG_TIC_INITED) {
> +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> +		ticket->t_flags &= ~XLOG_TIC_INITED;
> +	}
>  
>  	/*
> -	 * Commit record headers need to be accounted for. These
> -	 * come in as separate writes so are easy to detect.
> +	 * Commit record headers and unmount records need to be accounted for.
> +	 * These come in as separate writes so are easy to detect.
>  	 */
> -	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
> -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> -
> +	if (!need_start_rec)
> +		ticket->t_curr_res -= sizeof(struct xlog_op_header);

So this technically is still different from upstream in that if the
caller clears XLOG_UNMOUNT_TRANS, upstream wouldn't account this
reservation from the log ticket because both t_flags and flags are
zeroed. With this change, we always account the op header one way or the
other. I don't see anything else that changes functionally in the
xlog_write() code based on flags (i.e. other than what we write out to
disk), so perhaps this logic is correct.

Could you (or Darrick?) update the commit log to note that tweak? With
that, the rest looks good to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  	if (ticket->t_curr_res < 0) {
>  		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
>  		     "ctx ticket reservation ran out. Need to up reservation");
> @@ -2399,6 +2386,8 @@ xlog_write(
>  		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
>  	}
>  
> +	len = xlog_write_calc_vec_length(ticket, log_vector, need_start_rec);
> +
>  	index = 0;
>  	lv = log_vector;
>  	vecp = lv->lv_iovecp;
> @@ -2425,7 +2414,6 @@ xlog_write(
>  		while (lv && (!lv->lv_niovecs || index < lv->lv_niovecs)) {
>  			struct xfs_log_iovec	*reg;
>  			struct xlog_op_header	*ophdr;
> -			int			start_rec_copy;
>  			int			copy_len;
>  			int			copy_off;
>  			bool			ordered = false;
> @@ -2441,11 +2429,15 @@ xlog_write(
>  			ASSERT(reg->i_len % sizeof(int32_t) == 0);
>  			ASSERT((unsigned long)ptr % sizeof(int32_t) == 0);
>  
> -			start_rec_copy = xlog_write_start_rec(ptr, ticket);
> -			if (start_rec_copy) {
> -				record_cnt++;
> +			/*
> +			 * Before we start formatting log vectors, we need to
> +			 * write a start record. Only do this for the first
> +			 * iclog we write to.
> +			 */
> +			if (need_start_rec) {
> +				xlog_write_start_rec(ptr, ticket);
>  				xlog_write_adv_cnt(&ptr, &len, &log_offset,
> -						   start_rec_copy);
> +						sizeof(struct xlog_op_header));
>  			}
>  
>  			ophdr = xlog_write_setup_ophdr(log, ptr, ticket, flags);
> @@ -2477,8 +2469,13 @@ xlog_write(
>  				xlog_write_adv_cnt(&ptr, &len, &log_offset,
>  						   copy_len);
>  			}
> -			copy_len += start_rec_copy + sizeof(xlog_op_header_t);
> +			copy_len += sizeof(struct xlog_op_header);
>  			record_cnt++;
> +			if (need_start_rec) {
> +				copy_len += sizeof(struct xlog_op_header);
> +				record_cnt++;
> +				need_start_rec = false;
> +			}
>  			data_cnt += contwr ? copy_len : 0;
>  
>  			error = xlog_write_copy_finish(log, iclog, flags,
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 64cc0bf2ab3b..e0aeb316ce6c 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -801,7 +801,7 @@ xlog_cil_push_work(
>  	lvhdr.lv_iovecp = &lhdr;
>  	lvhdr.lv_next = ctx->lv_chain;
>  
> -	error = xlog_write(log, &lvhdr, tic, &ctx->start_lsn, NULL, 0);
> +	error = xlog_write(log, &lvhdr, tic, &ctx->start_lsn, NULL, 0, true);
>  	if (error)
>  		goto out_abort_free_ticket;
>  
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 2b0aec37e73e..b895e16460ee 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -439,14 +439,10 @@ xlog_write_adv_cnt(void **ptr, int *len, int *off, size_t bytes)
>  
>  void	xlog_print_tic_res(struct xfs_mount *mp, struct xlog_ticket *ticket);
>  void	xlog_print_trans(struct xfs_trans *);
> -int
> -xlog_write(
> -	struct xlog		*log,
> -	struct xfs_log_vec	*log_vector,
> -	struct xlog_ticket	*tic,
> -	xfs_lsn_t		*start_lsn,
> -	struct xlog_in_core	**commit_iclog,
> -	uint			flags);
> +int	xlog_write(struct xlog *log, struct xfs_log_vec *log_vector,
> +		struct xlog_ticket *tic, xfs_lsn_t *start_lsn,
> +		struct xlog_in_core **commit_iclog, uint flags,
> +		bool need_start_rec);
>  
>  /*
>   * When we crack an atomic LSN, we sample it first so that the value will not
> -- 
> 2.25.1
>