[PATCH] xfs_mdrestore: don't allow restoring onto zoned block devices

[PATCH] xfs_mdrestore: don't allow restoring onto zoned block devices

[Christoph Hellwig]

The way mdrestore works is not very amendable to zone devices.  The code
that checks the device size tries to write to the highest offset, which
doesn't match the write pointer of a clean zone device.  And while that
is relatively easily fixable, the metadata for each RTG records the
highest written offset, and the mount code compares that to the hardware
write pointer, which will mismatch.  This could be fixed by using write
zeroes to pad the RTG until the expected write pointer, but this turns
the quick metadata operation that mdrestore is supposed to be into
something that could take hours on HDD.

So instead error out when someone tries to mdrestore onto a zoned device
to clearly document that this won't work.  Doing a mdrestore into a file
still works perfectly fine, and we might look into a new mdrestore option
to restore into a set of files suitable for the zoned loop device driver
to make mdrestore fully usable for debugging.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mdrestore/xfs_mdrestore.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
index 95b01a99a154..f10c4befb2fc 100644
--- a/mdrestore/xfs_mdrestore.c
+++ b/mdrestore/xfs_mdrestore.c
@@ -8,6 +8,7 @@
 #include "xfs_metadump.h"
 #include <libfrog/platform.h>
 #include "libfrog/div64.h"
+#include <linux/blkzoned.h>
 
 union mdrestore_headers {
 	__be32				magic;
@@ -148,6 +149,13 @@ open_device(
 	dev->fd = open(path, open_flags, 0644);
 	if (dev->fd < 0)
 		fatal("couldn't open \"%s\"\n", path);
+
+	if (!dev->is_file) {
+		uint32_t zone_size;
+
+		if (ioctl(dev->fd, BLKGETZONESZ, &zone_size) == 0 && zone_size)
+			fatal("can't restore to zoned device \"%s\"\n", path);
+	}
 }
 
 static void
-- 
2.47.2

Re: [PATCH] xfs_mdrestore: don't allow restoring onto zoned block devices

[Darrick J. Wong]

On Mon, May 12, 2025 at 03:17:37PM +0200, Christoph Hellwig wrote:
> The way mdrestore works is not very amendable to zone devices.  The code
> that checks the device size tries to write to the highest offset, which
> doesn't match the write pointer of a clean zone device.  And while that
> is relatively easily fixable, the metadata for each RTG records the
> highest written offset, and the mount code compares that to the hardware
> write pointer, which will mismatch.  This could be fixed by using write
> zeroes to pad the RTG until the expected write pointer, but this turns
> the quick metadata operation that mdrestore is supposed to be into
> something that could take hours on HDD.
> 
> So instead error out when someone tries to mdrestore onto a zoned device
> to clearly document that this won't work.  Doing a mdrestore into a file
> still works perfectly fine, and we might look into a new mdrestore option
> to restore into a set of files suitable for the zoned loop device driver
> to make mdrestore fully usable for debugging.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  mdrestore/xfs_mdrestore.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 95b01a99a154..f10c4befb2fc 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -8,6 +8,7 @@
>  #include "xfs_metadump.h"
>  #include <libfrog/platform.h>
>  #include "libfrog/div64.h"
> +#include <linux/blkzoned.h>

I wonder if there ought to be guards around blkzoned.h, but OTOH that
seems to have been introduced in 4.9 around 8 years ago so maybe it's
fine?

/me is willing to go along with that if the maintainer is.  Meanwhile
the code changes make sense so as long as there isn't some "set the
write pointer to an arbitrary LBA" command that I missed,

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

since we wouldn't be mdrestoring user file data to the zoned section,
right?

--D

>  
>  union mdrestore_headers {
>  	__be32				magic;
> @@ -148,6 +149,13 @@ open_device(
>  	dev->fd = open(path, open_flags, 0644);
>  	if (dev->fd < 0)
>  		fatal("couldn't open \"%s\"\n", path);
> +
> +	if (!dev->is_file) {
> +		uint32_t zone_size;
> +
> +		if (ioctl(dev->fd, BLKGETZONESZ, &zone_size) == 0 && zone_size)
> +			fatal("can't restore to zoned device \"%s\"\n", path);
> +	}
>  }
>  
>  static void
> -- 
> 2.47.2
> 
> 

Re: [PATCH] xfs_mdrestore: don't allow restoring onto zoned block devices

[Christoph Hellwig]

On Mon, May 12, 2025 at 08:12:40AM -0700, Darrick J. Wong wrote:
> I wonder if there ought to be guards around blkzoned.h, but OTOH that
> seems to have been introduced in 4.9 around 8 years ago so maybe it's
> fine?

I think we're fine.  If not we'll also need to do this for mkfs and
repair anyway.

> /me is willing to go along with that if the maintainer is.  Meanwhile
> the code changes make sense so as long as there isn't some "set the
> write pointer to an arbitrary LBA" command that I missed,

There isn't.  It would have been really useful for testing, though..

> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> 
> since we wouldn't be mdrestoring user file data to the zoned section,
> right?

Well, mdrestore can't even restore user data :)

Re: [PATCH] xfs_mdrestore: don't allow restoring onto zoned block devices

[Andrey Albershteyn]

On 2025-05-12 17:25:25, Christoph Hellwig wrote:
> On Mon, May 12, 2025 at 08:12:40AM -0700, Darrick J. Wong wrote:
> > I wonder if there ought to be guards around blkzoned.h, but OTOH that
> > seems to have been introduced in 4.9 around 8 years ago so maybe it's
> > fine?
> 
> I think we're fine.  If not we'll also need to do this for mkfs and
> repair anyway.
> 
> > /me is willing to go along with that if the maintainer is.  Meanwhile

Fine by me, looks reasonable

> > the code changes make sense so as long as there isn't some "set the
> > write pointer to an arbitrary LBA" command that I missed,
> 
> There isn't.  It would have been really useful for testing, though..
> 
> > Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> > 
> > since we wouldn't be mdrestoring user file data to the zoned section,
> > right?
> 
> Well, mdrestore can't even restore user data :)
> 
> 

-- 
- Andrey