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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Jun 4, 2023 at 5:38 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Sat, Jun 03, 2023 at 11:34:14AM +0200, Andreas Gruenbacher wrote:
> > >   * 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.
>
> Well, that was stupid of me.
>
> > 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.
>
> Understood.  So we probably don't want to waste time zeroing the folio
> if it is entirely beyond i_size, right?  Because at the moment we'd
> zero some essentially random part of the folio if I just take out the
> check.  Should it look like this?
>
>         if (folio_pos(folio) < i_size &&
>             i_size < folio_pos(folio) + folio_size(folio))
>                folio_zero_segment(folio, offset_in_folio(folio, i_size),
>                                 folio_size(folio));

Yes, looking good, thanks.

If you haven't already, could you please consider my other comment as
well before you repost?

https://lore.kernel.org/linux-fsdevel/CAHc6FU6GowpTfX-MgRiqqwZZJ0r-85C9exc2pNkBkySCGUT0FA@xxxxxxxxxxxxxx/

Thanks,
Andreas





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux