[PATCH 00/11] xfs: clean up log tickets and record writes

[PATCH 00/11] xfs: clean up log tickets and record writes

[Dave Chinner]

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,
and removes a heap of duplicated error handling code in the unmount
record writing path.

And in looking over all this code, I noticed a heap of stale and
unnecessary comments through the code which can be cleaned up.

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

This has made it through fstests without causing any obvious
regressions, so it's time for thoughts and comments. This can also
be found at:

https://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git/h?xlog-ticket-cleanup

Note that this message appears as the first (empty) commit of the
series, as this is how I get my local hacked version of guilt to
format this cover message automatically.

Diffstat:
 fs/xfs/xfs_log.c      | 509 +++++++++++++++++++-------------------------------
 fs/xfs/xfs_log.h      |   4 -
 fs/xfs/xfs_log_cil.c  |  13 +-
 fs/xfs/xfs_log_priv.h |  22 +--
 fs/xfs/xfs_trans.c    |  24 +--
 5 files changed, 225 insertions(+), 347 deletions(-)

Signed-off-by: Dave Chinner <dchinner@redhat.com>


Dave Chinner (11):
  xfs: don't try to write a start record into every iclog
  xfs: re-order initial space accounting checks in xlog_write
  xfs: refactor and split xfs_log_done()
  xfs: merge xlog_commit_record with xlog_write_done()
  xfs: factor out unmount record writing
  xfs: move xlog_state_ioerror()
  xfs: clean up xlog_state_ioerror()
  xfs: rename the log unmount writing functions.
  xfs: merge unmount record write iclog cleanup.
  xfs: remove some stale comments from the log code
  xfs: kill XLOG_TIC_INITED

 fs/xfs/xfs_log.c      | 509 ++++++++++++++++--------------------------
 fs/xfs/xfs_log.h      |   4 -
 fs/xfs/xfs_log_cil.c  |  13 +-
 fs/xfs/xfs_log_priv.h |  22 +-
 fs/xfs/xfs_trans.c    |  24 +-
 5 files changed, 225 insertions(+), 347 deletions(-)

-- 
2.24.0.rc0

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

[Dave Chinner]

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() 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>
---
 fs/xfs/xfs_log.c | 68 +++++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 33 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index f6006d94a581..5b0568a86c07 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2148,23 +2148,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 xlog_op_header_t 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)
@@ -2186,27 +2184,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 *
@@ -2404,25 +2391,29 @@ xlog_write(
 	int			record_cnt = 0;
 	int			data_cnt = 0;
 	int			error = 0;
+	int			start_rec_size = sizeof(struct xlog_op_header);
 
 	*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)
+	if (ticket->t_flags & XLOG_TIC_INITED) {
 		ticket->t_curr_res -= sizeof(xlog_op_header_t);
+		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))
+	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
 		ticket->t_curr_res -= sizeof(xlog_op_header_t);
