On Sat, Dec 30, 2023 at 6:23 PM Matthew Wilcox wrote:
>
> 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.
Agree. I also think this fix is better.
Regards,
Ryusuke Konishi
> 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);
