Re: [PATCH V2 22/23] mdrestore: Define mdrestore ops for v2 format

From: "Darrick J. Wong" <djwong@kernel.org>
To: Chandan Babu R <chandan.babu@oracle.com>
Cc: linux-xfs@vger.kernel.org, cem@kernel.org
Subject: Re: [PATCH V2 22/23] mdrestore: Define mdrestore ops for v2 format
Date: Thu, 13 Jul 2023 07:22:52 -0700	[thread overview]
Message-ID: <20230713142252.GH11476@frogsfrogsfrogs> (raw)
In-Reply-To: <87wmz4i9y5.fsf@debian-BULLSEYE-live-builder-AMD64>

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@oracle.com>
> >> ---
> >>  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