On Thu, Jul 13, 2023 at 11:57:27AM +0530, Chandan Babu R wrote:
> On Wed, Jul 12, 2023 at 11:10:03 AM -0700, Darrick J. Wong wrote:
> > On Tue, Jun 06, 2023 at 02:58:05PM +0530, Chandan Babu R wrote:
> >> This commit adds functionality to restore metadump stored in v2 format.
> >>
> >> Signed-off-by: Chandan Babu R <chandan.babu@xxxxxxxxxx>
> >> ---
> >> mdrestore/xfs_mdrestore.c | 251 +++++++++++++++++++++++++++++++++++---
> >> 1 file changed, 233 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> >> index c395ae90..7b484071 100644
> >> --- a/mdrestore/xfs_mdrestore.c
> >> +++ b/mdrestore/xfs_mdrestore.c
> >> @@ -12,7 +12,8 @@ struct mdrestore_ops {
> >> void (*read_header)(void *header, FILE *md_fp);
> >> void (*show_info)(void *header, const char *md_file);
> >> void (*restore)(void *header, FILE *md_fp, int ddev_fd,
> >> - bool is_target_file);
> >> + bool is_data_target_file, int logdev_fd,
> >> + bool is_log_target_file);
> >> };
> >>
> >> static struct mdrestore {
> >> @@ -20,6 +21,7 @@ static struct mdrestore {
> >> bool show_progress;
> >> bool show_info;
> >> bool progress_since_warning;
> >> + bool external_log;
> >> } mdrestore;
> >>
> >> static void
> >> @@ -143,10 +145,12 @@ show_info_v1(
> >>
> >> static void
> >> restore_v1(
> >> - void *header,
> >> - FILE *md_fp,
> >> - int ddev_fd,
> >> - bool is_target_file)
> >> + void *header,
> >> + FILE *md_fp,
> >> + int ddev_fd,
> >> + bool is_data_target_file,
> >
> > Why does the indent level change here...
> >
> >> + int logdev_fd,
> >> + bool is_log_target_file)
> >> {
> >> struct xfs_metablock *metablock;
> >> struct xfs_metablock *mbp;
> >
> > ...but not here?
> >
>
> Sorry, I will fix this.
>
> >> @@ -203,7 +207,7 @@ restore_v1(
> >>
> >> ((struct xfs_dsb*)block_buffer)->sb_inprogress = 1;
> >>
> >> - verify_device_size(ddev_fd, is_target_file, sb.sb_dblocks,
> >> + verify_device_size(ddev_fd, is_data_target_file, sb.sb_dblocks,
> >> sb.sb_blocksize);
> >>
> >> bytes_read = 0;
> >> @@ -264,6 +268,195 @@ static struct mdrestore_ops mdrestore_ops_v1 = {
> >> .restore = restore_v1,
> >> };
> >>
> >> +static void
> >> +read_header_v2(
> >> + void *header,
> >> + FILE *md_fp)
> >> +{
> >> + struct xfs_metadump_header *xmh = header;
> >> + bool want_external_log;
> >> +
> >> + xmh->xmh_magic = cpu_to_be32(XFS_MD_MAGIC_V2);
> >> +
> >> + if (fread((uint8_t *)xmh + sizeof(xmh->xmh_magic),
> >> + sizeof(*xmh) - sizeof(xmh->xmh_magic), 1, md_fp) != 1)
> >> + fatal("error reading from metadump file\n");
> >> +
> >> + want_external_log = !!(be32_to_cpu(xmh->xmh_incompat_flags) &
> >> + XFS_MD2_INCOMPAT_EXTERNALLOG);
> >> +
> >> + if (want_external_log && !mdrestore.external_log)
> >> + fatal("External Log device is required\n");
> >> +}
> >> +
> >> +static void
> >> +show_info_v2(
> >> + void *header,
> >> + const char *md_file)
> >> +{
> >> + 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, external log contents are %sdumped, %s metadata blocks,\n",
> >> + md_file,
> >> + incompat_flags & XFS_MD2_INCOMPAT_OBFUSCATED ? "":"not ",
> >> + incompat_flags & XFS_MD2_INCOMPAT_DIRTYLOG ? "dirty":"clean",
> >> + incompat_flags & XFS_MD2_INCOMPAT_EXTERNALLOG ? "":"not ",
> >> + incompat_flags & XFS_MD2_INCOMPAT_FULLBLOCKS ? "full":"zeroed");
> >> +}
> >> +
> >> +#define MDR_IO_BUF_SIZE (8 * 1024 * 1024)
> >> +
> >> +static void
> >> +dump_meta_extent(
> >
> > Aren't we restoring here? And not dumping?
> >
>
> You are right. I will rename the function to restore_meta_extent().
>
> >> + FILE *md_fp,
> >> + int dev_fd,
> >> + char *device,
> >> + void *buf,
> >> + uint64_t offset,
> >> + int len)
> >> +{
> >> + int io_size;
> >> +
> >> + io_size = min(len, MDR_IO_BUF_SIZE);
> >> +
> >> + do {
> >> + if (fread(buf, io_size, 1, md_fp) != 1)
> >> + fatal("error reading from metadump file\n");
> >> + if (pwrite(dev_fd, buf, io_size, offset) < 0)
> >> + fatal("error writing to %s device at offset %llu: %s\n",
> >> + device, offset, strerror(errno));
> >> + len -= io_size;
> >> + offset += io_size;
> >> +
> >> + io_size = min(len, io_size);
> >> + } while (len);
> >> +}
> >> +
> >> +static void
> >> +restore_v2(
> >> + void *header,
> >> + FILE *md_fp,
> >> + int ddev_fd,
> >> + bool is_data_target_file,
> >> + int logdev_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 len;
> >> +
> >> + block_buffer = malloc(MDR_IO_BUF_SIZE);
> >> + if (block_buffer == NULL)
> >> + fatal("Unable to allocate input buffer memory\n");
> >> +
> >> + if (fread(&xme, sizeof(xme), 1, md_fp) != 1)
> >> + fatal("error reading from metadump file\n");
> >> +
> >> + if (xme.xme_addr != 0 || xme.xme_len == 1)
> >> + fatal("Invalid superblock disk address/length\n");
> >
> > Shouldn't we check that xme_addr points to XME_ADDR_DATA_DEVICE?
> >
>
> Yes, you are right. I will add the check.
>
> >> + len = BBTOB(be32_to_cpu(xme.xme_len));
> >> +
> >> + if (fread(block_buffer, len, 1, md_fp) != 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");
> >> +
> >> + ((struct xfs_dsb *)block_buffer)->sb_inprogress = 1;
> >> +
> >> + verify_device_size(ddev_fd, is_data_target_file, sb.sb_dblocks,
> >> + sb.sb_blocksize);
> >> +
> >> + if (sb.sb_logstart == 0) {
> >> + ASSERT(mdrestore.external_log == true);
> >
> > This should be more graceful to users:
> >
> > if (!mdrestore.external_log)
> > fatal("Filesystem has external log but -l not specified.\n");
>
> mdrestore.external_log is set to true only when the user passes the -l
> option. Also, read_header_v2() would have already verified if the metadump
> file contains data captured from an external log device and that an external
> log device was indeed passed to the restore program. It should be impossible
> to have mdrestore.external_log set to false at this point in
> restore_v2(). Hence, I think a call to ASSERT() is more appropriate.
Ah, ok. I would've expected ASSERT(logstart != 0 || external_log); but
it's true that Dave more recently has been asking for conditional
assertions to be structured the way you originally wrote it. Withdrawn.
:)
--D
> --
> chandan
