Re: [PATCH] ext4: fix quota accounting WARN in bigalloc punch hole

[Qiliang Yuan <realwujing@gmail.com>]

Hi Zhang Yi,

On 5/29/2026 3:32 PM, Zhang Yi wrote:
> On 5/28/2026 6:21 PM, Qiliang Yuan wrote:
>> __es_remove_extent() -> get_rsvd() already correctly excludes
>> boundary clusters that still contain delayed blocks from resv_used.
>> Adding pending to resv_used double-counts those boundary clusters,
>> erroneously releasing reservations that are still needed.
>
> Hmm, the analysis doesn't seem correct to me. Do you mean the
> following case?
>
> # Assume the cluster size is 16KB.
> xfs_io -f -c "pwrite 12k 4k" /mnt/foo
> xfs_io -d -c "pwrite 0 4k" /mnt/foo
> xfs_io -c "fpunch 0 4k" /mnt/foo
>
> During the direct I/O write, quota space will be added in
> ext4_mb_new_blocks() because the EXT4_MB_DELALLOC_RESERVED flag is
> not set. Therefore, in ext4_es_insert_extent(), we should release the
> quota reservations, since this cluster has already been allocated.
>
> Then, in the third operation (punch hole), it will reclaim the added
> dqb_curspace. This should not cause an insufficiency.
>
> Am I missing something?

Thanks for the review!  Let me explain the issue with your specific
example.

After step 1 (delalloc write 4KB@12KB in a 16KB cluster), we have:

  ES tree: [0,1) hole, [1,3) delayed, [3,4) hole   (blocks 0..3)
  Quota:   dqb_rsvspace += 16KB (one cluster reserved)

Step 2 (DIO write 4KB@0KB, RWF_DSYNC):

  The DIO allocates one cluster, but the mapped extent from
  ext4_ext_map_blocks() only covers the written range, e.g. [0,0].

  In ext4_es_insert_extent():

    a) __es_remove_extent([0,0]) removes the [0,1) hole entry, but
       there are no delayed extents within [0,0], so resv_used = 0.

       This is correct: the DIO extent [0,0] does not overlap the
       delayed region [1,3).

    b) __revise_pending() scans outside the newly inserted extent [0,0]:

       - Left boundary (block 0): the range starts at cluster
         boundary, no blocks to scan on the left → no pending insert.

       - Right boundary (block 3): blocks [1,3] are outside [0,0]
         and are delayed → __revise_pending() inserts a pending
         reservation and returns pending = 1.

       This pending reservation means: "cluster containing block 3
       still has delayed blocks, keep this cluster reserved."

    c) Then comes the bug:

         resv_used += pending;   // resv_used = 0 + 1 = 1

       This causes ext4_da_update_reserve_space() to release 16KB
       of quota reservation (dquot_release_reservation_block()).  But
       block 3 is still delayed!  Its quota reservation should NOT
       be released — no blocks within [0,0] actually used the delalloc
       reservation.

    So after DIO:
      dqb_rsvspace: originally 16KB, now 0 (incorrectly released)
      dqb_curspace: 16KB (from ext4_mb_new_blocks)

Step 3 (punch 4KB@0KB):

  ext4_remove_blocks() sees that block 0's cluster has a pending
  reservation → calls ext4_rereserve_cluster() → dquot_reclaim_block()
  tries to move 16KB from dqb_curspace to dqb_rsvspace.  But
  dqb_curspace may already be insufficient (depending on whether
  other allocs/frees have happened), triggering:

    WARNING at dquot_reclaim_space_nodirty

Step 4 (delalloc writeback of block 3):

  ext4_da_update_reserve_space() → dquot_claim_block() tries to
  move 16KB from dqb_rsvspace to dqb_curspace.  Since rsvspace was
  incorrectly released and not fully restored by the punch hole's
  rereseve, this triggers:

    WARNING at dquot_claim_space_nodirty

The key point is: pending from __revise_pending() indicates clusters
that *still contain delayed blocks outside the newly inserted extent*.
These clusters' quota reservations must be preserved — get_rsvd()
inside __es_remove_extent() already correctly excludes them from
resv_used.  Adding pending to resv_used double-counts them.

I also found a pre-existing retry loop issue: if __es_insert_extent()
fails with -ENOMEM on the first pass, resv_used carries a stale value
back to retry and could be double-released.  I'll include a fix for
that in v2.

-- 
Qiliang Yuan