Re: [PATCH 30/45] xfs_mkfs: support creating zoned file systems

[Christoph Hellwig <hch@lst.de>]

On Wed, Apr 09, 2025 at 11:54:49AM -0700, Darrick J. Wong wrote:
> > +	if (ioctl(fd, BLKRESETZONE, &range) < 0) {
> > +		if (!quiet)
> > +			printf(" FAILED\n");
> 
> Should we print /why/ the zone reset failed?

As in the errno value?  Sure.

> > +static int report_zones(const char *name, struct zone_info *zi)
> > +{
> > +	struct blk_zone_report *rep;
> > +	size_t rep_size;
> > +	struct stat st;
> > +	unsigned int i, n = 0;
> > +	uint64_t device_size;
> > +	uint64_t sector = 0;
> > +	bool found_seq = false;
> > +	int ret = 0;
> > +	int fd;
> 
> Nit: indenting

Fixed.

> > +		goto out_close;
> > +
> > +	if (ioctl(fd, BLKGETSIZE64, &device_size)) {
> > +		ret = -EIO;
> 
> ret = errno; ?  But then...
> 
> > +		goto out_close;
> > +	}
> 
> ...what's the point in returning errors if the caller never checks?

Heh, I'll look into that.

> > +	if (cli->xi->log.name && !cli->xi->log.isfile) {
> > +		report_zones(cli->xi->log.name, &zt->log);
> > +		if (zt->log.nr_zones) {
> > +			fprintf(stderr,
> > +_("Zoned devices not supported as log device!\n"));
> 
> Too bad, we really ought to be able to write logs to a zone device.
> But that's not in scope here.

That is on my todo list, but I need to finish support for the zoned RT
device first.

> 
> > +			usage();
> > +		}
> > +	}
> > +
> > +	if (cli->rtstart) {
> > +		if (cfg->rtstart) {
> 
> Er... why are we checking the variable that we set four lines down?
> Is this supposed to be a check for external zoned rt devices?
> 
> > +			fprintf(stderr,
> > +_("rtstart override not allowed on zoned devices.\n"));
> > +			usage();
> > +		}
> > +		cfg->rtstart = getnum(cli->rtstart, &ropts, R_START) / 512;

For devices with hardware zones rtstart is already set when we get
here and we don't want to allow overriding with the command line
parameter as that won't work.

> > +static void
> > +validate_rtgroup_geometry(
> > +	struct mkfs_params	*cfg)

> Hoisting this out probably should've been a separate patch.

Sure, I'll add a new one for the refactoring.

> <snip>
> 
> > diff --git a/repair/agheader.c b/repair/agheader.c
> > index 5bb4e47e0c5b..048e6c3143b5 100644
> > --- a/repair/agheader.c
> > +++ b/repair/agheader.c
> 
> Should this be in a different patch?

Yes.