| Message ID | 20240823162730.521499-7-joannelkoong@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fuse: writeback clean up / refactoring | expand |
On Fri, Aug 23, 2024 at 09:27:27AM -0700, Joanne Koong wrote: > To pave the way for refactoring out the shared logic in > fuse_writepages_fill() and fuse_writepage_locked(), this change converts > the temporary page in fuse_writepages_fill() to use the folio API. > > This is similar to the change in e0887e095a80 ("fuse: Convert > fuse_writepage_locked to take a folio"), which converted the tmp page in > fuse_writepage_locked() to use the folio API. > > inc_node_page_state() is intentionally preserved here instead of > converting to node_stat_add_folio() since it is updating the stat of the > underlying page and to better maintain API symmetry with > dec_node_page_stat() in fuse_writepage_finish_stat(). > > No functional changes added. > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > --- > fs/fuse/file.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index a51b0b085616..905b202a7acd 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -2260,7 +2260,7 @@ static int fuse_writepages_fill(struct folio *folio, > struct inode *inode = data->inode; > struct fuse_inode *fi = get_fuse_inode(inode); > struct fuse_conn *fc = get_fuse_conn(inode); > - struct page *tmp_page; > + struct folio *tmp_folio; > int err; > > if (wpa && fuse_writepage_need_send(fc, &folio->page, ap, data)) { > @@ -2269,8 +2269,8 @@ static int fuse_writepages_fill(struct folio *folio, > } > > err = -ENOMEM; > - tmp_page = alloc_page(GFP_NOFS | __GFP_HIGHMEM); > - if (!tmp_page) > + tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0); > + if (!tmp_folio) > goto out_unlock; > > /* > @@ -2290,7 +2290,7 @@ static int fuse_writepages_fill(struct folio *folio, > err = -ENOMEM; > wpa = fuse_writepage_args_alloc(); > if (!wpa) { > - __free_page(tmp_page); > + folio_put(tmp_folio); > goto out_unlock; > } > fuse_writepage_add_to_bucket(fc, wpa); > @@ -2308,14 +2308,14 @@ static int fuse_writepages_fill(struct folio *folio, > } > folio_start_writeback(folio); > > - copy_highpage(tmp_page, &folio->page); > - ap->pages[ap->num_pages] = tmp_page; > + folio_copy(tmp_folio, folio); > + ap->pages[ap->num_pages] = &tmp_folio->page; > ap->descs[ap->num_pages].offset = 0; > ap->descs[ap->num_pages].length = PAGE_SIZE; > data->orig_pages[ap->num_pages] = &folio->page; > > inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK); > - inc_node_page_state(tmp_page, NR_WRITEBACK_TEMP); > + inc_node_page_state(&tmp_folio->page, NR_WRITEBACK_TEMP); I *think* you can use node_stat_add_folio(tmp_folio, NR_WRITEBACK_TEMP); here instead of inc_node_page_state(). Thanks, Josef
On Fri, Aug 23, 2024 at 12:03 PM Josef Bacik <josef@toxicpanda.com> wrote: > > On Fri, Aug 23, 2024 at 09:27:27AM -0700, Joanne Koong wrote: > > To pave the way for refactoring out the shared logic in > > fuse_writepages_fill() and fuse_writepage_locked(), this change converts > > the temporary page in fuse_writepages_fill() to use the folio API. > > > > This is similar to the change in e0887e095a80 ("fuse: Convert > > fuse_writepage_locked to take a folio"), which converted the tmp page in > > fuse_writepage_locked() to use the folio API. > > > > inc_node_page_state() is intentionally preserved here instead of > > converting to node_stat_add_folio() since it is updating the stat of the > > underlying page and to better maintain API symmetry with > > dec_node_page_stat() in fuse_writepage_finish_stat(). > > > > No functional changes added. > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > > --- > > fs/fuse/file.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > > index a51b0b085616..905b202a7acd 100644 > > --- a/fs/fuse/file.c > > +++ b/fs/fuse/file.c > > @@ -2260,7 +2260,7 @@ static int fuse_writepages_fill(struct folio *folio, > > struct inode *inode = data->inode; > > struct fuse_inode *fi = get_fuse_inode(inode); > > struct fuse_conn *fc = get_fuse_conn(inode); > > - struct page *tmp_page; > > + struct folio *tmp_folio; > > int err; > > > > if (wpa && fuse_writepage_need_send(fc, &folio->page, ap, data)) { > > @@ -2269,8 +2269,8 @@ static int fuse_writepages_fill(struct folio *folio, > > } > > > > err = -ENOMEM; > > - tmp_page = alloc_page(GFP_NOFS | __GFP_HIGHMEM); > > - if (!tmp_page) > > + tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0); > > + if (!tmp_folio) > > goto out_unlock; > > > > /* > > @@ -2290,7 +2290,7 @@ static int fuse_writepages_fill(struct folio *folio, > > err = -ENOMEM; > > wpa = fuse_writepage_args_alloc(); > > if (!wpa) { > > - __free_page(tmp_page); > > + folio_put(tmp_folio); > > goto out_unlock; > > } > > fuse_writepage_add_to_bucket(fc, wpa); > > @@ -2308,14 +2308,14 @@ static int fuse_writepages_fill(struct folio *folio, > > } > > folio_start_writeback(folio); > > > > - copy_highpage(tmp_page, &folio->page); > > - ap->pages[ap->num_pages] = tmp_page; > > + folio_copy(tmp_folio, folio); > > + ap->pages[ap->num_pages] = &tmp_folio->page; > > ap->descs[ap->num_pages].offset = 0; > > ap->descs[ap->num_pages].length = PAGE_SIZE; > > data->orig_pages[ap->num_pages] = &folio->page; > > > > inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK); > > - inc_node_page_state(tmp_page, NR_WRITEBACK_TEMP); > > + inc_node_page_state(&tmp_folio->page, NR_WRITEBACK_TEMP); > > I *think* you can use > > node_stat_add_folio(tmp_folio, NR_WRITEBACK_TEMP); > > here instead of inc_node_page_state(). Thanks, I was thinking inc_node_page_state() here would be better for preserving the API symmetry with the dec_node_page_state() function that gets called when the writeback gets finished (in fuse_writepage_finish_stat) - I don't think it's immediately obvious that node_stat_add_folio() and dec_node_page_state() are inverses of each other. I don't feel strongly about this though, so i'm happy to change this to node_stat_add_folio as well. Thanks, Joanne > > Josef
On Fri, Aug 23, 2024 at 02:38:02PM -0700, Joanne Koong wrote: > On Fri, Aug 23, 2024 at 12:03 PM Josef Bacik <josef@toxicpanda.com> wrote: > > > > On Fri, Aug 23, 2024 at 09:27:27AM -0700, Joanne Koong wrote: > > > To pave the way for refactoring out the shared logic in > > > fuse_writepages_fill() and fuse_writepage_locked(), this change converts > > > the temporary page in fuse_writepages_fill() to use the folio API. > > > > > > This is similar to the change in e0887e095a80 ("fuse: Convert > > > fuse_writepage_locked to take a folio"), which converted the tmp page in > > > fuse_writepage_locked() to use the folio API. > > > > > > inc_node_page_state() is intentionally preserved here instead of > > > converting to node_stat_add_folio() since it is updating the stat of the > > > underlying page and to better maintain API symmetry with > > > dec_node_page_stat() in fuse_writepage_finish_stat(). > > > > > > No functional changes added. > > > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > > > --- > > > fs/fuse/file.c | 14 +++++++------- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > > > index a51b0b085616..905b202a7acd 100644 > > > --- a/fs/fuse/file.c > > > +++ b/fs/fuse/file.c > > > @@ -2260,7 +2260,7 @@ static int fuse_writepages_fill(struct folio *folio, > > > struct inode *inode = data->inode; > > > struct fuse_inode *fi = get_fuse_inode(inode); > > > struct fuse_conn *fc = get_fuse_conn(inode); > > > - struct page *tmp_page; > > > + struct folio *tmp_folio; > > > int err; > > > > > > if (wpa && fuse_writepage_need_send(fc, &folio->page, ap, data)) { > > > @@ -2269,8 +2269,8 @@ static int fuse_writepages_fill(struct folio *folio, > > > } > > > > > > err = -ENOMEM; > > > - tmp_page = alloc_page(GFP_NOFS | __GFP_HIGHMEM); > > > - if (!tmp_page) > > > + tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0); > > > + if (!tmp_folio) > > > goto out_unlock; > > > > > > /* > > > @@ -2290,7 +2290,7 @@ static int fuse_writepages_fill(struct folio *folio, > > > err = -ENOMEM; > > > wpa = fuse_writepage_args_alloc(); > > > if (!wpa) { > > > - __free_page(tmp_page); > > > + folio_put(tmp_folio); > > > goto out_unlock; > > > } > > > fuse_writepage_add_to_bucket(fc, wpa); > > > @@ -2308,14 +2308,14 @@ static int fuse_writepages_fill(struct folio *folio, > > > } > > > folio_start_writeback(folio); > > > > > > - copy_highpage(tmp_page, &folio->page); > > > - ap->pages[ap->num_pages] = tmp_page; > > > + folio_copy(tmp_folio, folio); > > > + ap->pages[ap->num_pages] = &tmp_folio->page; > > > ap->descs[ap->num_pages].offset = 0; > > > ap->descs[ap->num_pages].length = PAGE_SIZE; > > > data->orig_pages[ap->num_pages] = &folio->page; > > > > > > inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK); > > > - inc_node_page_state(tmp_page, NR_WRITEBACK_TEMP); > > > + inc_node_page_state(&tmp_folio->page, NR_WRITEBACK_TEMP); > > > > I *think* you can use > > > > node_stat_add_folio(tmp_folio, NR_WRITEBACK_TEMP); > > > > here instead of inc_node_page_state(). Thanks, > > I was thinking inc_node_page_state() here would be better for > preserving the API symmetry with the dec_node_page_state() function > that gets called when the writeback gets finished (in > fuse_writepage_finish_stat) - I don't think it's immediately obvious > that node_stat_add_folio() and dec_node_page_state() are inverses of > each other. I don't feel strongly about this though, so i'm happy to > change this to node_stat_add_folio as well. Ah yeah that's a good point, probably better to convert those in one shot so everything is consistent. Thanks, Josef
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index a51b0b085616..905b202a7acd 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -2260,7 +2260,7 @@ static int fuse_writepages_fill(struct folio *folio, struct inode *inode = data->inode; struct fuse_inode *fi = get_fuse_inode(inode); struct fuse_conn *fc = get_fuse_conn(inode); - struct page *tmp_page; + struct folio *tmp_folio; int err; if (wpa && fuse_writepage_need_send(fc, &folio->page, ap, data)) { @@ -2269,8 +2269,8 @@ static int fuse_writepages_fill(struct folio *folio, } err = -ENOMEM; - tmp_page = alloc_page(GFP_NOFS | __GFP_HIGHMEM); - if (!tmp_page) + tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0); + if (!tmp_folio) goto out_unlock; /* @@ -2290,7 +2290,7 @@ static int fuse_writepages_fill(struct folio *folio, err = -ENOMEM; wpa = fuse_writepage_args_alloc(); if (!wpa) { - __free_page(tmp_page); + folio_put(tmp_folio); goto out_unlock; } fuse_writepage_add_to_bucket(fc, wpa); @@ -2308,14 +2308,14 @@ static int fuse_writepages_fill(struct folio *folio, } folio_start_writeback(folio); - copy_highpage(tmp_page, &folio->page); - ap->pages[ap->num_pages] = tmp_page; + folio_copy(tmp_folio, folio); + ap->pages[ap->num_pages] = &tmp_folio->page; ap->descs[ap->num_pages].offset = 0; ap->descs[ap->num_pages].length = PAGE_SIZE; data->orig_pages[ap->num_pages] = &folio->page; inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK); - inc_node_page_state(tmp_page, NR_WRITEBACK_TEMP); + inc_node_page_state(&tmp_folio->page, NR_WRITEBACK_TEMP); err = 0; if (data->wpa) {
To pave the way for refactoring out the shared logic in fuse_writepages_fill() and fuse_writepage_locked(), this change converts the temporary page in fuse_writepages_fill() to use the folio API. This is similar to the change in e0887e095a80 ("fuse: Convert fuse_writepage_locked to take a folio"), which converted the tmp page in fuse_writepage_locked() to use the folio API. inc_node_page_state() is intentionally preserved here instead of converting to node_stat_add_folio() since it is updating the stat of the underlying page and to better maintain API symmetry with dec_node_page_stat() in fuse_writepage_finish_stat(). No functional changes added. Signed-off-by: Joanne Koong <joannelkoong@gmail.com> --- fs/fuse/file.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)