Re: [PATCH 23/24] mdrestore: Add support for passing log device as an argument

[Chandan Babu R <chandan.babu@oracle.com>]

On Fri, Jun 02, 2023 at 08:02:26 AM -0700, Darrick J. Wong wrote:
> On Thu, May 25, 2023 at 07:13:03PM +0530, Chandan Babu R wrote:
>> On Tue, May 23, 2023 at 11:09:59 AM -0700, Darrick J. Wong wrote:
>> > On Tue, May 23, 2023 at 02:30:49PM +0530, Chandan Babu R wrote:
>> >> metadump v2 format allows dumping metadata from external log devices. This
>> >> commit allows passing the device file to which log data must be restored from
>> >> the corresponding metadump file.
>> >> 
>> >> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> >> ---
>> >>  mdrestore/xfs_mdrestore.c | 10 ++++++++--
>> >>  1 file changed, 8 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
>> >> index 9e06d37dc..f5eff62ef 100644
>> >> --- a/mdrestore/xfs_mdrestore.c
>> >> +++ b/mdrestore/xfs_mdrestore.c
>> >> @@ -427,7 +427,8 @@ static struct mdrestore_ops mdrestore_ops_v2 = {
>> >>  static void
>> >>  usage(void)
>> >>  {
>> >> -	fprintf(stderr, "Usage: %s [-V] [-g] [-i] source target\n", progname);
>> >> +	fprintf(stderr, "Usage: %s [-V] [-g] [-i] [-l logdev] source target\n",
>> >> +		progname);
>> >>  	exit(1);
>> >>  }
>> >>  
>> >> @@ -453,7 +454,7 @@ main(
>> >>  
>> >>  	progname = basename(argv[0]);
>> >>  
>> >> -	while ((c = getopt(argc, argv, "giV")) != EOF) {
>> >> +	while ((c = getopt(argc, argv, "gil:V")) != EOF) {
>> >>  		switch (c) {
>> >>  			case 'g':
>> >>  				mdrestore.show_progress = 1;
>> >> @@ -461,6 +462,9 @@ main(
>> >>  			case 'i':
>> >>  				mdrestore.show_info = 1;
>> >>  				break;
>> >> +			case 'l':
>> >> +				logdev = optarg;
>> >> +				break;
>> >>  			case 'V':
>> >>  				printf("%s version %s\n", progname, VERSION);
>> >>  				exit(0);
>> >> @@ -493,6 +497,8 @@ main(
>> >>  	}
>> >>  
>> >>  	if (mdrestore_ops_v1.read_header(&mb, src_f) == 0) {
>> >> +		if (logdev != NULL)
>> >> +			usage();
>> >>  		mdrestore.mdrops = &mdrestore_ops_v1;
>> >>  		header = &mb;
>> >>  	} else if (mdrestore_ops_v2.read_header(&xmh, src_f) == 0) {
>> >
>> > What if we have a v2 with XME_ADDR_LOG_DEVICE meta_extents but the
>> > caller doesn't specify -l?  Do we proceed with the metadump, only to
>> > fail midway through the restore?
>> 
>> restore_v2() has the following statement just after reading in the superblock,
>> 
>> 	if (sb.sb_logstart == 0 && log_fd == -1)
>>                 fatal("External Log device is required\n");
>> 
>> Hence, In the case of a missing log device argument, the program exits before
>> any metadata is written to the target device.
>
> Ah, ok, that's how you handle that.  In that case the only reason for a
> flag in the v2 metadump header would be the principle of declaring
> things that happen later in the metadump stream/file.  Your call. :)

I think I will implement your suggestion regarding introducing a new header
flag to indicate that the metadump contains data which was copied from an
external log device.

This will make it easier for mdrestore to detect the requirement for the "-l
logdev" option much earlier in the restore process.

Thanks for the suggestion.

-- 
chandan