Re: [PATCH 05/24] set_cur: Add support to read from external log device

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, May 23, 2023 at 02:30:31PM +0530, Chandan Babu R wrote:
> This commit changes set_cur() to be able to read from external log
> devices. This is required by a future commit which will add the ability to
> dump metadata from external log devices.
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  db/io.c   | 22 +++++++++++++++-------
>  db/type.c |  2 ++
>  db/type.h |  2 +-
>  3 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/db/io.c b/db/io.c
> index 3d2572364..e8c8f57e2 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -516,12 +516,13 @@ set_cur(
>  	int		ring_flag,
>  	bbmap_t		*bbmap)
>  {
> -	struct xfs_buf	*bp;
> -	xfs_ino_t	dirino;
> -	xfs_ino_t	ino;
> -	uint16_t	mode;
> +	struct xfs_buftarg	*btargp;
> +	struct xfs_buf		*bp;
> +	xfs_ino_t		dirino;
> +	xfs_ino_t		ino;
> +	uint16_t		mode;
>  	const struct xfs_buf_ops *ops = type ? type->bops : NULL;
> -	int		error;
> +	int			error;
>  
>  	if (iocur_sp < 0) {
>  		dbprintf(_("set_cur no stack element to set\n"));
> @@ -534,7 +535,14 @@ set_cur(
>  	pop_cur();
>  	push_cur();
>  
> +	btargp = mp->m_ddev_targp;
> +	if (type->typnm == TYP_ELOG) {

This feels like a layering violation, see below...

> +		ASSERT(mp->m_ddev_targp != mp->m_logdev_targp);
> +		btargp = mp->m_logdev_targp;
> +	}
> +
>  	if (bbmap) {
> +		ASSERT(btargp == mp->m_ddev_targp);
>  #ifdef DEBUG_BBMAP
>  		int i;
>  		printf(_("xfs_db got a bbmap for %lld\n"), (long long)blknum);
> @@ -548,11 +556,11 @@ set_cur(
>  		if (!iocur_top->bbmap)
>  			return;
>  		memcpy(iocur_top->bbmap, bbmap, sizeof(struct bbmap));
> -		error = -libxfs_buf_read_map(mp->m_ddev_targp, bbmap->b,
> +		error = -libxfs_buf_read_map(btargp, bbmap->b,
>  				bbmap->nmaps, LIBXFS_READBUF_SALVAGE, &bp,
>  				ops);
>  	} else {
> -		error = -libxfs_buf_read(mp->m_ddev_targp, blknum, len,
> +		error = -libxfs_buf_read(btargp, blknum, len,
>  				LIBXFS_READBUF_SALVAGE, &bp, ops);
>  		iocur_top->bbmap = NULL;
>  	}
> diff --git a/db/type.c b/db/type.c
> index efe704456..cc406ae4c 100644
> --- a/db/type.c
> +++ b/db/type.c
> @@ -100,6 +100,7 @@ static const typ_t	__typtab_crc[] = {
>  	{ TYP_INODE, "inode", handle_struct, inode_crc_hfld,
>  		&xfs_inode_buf_ops, TYP_F_CRC_FUNC, xfs_inode_set_crc },
>  	{ TYP_LOG, "log", NULL, NULL, NULL, TYP_F_NO_CRC_OFF },
> +	{ TYP_ELOG, "elog", NULL, NULL, NULL, TYP_F_NO_CRC_OFF },

It strikes me as a little odd to create a new /metadata type/ to
reference the external log.  If we someday want to add a bunch of new
types to xfs_db to allow us to decode/fuzz the log contents, wouldn't we
have to add them twice -- once for decoding an internal log, and again
to decode the external log?  And the only difference between the two
would be the buftarg, right?  The set_cur caller needs to know the
daddr already, so I don't think it's unreasonable for the caller to have
to know which buftarg too.

IOWs, I think set_cur ought to take the buftarg, the typ_t, and a daddr
as explicit arguments.  But maybe others have opinions?

e.g. rename set_cur to __set_cur and make it take a buftarg, and then:

int
set_log_cur(
	const typ_t	*type,
	xfs_daddr_t	blknum,
	int		len,
	int		ring_flag,
	bbmap_t		*bbmap)
{
	if (!mp->m_logdev_targp->bt_bdev ||
	    mp->m_logdev_targp->bt_bdev == mp->m_ddev_targp->bt_bdev) {
		printf(_("external log device not loaded, use -l.\n"));
		return ENODEV;
	}

	__set_cur(mp->m_logdev_targp, type, blknum, len, ring_flag, bbmap);
	return 0;
}

and then metadump can do something like ....

	error = set_log_cur(&typtab[TYP_LOG], 0,
			mp->m_sb.sb_logblocks * blkbb, DB_RING_IGN, NULL);

--D

>  	{ TYP_RTBITMAP, "rtbitmap", handle_text, NULL, NULL, TYP_F_NO_CRC_OFF },
>  	{ TYP_RTSUMMARY, "rtsummary", handle_text, NULL, NULL, TYP_F_NO_CRC_OFF },
>  	{ TYP_SB, "sb", handle_struct, sb_hfld, &xfs_sb_buf_ops,
> @@ -144,6 +145,7 @@ static const typ_t	__typtab_spcrc[] = {
>  	{ TYP_INODE, "inode", handle_struct, inode_crc_hfld,
>  		&xfs_inode_buf_ops, TYP_F_CRC_FUNC, xfs_inode_set_crc },
>  	{ TYP_LOG, "log", NULL, NULL, NULL, TYP_F_NO_CRC_OFF },
> +	{ TYP_ELOG, "elog", NULL, NULL, NULL, TYP_F_NO_CRC_OFF },
>  	{ TYP_RTBITMAP, "rtbitmap", handle_text, NULL, NULL, TYP_F_NO_CRC_OFF },
>  	{ TYP_RTSUMMARY, "rtsummary", handle_text, NULL, NULL, TYP_F_NO_CRC_OFF },
>  	{ TYP_SB, "sb", handle_struct, sb_hfld, &xfs_sb_buf_ops,
> diff --git a/db/type.h b/db/type.h
> index 411bfe90d..feb5c8219 100644
> --- a/db/type.h
> +++ b/db/type.h
> @@ -14,7 +14,7 @@ typedef enum typnm
>  	TYP_AGF, TYP_AGFL, TYP_AGI, TYP_ATTR, TYP_BMAPBTA,
>  	TYP_BMAPBTD, TYP_BNOBT, TYP_CNTBT, TYP_RMAPBT, TYP_REFCBT, TYP_DATA,
>  	TYP_DIR2, TYP_DQBLK, TYP_INOBT, TYP_INODATA, TYP_INODE,
> -	TYP_LOG, TYP_RTBITMAP, TYP_RTSUMMARY, TYP_SB, TYP_SYMLINK,
> +	TYP_LOG, TYP_ELOG, TYP_RTBITMAP, TYP_RTSUMMARY, TYP_SB, TYP_SYMLINK,
>  	TYP_TEXT, TYP_FINOBT, TYP_NONE
>  } typnm_t;
>  
> -- 
> 2.39.1
>