Re: [PATCH] xfs: prevent race in zoned space reservations

[Carlos Maiolino <cem@kernel.org>]

On Tue, Sep 01, 2026 at 09:07:21AM +0200, Hans Holmberg wrote:
> On 31/08/2026 09:19, Carlos Maiolino wrote:
> > On Wed, Aug 26, 2026 at 02:32:19PM +0200, Hans Holmberg wrote:
> >> xfs_zoned_add_available() checks whether the reservation list is empty
> >> before adding blocks to the available-space counter.  This check is not
> >> serialized against a task adding itself to the reservation list however.
> >>
> >> This allows the space provider to observe an empty list, after which a
> >> reserver can enqueue itself and retry the counter before the new space is
> >> added.  The provider then adds the space and returns without waking the
> >> now-eligible reserver, leaving it asleep until GC or another event
> >> provides a wakeup, potentially adding seconds to max write latency.
> >>
> >> Take the reservation lock before updating the counter and checking the
> >> list.  Use list_empty() because the list is now inspected under its lock.
> >>
> >> Taking a per-mount lock when handing back space is far from ideal, but
> >> benchmarking with null_blk showed no measurable performance regression.
> >>
> >> Fixes: 0bb2193056b5 ("xfs: add support for zoned space reservations")
> >> Reported-by: Sashiko <sashiko-bot@kernel.org>
> >> Closes: https://sashiko.dev/#/patchset/20260609075655.1698743-1-hch@lst.de?part=2
> >> Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
> >> ---
> >>  fs/xfs/xfs_zone_space_resv.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/xfs/xfs_zone_space_resv.c b/fs/xfs/xfs_zone_space_resv.c
> >> index 5c6e6ef627e4..7aa3c74fb2e0 100644
> >> --- a/fs/xfs/xfs_zone_space_resv.c
> >> +++ b/fs/xfs/xfs_zone_space_resv.c
> >> @@ -85,13 +85,13 @@ xfs_zoned_add_available(
> >>  	struct xfs_zone_info		*zi = mp->m_zone_info;
> >>  	struct xfs_zone_reservation	*reservation;
> >>  
> >> -	if (list_empty_careful(&zi->zi_reclaim_reservations)) {
> >> -		xfs_add_freecounter(mp, XC_FREE_RTAVAILABLE, count_fsb);
> >> +	spin_lock(&zi->zi_reservation_lock);
> >> +	xfs_add_freecounter(mp, XC_FREE_RTAVAILABLE, count_fsb);
> >> +	if (list_empty(&zi->zi_reclaim_reservations)) {
> >> +		spin_unlock(&zi->zi_reservation_lock);
> >>  		return;
> >>  	}
> >>  
> >> -	spin_lock(&zi->zi_reservation_lock);
> >> -	xfs_add_freecounter(mp, XC_FREE_RTAVAILABLE, count_fsb);
> >>  	count_fsb = xfs_sum_freecounter(mp, XC_FREE_RTAVAILABLE);
> >>  	list_for_each_entry(reservation, &zi->zi_reclaim_reservations, entry) {
> >>  		if (reservation->count_fsb > count_fsb)
> > 
> > Looks good to me:
> > 
> > Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> > 
> > FWIW, don't xfs_zoned_reserve_available() might have a similar problem
> > when decreasing the free counter? I'm not that much literate on zoned,
> > but a quick look seemed reserving space might hit a similar problem?!
> > 
> 
> It's a good question, in xfs_zoned_reserve_available, a writer could
> theoretically race and skip ahead of another writer that was lining up
> to wait for available space.
> 
> So while xfs_zoned_reserve_available is not guaranteed to be 100% fair,
> I think the alternative (adding the lock on the reserve path) is too costly. 

Oh, thanks for looking into it. I certainly didn't think about the costs
implied here!