+		start_rec_size = 0;
+	}
 
 	if (ticket->t_curr_res < 0) {
 		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
@@ -2431,6 +2422,8 @@ xlog_write(
 		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
 	}
 
+	len = xlog_write_calc_vec_length(ticket, log_vector, start_rec_size);
+
 	index = 0;
 	lv = log_vector;
 	vecp = lv->lv_iovecp;
@@ -2446,9 +2439,20 @@ xlog_write(
 		ASSERT(log_offset <= iclog->ic_size - 1);
 		ptr = iclog->ic_datap + log_offset;
 
-		/* start_lsn is the first lsn written to. That's all we need. */
+		/*
+		 * Before we start formatting log vectors, we need to write a
+		 * start record and record the lsn of the iclog that we write
+		 * the start record into. Only do this for the first iclog we
+		 * write to.
+		 */
 		if (!*start_lsn)
 			*start_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
+		if (start_rec_size) {
+			xlog_write_start_rec(ptr, ticket);
+			xlog_write_adv_cnt(&ptr, &len, &log_offset,
+					start_rec_size);
+			record_cnt++;
+		}
 
 		/*
 		 * This loop writes out as many regions as can fit in the amount
@@ -2457,7 +2461,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;
@@ -2473,13 +2476,6 @@ 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++;
-				xlog_write_adv_cnt(&ptr, &len, &log_offset,
-						   start_rec_copy);
-			}
-
 			ophdr = xlog_write_setup_ophdr(log, ptr, ticket, flags);
 			if (!ophdr)
 				return -EIO;
@@ -2509,10 +2505,15 @@ 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(xlog_op_header_t);
 			record_cnt++;
 			data_cnt += contwr ? copy_len : 0;
 
+			if (start_rec_size) {
+				copy_len += start_rec_size;
+				start_rec_size = 0;
+			}
+
 			error = xlog_write_copy_finish(log, iclog, flags,
 						       &record_cnt, &data_cnt,
 						       &partial_copy,
@@ -2550,6 +2551,7 @@ xlog_write(
 				break;
 			}
 		}
+		start_rec_size = 0;
 	}
 
 	ASSERT(len == 0);
-- 
2.24.0.rc0

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

[Christoph Hellwig]

>  /*
> - * 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 xlog_op_header_t and may need to
> + * be double word aligned.

s/xlog_op_header_t/struct xlog_op_header/ while you're at it.

> @@ -2404,25 +2391,29 @@ xlog_write(
>  	int			record_cnt = 0;
>  	int			data_cnt = 0;
>  	int			error = 0;
> +	int			start_rec_size = sizeof(struct xlog_op_header);
>  
>  	*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)
> +	if (ticket->t_flags & XLOG_TIC_INITED) {
>  		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> +		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))
> +	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
>  		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> +		start_rec_size = 0;
> +	}
>  
>  	if (ticket->t_curr_res < 0) {
>  		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
> @@ -2431,6 +2422,8 @@ xlog_write(
>  		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
>  	}
>  
> +	len = xlog_write_calc_vec_length(ticket, log_vector, start_rec_size);

The last arg is used as a boolean in xlog_write_calc_vec_length. I
think it would make sense to have a need_start_rec boolean in this
function as well, and just hardcode the sizeof in the two places that
actually need the size.

> +			copy_len += sizeof(xlog_op_header_t);

s/xlog_op_header_t/struct xlog_op_header/

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

[Dave Chinner]

On Wed, Mar 04, 2020 at 07:44:21AM -0800, Christoph Hellwig wrote:
> >  /*
> > - * 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 xlog_op_header_t and may need to
> > + * be double word aligned.
> 
> s/xlog_op_header_t/struct xlog_op_header/ while you're at it.
> 
> > @@ -2404,25 +2391,29 @@ xlog_write(
> >  	int			record_cnt = 0;
> >  	int			data_cnt = 0;
> >  	int			error = 0;
> > +	int			start_rec_size = sizeof(struct xlog_op_header);
> >  
> >  	*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)
> > +	if (ticket->t_flags & XLOG_TIC_INITED) {
> >  		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > +		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))
> > +	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
> >  		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > +		start_rec_size = 0;
> > +	}
> >  
> >  	if (ticket->t_curr_res < 0) {
> >  		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
> > @@ -2431,6 +2422,8 @@ xlog_write(
> >  		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
> >  	}
> >  
> > +	len = xlog_write_calc_vec_length(ticket, log_vector, start_rec_size);
> 
> The last arg is used as a boolean in xlog_write_calc_vec_length. I
> think it would make sense to have a need_start_rec boolean in this
> function as well, and just hardcode the sizeof in the two places that
> actually need the size.

I originally had that and while the code looked kinda weird
opencoding an ophdr everywhere we wanted the size of a start record,
that wasn't an issue. The biggest problem was that using a boolean
resulted in _several_ logic bugs that I only tracked down once I
realised I'd forgotten to replace existing the start record size
variable that now wasn't initialised inside the inner loop.

So, yes, it gets converted to a boolean inside that function call,
but I think the code in this set of nested loops is more reliable if
it carries the size of the structure rather than open coding it
everywhere. Making it a boolean doesn't improve the readability of
the code at all.

> > +			copy_len += sizeof(xlog_op_header_t);
> 
> s/xlog_op_header_t/struct xlog_op_header/

Ok.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Christoph Hellwig]

On Thu, Mar 05, 2020 at 08:26:48AM +1100, Dave Chinner wrote:
> > The last arg is used as a boolean in xlog_write_calc_vec_length. I
> > think it would make sense to have a need_start_rec boolean in this
> > function as well, and just hardcode the sizeof in the two places that
> > actually need the size.
> 
> I originally had that and while the code looked kinda weird
> opencoding an ophdr everywhere we wanted the size of a start record,
> that wasn't an issue. The biggest problem was that using a boolean
> resulted in _several_ logic bugs that I only tracked down once I
> realised I'd forgotten to replace existing the start record size
> variable that now wasn't initialised inside the inner loop.
> 
> So, yes, it gets converted to a boolean inside that function call,
> but I think the code in this set of nested loops is more reliable if
> it carries the size of the structure rather than open coding it
> everywhere. Making it a boolean doesn't improve the readability of
> the code at all.

I found the sized variable rather confusion.  Here is how I would
have done it, also more closely following the existing code.  Looking
at this I also think your current patch changes behavior in that
previously the start record was included in data_cnt, while now it
isn't.

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index f6006d94a581..7eb72792e1e6 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2148,23 +2148,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)
@@ -2186,27 +2184,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 *
@@ -2404,25 +2391,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,
@@ -2431,6 +2422,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;
@@ -2457,7 +2450,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;
@@ -2473,11 +2465,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);
@@ -2509,8 +2505,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,

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

[Brian Foster]

On Wed, Mar 04, 2020 at 06:53:51PM +1100, Dave Chinner 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() we only ever need to write a start record once per

	      ^ "so we only ever ..." ?

> 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>
> ---
>  fs/xfs/xfs_log.c | 68 +++++++++++++++++++++++++-----------------------
>  1 file changed, 35 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index f6006d94a581..5b0568a86c07 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
...
> @@ -2404,25 +2391,29 @@ xlog_write(
>  	int			record_cnt = 0;
>  	int			data_cnt = 0;
>  	int			error = 0;
> +	int			start_rec_size = sizeof(struct xlog_op_header);
>  
>  	*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)
> +	if (ticket->t_flags & XLOG_TIC_INITED) {
>  		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> +		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))
> +	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
>  		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> +		start_rec_size = 0;
> +	}

This is more of a flaw in the existing code than this patch, but please
update the comment to document why we reset start_rec_size here since
it's not obvious from the context of the function. Otherwise looks Ok to
me modulo Christoph's comments (in particular the data_cnt bit). My one
nit is that I think start_rec might be a better name than start_rec_size
given how it's used, but I don't care that much.

Brian

>  
>  	if (ticket->t_curr_res < 0) {
>  		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
> @@ -2431,6 +2422,8 @@ xlog_write(
>  		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
>  	}
>  
> +	len = xlog_write_calc_vec_length(ticket, log_vector, start_rec_size);
> +
>  	index = 0;
>  	lv = log_vector;
>  	vecp = lv->lv_iovecp;
> @@ -2446,9 +2439,20 @@ xlog_write(
>  		ASSERT(log_offset <= iclog->ic_size - 1);
>  		ptr = iclog->ic_datap + log_offset;
>  
> -		/* start_lsn is the first lsn written to. That's all we need. */
> +		/*
> +		 * Before we start formatting log vectors, we need to write a
> +		 * start record and record the lsn of the iclog that we write
> +		 * the start record into. Only do this for the first iclog we
> +		 * write to.
> +		 */
>  		if (!*start_lsn)
>  			*start_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> +		if (start_rec_size) {
> +			xlog_write_start_rec(ptr, ticket);
> +			xlog_write_adv_cnt(&ptr, &len, &log_offset,
> +					start_rec_size);
> +			record_cnt++;
> +		}
>  
>  		/*
>  		 * This loop writes out as many regions as can fit in the amount
> @@ -2457,7 +2461,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;
> @@ -2473,13 +2476,6 @@ 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++;
> -				xlog_write_adv_cnt(&ptr, &len, &log_offset,
> -						   start_rec_copy);
> -			}
> -
>  			ophdr = xlog_write_setup_ophdr(log, ptr, ticket, flags);
>  			if (!ophdr)
>  				return -EIO;
> @@ -2509,10 +2505,15 @@ 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(xlog_op_header_t);
>  			record_cnt++;
>  			data_cnt += contwr ? copy_len : 0;
>  
> +			if (start_rec_size) {
> +				copy_len += start_rec_size;
> +				start_rec_size = 0;
> +			}
> +
>  			error = xlog_write_copy_finish(log, iclog, flags,
>  						       &record_cnt, &data_cnt,
>  						       &partial_copy,
> @@ -2550,6 +2551,7 @@ xlog_write(
>  				break;
>  			}
>  		}
> +		start_rec_size = 0;
>  	}
>  
>  	ASSERT(len == 0);
> -- 
> 2.24.0.rc0
> 

[PATCH 02/11] xfs: re-order initial space accounting checks in xlog_write

[Dave Chinner]

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>
---
 fs/xfs/xfs_log.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 5b0568a86c07..d6c42954b70c 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2381,10 +2381,10 @@ 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;
@@ -2393,27 +2393,17 @@ xlog_write(
 	int			error = 0;
 	int			start_rec_size = sizeof(struct xlog_op_header);
 
-	*start_lsn = 0;
-
-
 	/*
-	 * 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(xlog_op_header_t);
+	ticket->t_curr_res -= sizeof(xlog_op_header_t);
+	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(xlog_op_header_t);
+	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
 		start_rec_size = 0;
-	}
 
 	if (ticket->t_curr_res < 0) {
 		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
@@ -2423,10 +2413,7 @@ xlog_write(
 	}
 
 	len = xlog_write_calc_vec_length(ticket, log_vector, start_rec_size);
-
-	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.24.0.rc0

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

[Brian Foster]

On Wed, Mar 04, 2020 at 06:53:52PM +1100, Dave Chinner 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>
> ---
>  fs/xfs/xfs_log.c | 35 +++++++++++------------------------
>  1 file changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 5b0568a86c07..d6c42954b70c 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
...
> @@ -2393,27 +2393,17 @@ xlog_write(
>  	int			error = 0;
>  	int			start_rec_size = sizeof(struct xlog_op_header);
>  
> -	*start_lsn = 0;
> -
> -
>  	/*
> -	 * 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.
>  	 */

This addresses my comment on the previous patch, thanks. ;)

> -	if (ticket->t_flags & XLOG_TIC_INITED) {
> -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> +	ticket->t_curr_res -= sizeof(xlog_op_header_t);

Ok, so we're combining the fact that either the ticket is inited (we
have a start rec) or otherwise this is an unmount or commit_trans write
with an associated header.

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

> +	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(xlog_op_header_t);
> +	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
>  		start_rec_size = 0;
> -	}
>  
>  	if (ticket->t_curr_res < 0) {
>  		xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
> @@ -2423,10 +2413,7 @@ xlog_write(
>  	}
>  
>  	len = xlog_write_calc_vec_length(ticket, log_vector, start_rec_size);
> -
> -	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.24.0.rc0
> 

[PATCH 03/11] xfs: refactor and split xfs_log_done()

[Dave Chinner]

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>
---
 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, 47 insertions(+), 74 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index d6c42954b70c..702b38e4db6e 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -493,62 +493,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);
+}
 
+/*
+ * 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) {
 		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
@@ -2400,8 +2378,6 @@ xlog_write(
 	 * to account for an extra xlog_op_header here.
 	 */
 	ticket->t_curr_res -= sizeof(xlog_op_header_t);
-	if (ticket->t_flags & XLOG_TIC_INITED)
-		ticket->t_flags &= ~XLOG_TIC_INITED;
 	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
 		start_rec_size = 0;
 
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index 84e06805160f..85f8d0966811 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 48435cf2aa16..255065d276fc 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -841,10 +841,11 @@ xlog_cil_push(
 	}
 	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) {
@@ -876,7 +877,7 @@ xlog_cil_push(
 	return 0;
 
 out_abort_free_ticket:
-	xfs_log_ticket_put(tic);
+	xlog_ticket_done(log, tic, false);
 out_abort:
 	xlog_cil_committed(ctx, true);
 	return -EIO;
@@ -1017,7 +1018,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 b192c5a9f9fd..081d4c6de2c8 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -438,14 +438,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 3b208f9a865c..85ea3727878b 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.24.0.rc0

Re: [PATCH 03/11] xfs: refactor and split xfs_log_done()

[Christoph Hellwig]

> +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);
> +}

Can we just move the XLOG_FORCED_SHUTDOWN check into xlog_commit_record
and call xlog_commit_record directly?

