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

[Christoph Hellwig <hch@lst.de>]

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.