From: Chandan Babu R <chandan.babu@oracle.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: cem@kernel.org, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 05/24] set_cur: Add support to read from external log device
Date: Tue, 06 Jun 2023 10:17:47 +0530 [thread overview]
Message-ID: <871qiphx5j.fsf@debian-BULLSEYE-live-builder-AMD64> (raw)
In-Reply-To: <20230605192235.GA1325469@frogsfrogsfrogs>
On Mon, Jun 05, 2023 at 12:22:35 PM -0700, Darrick J. Wong wrote:
> On Mon, Jun 05, 2023 at 02:49:49PM +0530, Chandan Babu R wrote:
>> 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@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);
>> >
>>
>> Darrick, How about implementing the following instead,
>>
>> static void
>> __set_cur(
>> struct xfs_buftarg *btargp,
>> const typ_t *type,
>> xfs_daddr_t blknum,
>> int len,
>> int ring_flag,
>> bbmap_t *bbmap)
>> {
>> 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;
>>
>> if (iocur_sp < 0) {
>> dbprintf(_("set_cur no stack element to set\n"));
>> return;
>> }
>>
>> ino = iocur_top->ino;
>> dirino = iocur_top->dirino;
>> mode = iocur_top->mode;
>> pop_cur();
>> push_cur();
>>
>> if (bbmap) {
>> #ifdef DEBUG_BBMAP
>> int i;
>> printf(_("xfs_db got a bbmap for %lld\n"), (long long)blknum);
>> printf(_("\tblock map"));
>> for (i = 0; i < bbmap->nmaps; i++)
>> printf(" %lld:%d", (long long)bbmap->b[i].bm_bn,
>> bbmap->b[i].bm_len);
>> printf("\n");
>> #endif
>> iocur_top->bbmap = malloc(sizeof(struct bbmap));
>> if (!iocur_top->bbmap)
>> return;
>> memcpy(iocur_top->bbmap, bbmap, sizeof(struct bbmap));
>> error = -libxfs_buf_read_map(btargp, bbmap->b,
>> bbmap->nmaps, LIBXFS_READBUF_SALVAGE, &bp,
>> ops);
>> } else {
>> error = -libxfs_buf_read(btargp, blknum, len,
>> LIBXFS_READBUF_SALVAGE, &bp, ops);
>> iocur_top->bbmap = NULL;
>> }
>>
>> /*
>> * Salvage mode means that we still get a buffer even if the verifier
>> * says the metadata is corrupt. Therefore, the only errors we should
>> * get are for IO errors or runtime errors.
>> */
>> if (error)
>> return;
>> iocur_top->buf = bp->b_addr;
>> iocur_top->bp = bp;
>> if (!ops) {
>> bp->b_ops = NULL;
>> bp->b_flags |= LIBXFS_B_UNCHECKED;
>> }
>>
>> iocur_top->bb = blknum;
>> iocur_top->blen = len;
>> iocur_top->boff = 0;
>> iocur_top->data = iocur_top->buf;
>> iocur_top->len = BBTOB(len);
>> iocur_top->off = blknum << BBSHIFT;
>> iocur_top->typ = cur_typ = type;
>> iocur_top->ino = ino;
>> iocur_top->dirino = dirino;
>> iocur_top->mode = mode;
>> iocur_top->ino_buf = 0;
>> iocur_top->dquot_buf = 0;
>>
>> /* store location in ring */
>> if (ring_flag)
>> ring_add();
>> }
>>
>> void
>> set_cur(
>> const typ_t *type,
>> xfs_daddr_t blknum,
>> int len,
>> int ring_flag,
>> bbmap_t *bbmap)
>> {
>> struct xfs_buftarg *btargp = mp->m_ddev_targp;
>>
>> if (type->typnm == TYP_LOG &&
>> mp->m_logdev_targp->bt_bdev != mp->m_ddev_targp->bt_bdev) {
>> ASSERT(mp->m_sb.sb_logstart == 0);
>> btargp = mp->m_logdev_targp;
>> }
>>
>> __set_cur(btargp, type, blknum, len, ring_flag, bbmap);
>> }
>>
>> i.e. We continue to have just one type for the log and set_cur() will
>> internally decide which buftarg to pass to __set_cur(). Please let me know
>> your opinion on this approach.
>
> If I'm understanding this correctly, you're proposing to push the
> buftarg decision down into set_cur instead of encoding it in the typ_t
> information?
>
> I still don't like this, because that decision should be made by the
> callers of set_*cur, not down in the io cursor handling code.
>
> Take a look at the users of set_log_cur and set_rt_cur in the 'dblock'
> command as of djwong-wtf:
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfsprogs-dev.git/tree/db/block.c?h=djwong-wtf_2023-06-05#n217
>
> Notice this bit here:
>
> static inline bool
> is_rtfile(
> struct xfs_dinode *dip)
> {
> return dip->di_flags & cpu_to_be16(XFS_DIFLAG_REALTIME);
> }
>
> static int
> dblock_f(...)
> {
> ...
>
> if (is_rtfile(iocur_top->data))
> set_rt_cur(&typtab[type], (int64_t)XFS_FSB_TO_DADDR(mp, dfsbno),
> nb * blkbb, DB_RING_ADD,
> nex > 1 ? &bbmap : NULL);
> else
> set_cur(&typtab[type], (int64_t)XFS_FSB_TO_DADDR(mp, dfsbno),
> nb * blkbb, DB_RING_ADD,
> nex > 1 ? &bbmap : NULL);
>
> xfs_db can now access the data blocks of realtime files, because we have
> the high level logic to decide which buftarg based on the di_flags set
> in the inode core. TYP_DATA doesn't know anything at all about inodes
> or data blocks or whatever -- down at the level of "data block" we don't
> actually have the context we need to select a device.
>
Ok. Now I understand. Thanks for the explaination.
--
chandan
next prev parent reply other threads:[~2023-06-06 7:23 UTC|newest]
Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-23 9:00 [PATCH 00/24] Metadump v2 Chandan Babu R
2023-05-23 9:00 ` [PATCH 01/24] metadump: Use boolean values true/false instead of 1/0 Chandan Babu R
2023-05-23 16:31 ` Darrick J. Wong
2023-05-23 9:00 ` [PATCH 02/24] mdrestore: Fix logic used to check if target device is large enough Chandan Babu R
2023-05-23 16:32 ` Darrick J. Wong
2023-05-23 9:00 ` [PATCH 03/24] metadump: Define and use struct metadump Chandan Babu R
2023-05-23 16:35 ` Darrick J. Wong
2023-05-24 4:50 ` Chandan Babu R
2023-05-23 9:00 ` [PATCH 04/24] metadump: Add initialization and release functions Chandan Babu R
2023-05-23 16:36 ` Darrick J. Wong
2023-05-24 5:03 ` Chandan Babu R
2023-05-23 9:00 ` [PATCH 05/24] set_cur: Add support to read from external log device Chandan Babu R
2023-05-23 16:48 ` Darrick J. Wong
2023-05-25 8:27 ` Chandan Babu R
2023-06-05 9:19 ` Chandan Babu R
2023-06-05 19:22 ` Darrick J. Wong
2023-06-06 4:47 ` Chandan Babu R [this message]
2023-05-23 9:00 ` [PATCH 06/24] metadump: Dump external log device contents Chandan Babu R
2023-05-23 17:02 ` Darrick J. Wong
2023-05-26 6:54 ` Chandan Babu R
2023-05-23 9:00 ` [PATCH 07/24] metadump: Postpone invocation of init_metadump() Chandan Babu R
2023-05-23 17:13 ` Darrick J. Wong
2023-05-25 8:45 ` Chandan Babu R
2023-05-23 9:00 ` [PATCH 08/24] metadump: Introduce struct metadump_ops Chandan Babu R
2023-05-23 17:15 ` Darrick J. Wong
2023-05-25 8:48 ` Chandan Babu R
2023-05-23 9:00 ` [PATCH 09/24] metadump: Introduce metadump v1 operations Chandan Babu R
2023-05-23 17:25 ` Darrick J. Wong
2023-05-25 14:19 ` Chandan Babu R
2023-06-02 14:34 ` Darrick J. Wong
2023-05-23 9:00 ` [PATCH 10/24] metadump: Rename XFS_MD_MAGIC to XFS_MD_MAGIC_V1 Chandan Babu R
2023-05-23 17:27 ` Darrick J. Wong
2023-05-23 9:00 ` [PATCH 11/24] metadump: Define metadump v2 ondisk format structures and macros Chandan Babu R
2023-05-23 17:34 ` Darrick J. Wong
2023-05-25 9:26 ` Chandan Babu R
2023-06-02 14:46 ` Darrick J. Wong
2023-05-23 9:00 ` [PATCH 12/24] metadump: Define metadump ops for v2 format Chandan Babu R
2023-05-23 17:37 ` Darrick J. Wong
2023-05-23 9:00 ` [PATCH 13/24] metadump: Add support for passing version option Chandan Babu R
2023-05-23 17:41 ` Darrick J. Wong
2023-05-23 9:00 ` [PATCH 14/24] xfs_metadump.sh: " Chandan Babu R
2023-05-23 17:39 ` Darrick J. Wong
2023-05-25 9:31 ` Chandan Babu R
2023-05-23 9:00 ` [PATCH 15/24] xfs_metadump.8: Add description for the newly introduced -v option Chandan Babu R
2023-05-23 17:40 ` Darrick J. Wong
2023-05-25 10:04 ` Chandan Babu R
2023-06-02 14:58 ` Darrick J. Wong
2023-05-23 9:00 ` [PATCH 16/24] mdrestore: Define and use struct mdrestore Chandan Babu R
2023-05-23 17:42 ` Darrick J. Wong
2023-05-26 8:38 ` Chandan Babu R
2023-05-23 9:00 ` [PATCH 17/24] mdrestore: Add open_device(), read_header() and show_info() functions Chandan Babu R
2023-05-23 17:44 ` Darrick J. Wong
2023-05-25 10:11 ` Chandan Babu R
2023-05-23 9:00 ` [PATCH 18/24] mdrestore: Introduce struct mdrestore_ops Chandan Babu R
2023-05-23 17:44 ` Darrick J. Wong
2023-05-25 10:34 ` Chandan Babu R
2023-05-23 9:00 ` [PATCH 19/24] mdrestore: Introduce mdrestore v1 operations Chandan Babu R
2023-05-23 17:48 ` Darrick J. Wong
2023-05-25 10:39 ` Chandan Babu R
2023-05-23 9:00 ` [PATCH 20/24] mdrestore: Detect metadump version from metadump image Chandan Babu R
2023-05-23 18:11 ` Darrick J. Wong
2023-05-23 9:00 ` [PATCH 21/24] mdrestore: Extract target device size verification into a function Chandan Babu R
2023-05-23 18:07 ` Darrick J. Wong
2023-05-25 12:02 ` Chandan Babu R
2023-05-23 9:00 ` [PATCH 22/24] mdrestore: Define mdrestore ops for v2 format Chandan Babu R
2023-05-23 18:06 ` Darrick J. Wong
2023-05-25 12:10 ` Chandan Babu R
2023-06-02 15:01 ` Darrick J. Wong
2023-05-23 9:00 ` [PATCH 23/24] mdrestore: Add support for passing log device as an argument Chandan Babu R
2023-05-23 18:09 ` Darrick J. Wong
2023-05-25 13:43 ` Chandan Babu R
2023-06-02 15:02 ` Darrick J. Wong
2023-06-05 6:19 ` Chandan Babu R
2023-05-23 9:00 ` [PATCH 24/24] xfs_mdrestore.8: Add description for the newly introduced -l option Chandan Babu R
2023-05-23 18:10 ` Darrick J. Wong
2023-05-25 13:45 ` Chandan Babu R
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=871qiphx5j.fsf@debian-BULLSEYE-live-builder-AMD64 \
--to=chandan.babu@oracle.com \
--cc=cem@kernel.org \
--cc=djwong@kernel.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox