Re: [PATCH v2 1/3] erofs: get rid of magical Z_EROFS_MAPPING_STAGING

From: Gao Xiang
Date: Tue Dec 08 2020 - 03:51:02 EST


On Tue, Dec 08, 2020 at 04:44:12PM +0800, Chao Yu wrote:
> Hi Xiang,

...

> >
> > I discussed this case in the original thread,
> > https://lore.kernel.org/r/20200519100612.GA3687@hsiangkao-HP-ZHAN-66-Pro-G1
> >
> > The previous conclusion is that for EROFS case (see Matthew's reply) this
> > pair won't have too much usage. since EROFS pattern saves extra page
> > reference count (- and +) by cases.
>
> Alright, I see.

Yeah, yet in order for further confusion (or questions from others), let me
update to use this pair as much as possible in the next version :( (If someone
breaks in the future, I may need to remind him in time though...)

>

...

>
> Reviewed-by: Chao Yu <yuchao0@xxxxxxxxxx>

Thanks for the review!

Thanks,
Gao Xiang

>
> Thanks,
>
> >
> > Thanks,
> > Gao Xiang
> >
> > >
> > > Thanks,
> > >
> > > > }
> > > > @@ -648,12 +649,12 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
> > > > retry:
> > > > err = z_erofs_attach_page(clt, page, page_type);
> > > > - /* should allocate an additional staging page for pagevec */
> > > > + /* should allocate an additional short-lived page for pagevec */
> > > > if (err == -EAGAIN) {
> > > > struct page *const newpage =
> > > > alloc_page(GFP_NOFS | __GFP_NOFAIL);
> > > > - newpage->mapping = Z_EROFS_MAPPING_STAGING;
> > > > + set_page_private(newpage, Z_EROFS_SHORTLIVED_PAGE);
> > > > err = z_erofs_attach_page(clt, newpage,
> > > > Z_EROFS_PAGE_TYPE_EXCLUSIVE);
> > > > if (!err)
> > > > @@ -710,6 +711,11 @@ static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
> > > > queue_work(z_erofs_workqueue, &io->u.work);
> > > > }
> > > > +static bool z_erofs_page_is_invalidated(struct page *page)
> > > > +{
> > > > + return !page->mapping && !z_erofs_is_shortlived_page(page);
> > > > +}
> > > > +
> > > > static void z_erofs_decompressqueue_endio(struct bio *bio)
> > > > {
> > > > tagptr1_t t = tagptr_init(tagptr1_t, bio->bi_private);
> > > > @@ -722,7 +728,7 @@ static void z_erofs_decompressqueue_endio(struct bio *bio)
> > > > struct page *page = bvec->bv_page;
> > > > DBG_BUGON(PageUptodate(page));
> > > > - DBG_BUGON(!page->mapping);
> > > > + DBG_BUGON(z_erofs_page_is_invalidated(page));
> > > > if (err)
> > > > SetPageError(page);
> > > > @@ -795,9 +801,9 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> > > > /* all pages in pagevec ought to be valid */
> > > > DBG_BUGON(!page);
> > > > - DBG_BUGON(!page->mapping);
> > > > + DBG_BUGON(z_erofs_page_is_invalidated(page));
> > > > - if (z_erofs_put_stagingpage(pagepool, page))
> > > > + if (z_erofs_put_shortlivedpage(pagepool, page))
> > > > continue;
> > > > if (page_type == Z_EROFS_VLE_PAGE_TYPE_HEAD)
> > > > @@ -831,9 +837,9 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> > > > /* all compressed pages ought to be valid */
> > > > DBG_BUGON(!page);
> > > > - DBG_BUGON(!page->mapping);
> > > > + DBG_BUGON(z_erofs_page_is_invalidated(page));
> > > > - if (!z_erofs_page_is_staging(page)) {
> > > > + if (!z_erofs_is_shortlived_page(page)) {
> > > > if (erofs_page_is_managed(sbi, page)) {
> > > > if (!PageUptodate(page))
> > > > err = -EIO;
> > > > @@ -858,7 +864,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> > > > overlapped = true;
> > > > }
> > > > - /* PG_error needs checking for inplaced and staging pages */
> > > > + /* PG_error needs checking for all non-managed pages */
> > > > if (PageError(page)) {
> > > > DBG_BUGON(PageUptodate(page));
> > > > err = -EIO;
> > > > @@ -897,8 +903,8 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> > > > if (erofs_page_is_managed(sbi, page))
> > > > continue;
> > > > - /* recycle all individual staging pages */
> > > > - (void)z_erofs_put_stagingpage(pagepool, page);
> > > > + /* recycle all individual short-lived pages */
> > > > + (void)z_erofs_put_shortlivedpage(pagepool, page);
> > > > WRITE_ONCE(compressed_pages[i], NULL);
> > > > }
> > > > @@ -908,10 +914,10 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
> > > > if (!page)
> > > > continue;
> > > > - DBG_BUGON(!page->mapping);
> > > > + DBG_BUGON(z_erofs_page_is_invalidated(page));
> > > > - /* recycle all individual staging pages */
> > > > - if (z_erofs_put_stagingpage(pagepool, page))
> > > > + /* recycle all individual short-lived pages */
> > > > + if (z_erofs_put_shortlivedpage(pagepool, page))
> > > > continue;
> > > > if (err < 0)
> > > > @@ -1011,13 +1017,17 @@ static struct page *pickup_page_for_submission(struct z_erofs_pcluster *pcl,
> > > > mapping = READ_ONCE(page->mapping);
> > > > /*
> > > > - * unmanaged (file) pages are all locked solidly,
> > > > + * file-backed online pages in plcuster are all locked steady,
> > > > * therefore it is impossible for `mapping' to be NULL.
> > > > */
> > > > if (mapping && mapping != mc)
> > > > /* ought to be unmanaged pages */
> > > > goto out;
> > > > + /* directly return for shortlived page as well */
> > > > + if (z_erofs_is_shortlived_page(page))
> > > > + goto out;
> > > > +
> > > > lock_page(page);
> > > > /* only true if page reclaim goes wrong, should never happen */
> > > > @@ -1062,8 +1072,8 @@ static struct page *pickup_page_for_submission(struct z_erofs_pcluster *pcl,
> > > > out_allocpage:
> > > > page = erofs_allocpage(pagepool, gfp | __GFP_NOFAIL);
> > > > if (!tocache || add_to_page_cache_lru(page, mc, index + nr, gfp)) {
> > > > - /* non-LRU / non-movable temporary page is needed */
> > > > - page->mapping = Z_EROFS_MAPPING_STAGING;
> > > > + /* turn into temporary page if fails */
> > > > + set_page_private(page, Z_EROFS_SHORTLIVED_PAGE);
> > > > tocache = false;
> > > > }
> > > > diff --git a/fs/erofs/zdata.h b/fs/erofs/zdata.h
> > > > index 68c9b29fc0ca..b503b353d4ab 100644
> > > > --- a/fs/erofs/zdata.h
> > > > +++ b/fs/erofs/zdata.h
> > > > @@ -173,6 +173,7 @@ static inline void z_erofs_onlinepage_endio(struct page *page)
> > > > v = atomic_dec_return(u.o);
> > > > if (!(v & Z_EROFS_ONLINEPAGE_COUNT_MASK)) {
> > > > + set_page_private(page, 0);
> > > > ClearPagePrivate(page);
> > > > if (!PageError(page))
> > > > SetPageUptodate(page);
> > > >
> > >
> >
> > .
> >
>