Re: [PATCH 1/2] xfs: IO time one extent per EFI

[Dave Chinner <david@fromorbit.com>]

On Fri, Apr 14, 2023 at 03:58:35PM -0700, Wengang Wang wrote:
> At IO time, make sure an EFI contains only one extent. Transaction rolling in
> xfs_defer_finish() would commit the busy blocks for previous EFIs. By that we
> avoid holding busy extents (for previously extents in the same EFI) in current
> transaction when allocating blocks for AGFL where we could be otherwise stuck
> waiting the busy extents held by current transaction to be flushed (thus a
> deadlock).
> 
> The log changes
> 1) before change:
> 
>     358 rbbn 13 rec_lsn: 1,12 Oper 5: tid: ee327fd2  len: 48 flags: None
>     359 EFI  nextents:2 id:ffff9fef708ba940
>     360 EFI id=ffff9fef708ba940 (0x21, 7)
>     361 EFI id=ffff9fef708ba940 (0x18, 8)
>     362 -----------------------------------------------------------------
>     363 rbbn 13 rec_lsn: 1,12 Oper 6: tid: ee327fd2  len: 48 flags: None
>     364 EFD  nextents:2 id:ffff9fef708ba940
>     365 EFD id=ffff9fef708ba940 (0x21, 7)
>     366 EFD id=ffff9fef708ba940 (0x18, 8)
> 
> 2) after change:
> 
>     830 rbbn 31 rec_lsn: 1,30 Oper 5: tid: 319f015f  len: 32 flags: None
>     831 EFI  nextents:1 id:ffff9fef708b9b80
>     832 EFI id=ffff9fef708b9b80 (0x21, 7)
>     833 -----------------------------------------------------------------
>     834 rbbn 31 rec_lsn: 1,30 Oper 6: tid: 319f015f  len: 32 flags: None
>     835 EFI  nextents:1 id:ffff9fef708b9d38
>     836 EFI id=ffff9fef708b9d38 (0x18, 8)
>     837 -----------------------------------------------------------------
>     838 rbbn 31 rec_lsn: 1,30 Oper 7: tid: 319f015f  len: 32 flags: None
>     839 EFD  nextents:1 id:ffff9fef708b9b80
>     840 EFD id=ffff9fef708b9b80 (0x21, 7)
>     841 -----------------------------------------------------------------
>     842 rbbn 31 rec_lsn: 1,30 Oper 8: tid: 319f015f  len: 32 flags: None
>     843 EFD  nextents:1 id:ffff9fef708b9d38
>     844 EFD id=ffff9fef708b9d38 (0x18, 8)
> 
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> ---
>  fs/xfs/xfs_extfree_item.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
> index da6a5afa607c..ae84d77eaf30 100644
> --- a/fs/xfs/xfs_extfree_item.h
> +++ b/fs/xfs/xfs_extfree_item.h
> @@ -13,8 +13,15 @@ struct kmem_cache;
>  
>  /*
>   * Max number of extents in fast allocation path.
> + *
> + * At IO time, make sure an EFI contains only one extent. Transaction rolling
> + * in xfs_defer_finish() would commit the busy blocks for previous EFIs. By
> + * that we avoid holding busy extents (for previously extents in the same EFI)
> + * in current transaction when allocating blocks for AGFL where we could be
> + * otherwise stuck waiting the busy extents held by current transaction to be
> + * flushed (thus a deadlock).
>   */
> -#define	XFS_EFI_MAX_FAST_EXTENTS	16
> +#define	XFS_EFI_MAX_FAST_EXTENTS	1

IIRC, this doesn't have anything to do with the number of extents an
EFI can hold. All it does is control how the memory for the EFI
allocated.

Oh, at some point it got overloaded code to define the max items in
a defer ops work item. Ok, I now see why you changed this, but I
don't think this is right way to solve the problem. We can handle
processing multiple extents per EFI just fine, we just need to
update the EFD and roll the transaction on each extent we process,
yes?

In hindsight, the use of XFS_EFI_MAX_FAST_EXTENTS to limit intent
size is pretty awful. I also see the same pattern copied in every
other intent.

Darrick, if the deferops code has been limiting the number of
extents in a work item to this value, when will we ever see an
intent with more extents that .max_items in it? And if that is the
case, then why wouldn't we consider an intent with more extents than
we support in log recovery a corruption event?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com