Re: [PATCH 22/43] xfs: add the zoned space allocator

Darrick J. Wong
2025-02-13T22:08:10Z

On Thu, Feb 13, 2025 at 06:14:48AM +0100, Christoph Hellwig wrote:
> On Fri, Feb 07, 2025 at 09:39:42AM -0800, Darrick J. Wong wrote:
> > > + /* XXX: this is a little verbose, but let's keep it for now */
> > > + xfs_info(mp, "using zone %u (%u)",
> > > + rtg_rgno(oz->oz_rtg), zi->zi_nr_open_zones);
> >
> > Should this XXX become a tracepoint?
> >
> > > + trace_xfs_zone_activate(oz->oz_rtg);
>
> The tracepoint is just below, but yes - this obviously was left as a
> canary in the coalmine to check if anyone actually reviews the code :)
>
> > > + if (xfs_is_shutdown(mp))
> > > + goto out_error;
> > > +
> > > + /*
> > > + * If we don't have a cached zone in this write context, see if the
> > > + * last extent before the one we are writing points of an active zone.
> >
> > "...writing points *to the end* of an active zone" ?
>
> We only really care about the same zone. Even if that doesn't create a
> contiguous extent, it means the next GC cycle will make it contiguous.
> But even before that the locality is kinda useful at least on HDD.
>
> There's still grammar issues, though which I've fixed up.
>
> >
> > > + * If so, just continue writing to it.
> > > + */
> > > + if (!*oz && ioend->io_offset)
> > > + *oz = xfs_last_used_zone(ioend);
> >
> > Also, why not return oz instead of passing it out via double pointer?
>
> I remember going back and forth a few times. Let me give it a try to
> see how it works out this time.
>
> > > + mp->m_zone_info = xfs_alloc_zone_info(mp);
> > > + if (!mp->m_zone_info)
> > > + return -ENOMEM;
> > > +
> > > + xfs_info(mp, "%u zones of %u blocks size (%u max open)",
> > > + mp->m_sb.sb_rgcount, mp->m_groups[XG_TYPE_RTG].blocks,
> > > + mp->m_max_open_zones);
> >
> > Tracepoint?
>
> I think this actually pretty usueful mount time information in the
> kernel log. But if you mean a trace point on top of the message and
> not instead I can look into it.

Yeah, I like to just set up ftrace for 'xfs_zone*' and see what falls
out of a test run, instead of pulling in printk and then having to
filter out a bunch of other stuff. :)

--D