>  
> +/*
> + * 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) {
>  		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);

I'd move the trace points and the call to xfs_log_ticket_put into
xlog_ungrant_log_space and xlog_regrant_reserve_log_space, and then call
the two functions directly from the callers.  There is only a single
place the ever regrants anyway.

Re: [PATCH 03/11] xfs: refactor and split xfs_log_done()

[Dave Chinner]

On Wed, Mar 04, 2020 at 07:49:55AM -0800, Christoph Hellwig wrote:
> > +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);
> > +}
> 
> Can we just move the XLOG_FORCED_SHUTDOWN check into xlog_commit_record
> and call xlog_commit_record directly?

Next patch, because merging isn't obvious until the split is done.

> > +/*
> > + * 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) {
> >  		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);
> 
> I'd move the trace points and the call to xfs_log_ticket_put into
> xlog_ungrant_log_space and xlog_regrant_reserve_log_space, and then call
> the two functions directly from the callers.  There is only a single
> place the ever regrants anyway.

Ok.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 03/11] xfs: refactor and split xfs_log_done()

[Christoph Hellwig]

On Thu, Mar 05, 2020 at 08:28:18AM +1100, Dave Chinner wrote:
> On Wed, Mar 04, 2020 at 07:49:55AM -0800, Christoph Hellwig wrote:
> > > +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);
> > > +}
> > 
> > Can we just move the XLOG_FORCED_SHUTDOWN check into xlog_commit_record
> > and call xlog_commit_record directly?
> 
> Next patch, because merging isn't obvious until the split is done.

Can you please at least avoid moving the code around in the next
patch then?  With the function now non-static there shouldn't really
be any reason to move it.

Re: [PATCH 03/11] xfs: refactor and split xfs_log_done()

[Brian Foster]

On Wed, Mar 04, 2020 at 06:53:53PM +1100, Dave Chinner 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>
> ---
>  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, 47 insertions(+), 74 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index d6c42954b70c..702b38e4db6e 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -493,62 +493,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);
> +}
>  

Ok, but it looks like the original xfs_log_done() includes a bit of
logic to end a regrant in the event of commit_record() error (shutdown).
Do we need to lift that logic into callers (that might regrant) as well?

> +/*
> + * 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) {
>  		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
> @@ -2400,8 +2378,6 @@ xlog_write(
>  	 * to account for an extra xlog_op_header here.
>  	 */
>  	ticket->t_curr_res -= sizeof(xlog_op_header_t);
> -	if (ticket->t_flags & XLOG_TIC_INITED)
> -		ticket->t_flags &= ~XLOG_TIC_INITED;

ISTM the above TIC_INITED changes belong in a separate patch from
splitting up xfs_log_done().

>  	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
>  		start_rec_size = 0;
>  
...
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 48435cf2aa16..255065d276fc 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -841,10 +841,11 @@ xlog_cil_push(
>  	}
>  	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);

Seems it would be more simple to call xlog_ticket_done() before the
error check and use the out_abort label (killing off the free ticket
one). Otherwise looks Ok.

Brian

