Re: [PATCH 0/3] btrfs: remove __GFP_NOFAIL usage for debug builds

[Josef Bacik <josef@toxicpanda.com>]

On Fri, Jul 05, 2024 at 03:45:37PM +0930, Qu Wenruo wrote:
> This patchset removes all __GFP_NOFAIL flags usage inside btrfs for
> DEBUG builds.
> 
> There are 3 call sites utilizing __GFP_NOFAIL:
> 
> - __alloc_extent_buffer()
>   It's for the extent_buffer structure allocation.
>   All callers are already handling the errors.
> 
> - attach_eb_folio_to_filemap()
>   It's for the filemap_add_folio() call, the flag is also passed to mem
>   cgroup, which I suspect is not handling larger folio and __GFP_NOFAIL
>   correctly, as I'm hitting soft lockups when testing larger folios
> 
>   New error handling is added.
> 
> - btrfs_alloc_folio_array()
>   This is for page allocation for extent buffers.
>   All callers are already handling the errors.
> 
> Furthermore, to enable more testing while not affecting end users, the
> change is only implemented for DEBUG builds.
> 
> Qu Wenruo (3):
>   btrfs: do not use __GFP_NOFAIL flag for __alloc_extent_buffer()
>   btrfs: do not use __GFP_NOFAIL flag for attach_eb_folio_to_filemap()
>   btrfs: do not use __GFP_NOFAIL flag for btrfs_alloc_folio_array()

The reason I want to leave NOFAIL is because in a cgroup memory constrained
environment we could get an errant ENOMEM on some sort of metadata operation,
which then gets turned into an aborted transaction.  I don't want a memory
constrained cgroup flipping the whole file system read only because it got an
ENOMEM in a place where we have no choice but to abort the transaction.

If we could eliminate that possibility then hooray, but that's not actually
possible, because any COW for a multi-modification case (think finish ordered
io) could result in an ENOMEM and thus a transaction abort.  We need to live
with NOFAIL for these cases.  Thanks,

Josef