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
