On Tue, May 23, 2023 at 09:48:07 AM -0700, Darrick J. Wong wrote:
> 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@xxxxxxxxxx>
>> ---
>> 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?
You are right about the requirement to add two entries for each possible
operation related to the log.
>
> 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);
The above suggestion looks correct to me. I will include this change unless
others have any objections to it.
--
chandan
