Re: [PATCH 3/6] gfs2: Convert gfs2_write_jdata_page() to gfs2_write_jdata_folio()

[Andreas Gruenbacher <agruenba@redhat.com>]

Hi Willy,

thanks for these patches. This particular one looks problematic:

On Wed, May 17, 2023 at 5:24 AM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
> This function now supports large folios, even if nothing around it does.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/gfs2/aops.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> index 749135252d52..0f92e3e117da 100644
> --- a/fs/gfs2/aops.c
> +++ b/fs/gfs2/aops.c
> @@ -82,33 +82,34 @@ static int gfs2_get_block_noalloc(struct inode *inode, sector_t lblock,
>  }
>
>  /**
> - * gfs2_write_jdata_page - gfs2 jdata-specific version of block_write_full_page
> - * @page: The page to write
> + * gfs2_write_jdata_folio - gfs2 jdata-specific version of block_write_full_page
> + * @folio: The folio to write
>   * @wbc: The writeback control
>   *
>   * This is the same as calling block_write_full_page, but it also
>   * writes pages outside of i_size
>   */
> -static int gfs2_write_jdata_page(struct page *page,
> +static int gfs2_write_jdata_folio(struct folio *folio,
>                                  struct writeback_control *wbc)
>  {
> -       struct inode * const inode = page->mapping->host;
> +       struct inode * const inode = folio->mapping->host;
>         loff_t i_size = i_size_read(inode);
> -       const pgoff_t end_index = i_size >> PAGE_SHIFT;
> -       unsigned offset;
>
> +       if (folio_pos(folio) >= i_size)
> +               return 0;

Function gfs2_write_jdata_page was originally introduced as
gfs2_write_full_page in commit fd4c5748b8d3 ("gfs2: writeout truncated
pages") to allow writing pages even when they are beyond EOF, as the
function description documents.

This hack was added because simply skipping journaled pages isn't
enough on gfs2; before a journaled page can be freed, it needs to be
marked as "revoked" in the journal. Journal recovery will then skip
the revoked blocks, which allows them to be reused for regular,
non-journaled data. We can end up here in contexts in which we cannot
"revoke" pages, so instead, we write the original pages even when they
are beyond EOF. This hack could be revisited, but it's pretty nasty
code to pick apart.

So at least the above if needs to go for now.

>         /*
> -        * The page straddles i_size.  It must be zeroed out on each and every
> +        * The folio straddles i_size.  It must be zeroed out on each and every
>          * writepage invocation because it may be mmapped.  "A file is mapped
>          * in multiples of the page size.  For a file that is not a multiple of
> -        * the  page size, the remaining memory is zeroed when mapped, and
> +        * the page size, the remaining memory is zeroed when mapped, and
>          * writes to that region are not written out to the file."
>          */
> -       offset = i_size & (PAGE_SIZE - 1);
> -       if (page->index == end_index && offset)
> -               zero_user_segment(page, offset, PAGE_SIZE);
> +       if (i_size < folio_pos(folio) + folio_size(folio))
> +               folio_zero_segment(folio, offset_in_folio(folio, i_size),
> +                               folio_size(folio));
>
> -       return __block_write_full_page(inode, page, gfs2_get_block_noalloc, wbc,
> +       return __block_write_full_page(inode, &folio->page,
> +                                      gfs2_get_block_noalloc, wbc,
>                                        end_buffer_async_write);
>  }
>
> @@ -137,7 +138,7 @@ static int __gfs2_jdata_write_folio(struct folio *folio,
>                 }
>                 gfs2_trans_add_databufs(ip, folio, 0, folio_size(folio));
>         }
> -       return gfs2_write_jdata_page(&folio->page, wbc);
> +       return gfs2_write_jdata_folio(folio, wbc);
>  }
>
>  /**
> --
> 2.39.2
>

Thanks,
Andreas