>  
>  	spin_lock(&commit_iclog->ic_callback_lock);
>  	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
> @@ -876,7 +877,7 @@ xlog_cil_push(
>  	return 0;
>  
>  out_abort_free_ticket:
> -	xfs_log_ticket_put(tic);
> +	xlog_ticket_done(log, tic, false);
>  out_abort:
>  	xlog_cil_committed(ctx, true);
>  	return -EIO;
> @@ -1017,7 +1018,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 b192c5a9f9fd..081d4c6de2c8 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -438,14 +438,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 3b208f9a865c..85ea3727878b 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.24.0.rc0
> 

Re: [PATCH 03/11] xfs: refactor and split xfs_log_done()

[Christoph Hellwig]

On Thu, Mar 05, 2020 at 01:06:44PM -0500, Brian Foster wrote:
> Ok, but it looks like the original xfs_log_done() includes a bit of
> logic to end a regrant in the event of commit_record() error (shutdown).
> Do we need to lift that logic into callers (that might regrant) as well?

Yes.

> 
> > +/*
> > + * 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) {
> >  		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_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);
> 
> Seems it would be more simple to call xlog_ticket_done() before the
> error check and use the out_abort label (killing off the free ticket
> one). Otherwise looks Ok.

There are two other jumps to that label, so it can't be removed.

[PATCH 04/11] xfs: merge xlog_commit_record with xlog_write_done()

[Dave Chinner]

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>
---
 fs/xfs/xfs_log.c | 60 +++++++++++++++---------------------------------
 1 file changed, 19 insertions(+), 41 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 702b38e4db6e..100eeaed4a7d 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,
@@ -493,7 +486,8 @@ xfs_log_reserve(
  */
 
 /*
- * Write a commit record to the log to close off a running log write.
+ * Write out the commit record of a transaction associated with the given
+ * ticket to close off a running log write. Return the lsn of the commit record.
  */
 int
 xlog_write_done(
@@ -502,10 +496,26 @@ xlog_write_done(
 	struct xlog_in_core	**iclog,
 	xfs_lsn_t		*lsn)
 {
+	struct xfs_log_iovec reg = {
+		.i_addr = NULL,
+		.i_len = 0,
+		.i_type = XLOG_REG_TYPE_COMMIT,
+	};
+	struct xfs_log_vec vec = {
+		.lv_niovecs = 1,
+		.lv_iovecp = &reg,
+	};
+	int	error;
+
+	ASSERT_ALWAYS(iclog);
+
 	if (XLOG_FORCED_SHUTDOWN(log))
 		return -EIO;
 
-	return xlog_commit_record(log, ticket, iclog, lsn);
+	error = xlog_write(log, &vec, ticket, lsn, iclog, XLOG_COMMIT_TRANS);
+	if (error)
+		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
+	return error;
 }
 
 /*
@@ -1529,38 +1539,6 @@ 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.
- */
-STATIC int
-xlog_commit_record(
-	struct xlog		*log,
-	struct xlog_ticket	*ticket,
-	struct xlog_in_core	**iclog,
-	xfs_lsn_t		*commitlsnp)
-{
-	struct xfs_mount *mp = log->l_mp;
-	int	error;
-	struct xfs_log_iovec reg = {
-		.i_addr = NULL,
-		.i_len = 0,
-		.i_type = XLOG_REG_TYPE_COMMIT,
-	};
-	struct xfs_log_vec vec = {
-		.lv_niovecs = 1,
-		.lv_iovecp = &reg,
-	};
-
-	ASSERT_ALWAYS(iclog);
-	error = xlog_write(log, &vec, ticket, commitlsnp, iclog,
-					XLOG_COMMIT_TRANS);
-	if (error)
-		xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
-	return error;
-}
-
 /*
  * Push on the buffer cache code if we ever use more than 75% of the on-disk
  * log space.  This code pushes on the lsn which would supposedly free up
-- 
2.24.0.rc0

Re: [PATCH 04/11] xfs: merge xlog_commit_record with xlog_write_done()

[Christoph Hellwig]

On Wed, Mar 04, 2020 at 06:53:54PM +1100, Dave Chinner 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.

Ok, you are doing the merge here.  I think it would be nicer to just
simply do that from the beginning, though.

Re: [PATCH 04/11] xfs: merge xlog_commit_record with xlog_write_done()

[Brian Foster]

On Wed, Mar 04, 2020 at 06:53:54PM +1100, Dave Chinner 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>
> ---

I think Christoph disagrees but I actually prefer to see the incremental
steps:

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

>  fs/xfs/xfs_log.c | 60 +++++++++++++++---------------------------------
>  1 file changed, 19 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 702b38e4db6e..100eeaed4a7d 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,
> @@ -493,7 +486,8 @@ xfs_log_reserve(
>   */
>  
>  /*
> - * Write a commit record to the log to close off a running log write.
> + * Write out the commit record of a transaction associated with the given
> + * ticket to close off a running log write. Return the lsn of the commit record.
>   */
>  int
>  xlog_write_done(
> @@ -502,10 +496,26 @@ xlog_write_done(
>  	struct xlog_in_core	**iclog,
>  	xfs_lsn_t		*lsn)
>  {
> +	struct xfs_log_iovec reg = {
> +		.i_addr = NULL,
> +		.i_len = 0,
> +		.i_type = XLOG_REG_TYPE_COMMIT,
> +	};
> +	struct xfs_log_vec vec = {
> +		.lv_niovecs = 1,
> +		.lv_iovecp = &reg,
> +	};
> +	int	error;
> +
> +	ASSERT_ALWAYS(iclog);
> +
>  	if (XLOG_FORCED_SHUTDOWN(log))
>  		return -EIO;
>  
> -	return xlog_commit_record(log, ticket, iclog, lsn);
> +	error = xlog_write(log, &vec, ticket, lsn, iclog, XLOG_COMMIT_TRANS);
> +	if (error)
> +		xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
> +	return error;
>  }
>  
>  /*
> @@ -1529,38 +1539,6 @@ 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.
> - */
> -STATIC int
> -xlog_commit_record(
> -	struct xlog		*log,
> -	struct xlog_ticket	*ticket,
> -	struct xlog_in_core	**iclog,
> -	xfs_lsn_t		*commitlsnp)
> -{
> -	struct xfs_mount *mp = log->l_mp;
> -	int	error;
> -	struct xfs_log_iovec reg = {
> -		.i_addr = NULL,
> -		.i_len = 0,
> -		.i_type = XLOG_REG_TYPE_COMMIT,
> -	};
> -	struct xfs_log_vec vec = {
> -		.lv_niovecs = 1,
> -		.lv_iovecp = &reg,
> -	};
> -
> -	ASSERT_ALWAYS(iclog);
> -	error = xlog_write(log, &vec, ticket, commitlsnp, iclog,
> -					XLOG_COMMIT_TRANS);
> -	if (error)
> -		xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
> -	return error;
> -}
> -
>  /*
>   * Push on the buffer cache code if we ever use more than 75% of the on-disk
>   * log space.  This code pushes on the lsn which would supposedly free up
> -- 
> 2.24.0.rc0
> 

[PATCH 05/11] xfs: factor out unmount record writing

[Dave Chinner]

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>
---
 fs/xfs/xfs_log.c | 59 ++++++++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 100eeaed4a7d..2e9f3baa7cc8 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -485,6 +485,38 @@ 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(
+	struct xlog		*log,
+	struct xlog_ticket	*ticket,
+	xfs_lsn_t		*lsn,
+	uint			flags)
+{
+	/* 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,
+	};
+
+	/* account for space used by record data */
+	ticket->t_curr_res -= sizeof(magic);
+
+	return xlog_write(log, &vec, ticket, lsn, NULL, flags);
+}
+
 /*
  * Write out the commit record of a transaction associated with the given
  * ticket to close off a running log write. Return the lsn of the commit record.
@@ -843,31 +875,13 @@ xfs_log_mount_cancel(
 }
 
 /*
- * 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)
 {
-	/* 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 xlog_in_core	*iclog;
 	struct xlog_ticket	*tic = NULL;
@@ -892,10 +906,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(log, tic, &lsn, flags);
 	/*
 	 * At this point, we're umounting anyway, so there's no point in
 	 * transitioning log state to IOERROR. Just continue...
-- 
2.24.0.rc0

Re: [PATCH 05/11] xfs: factor out unmount record writing

[Christoph Hellwig]

On Wed, Mar 04, 2020 at 06:53:55PM +1100, Dave Chinner 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>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 05/11] xfs: factor out unmount record writing

[Brian Foster]

On Wed, Mar 04, 2020 at 06:53:55PM +1100, Dave Chinner 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>
> ---

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

>  fs/xfs/xfs_log.c | 59 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 100eeaed4a7d..2e9f3baa7cc8 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -485,6 +485,38 @@ 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(
> +	struct xlog		*log,
> +	struct xlog_ticket	*ticket,
> +	xfs_lsn_t		*lsn,
> +	uint			flags)
> +{
> +	/* 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,
> +	};
> +
> +	/* account for space used by record data */
> +	ticket->t_curr_res -= sizeof(magic);
> +
> +	return xlog_write(log, &vec, ticket, lsn, NULL, flags);
> +}
> +
>  /*
>   * Write out the commit record of a transaction associated with the given
>   * ticket to close off a running log write. Return the lsn of the commit record.
> @@ -843,31 +875,13 @@ xfs_log_mount_cancel(
>  }
>  
>  /*
> - * 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)
>  {
> -	/* 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 xlog_in_core	*iclog;
>  	struct xlog_ticket	*tic = NULL;
> @@ -892,10 +906,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(log, tic, &lsn, flags);
>  	/*
>  	 * At this point, we're umounting anyway, so there's no point in
>  	 * transitioning log state to IOERROR. Just continue...
> -- 
> 2.24.0.rc0
> 

[PATCH 06/11] xfs: move xlog_state_ioerror()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

To clean up unmount record writing error handling, we need to move
xlog_state_ioerror() higher up in the file. Also move the setting of
the XLOG_IO_ERROR state to inside the function.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log.c | 59 ++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 2e9f3baa7cc8..0de3c32d42b6 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -874,6 +874,36 @@ xfs_log_mount_cancel(
 	xfs_log_unmount(mp);
 }
 
+/*
+ * Mark all iclogs IOERROR. l_icloglock is held by the caller.
+ */
+STATIC int
+xlog_state_ioerror(
+	struct xlog	*log)
+{
+	xlog_in_core_t	*iclog, *ic;
+
+	log->l_flags |= XLOG_IO_ERROR;
+
+	iclog = log->l_iclog;
+	if (iclog->ic_state != XLOG_STATE_IOERROR) {
+		/*
+		 * Mark all the incore logs IOERROR.
+		 * From now on, no log flushes will result.
+		 */
+		ic = iclog;
+		do {
+			ic->ic_state = XLOG_STATE_IOERROR;
+			ic = ic->ic_next;
+		} while (ic != iclog);
+		return 0;
+	}
+	/*
+	 * Return non-zero, if state transition has already happened.
+	 */
+	return 1;
+}
+
 /*
  * Mark the filesystem clean by writing an unmount record to the head of the
  * log.
@@ -3770,34 +3800,6 @@ xlog_verify_iclog(
 }	/* xlog_verify_iclog */
 #endif
 
-/*
- * Mark all iclogs IOERROR. l_icloglock is held by the caller.
- */
-STATIC int
-xlog_state_ioerror(
-	struct xlog	*log)
-{
-	xlog_in_core_t	*iclog, *ic;
-
-	iclog = log->l_iclog;
-	if (iclog->ic_state != XLOG_STATE_IOERROR) {
-		/*
-		 * Mark all the incore logs IOERROR.
-		 * From now on, no log flushes will result.
-		 */
-		ic = iclog;
-		do {
-			ic->ic_state = XLOG_STATE_IOERROR;
-			ic = ic->ic_next;
-		} while (ic != iclog);
-		return 0;
-	}
-	/*
-	 * Return non-zero, if state transition has already happened.
-	 */
-	return 1;
-}
-
 /*
  * This is called from xfs_force_shutdown, when we're forcibly
  * shutting down the filesystem, typically because of an IO error.
@@ -3868,7 +3870,6 @@ xfs_log_force_umount(
 	 * Mark the log and the iclogs with IO error flags to prevent any
 	 * further log IO from being issued or completed.
 	 */
-	log->l_flags |= XLOG_IO_ERROR;
 	retval = xlog_state_ioerror(log);
 	spin_unlock(&log->l_icloglock);
 
-- 
2.24.0.rc0

Re: [PATCH 06/11] xfs: move xlog_state_ioerror()

[Christoph Hellwig]

On Wed, Mar 04, 2020 at 06:53:56PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> To clean up unmount record writing error handling, we need to move
> xlog_state_ioerror() higher up in the file. Also move the setting of
> the XLOG_IO_ERROR state to inside the function.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

FYI, I have a pending series that kills xlog_state_ioerror and
XLOG_IO_ERROR.  Let me send that out now that Brians fix is in for-next.

Re: [PATCH 06/11] xfs: move xlog_state_ioerror()

[Dave Chinner]

On Wed, Mar 04, 2020 at 07:51:40AM -0800, Christoph Hellwig wrote:
> On Wed, Mar 04, 2020 at 06:53:56PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > To clean up unmount record writing error handling, we need to move
> > xlog_state_ioerror() higher up in the file. Also move the setting of
> > the XLOG_IO_ERROR state to inside the function.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> FYI, I have a pending series that kills xlog_state_ioerror and
> XLOG_IO_ERROR.  Let me send that out now that Brians fix is in for-next.

Can you rebase that on top of this? removing IOERROR is a much more
invasive and riskier set of state machine changes compared to what
this patchset does. This patchset simplifies some of the error
handling that handles IOERROR, too...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 06/11] xfs: move xlog_state_ioerror()

[Christoph Hellwig]

On Thu, Mar 05, 2020 at 08:41:15AM +1100, Dave Chinner wrote:
> On Wed, Mar 04, 2020 at 07:51:40AM -0800, Christoph Hellwig wrote:
> > On Wed, Mar 04, 2020 at 06:53:56PM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > To clean up unmount record writing error handling, we need to move
> > > xlog_state_ioerror() higher up in the file. Also move the setting of
> > > the XLOG_IO_ERROR state to inside the function.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > 
> > FYI, I have a pending series that kills xlog_state_ioerror and
> > XLOG_IO_ERROR.  Let me send that out now that Brians fix is in for-next.
> 
> Can you rebase that on top of this? removing IOERROR is a much more
> invasive and riskier set of state machine changes compared to what
> this patchset does. This patchset simplifies some of the error
> handling that handles IOERROR, too...

