On Thu, Apr 10, 2025 at 08:45:01AM +0200, Christoph Hellwig wrote:
> 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.
Yes.
> > > +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.
<nod>
> >
> > > + 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.
Oh, ok. Maybe a comment?
/*
* Device probing might already have set cfg->rtstart
* from the zone data.
*/
if (cfg->rtstart) {
--D
> > > +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.
