Re: Should we still go __GFP_NOFAIL? (Was Re: [PATCH] btrfs: refactor alloc_extent_buffer() to allocate-then-attach method)

[Josef Bacik <josef@toxicpanda.com>]

On Mon, Nov 27, 2023 at 03:40:41PM +1030, Qu Wenruo wrote:
> On 2023/11/23 06:33, Qu Wenruo wrote:
> [...]
> > > I wonder if we still can keep the __GFP_NOFAIL for the fallback
> > > allocation, it's there right now and seems to work on sysmtems under
> > > stress and does not cause random failures due to ENOMEM.
> > > 
> > Oh, I forgot the __NOFAIL gfp flags, that's not hard to fix, just
> > re-introduce the gfp flags to btrfs_alloc_page_array().
> 
> BTW, I think it's a good time to start a new discussion on whether we
> should go __GFP_NOFAIL.
> (Although I have updated the patch to keep the GFP_NOFAIL behavior)
> 
> I totally understand that we need some memory for tree block during
> transaction commitment and other critical sections.
> 
> And it's not that uncommon to see __GFP_NOFAIL usage in other mainstream
> filesystems.
> 
> But my concern is, we also have a lot of memory allocation which can
> lead to a lot of problems either, like btrfs_csum_one_bio() or even
> join_transaction().
> 
> I doubt if btrfs (or any other filesystems) would be to blamed if we're
> really running out of memory.
> Should the memory hungry user space programs to be firstly killed far
> before we failed to allocate memory?
> 
> 
> Furthermore, at least for btrfs, I don't think we would hit a situation
> where memory allocation failure for metadata would lead to any data
> corruption.
> The worst case is we hit transaction abort, and the fs flips RO.
> 
> Thus I'm wondering if we really need __NOFAIL for btrfs?

It's my preference that we don't use GFP_NOFAIL at all, anywhere.  We don't
really have this option for some things, mostly lock_extent/unlock_extent, but
for extent buffers we check for errors here and generally can safely handle
ENOMEM in these cases.  I would prefer to drop it.  Thanks,

Josef