I find this lipstick on the pig here a little pointless, especially
moving things around that are planned to be removed..

Re: [PATCH 06/11] xfs: move xlog_state_ioerror()

[Brian Foster]

On Wed, Mar 04, 2020 at 06:53:56PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> To clean up unmount record writing error handling, we need to move
> xlog_state_ioerror() higher up in the file. Also move the setting of
> the XLOG_IO_ERROR state to inside the function.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log.c | 59 ++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 2e9f3baa7cc8..0de3c32d42b6 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -874,6 +874,36 @@ xfs_log_mount_cancel(
>  	xfs_log_unmount(mp);
>  }
>  
> +/*
> + * Mark all iclogs IOERROR. l_icloglock is held by the caller.
> + */
> +STATIC int
> +xlog_state_ioerror(
> +	struct xlog	*log)
> +{
> +	xlog_in_core_t	*iclog, *ic;

Might as well kill off the typedef usage, though it sounds like
Christoph has code to remove this. Eh, that discussion aside this looks
fine to me:

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

> +
> +	log->l_flags |= XLOG_IO_ERROR;
> +
> +	iclog = log->l_iclog;
> +	if (iclog->ic_state != XLOG_STATE_IOERROR) {
> +		/*
> +		 * Mark all the incore logs IOERROR.
> +		 * From now on, no log flushes will result.
> +		 */
> +		ic = iclog;
> +		do {
> +			ic->ic_state = XLOG_STATE_IOERROR;
> +			ic = ic->ic_next;
> +		} while (ic != iclog);
> +		return 0;
> +	}
> +	/*
> +	 * Return non-zero, if state transition has already happened.
> +	 */
> +	return 1;
> +}
> +
>  /*
>   * Mark the filesystem clean by writing an unmount record to the head of the
>   * log.
> @@ -3770,34 +3800,6 @@ xlog_verify_iclog(
>  }	/* xlog_verify_iclog */
>  #endif
>  
> -/*
> - * Mark all iclogs IOERROR. l_icloglock is held by the caller.
> - */
> -STATIC int
> -xlog_state_ioerror(
> -	struct xlog	*log)
> -{
> -	xlog_in_core_t	*iclog, *ic;
> -
> -	iclog = log->l_iclog;
> -	if (iclog->ic_state != XLOG_STATE_IOERROR) {
> -		/*
> -		 * Mark all the incore logs IOERROR.
> -		 * From now on, no log flushes will result.
> -		 */
> -		ic = iclog;
> -		do {
> -			ic->ic_state = XLOG_STATE_IOERROR;
> -			ic = ic->ic_next;
> -		} while (ic != iclog);
> -		return 0;
> -	}
> -	/*
> -	 * Return non-zero, if state transition has already happened.
> -	 */
> -	return 1;
> -}
> -
>  /*
>   * This is called from xfs_force_shutdown, when we're forcibly
>   * shutting down the filesystem, typically because of an IO error.
> @@ -3868,7 +3870,6 @@ xfs_log_force_umount(
>  	 * Mark the log and the iclogs with IO error flags to prevent any
>  	 * further log IO from being issued or completed.
>  	 */
> -	log->l_flags |= XLOG_IO_ERROR;
>  	retval = xlog_state_ioerror(log);
>  	spin_unlock(&log->l_icloglock);
>  
> -- 
> 2.24.0.rc0
> 

