Re: [PATCH v2 1/7] buffer: Return bool from grow_dev_folio()

From: Matthew Wilcox <willy@infradead.org>
To: Ryusuke Konishi <konishi.ryusuke@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Hannes Reinecke <hare@suse.de>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Pankaj Raghav <p.raghav@samsung.com>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2 1/7] buffer: Return bool from grow_dev_folio()
Date: Sat, 30 Dec 2023 09:23:01 +0000	[thread overview]
Message-ID: <ZY/hdUqeKMi0IVp6@casper.infradead.org> (raw)
In-Reply-To: <CAKFNMo=1yLxL9RR4XBY6=SkWej7tstTEiFQjUFWjxMs+5=YPFw@mail.gmail.com>

On Fri, Nov 10, 2023 at 12:43:43PM +0900, Ryusuke Konishi wrote:
> On Fri, Nov 10, 2023 at 6:07 AM Matthew Wilcox (Oracle) wrote:
> > +               /* Caller should retry if this call fails */
> > +               end_block = ~0ULL;
> >                 if (!try_to_free_buffers(folio))
> > -                       goto failed;
> > +                       goto unlock;
> >         }
> >
> 
> > -       ret = -ENOMEM;
> >         bh = folio_alloc_buffers(folio, size, gfp | __GFP_ACCOUNT);
> >         if (!bh)
> > -               goto failed;
> > +               goto unlock;
> 
> Regarding this folio_alloc_buffers() error path,
> If folio_buffers() was NULL, here end_block is 0, so this function
> returns false (which means "have a permanent failure").
> 
> But, if folio_buffers() existed and they were freed with
> try_to_free_buffers() because of bh->b_size != size, here end_block
> has been set to ~0ULL, so it seems to return true ("succeeded").
> 
> Does this semantic change match your intent?
> 
> Otherwise, I think end_block should be set to 0 just before calling
> folio_alloc_buffers().

Thanks for the review, and sorry for taking so long to get back to you.
The change was unintentional (but memory allocation failure wth GFP_KERNEL
happens so rarely that our testing was never going to catch it)

I think I should just move the assignment to end_block inside the 'if'.
It's just more obvious what's going on.  Andrew prodded me to be more
explicit about why memory allocation is a "permanent" failure, but
failing to free buffers is not.

I'll turn this into a proper patch submission later.

diff --git a/fs/buffer.c b/fs/buffer.c
index d5ce6b29c893..d3bcf601d3e5 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1028,8 +1028,8 @@ static sector_t folio_init_buffers(struct folio *folio,
  *
  * This is used purely for blockdev mappings.
  *
- * Returns false if we have a 'permanent' failure.  Returns true if
- * we succeeded, or the caller should retry.
+ * Returns false if we have a failure which cannot be cured by retrying
+ * without sleeping.  Returns true if we succeeded, or the caller should retry.
  */
 static bool grow_dev_folio(struct block_device *bdev, sector_t block,
 		pgoff_t index, unsigned size, gfp_t gfp)
@@ -1051,10 +1051,17 @@ static bool grow_dev_folio(struct block_device *bdev, sector_t block,
 			goto unlock;
 		}
 
-		/* Caller should retry if this call fails */
-		end_block = ~0ULL;
-		if (!try_to_free_buffers(folio))
+		/*
+		 * Retrying may succeed; for example the folio may finish
+		 * writeback, or buffers may be cleaned.  This should not
+		 * happen very often; maybe we have old buffers attached to
+		 * this blockdev's page cache and we're trying to change
+		 * the block size?
+		 */
+		if (!try_to_free_buffers(folio)) {
+			end_block = ~0ULL;
 			goto unlock;
+		}
 	}
 
 	bh = folio_alloc_buffers(folio, size, gfp | __GFP_ACCOUNT);
