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

[Ryusuke Konishi <konishi.ryusuke@gmail.com>]

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);