Re: [PATCH V3 00/23] Metadump v2

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Nov 01, 2023 at 03:58:57PM +0100, Carlos Maiolino wrote:
> On Tue, Oct 31, 2023 at 09:46:40AM -0700, Darrick J. Wong wrote:
> > On Mon, Jul 24, 2023 at 10:05:04AM +0530, Chandan Babu R wrote:
> > > Hi all,
> > >
> > > This patch series extends metadump/mdrestore tools to be able to dump
> > > and restore contents of an external log device. It also adds the
> > > ability to copy larger blocks (e.g. 4096 bytes instead of 512 bytes)
> > > into the metadump file. These objectives are accomplished by
> > > introducing a new metadump file format.
> > 
> > Gentle maintainer ping: Carlos, do you have any thoughts about this
> > series?  The only unaddressed review comment was <cough> me asking for
> > code golf around patch 18 or so.  Do you (or anyone else) see any
> > problems that I've not spotted?
> 
> Hi, it's been a while, but I looked into this series before, and I was mostly ok
> with it, I was actually waiting for a follow-up patch addressing your comments
> on patch 19, regarding mdrestore ops definition. My emails have been a bit funky
> recently, did I miss a new version of this series?

AFAIK there hasn't been a new revision; this was a "hey it's been a few
months, are we all on the same page still?" inquiry. :)

--D

> Carlos
> 
> > 
> > --D
> > 
> > >
> > > I have tested the patchset by extending metadump/mdrestore tests in
> > > fstests to cover the newly introduced metadump v2 format. The tests
> > > can be found at
> > > https://github.com/chandanr/xfstests/commits/metadump-v2.
> > >
> > > The patch series can also be obtained from
> > > https://github.com/chandanr/xfsprogs-dev/commits/metadump-v2.
> > >
> > > Darrick, Please note that I have removed your RVB from "metadump: Add
> > > support for passing version option" patch. copy_log() and metadump_f()
> > > were invoking set_log_cur() for both "internal log" and "external
> > > log". In the V3 patchset, I have modified the copy_log() function to,
> > > 1. Invoke set_log_cur() when the filesystem has an external log.
> > > 2. Invoke set_cur() when the filesystem has an internal log.
> > >
> > > Changelog:
> > > V2 -> V3:
> > >   1. Document the meanings of metadump v2's ondisk flags.
> > >   2. Rename metadump_ops->end_write() to metadump_ops->finish_dump().
> > >   3. Pass a pointer to the newly introduced "union mdrestore_headers"
> > >      to callbacks in "struct mdrestore_ops" instead of a pointer to
> > >      "void".
> > >   4. Use set_log_cur() only when metadump has to be read from an
> > >      external log device.
> > >   5. Verify that primary superblock read from metadump file was indeed
> > >      read from the data device.
> > >   6. Fix indentation issues.
> > >
> > > V1 -> V2:
> > >   1. Introduce the new incompat flag XFS_MD2_INCOMPAT_EXTERNALLOG to
> > >      indicate that the metadump file contains data obtained from an
> > >      external log.
> > >   2. Interpret bits 54 and 55 of xfs_meta_extent.xme_addr as a counter
> > >      such that 00 maps to the data device and 01 maps to the log
> > >      device.
> > >   3. Define the new function set_log_cur() to read from
> > >      internal/external log device. This allows us to continue using
> > >      TYP_LOG to read from both internal and external log.
> > >   4. In order to support reading metadump from a pipe, mdrestore now
> > >      reads the first four bytes of the header to determine the
> > >      metadump version rather than reading the entire header in a
> > >      single call to fread().
> > >   5. Add an ASCII diagram to describe metadump v2's ondisk layout in
> > >      xfs_metadump.h.
> > >   6. Update metadump's man page to indicate that metadump in v2 format
> > >      is generated by default if the filesystem has an external log and
> > >      the metadump version to use is not explicitly mentioned on the
> > >      command line.
> > >   7. Remove '_metadump' suffix from function pointer names in "struct
> > >      metadump_ops".
> > >   8. Use xfs_daddr_t type for declaring variables containing disk
> > >      offset value.
> > >   9. Use bool type rather than int for variables holding a boolean
> > >      value.
> > >   11. Remove unnecessary whitespace.
> > >
> > >
> > > Chandan Babu R (23):
> > >   metadump: Use boolean values true/false instead of 1/0
> > >   mdrestore: Fix logic used to check if target device is large enough
> > >   metadump: Declare boolean variables with bool type
> > >   metadump: Define and use struct metadump
> > >   metadump: Add initialization and release functions
> > >   metadump: Postpone invocation of init_metadump()
> > >   metadump: Introduce struct metadump_ops
> > >   metadump: Introduce metadump v1 operations
> > >   metadump: Rename XFS_MD_MAGIC to XFS_MD_MAGIC_V1
> > >   metadump: Define metadump v2 ondisk format structures and macros
> > >   metadump: Define metadump ops for v2 format
> > >   xfs_db: Add support to read from external log device
> > >   metadump: Add support for passing version option
> > >   mdrestore: Declare boolean variables with bool type
> > >   mdrestore: Define and use struct mdrestore
> > >   mdrestore: Detect metadump v1 magic before reading the header
> > >   mdrestore: Add open_device(), read_header() and show_info() functions
> > >   mdrestore: Introduce struct mdrestore_ops
> > >   mdrestore: Replace metadump header pointer argument with a union
> > >     pointer
> > >   mdrestore: Introduce mdrestore v1 operations
> > >   mdrestore: Extract target device size verification into a function
> > >   mdrestore: Define mdrestore ops for v2 format
> > >   mdrestore: Add support for passing log device as an argument
> > >
> > >  db/io.c                   |  56 ++-
> > >  db/io.h                   |   2 +
> > >  db/metadump.c             | 777 ++++++++++++++++++++++++--------------
> > >  db/xfs_metadump.sh        |   3 +-
> > >  include/xfs_metadump.h    |  70 +++-
> > >  man/man8/xfs_mdrestore.8  |   8 +
> > >  man/man8/xfs_metadump.8   |  14 +
> > >  mdrestore/xfs_mdrestore.c | 497 ++++++++++++++++++------
> > >  8 files changed, 1014 insertions(+), 413 deletions(-)
> > >
> > > --
> > > 2.39.1
> > >