From: "Darrick J. Wong" <djwong@kernel.org>
To: Chandan Babu R <chandan.babu@oracle.com>
Cc: cem@kernel.org, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 22/24] mdrestore: Define mdrestore ops for v2 format
Date: Fri, 2 Jun 2023 08:01:06 -0700 [thread overview]
Message-ID: <20230602150106.GN16865@frogsfrogsfrogs> (raw)
In-Reply-To: <87ilcfiib8.fsf@debian-BULLSEYE-live-builder-AMD64>
On Thu, May 25, 2023 at 05:40:24PM +0530, Chandan Babu R wrote:
> On Tue, May 23, 2023 at 11:06:02 AM -0700, Darrick J. Wong wrote:
> > On Tue, May 23, 2023 at 02:30:48PM +0530, Chandan Babu R wrote:
> >> This commit adds functionality to restore metadump stored in v2 format.
> >>
> >> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> >> ---
> >> mdrestore/xfs_mdrestore.c | 209 +++++++++++++++++++++++++++++++++++---
> >> 1 file changed, 194 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> >> index 615ecdc77..9e06d37dc 100644
> >> --- a/mdrestore/xfs_mdrestore.c
> >> +++ b/mdrestore/xfs_mdrestore.c
> >> @@ -11,7 +11,8 @@ struct mdrestore_ops {
> >> int (*read_header)(void *header, FILE *mdfp);
> >> void (*show_info)(void *header, const char *mdfile);
> >> void (*restore)(void *header, FILE *mdfp, int data_fd,
> >> - bool is_target_file);
> >> + bool is_data_target_file, int log_fd,
> >> + bool is_log_target_file);
> >> };
> >>
> >> static struct mdrestore {
> >> @@ -148,7 +149,9 @@ restore_v1(
> >> void *header,
> >> FILE *mdfp,
> >> int data_fd,
> >> - bool is_target_file)
> >> + bool is_data_target_file,
> >> + int log_fd,
> >> + bool is_log_target_file)
> >> {
> >> struct xfs_metablock *mbp = header;
> >> struct xfs_metablock *metablock;
> >> @@ -203,7 +206,7 @@ restore_v1(
> >>
> >> ((struct xfs_dsb*)block_buffer)->sb_inprogress = 1;
> >>
> >> - verify_device_size(data_fd, is_target_file, sb.sb_dblocks,
> >> + verify_device_size(data_fd, is_data_target_file, sb.sb_dblocks,
> >> sb.sb_blocksize);
> >>
> >> bytes_read = 0;
> >> @@ -264,6 +267,163 @@ static struct mdrestore_ops mdrestore_ops_v1 = {
> >> .restore = restore_v1,
> >> };
> >>
> >> +static int
> >> +read_header_v2(
> >> + void *header,
> >> + FILE *mdfp)
> >> +{
> >> + struct xfs_metadump_header *xmh = header;
> >> +
> >> + rewind(mdfp);
> >
> > Does rewind() work if @mdfp is a pipe?
> >
> > I suspect the best you can do is read the first 4 bytes in main, pick
> > the read_header function from that, and have the read_header_v[12] read
> > in the rest of the header from there. I use a lot of:
> >
> > xfs_metadump -ago /dev/sda - | gzip > foo.md.gz
> > gzip -d < foo.md.gz | xfs_mdrestore -g - /dev/sdb
> >
> > to store compressed metadumps for future reference.
> >
> > (Well ok I use xz or zstd, but you get the point.)
> >
>
> Thanks for pointing out the bug and suggesting the fix.
>
> >> +
> >> + if (fread(xmh, sizeof(*xmh), 1, mdfp) != 1)
> >> + fatal("error reading from metadump file\n");
> >> + if (xmh->xmh_magic != cpu_to_be32(XFS_MD_MAGIC_V2))
> >> + return -1;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void
> >> +show_info_v2(
> >> + void *header,
> >> + const char *mdfile)
> >> +{
> >> + struct xfs_metadump_header *xmh;
> >> + uint32_t incompat_flags;
> >> +
> >> + xmh = header;
> >> + incompat_flags = be32_to_cpu(xmh->xmh_incompat_flags);
> >> +
> >> + printf("%s: %sobfuscated, %s log, %s metadata blocks\n",
> >> + mdfile,
> >> + incompat_flags & XFS_MD2_INCOMPAT_OBFUSCATED ? "":"not ",
> >> + incompat_flags & XFS_MD2_INCOMPAT_DIRTYLOG ? "dirty":"clean",
> >> + incompat_flags & XFS_MD2_INCOMPAT_FULLBLOCKS ? "full":"zeroed");
> >> +}
> >> +
> >> +static void
> >> +restore_v2(
> >> + void *header,
> >> + FILE *mdfp,
> >> + int data_fd,
> >> + bool is_data_target_file,
> >> + int log_fd,
> >> + bool is_log_target_file)
> >> +{
> >> + struct xfs_sb sb;
> >> + struct xfs_meta_extent xme;
> >> + char *block_buffer;
> >> + int64_t bytes_read;
> >> + uint64_t offset;
> >> + int prev_len;
> >> + int len;
> >> +
> >> + if (fread(&xme, sizeof(xme), 1, mdfp) != 1)
> >> + fatal("error reading from metadump file\n");
> >> +
> >> + len = be32_to_cpu(xme.xme_len);
> >> + len <<= BBSHIFT;
> >
> > Do we need to validate xme_addr==0 and xme_len==1 here?
> >
>
> Yes, I will add the check.
>
> >> +
> >> + block_buffer = calloc(1, len);
> >> + if (block_buffer == NULL)
> >> + fatal("memory allocation failure\n");
> >> +
> >> + if (fread(block_buffer, len, 1, mdfp) != 1)
> >> + fatal("error reading from metadump file\n");
> >> +
> >> + libxfs_sb_from_disk(&sb, (struct xfs_dsb *)block_buffer);
> >> +
> >> + if (sb.sb_magicnum != XFS_SB_MAGIC)
> >> + fatal("bad magic number for primary superblock\n");
> >> +
> >> + if (sb.sb_logstart == 0 && log_fd == -1)
> >> + fatal("External Log device is required\n");
> >> +
> >> + ((struct xfs_dsb *)block_buffer)->sb_inprogress = 1;
> >> +
> >> + verify_device_size(data_fd, is_data_target_file, sb.sb_dblocks,
> >> + sb.sb_blocksize);
> >> +
> >> + if (sb.sb_logstart == 0)
> >> + verify_device_size(log_fd, is_log_target_file, sb.sb_logblocks,
> >> + sb.sb_blocksize);
> >> +
> >> + bytes_read = 0;
> >> +
> >> + do {
> >> + int fd;
> >> +
> >> + if (mdrestore.show_progress &&
> >> + (bytes_read & ((1 << 20) - 1)) == 0)
> >> + print_progress("%lld MB read", bytes_read >> 20);
> >
> > Doesn't this miss a progress report if a metadata extent bumps
> > bytes_read across a MB boundary without actually landing on it? Say
> > you've written 1020K, and the next xfs_meta_extent is 8k long.
> >
> > if (metadump.show_progress) {
> > static int64_t mb_read;
> > int64_t mb_now = bytes_read >> 20;
> >
> > if (mb_now != mb_read) {
> > print_progress("%lld MB read", mb_now);
> > mb_read = mb_now;
> > }
> > }
> >
>
> I will include the above suggestion in the patchset.
>
> >> +
> >> + offset = be64_to_cpu(xme.xme_addr) & XME_ADDR_DEVICE_MASK;
> >> + offset <<= BBSHIFT;
> >
> > offset = BBTOB(be64_to_cpu() ... ); ?
>
> Yes, this is much better than what I had written.
>
> >
> > Also, I'd have thought that XME_ADDR_DEVICE_MASK is what you use to
> > decode the device, not what you use to decode the address within a
> > device.
> >
> >> +
> >> + if (be64_to_cpu(xme.xme_addr) & XME_ADDR_DATA_DEVICE)
> >> + fd = data_fd;
> >> + else if (be64_to_cpu(xme.xme_addr) & XME_ADDR_LOG_DEVICE)
> >> + fd = log_fd;
> >> + else
> >> + ASSERT(0);
> >
> > If you instead defined the constants like this:
> >
> > #define XME_ADDR_DEVICE_SHIFT 54
> > #define XME_ADDR_DEVICE_MASK ((1ULL << XME_ADDR_DEVICE_SHIFT) - 1)
>
> You probably meant to define XME_ADDR_DADDR_MASK.
Yep. Sorry about that.
> > #define XME_ADDR_DATA_DEVICE (0 << XME_ADDR_DEVICE_SHIFT)
> > #define XME_ADDR_LOG_DEVICE (1 << XME_ADDR_DEVICE_SHIFT)
> > #define XME_ADDR_RT_DEVICE (2 << XME_ADDR_DEVICE_SHIFT)
> >
> > #define XME_ADDR_DEVICE_MASK (3 << XME_ADDR_DEVICE_SHIFT)
> >
> > Then the above becomes:
> >
> > offset = BBTOB(be64_to_cpu(xme.xme_addr) & XME_ADDR_DADDR_MASK);
> >
> > switch (be64_to_cpu(xme.xme_addr) & XME_ADDR_DEVICE_MASK) {
> > case XME_ADDR_DATA_DEVICE:
> > fd = data_fd;
> > break;
> > ...
> > }
>
> Yes, this looks much better. Thanks for your suggestion.
>
> >> +
> >> + if (pwrite(fd, block_buffer, len, offset) < 0)
> >> + fatal("error writing to %s device at offset %llu: %s\n",
> >> + fd == data_fd ? "data": "log", offset,
> >> + strerror(errno));
> >> +
> >> + if (fread(&xme, sizeof(xme), 1, mdfp) != 1) {
> >> + if (feof(mdfp))
> >> + break;
> >> + fatal("error reading from metadump file\n");
> >> + }
> >> +
> >> + prev_len = len;
> >> + len = be32_to_cpu(xme.xme_len);
> >> + len <<= BBSHIFT;
> >> + if (len > prev_len) {
> >> + void *p;
> >> + p = realloc(block_buffer, len);
> >
> > Would it be preferable to declare an 8MB buffer and only copy contents
> > in that granularity? Technically speaking, xme_len == -1U would require
> > us to allocate a 2TB buffer, wouldn't it?
> >
>
> Yes, I agree. I will make the required changes.
I don't know if 8MB is a reasonable buffer size; I picked it
arbitrarily.
> >> + if (p == NULL) {
> >> + free(block_buffer);
> >> + fatal("memory allocation failure\n");
> >> + }
> >> + block_buffer = p;
> >> + }
> >> +
> >> + if (fread(block_buffer, len, 1, mdfp) != 1)
> >> + fatal("error reading from metadump file\n");
> >> +
> >> + bytes_read += len;
> >> + } while (1);
> >> +
> >> + if (mdrestore.progress_since_warning)
> >> + putchar('\n');
> >> +
> >> + memset(block_buffer, 0, sb.sb_sectsize);
> >
> > Tabs not spaces.
> >
> >> + sb.sb_inprogress = 0;
> >> + libxfs_sb_to_disk((struct xfs_dsb *)block_buffer, &sb);
> >> + if (xfs_sb_version_hascrc(&sb)) {
> >> + xfs_update_cksum(block_buffer, sb.sb_sectsize,
> >> + offsetof(struct xfs_sb, sb_crc));
> >> + }
> >> +
> >> + if (pwrite(data_fd, block_buffer, sb.sb_sectsize, 0) < 0)
> >> + fatal("error writing primary superblock: %s\n",
> >> + strerror(errno));
> >> +
> >> + free(block_buffer);
> >> +
> >> + return;
> >> +}
> >> +
> >> +static struct mdrestore_ops mdrestore_ops_v2 = {
> >> + .read_header = read_header_v2,
> >> + .show_info = show_info_v2,
> >> + .restore = restore_v2,
> >> +};
> >> +
> >> static void
> >> usage(void)
> >> {
> >> @@ -276,11 +436,16 @@ main(
> >> int argc,
> >> char **argv)
> >> {
> >> - FILE *src_f;
> >> - int dst_fd;
> >> - int c;
> >> - bool is_target_file;
> >> - struct xfs_metablock mb;
> >> + struct xfs_metadump_header xmh;
> >> + struct xfs_metablock mb;
> >
> > Hmm...
> >
> >> + FILE *src_f;
> >> + char *logdev = NULL;
> >> + void *header;
> >> + int data_dev_fd;
> >> + int log_dev_fd;
> >> + int c;
> >> + bool is_data_dev_file;
> >> + bool is_log_dev_file;
> >>
> >> mdrestore.show_progress = 0;
> >> mdrestore.show_info = 0;
> >> @@ -327,13 +492,18 @@ main(
> >> fatal("cannot open source dump file\n");
> >> }
> >>
> >> - if (mdrestore_ops_v1.read_header(&mb, src_f) == 0)
> >> + if (mdrestore_ops_v1.read_header(&mb, src_f) == 0) {
> >> mdrestore.mdrops = &mdrestore_ops_v1;
> >> - else
> >> + header = &mb;
> >> + } else if (mdrestore_ops_v2.read_header(&xmh, src_f) == 0) {
> >> + mdrestore.mdrops = &mdrestore_ops_v2;
> >> + header = &xmh;
> >
> > Perhaps define a union of both header formats, then pass that to
> > ->read_header, ->show_info, and ->restore?
>
> Sure. I will do that.
Ok.
--D
> --
> chandan
next prev parent reply other threads:[~2023-06-02 15:01 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
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 [this message]
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=20230602150106.GN16865@frogsfrogsfrogs \
--to=djwong@kernel.org \
--cc=cem@kernel.org \
--cc=chandan.babu@oracle.com \
--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