Re: [PATCH] btrfs: wait between incomplete batch allocations

[David Sterba]

On Mon, Apr 11, 2022 at 07:11:24AM +0000, Naohiro Aota wrote:
> On Wed, Apr 06, 2022 at 02:24:18PM -0400, Sweet Tea Dorminy wrote:
> > When allocating memory in a loop, each iteration should call
> > memalloc_retry_wait() in order to prevent starving memory-freeing
> > processes (and to mark where allcoation loops are). ext4, f2fs, and xfs
> > all use this function at present for their allocation loops; btrfs ought
> > also.
> > 
> > The bulk page allocation is the only place in btrfs with an allocation
> > retry loop, so add an appropriate call to it.
> > 
> > Suggested-by: David Sterba <dsterba@xxxxxxx>
> > Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@xxxxxxxxxx>
> 
> The fstests btrfs/187 becomes incredibly slow with this patch applied.
> 
> For example, on a nvme ZNS SSD (zoned) device, it takes over 10 hours to
> finish the test case. It only takes 765 seconds if I revert this commit
> from the misc-next branch.
> 
> I also confirmed the same slowdown occurs on regular btrfs. For the
> baseline, with this commit reverted, it takes 335 seconds on 8GB ZRAM
> device running on QEMU (8GB RAM), and takes 768 seconds on a (non-zoned)
> HDD running on a real machine (128GB RAM). The tests on misc-next with the
> same setup above is still running, but it already took 2 hours.
> 
> The test case runs full btrfs sending 5 times and incremental btrfs sending
> 10 times at the same time. Also, dedupe loop and balance loop is running
> simultaneously while all the send commands finish.
> 
> The slowdown of the test case basically comes from slow "btrfs send"
> command. On the HDD run, it takes 25 minutes to run a full btrfs sending
> command and 1 hour 18 minutes to run a incremental btrfs sending
> command. Thus, we will need 78 minutes x 5 = 6.5 hours to finish all the
> send commands, making the test case incredibly slow.
> 
> > ---
> >  fs/btrfs/extent_io.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 9f2ada809dea..4bcc182744e4 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -6,6 +6,7 @@
> >  #include <linux/mm.h>
> >  #include <linux/pagemap.h>
> >  #include <linux/page-flags.h>
> > +#include <linux/sched/mm.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/blkdev.h>
> >  #include <linux/swap.h>
> > @@ -3159,6 +3160,8 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array)
> >  		 */
> >  		if (allocated == last)
> >  			return -ENOMEM;
> > +
> > +		memalloc_retry_wait(GFP_NOFS);
> 
> And, I just noticed this is because we are waiting for the retry even if we
> successfully allocated all the pages. We should exit the loop if (allocated
> == nr_pages).

Can you please test if the fixup restores the run time? This looks like
a mistake and the delays are not something we'd observe otherwise. If it
does not fix the problem then the last option is to revert the patch.