Re: [PATCH] xfs_logprint: handle the log split of inode item correctly

[Brian Foster <bfoster@redhat.com>]

On Wed, Jan 11, 2017 at 09:38:58PM +0800, Hou Tao wrote:
> It is possible that the data fork or the attribute fork
> of an inode will be splitted to the next log record, so
> we need to check the count of available operations
> in the log record and calculate the count of skipped
> operations properly.
> 

So what is the problem with the existing code? You need to describe the
problematic log state and the existing code flow in more detail (i.e.,
which op record covering the inode format is split across a log record?
what is the observed logprint behavior?) in the commit log description,
particularly since this is likely not a state easily tested/reproduced.

> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  logprint/log_misc.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/logprint/log_misc.c b/logprint/log_misc.c
> index a0f1766..20f0f89 100644
> --- a/logprint/log_misc.c
> +++ b/logprint/log_misc.c
> @@ -524,6 +524,7 @@ xlog_print_trans_inode(
>      xfs_inode_log_format_t *f;
>      int			   mode;
>      int			   size;
> +    int			   printed_ops;
>  
>      /*
>       * print inode type header region
> @@ -572,13 +573,6 @@ xlog_print_trans_inode(
>      xlog_print_trans_inode_core(&dino);
>      *ptr += xfs_log_dinode_size(dino.di_version);
>  

So it appears the inode item can have 2-4 op records: the format, core
inode and optionally the data and attr forks. Up to this point, we've
printed the format and core, which is two ops. If we hit the end of the
log record, we need to return the number of remaining ops in the format
so the subsequent record iteration can skip them and find the first new
transaction...

> -    if (*i == num_ops-1 && f->ilf_size == 3)  {
> -	return 1;
> -    }
> -

... which afaict, the above check does. E.g., If we've printed two ops
out of three and hit the record op count, return 1 op to skip.

So what are we trying to fix here? Is the problem that i isn't bumped
before we return, or that we're missing some information that should be
printed by the hunk that follows this check? Or am I missing something
else..?

> -    /* does anything come next */
> -    op_head = (xlog_op_header_t *)*ptr;
> -

So then op_head is set, may not necessarily be valid, but it isn't
actually used in this hunk so we can safely proceed without this
assignment (I'm starting to wonder if this code is intentionally obtuse
or just by accident :P).

>      switch (f->ilf_fields & (XFS_ILOG_DEV | XFS_ILOG_UUID)) {
>      case XFS_ILOG_DEV:
>  	printf(_("DEV inode: no extra region\n"));
> @@ -595,7 +589,13 @@ xlog_print_trans_inode(
>      ASSERT(f->ilf_size <= 4);
>      ASSERT((f->ilf_size == 3) || (f->ilf_fields & XFS_ILOG_AFORK));
>  
> +    /* does anything come next */
> +    printed_ops = 2;
> +    op_head = (xlog_op_header_t *)*ptr;
> +

Now we set op_head and add the checks below to see if we can increment
to another op header. If not, return the skip count.

So I think the logic here is Ok, but the existing code is confusing and
so it's not totally clear what you are trying to fix. Also, what I would
suggest is to do something like 'skip_count = f->ilf_size' once near the
top of the function, decrement it at each point as appropriate as we
step through the op headers, then update all of the return points to
just 'return skip_count;'. Thoughts?

>      if (f->ilf_fields & XFS_ILOG_DFORK) {
> +		if (*i == num_ops-1)
> +		return f->ilf_size-printed_ops;

I'm not really sure what we want to do here with regard to indentation
inconsistency with existing code. The existing code uses 4 spaces vs.
this patch using tabs. Perhaps that's a question for Eric..

Either way, the indentation of the 'if ()' itself is broken here...

>  	    (*i)++;
>  	    xlog_print_op_header(op_head, *i, ptr);
>  
> @@ -618,11 +618,14 @@ xlog_print_trans_inode(
>  
>  	    *ptr += be32_to_cpu(op_head->oh_len);
>  	    if (op_head->oh_flags & XLOG_CONTINUE_TRANS)
> -		return 1;
> +		return f->ilf_size-printed_ops;
>  	    op_head = (xlog_op_header_t *)*ptr;
> +		printed_ops++;
>      }
>  
>      if (f->ilf_fields & XFS_ILOG_AFORK) {
> +		if (*i == num_ops-1)
> +		return f->ilf_size-printed_ops;

... and the same thing here.

Brian

>  	    (*i)++;
>  	    xlog_print_op_header(op_head, *i, ptr);
>  
> @@ -644,7 +647,7 @@ xlog_print_trans_inode(
>  	    }
>  	    *ptr += be32_to_cpu(op_head->oh_len);
>  	    if (op_head->oh_flags & XLOG_CONTINUE_TRANS)
> -		return 1;
> +		return f->ilf_size-printed_ops;
>      }
>  
>      return 0;
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html