[PATCH 07/11] xfs: clean up xlog_state_ioerror()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 0de3c32d42b6..a310ca9e7615 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -875,33 +875,27 @@ xfs_log_mount_cancel(
 }
 
 /*
- * Mark all iclogs IOERROR. l_icloglock is held by the caller.
+ * Mark all iclogs IOERROR. l_icloglock is held by the caller. Returns 1 if the
+ * log was already in an IO state, 0 otherwise. From now one, no log flushes
+ * will occur.
  */
 STATIC int
 xlog_state_ioerror(
-	struct xlog	*log)
+	struct xlog		*log)
 {
-	xlog_in_core_t	*iclog, *ic;
+	struct xlog_in_core	*iclog = log->l_iclog;
+	struct xlog_in_core	*ic = iclog;
 
 	log->l_flags |= XLOG_IO_ERROR;
+	if (iclog->ic_state == XLOG_STATE_IOERROR)
+		return 1;
 
-	iclog = log->l_iclog;
-	if (iclog->ic_state != XLOG_STATE_IOERROR) {
-		/*
-		 * Mark all the incore logs IOERROR.
-		 * From now on, no log flushes will result.
-		 */
-		ic = iclog;
-		do {
-			ic->ic_state = XLOG_STATE_IOERROR;
-			ic = ic->ic_next;
-		} while (ic != iclog);
-		return 0;
-	}
-	/*
-	 * Return non-zero, if state transition has already happened.
-	 */
-	return 1;
+	do {
+		ic->ic_state = XLOG_STATE_IOERROR;
+		ic = ic->ic_next;
+	} while (ic != iclog);
+
+	return 0;
 }
 
 /*
-- 
2.24.0.rc0

Re: [PATCH 07/11] xfs: clean up xlog_state_ioerror()

[Brian Foster]

On Wed, Mar 04, 2020 at 06:53:57PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log.c | 34 ++++++++++++++--------------------
>  1 file changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 0de3c32d42b6..a310ca9e7615 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -875,33 +875,27 @@ xfs_log_mount_cancel(
>  }
>  
>  /*
> - * Mark all iclogs IOERROR. l_icloglock is held by the caller.
> + * Mark all iclogs IOERROR. l_icloglock is held by the caller. Returns 1 if the
> + * log was already in an IO state, 0 otherwise. From now one, no log flushes

							   s/one/on

Otherwise looks good:

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

> + * will occur.
>   */
>  STATIC int
>  xlog_state_ioerror(
> -	struct xlog	*log)
> +	struct xlog		*log)
>  {
> -	xlog_in_core_t	*iclog, *ic;
> +	struct xlog_in_core	*iclog = log->l_iclog;
> +	struct xlog_in_core	*ic = iclog;
>  
>  	log->l_flags |= XLOG_IO_ERROR;
> +	if (iclog->ic_state == XLOG_STATE_IOERROR)
> +		return 1;
>  
> -	iclog = log->l_iclog;
> -	if (iclog->ic_state != XLOG_STATE_IOERROR) {
> -		/*
> -		 * Mark all the incore logs IOERROR.
> -		 * From now on, no log flushes will result.
> -		 */
> -		ic = iclog;
> -		do {
> -			ic->ic_state = XLOG_STATE_IOERROR;
> -			ic = ic->ic_next;
> -		} while (ic != iclog);
> -		return 0;
> -	}
> -	/*
> -	 * Return non-zero, if state transition has already happened.
> -	 */
> -	return 1;
> +	do {
> +		ic->ic_state = XLOG_STATE_IOERROR;
> +		ic = ic->ic_next;
> +	} while (ic != iclog);
> +
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.24.0.rc0
> 

[PATCH 08/11] xfs: rename the log unmount writing functions.

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The naming and calling conventions are a bit of a mess. Clean it up
so the call chain looks like:

	xfs_log_unmount_write(mp)
	  xlog_unmount_write(log)
	    xlog_write_unmount_record(log, ticket)

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a310ca9e7615..bdf604d31d8c 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -491,7 +491,7 @@ xfs_log_reserve(
  * transaction context that has already done the accounting for us.
  */
 static int
-xlog_write_unmount(
+xlog_write_unmount_record(
 	struct xlog		*log,
 	struct xlog_ticket	*ticket,
 	xfs_lsn_t		*lsn,
@@ -903,10 +903,10 @@ xlog_state_ioerror(
  * log.
  */
 static void
-xfs_log_write_unmount_record(
-	struct xfs_mount	*mp)
+xlog_unmount_write(
+	struct xlog		*log)
 {
-	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;
@@ -930,7 +930,7 @@ xfs_log_write_unmount_record(
 		flags &= ~XLOG_UNMOUNT_TRANS;
 	}
 
-	error = xlog_write_unmount(log, tic, &lsn, 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...
@@ -1006,7 +1006,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 	} while (iclog != first_iclog);
 #endif
 	if (! (XLOG_FORCED_SHUTDOWN(log))) {
-		xfs_log_write_unmount_record(mp);
+		xlog_unmount_write(log);
 	} else {
 		/*
 		 * We're already in forced_shutdown mode, couldn't
-- 
2.24.0.rc0

Re: [PATCH 08/11] xfs: rename the log unmount writing functions.

[Christoph Hellwig]

On Wed, Mar 04, 2020 at 06:53:58PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The naming and calling conventions are a bit of a mess. Clean it up
> so the call chain looks like:
> 
> 	xfs_log_unmount_write(mp)
> 	  xlog_unmount_write(log)
> 	    xlog_write_unmount_record(log, ticket)
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 08/11] xfs: rename the log unmount writing functions.

[Brian Foster]

On Wed, Mar 04, 2020 at 06:53:58PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The naming and calling conventions are a bit of a mess. Clean it up
> so the call chain looks like:
> 
> 	xfs_log_unmount_write(mp)
> 	  xlog_unmount_write(log)
> 	    xlog_write_unmount_record(log, ticket)
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

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

>  fs/xfs/xfs_log.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index a310ca9e7615..bdf604d31d8c 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -491,7 +491,7 @@ xfs_log_reserve(
>   * transaction context that has already done the accounting for us.
>   */
>  static int
> -xlog_write_unmount(
> +xlog_write_unmount_record(
>  	struct xlog		*log,
>  	struct xlog_ticket	*ticket,
>  	xfs_lsn_t		*lsn,
> @@ -903,10 +903,10 @@ xlog_state_ioerror(
>   * log.
>   */
>  static void
> -xfs_log_write_unmount_record(
> -	struct xfs_mount	*mp)
> +xlog_unmount_write(
> +	struct xlog		*log)
>  {
> -	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;
> @@ -930,7 +930,7 @@ xfs_log_write_unmount_record(
>  		flags &= ~XLOG_UNMOUNT_TRANS;
>  	}
>  
> -	error = xlog_write_unmount(log, tic, &lsn, 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...
> @@ -1006,7 +1006,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  	} while (iclog != first_iclog);
>  #endif
>  	if (! (XLOG_FORCED_SHUTDOWN(log))) {
> -		xfs_log_write_unmount_record(mp);
> +		xlog_unmount_write(log);
>  	} else {
>  		/*
>  		 * We're already in forced_shutdown mode, couldn't
> -- 
> 2.24.0.rc0
> 

[PATCH 09/11] xfs: merge unmount record write iclog cleanup.

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The unmount iclog handling is duplicated in both
xfs_log_unmount_write() and xfs_log_write_unmount_record(). We only
need one copy of it in xfs_log_unmount_write() because that is the
only function that calls xfs_log_write_unmount_record().

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log.c | 130 +++++++++++++++++------------------------------
 1 file changed, 48 insertions(+), 82 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index bdf604d31d8c..a687c20dd77d 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -898,16 +898,11 @@ xlog_state_ioerror(
 	return 0;
 }
 
-/*
- * Mark the filesystem clean by writing an unmount record to the head of the
- * log.
- */
-static void
+static int
 xlog_unmount_write(
 	struct xlog		*log)
 {
 	struct xfs_mount	*mp = log->l_mp;
-	struct xlog_in_core	*iclog;
 	struct xlog_ticket	*tic = NULL;
 	xfs_lsn_t		lsn;
 	uint			flags = XLOG_UNMOUNT_TRANS;
@@ -931,61 +926,41 @@ xlog_unmount_write(
 	}
 
 	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...
-	 */
-out_err:
-	if (error)
-		xfs_alert(mp, "%s: unmount record failed", __func__);
-
-	spin_lock(&log->l_icloglock);
-	iclog = log->l_iclog;
-	atomic_inc(&iclog->ic_refcnt);
-	xlog_state_want_sync(log, iclog);
-	error = xlog_state_release_iclog(log, iclog);
-	switch (iclog->ic_state) {
-	default:
-		if (!XLOG_FORCED_SHUTDOWN(log)) {
-			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
-			break;
-		}
-		/* fall through */
-	case XLOG_STATE_ACTIVE:
-	case XLOG_STATE_DIRTY:
+	if (error) {
+		/* A full shutdown is unnecessary at this point of unmount */
+		spin_lock(&log->l_icloglock);
+		log->l_flags |= XLOG_IO_ERROR;
+		xlog_state_ioerror(log);
 		spin_unlock(&log->l_icloglock);
-		break;
 	}
 
-	if (tic) {
-		trace_xfs_log_umount_write(log, tic);
-		xlog_ungrant_log_space(log, tic);
-		xfs_log_ticket_put(tic);
-	}
+	trace_xfs_log_umount_write(log, tic);
+	xlog_ungrant_log_space(log, tic);
+	xfs_log_ticket_put(tic);
+out_err:
+	if (error)
+		xfs_alert(mp, "%s: unmount record failed", __func__);
+	return error;
 }
 
 /*
- * Unmount record used to have a string "Unmount filesystem--" in the
- * data section where the "Un" was really a magic number (XLOG_UNMOUNT_TYPE).
- * We just write the magic number now since that particular field isn't
- * currently architecture converted and "Unmount" is a bit foo.
- * As far as I know, there weren't any dependencies on the old behaviour.
+ * Finalise the unmount by writing the unmount record to the log. This is the
+ * mark that the filesystem was cleanly unmounted.
+ *
+ * Avoid writing the unmount record on no-recovery mounts, ro-devices, or when
+ * the log has already been shut down.
  */
-
 static int
-xfs_log_unmount_write(xfs_mount_t *mp)
+xfs_log_unmount_write(
+	struct xfs_mount	*mp)
 {
-	struct xlog	 *log = mp->m_log;
-	xlog_in_core_t	 *iclog;
+	struct xlog		*log = mp->m_log;
+	struct xlog_in_core	*iclog;
 #ifdef DEBUG
-	xlog_in_core_t	 *first_iclog;
+	struct xlog_in_core	*first_iclog;
 #endif
-	int		 error;
+	int			error;
 
-	/*
-	 * Don't write out unmount record on norecovery mounts or ro devices.
-	 * Or, if we are doing a forced umount (typically because of IO errors).
-	 */
 	if (mp->m_flags & XFS_MOUNT_NORECOVERY ||
 	    xfs_readonly_buftarg(log->l_targ)) {
 		ASSERT(mp->m_flags & XFS_MOUNT_RDONLY);
@@ -1005,41 +980,32 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 		iclog = iclog->ic_next;
 	} while (iclog != first_iclog);
 #endif
-	if (! (XLOG_FORCED_SHUTDOWN(log))) {
-		xlog_unmount_write(log);
-	} else {
-		/*
-		 * We're already in forced_shutdown mode, couldn't
-		 * even attempt to write out the unmount transaction.
-		 *
-		 * Go through the motions of sync'ing and releasing
-		 * the iclog, even though no I/O will actually happen,
-		 * we need to wait for other log I/Os that may already
-		 * be in progress.  Do this as a separate section of
-		 * code so we'll know if we ever get stuck here that
-		 * we're in this odd situation of trying to unmount
-		 * a file system that went into forced_shutdown as
-		 * the result of an unmount..
-		 */
-		spin_lock(&log->l_icloglock);
-		iclog = log->l_iclog;
-		atomic_inc(&iclog->ic_refcnt);
-		xlog_state_want_sync(log, iclog);
-		error =  xlog_state_release_iclog(log, iclog);
-		switch (iclog->ic_state) {
-		case XLOG_STATE_ACTIVE:
-		case XLOG_STATE_DIRTY:
-		case XLOG_STATE_IOERROR:
-			spin_unlock(&log->l_icloglock);
-			break;
-		default:
-			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
-			break;
-		}
-	}
 
+	if (!XLOG_FORCED_SHUTDOWN(log))
+		error = xlog_unmount_write(log);
+
+	/*
+	 * Sync and release the current iclog so the unmount record gets to
+	 * disk. If we are in a shutdown state, no IO will be done, but we still
+	 * we need to wait for other log I/Os that may already be in progress.
+	 */
+	spin_lock(&log->l_icloglock);
+	iclog = log->l_iclog;
+	atomic_inc(&iclog->ic_refcnt);
+	xlog_state_want_sync(log, iclog);
+	error =  xlog_state_release_iclog(log, iclog);
+	switch (iclog->ic_state) {
+	case XLOG_STATE_ACTIVE:
+	case XLOG_STATE_DIRTY:
+	case XLOG_STATE_IOERROR:
+		spin_unlock(&log->l_icloglock);
+		break;
+	default:
+		xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
+		break;
+	}
 	return error;
-}	/* xfs_log_unmount_write */
+}
 
 /*
  * Empty the log for unmount/freeze.
-- 
2.24.0.rc0

Re: [PATCH 09/11] xfs: merge unmount record write iclog cleanup.

[Christoph Hellwig]

On Wed, Mar 04, 2020 at 06:53:59PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The unmount iclog handling is duplicated in both
> xfs_log_unmount_write() and xfs_log_write_unmount_record(). We only
> need one copy of it in xfs_log_unmount_write() because that is the
> only function that calls xfs_log_write_unmount_record().

The copy in xfs_log_unmount_write actually is dead code.  It only
is called in the XLOG_FORCED_SHUTDOWN case, in which case all iclogs
are marked as STATE_IOERROR, and thus xlog_state_release_iclog is
a no-op.  I really need to send the series out to clean this up
ASAP..

Re: [PATCH 09/11] xfs: merge unmount record write iclog cleanup.

[Dave Chinner]

On Wed, Mar 04, 2020 at 07:53:32AM -0800, Christoph Hellwig wrote:
> On Wed, Mar 04, 2020 at 06:53:59PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The unmount iclog handling is duplicated in both
> > xfs_log_unmount_write() and xfs_log_write_unmount_record(). We only
> > need one copy of it in xfs_log_unmount_write() because that is the
> > only function that calls xfs_log_write_unmount_record().
> 
> The copy in xfs_log_unmount_write actually is dead code.  It only
> is called in the XLOG_FORCED_SHUTDOWN case, in which case all iclogs
> are marked as STATE_IOERROR, and thus xlog_state_release_iclog is
> a no-op.  I really need to send the series out to clean this up
> ASAP..

Well, this patch pretty much solves that "dead code" problem in that
it now handles already shut down, error in unmount record write and
successful unmount record write now. i.e. we run the same code in
all cases now, so you'll only need to fix the IOERROR handling in
one place :P

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 09/11] xfs: merge unmount record write iclog cleanup.

[Christoph Hellwig]

On Thu, Mar 05, 2020 at 08:38:54AM +1100, Dave Chinner wrote:
> On Wed, Mar 04, 2020 at 07:53:32AM -0800, Christoph Hellwig wrote:
> > On Wed, Mar 04, 2020 at 06:53:59PM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > The unmount iclog handling is duplicated in both
> > > xfs_log_unmount_write() and xfs_log_write_unmount_record(). We only
> > > need one copy of it in xfs_log_unmount_write() because that is the
> > > only function that calls xfs_log_write_unmount_record().
> > 
> > The copy in xfs_log_unmount_write actually is dead code.  It only
> > is called in the XLOG_FORCED_SHUTDOWN case, in which case all iclogs
> > are marked as STATE_IOERROR, and thus xlog_state_release_iclog is
> > a no-op.  I really need to send the series out to clean this up
> > ASAP..
> 
> Well, this patch pretty much solves that "dead code" problem in that
> it now handles already shut down, error in unmount record write and
> successful unmount record write now. i.e. we run the same code in
> all cases now, so you'll only need to fix the IOERROR handling in
> one place :P

It doesn't really.  We still end up with more convoluted logic after
your series, that goes through a whole bunch of steps that don't make
any sense when the log is already shut down.  Just like everywhere
else we should just return early with a shutdown log / iclog and just
delete this code instead of rearranging the chairs.

Re: [PATCH 09/11] xfs: merge unmount record write iclog cleanup.

[Brian Foster]

On Wed, Mar 04, 2020 at 06:53:59PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The unmount iclog handling is duplicated in both
> xfs_log_unmount_write() and xfs_log_write_unmount_record(). We only
> need one copy of it in xfs_log_unmount_write() because that is the
> only function that calls xfs_log_write_unmount_record().
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Any reason for the period at the end of the patch subject? I just
noticed it now, but the previous patch has one as well.

>  fs/xfs/xfs_log.c | 130 +++++++++++++++++------------------------------
>  1 file changed, 48 insertions(+), 82 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index bdf604d31d8c..a687c20dd77d 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
...
> @@ -931,61 +926,41 @@ xlog_unmount_write(
>  	}
>  
>  	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...
> -	 */
> -out_err:
> -	if (error)
> -		xfs_alert(mp, "%s: unmount record failed", __func__);
> -
> -	spin_lock(&log->l_icloglock);
> -	iclog = log->l_iclog;
> -	atomic_inc(&iclog->ic_refcnt);
> -	xlog_state_want_sync(log, iclog);
> -	error = xlog_state_release_iclog(log, iclog);
> -	switch (iclog->ic_state) {
> -	default:
> -		if (!XLOG_FORCED_SHUTDOWN(log)) {
> -			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> -			break;
> -		}
> -		/* fall through */
> -	case XLOG_STATE_ACTIVE:
> -	case XLOG_STATE_DIRTY:
> +	if (error) {
> +		/* A full shutdown is unnecessary at this point of unmount */
> +		spin_lock(&log->l_icloglock);
> +		log->l_flags |= XLOG_IO_ERROR;

A previous patch added this to xlog_state_ioerror(). Nits aside, the
rest looks fine to me:

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

> +		xlog_state_ioerror(log);
>  		spin_unlock(&log->l_icloglock);
> -		break;
>  	}
>  
> -	if (tic) {
> -		trace_xfs_log_umount_write(log, tic);
> -		xlog_ungrant_log_space(log, tic);
> -		xfs_log_ticket_put(tic);
> -	}
> +	trace_xfs_log_umount_write(log, tic);
> +	xlog_ungrant_log_space(log, tic);
> +	xfs_log_ticket_put(tic);
> +out_err:
> +	if (error)
> +		xfs_alert(mp, "%s: unmount record failed", __func__);
> +	return error;
>  }
>  
>  /*
> - * Unmount record used to have a string "Unmount filesystem--" in the
> - * data section where the "Un" was really a magic number (XLOG_UNMOUNT_TYPE).
> - * We just write the magic number now since that particular field isn't
> - * currently architecture converted and "Unmount" is a bit foo.
> - * As far as I know, there weren't any dependencies on the old behaviour.
> + * Finalise the unmount by writing the unmount record to the log. This is the
> + * mark that the filesystem was cleanly unmounted.
> + *
> + * Avoid writing the unmount record on no-recovery mounts, ro-devices, or when
> + * the log has already been shut down.
>   */
> -
>  static int
> -xfs_log_unmount_write(xfs_mount_t *mp)
> +xfs_log_unmount_write(
> +	struct xfs_mount	*mp)
>  {
> -	struct xlog	 *log = mp->m_log;
> -	xlog_in_core_t	 *iclog;
> +	struct xlog		*log = mp->m_log;
> +	struct xlog_in_core	*iclog;
>  #ifdef DEBUG
> -	xlog_in_core_t	 *first_iclog;
> +	struct xlog_in_core	*first_iclog;
>  #endif
> -	int		 error;
> +	int			error;
>  
> -	/*
> -	 * Don't write out unmount record on norecovery mounts or ro devices.
> -	 * Or, if we are doing a forced umount (typically because of IO errors).
> -	 */
>  	if (mp->m_flags & XFS_MOUNT_NORECOVERY ||
>  	    xfs_readonly_buftarg(log->l_targ)) {
>  		ASSERT(mp->m_flags & XFS_MOUNT_RDONLY);
> @@ -1005,41 +980,32 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  		iclog = iclog->ic_next;
>  	} while (iclog != first_iclog);
>  #endif
> -	if (! (XLOG_FORCED_SHUTDOWN(log))) {
> -		xlog_unmount_write(log);
> -	} else {
> -		/*
> -		 * We're already in forced_shutdown mode, couldn't
> -		 * even attempt to write out the unmount transaction.
> -		 *
> -		 * Go through the motions of sync'ing and releasing
> -		 * the iclog, even though no I/O will actually happen,
> -		 * we need to wait for other log I/Os that may already
> -		 * be in progress.  Do this as a separate section of
> -		 * code so we'll know if we ever get stuck here that
> -		 * we're in this odd situation of trying to unmount
> -		 * a file system that went into forced_shutdown as
> -		 * the result of an unmount..
> -		 */
> -		spin_lock(&log->l_icloglock);
> -		iclog = log->l_iclog;
> -		atomic_inc(&iclog->ic_refcnt);
> -		xlog_state_want_sync(log, iclog);
> -		error =  xlog_state_release_iclog(log, iclog);
> -		switch (iclog->ic_state) {
> -		case XLOG_STATE_ACTIVE:
> -		case XLOG_STATE_DIRTY:
> -		case XLOG_STATE_IOERROR:
> -			spin_unlock(&log->l_icloglock);
> -			break;
> -		default:
> -			xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> -			break;
> -		}
> -	}
>  
> +	if (!XLOG_FORCED_SHUTDOWN(log))
> +		error = xlog_unmount_write(log);
> +
> +	/*
> +	 * Sync and release the current iclog so the unmount record gets to
> +	 * disk. If we are in a shutdown state, no IO will be done, but we still
> +	 * we need to wait for other log I/Os that may already be in progress.
> +	 */
> +	spin_lock(&log->l_icloglock);
> +	iclog = log->l_iclog;
> +	atomic_inc(&iclog->ic_refcnt);
> +	xlog_state_want_sync(log, iclog);
> +	error =  xlog_state_release_iclog(log, iclog);
> +	switch (iclog->ic_state) {
> +	case XLOG_STATE_ACTIVE:
> +	case XLOG_STATE_DIRTY:
> +	case XLOG_STATE_IOERROR:
> +		spin_unlock(&log->l_icloglock);
> +		break;
> +	default:
> +		xlog_wait(&iclog->ic_force_wait, &log->l_icloglock);
> +		break;
> +	}
>  	return error;
> -}	/* xfs_log_unmount_write */
> +}
>  
>  /*
>   * Empty the log for unmount/freeze.
> -- 
> 2.24.0.rc0
> 

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

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log.c | 71 ++++++++++++++----------------------------------
 1 file changed, 20 insertions(+), 51 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a687c20dd77d..89956484848f 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -477,14 +477,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
@@ -1968,7 +1960,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.
@@ -2511,14 +2503,6 @@ xlog_write(
 	return error;
 }
 
-
-/*****************************************************************************
- *
- *		State Machine functions
- *
- *****************************************************************************
- */
-
 /*
  * An iclog has just finished IO completion processing, so we need to update
  * the iclog state and propagate that up into the overall log state. Hence we
@@ -2887,8 +2871,8 @@ xlog_state_done_syncing(
 	 */
 	wake_up_all(&iclog->ic_write_wait);
 	spin_unlock(&log->l_icloglock);
-	xlog_state_do_callback(log, aborted);	/* also cleans log */
-}	/* xlog_state_done_syncing */
+	xlog_state_do_callback(log, aborted);
+}
 
 
 /*
@@ -3008,14 +2992,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.
  */
 STATIC void
 xlog_regrant_reserve_log_space(
@@ -3047,7 +3031,7 @@ xlog_regrant_reserve_log_space(
 
 	ticket->t_curr_res = ticket->t_unit_res;
 	xlog_tic_reset_res(ticket);
-}	/* xlog_regrant_reserve_log_space */
+}
 
 
 /*
@@ -3096,11 +3080,11 @@ xlog_ungrant_log_space(
 }
 
 /*
- * 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.
- * When this routine is called from xlog_state_get_iclog_space(), the
- * exact size of the iclog has not yet been determined.  All we know is
- * that every data block.  We have run out of space in this log record.
+ * 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.  When this routine
+ * is called from xlog_state_get_iclog_space(), the exact size of the iclog has
+ * not yet been determined.  All we know is that every data block.  We have run
+ * out of space in this log record.
  */
 STATIC void
 xlog_state_switch_iclogs(
@@ -3143,7 +3127,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.
@@ -3397,14 +3381,6 @@ xlog_state_want_sync(
 	}
 }
 
-
-/*****************************************************************************
- *
- *		TICKET functions
- *
- *****************************************************************************
- */
-
 /*
  * Free a used ticket when its refcount falls to zero.
  */
@@ -3562,13 +3538,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
@@ -3654,7 +3623,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.
@@ -3757,7 +3726,7 @@ xlog_verify_iclog(
 		}
 		ptr += sizeof(xlog_op_header_t) + op_len;
 	}
-}	/* xlog_verify_iclog */
+}
 #endif
 
 /*
-- 
2.24.0.rc0

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

[Christoph Hellwig]

On Wed, Mar 04, 2020 at 06:54:00PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Brian Foster]

On Wed, Mar 04, 2020 at 06:54:00PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

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

>  fs/xfs/xfs_log.c | 71 ++++++++++++++----------------------------------
>  1 file changed, 20 insertions(+), 51 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index a687c20dd77d..89956484848f 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -477,14 +477,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
> @@ -1968,7 +1960,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.
> @@ -2511,14 +2503,6 @@ xlog_write(
>  	return error;
>  }
>  
> -
> -/*****************************************************************************
> - *
> - *		State Machine functions
> - *
> - *****************************************************************************
> - */
> -
>  /*
>   * An iclog has just finished IO completion processing, so we need to update
>   * the iclog state and propagate that up into the overall log state. Hence we
> @@ -2887,8 +2871,8 @@ xlog_state_done_syncing(
>  	 */
>  	wake_up_all(&iclog->ic_write_wait);
>  	spin_unlock(&log->l_icloglock);
> -	xlog_state_do_callback(log, aborted);	/* also cleans log */
> -}	/* xlog_state_done_syncing */
> +	xlog_state_do_callback(log, aborted);
> +}
>  
>  
>  /*
> @@ -3008,14 +2992,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.
>   */
>  STATIC void
>  xlog_regrant_reserve_log_space(
> @@ -3047,7 +3031,7 @@ xlog_regrant_reserve_log_space(
>  
>  	ticket->t_curr_res = ticket->t_unit_res;
>  	xlog_tic_reset_res(ticket);
> -}	/* xlog_regrant_reserve_log_space */
> +}
>  
>  
>  /*
> @@ -3096,11 +3080,11 @@ xlog_ungrant_log_space(
>  }
>  
>  /*
> - * 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.
> - * When this routine is called from xlog_state_get_iclog_space(), the
> - * exact size of the iclog has not yet been determined.  All we know is
> - * that every data block.  We have run out of space in this log record.
> + * 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.  When this routine
> + * is called from xlog_state_get_iclog_space(), the exact size of the iclog has
> + * not yet been determined.  All we know is that every data block.  We have run
> + * out of space in this log record.
>   */
>  STATIC void
>  xlog_state_switch_iclogs(
> @@ -3143,7 +3127,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.
> @@ -3397,14 +3381,6 @@ xlog_state_want_sync(
>  	}
>  }
>  
> -
> -/*****************************************************************************
> - *
> - *		TICKET functions
> - *
> - *****************************************************************************
> - */
> -
>  /*
>   * Free a used ticket when its refcount falls to zero.
>   */
> @@ -3562,13 +3538,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
> @@ -3654,7 +3623,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.
> @@ -3757,7 +3726,7 @@ xlog_verify_iclog(
>  		}
>  		ptr += sizeof(xlog_op_header_t) + op_len;
>  	}
> -}	/* xlog_verify_iclog */
> +}
>  #endif
>  
>  /*
> -- 
> 2.24.0.rc0
> 

[PATCH 11/11] xfs: kill XLOG_TIC_INITED

[Dave Chinner]

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>
---
 fs/xfs/xfs_log.c      | 1 -
 fs/xfs/xfs_log_priv.h | 6 ++----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 89956484848f..b91efc5829e1 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3529,7 +3529,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 081d4c6de2c8..e989cf024ffe 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.24.0.rc0

Re: [PATCH 11/11] xfs: kill XLOG_TIC_INITED

[Christoph Hellwig]

On Wed, Mar 04, 2020 at 06:54:01PM +1100, Dave Chinner 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>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 11/11] xfs: kill XLOG_TIC_INITED

[Brian Foster]

On Wed, Mar 04, 2020 at 06:54:01PM +1100, Dave Chinner 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>
> ---

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

>  fs/xfs/xfs_log.c      | 1 -
>  fs/xfs/xfs_log_priv.h | 6 ++----
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 89956484848f..b91efc5829e1 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3529,7 +3529,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 081d4c6de2c8..e989cf024ffe 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.24.0.rc0
> 

Re: [PATCH 00/11] xfs: clean up log tickets and record writes

[Christoph Hellwig]

FYI, I'd prefer to just see the series without patches 6, 7 and 9 for
now.  They aren't really related to the rest, and I think this series:

    http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-kill-XLOG_STATE_IOERROR

has a better approach to sort out those areas.  The rest looks really
good to me modulo minor cleanups here and there.

Re: [PATCH 00/11] xfs: clean up log tickets and record writes

[Dave Chinner]

On Thu, Mar 05, 2020 at 08:05:22AM -0800, Christoph Hellwig wrote:
> FYI, I'd prefer to just see the series without patches 6, 7 and 9 for
> now.  They aren't really related to the rest, and I think this series:
> 
>     http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-kill-XLOG_STATE_IOERROR

URL not found.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 00/11] xfs: clean up log tickets and record writes

[Christoph Hellwig]

On Fri, Mar 06, 2020 at 08:42:24AM +1100, Dave Chinner wrote:
> On Thu, Mar 05, 2020 at 08:05:22AM -0800, Christoph Hellwig wrote:
> > FYI, I'd prefer to just see the series without patches 6, 7 and 9 for
> > now.  They aren't really related to the rest, and I think this series:
> > 
> >     http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-kill-XLOG_STATE_IOERROR
> 
> URL not found.

Weird, works for me.