On Fri, Oct 18, 2024 at 2:24 AM Jingbo Xu <jefflexu@xxxxxxxxxxxxxxxxx> wrote:
>
>
> On 10/15/24 2:22 AM, Joanne Koong wrote:
> > static void fuse_writepage_args_page_fill(struct fuse_writepage_args *wpa, struct folio *folio,
> > - struct folio *tmp_folio, uint32_t page_index)
> > + uint32_t page_index)
> > {
> > struct inode *inode = folio->mapping->host;
> > struct fuse_args_pages *ap = &wpa->ia.ap;
> >
> > - folio_copy(tmp_folio, folio);
> > -
> > - ap->pages[page_index] = &tmp_folio->page;
> > + folio_get(folio);
>
> Why folio_get() is needed?
>
It's not strictly needed but I added this to ensure that the folio
will always be valid when we later on access it. My understanding is
that it's an implementation detail rather than a guarantee that the
folio can't be removed from the mapping until writeback finishes.
>
> >
> > @@ -2203,68 +2047,11 @@ static void fuse_writepages_send(struct fuse_fill_wb_data *data)
> > struct fuse_writepage_args *wpa = data->wpa;
> > struct inode *inode = data->inode;
> > struct fuse_inode *fi = get_fuse_inode(inode);
> > - int num_pages = wpa->ia.ap.num_pages;
> > - int i;
> >
> > spin_lock(&fi->lock);
> > list_add_tail(&wpa->queue_entry, &fi->queued_writes);
>
> Could we also drop fi->queued_writes list and writectr counter after we
> dropping the temp folio? I'm not sure. When I grep all callsites of
> fuse_set_nowrite(), it seems that apart from the direct IO
> path(fuse_direct_io -> fuse_sync_writes), the truncation related code
> also relies on this writectr mechanism.
I'm not sure either, but I think we still need it. My understanding is
that the main purpose of this writectr is to detect when the inode has
pending writebacks. I think even without the temporary pages, we still
would need a way overall to track if the inode is writing back any
folio in its mapping.
Thanks,
Joanne
>
>
>
> --
> Thanks,
> Jingbo
