* [PATCH 1/4] ubifs: Convert from writepage to writepages
2023-06-05 16:50 [PATCH 0/4] ubifs: Convert writeback to use folios Matthew Wilcox (Oracle)
@ 2023-06-05 16:50 ` Matthew Wilcox (Oracle)
2023-06-06 14:37 ` Zhihao Cheng
2023-06-05 16:50 ` [PATCH 2/4] ubifs: Convert ubifs_writepage to use a folio Matthew Wilcox (Oracle)
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Matthew Wilcox (Oracle) @ 2023-06-05 16:50 UTC (permalink / raw)
To: Richard Weinberger; +Cc: Matthew Wilcox (Oracle), linux-mtd, linux-fsdevel
This is a simplistic conversion to separate out any effects of
no longer having a writepage method.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/ubifs/file.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 979ab1d9d0c3..8bb4cb9d528f 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1003,8 +1003,10 @@ static int do_writepage(struct page *page, int len)
* on the page lock and it would not write the truncated inode node to the
* journal before we have finished.
*/
-static int ubifs_writepage(struct page *page, struct writeback_control *wbc)
+static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
+ void *data)
{
+ struct page *page = &folio->page;
struct inode *inode = page->mapping->host;
struct ubifs_info *c = inode->i_sb->s_fs_info;
struct ubifs_inode *ui = ubifs_inode(inode);
@@ -1076,6 +1078,12 @@ static int ubifs_writepage(struct page *page, struct writeback_control *wbc)
return err;
}
+static int ubifs_writepages(struct address_space *mapping,
+ struct writeback_control *wbc)
+{
+ return write_cache_pages(mapping, wbc, ubifs_writepage, NULL);
+}
+
/**
* do_attr_changes - change inode attributes.
* @inode: inode to change attributes for
@@ -1636,7 +1644,7 @@ static int ubifs_symlink_getattr(struct mnt_idmap *idmap,
const struct address_space_operations ubifs_file_address_operations = {
.read_folio = ubifs_read_folio,
- .writepage = ubifs_writepage,
+ .writepages = ubifs_writepages,
.write_begin = ubifs_write_begin,
.write_end = ubifs_write_end,
.invalidate_folio = ubifs_invalidate_folio,
--
2.39.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 1/4] ubifs: Convert from writepage to writepages
2023-06-05 16:50 ` [PATCH 1/4] ubifs: Convert from writepage to writepages Matthew Wilcox (Oracle)
@ 2023-06-06 14:37 ` Zhihao Cheng
2023-06-07 14:11 ` Zhihao Cheng
0 siblings, 1 reply; 18+ messages in thread
From: Zhihao Cheng @ 2023-06-06 14:37 UTC (permalink / raw)
To: Matthew Wilcox (Oracle), Richard Weinberger; +Cc: linux-mtd, linux-fsdevel
在 2023/6/6 0:50, Matthew Wilcox (Oracle) 写道:
Hi,
> This is a simplistic conversion to separate out any effects of
> no longer having a writepage method.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> fs/ubifs/file.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 979ab1d9d0c3..8bb4cb9d528f 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1003,8 +1003,10 @@ static int do_writepage(struct page *page, int len)
> * on the page lock and it would not write the truncated inode node to the
> * journal before we have finished.
> */
> -static int ubifs_writepage(struct page *page, struct writeback_control *wbc)
> +static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
> + void *data)
> {
> + struct page *page = &folio->page;
> struct inode *inode = page->mapping->host;
> struct ubifs_info *c = inode->i_sb->s_fs_info;
> struct ubifs_inode *ui = ubifs_inode(inode);
> @@ -1076,6 +1078,12 @@ static int ubifs_writepage(struct page *page, struct writeback_control *wbc)
> return err;
> }
>
> +static int ubifs_writepages(struct address_space *mapping,
> + struct writeback_control *wbc)
> +{
> + return write_cache_pages(mapping, wbc, ubifs_writepage, NULL);
> +}
> +
There is a small difference.
before patch applied:
do_writepages -> write_cache_pages -> writepage_cb:
ubifs_writepage
mapping_set_error(mapping, ret)
So, we can get error returned from errseq_check_and_advance in syncfs
syscall if ubifs_writepage occurs EIO.
after patch applied:
do_writepages -> ubifs_writepages -> write_cache_pages ->
ubifs_writepage, mapping won't be set error if ubifs_writepage failed.
Unfortunately, ubifs is not a block filesystem, so
sync_filesystem->sync_blockdev_nowait will return 0. Finally, syncfs
syscall will return 0.
> /**
> * do_attr_changes - change inode attributes.
> * @inode: inode to change attributes for
> @@ -1636,7 +1644,7 @@ static int ubifs_symlink_getattr(struct mnt_idmap *idmap,
>
> const struct address_space_operations ubifs_file_address_operations = {
> .read_folio = ubifs_read_folio,
> - .writepage = ubifs_writepage,
> + .writepages = ubifs_writepages,
> .write_begin = ubifs_write_begin,
> .write_end = ubifs_write_end,
> .invalidate_folio = ubifs_invalidate_folio,
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/4] ubifs: Convert from writepage to writepages
2023-06-06 14:37 ` Zhihao Cheng
@ 2023-06-07 14:11 ` Zhihao Cheng
0 siblings, 0 replies; 18+ messages in thread
From: Zhihao Cheng @ 2023-06-07 14:11 UTC (permalink / raw)
To: Matthew Wilcox (Oracle), Richard Weinberger; +Cc: linux-mtd, linux-fsdevel
在 2023/6/6 22:37, Zhihao Cheng 写道:
> 在 2023/6/6 0:50, Matthew Wilcox (Oracle) 写道:
> Hi,
>> This is a simplistic conversion to separate out any effects of
>> no longer having a writepage method.
>>
>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>> ---
>> fs/ubifs/file.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>> index 979ab1d9d0c3..8bb4cb9d528f 100644
>> --- a/fs/ubifs/file.c
>> +++ b/fs/ubifs/file.c
>> @@ -1003,8 +1003,10 @@ static int do_writepage(struct page *page, int
>> len)
>> * on the page lock and it would not write the truncated inode node
>> to the
>> * journal before we have finished.
>> */
>> -static int ubifs_writepage(struct page *page, struct
>> writeback_control *wbc)
>> +static int ubifs_writepage(struct folio *folio, struct
>> writeback_control *wbc,
>> + void *data)
>> {
>> + struct page *page = &folio->page;
>> struct inode *inode = page->mapping->host;
>> struct ubifs_info *c = inode->i_sb->s_fs_info;
>> struct ubifs_inode *ui = ubifs_inode(inode);
>> @@ -1076,6 +1078,12 @@ static int ubifs_writepage(struct page *page,
>> struct writeback_control *wbc)
>> return err;
>> }
>> +static int ubifs_writepages(struct address_space *mapping,
>> + struct writeback_control *wbc)
>> +{
>> + return write_cache_pages(mapping, wbc, ubifs_writepage, NULL);
>> +}
>> +
>
> There is a small difference.
> before patch applied:
> do_writepages -> write_cache_pages -> writepage_cb:
> ubifs_writepage
> mapping_set_error(mapping, ret)
>
> So, we can get error returned from errseq_check_and_advance in syncfs
> syscall if ubifs_writepage occurs EIO.
>
> after patch applied:
>
> do_writepages -> ubifs_writepages -> write_cache_pages ->
> ubifs_writepage, mapping won't be set error if ubifs_writepage failed.
> Unfortunately, ubifs is not a block filesystem, so
> sync_filesystem->sync_blockdev_nowait will return 0. Finally, syncfs
> syscall will return 0.
>
I think we can add mapping_set_error in error branch of ubifs_writepage
to solve it.
BTW, I notice that shrink_folio_list -> pageout will try to shrink page
by writepage method, if we remove '->writepage', the dirty page won't be
shrinked in that way?
>
>> /**
>> * do_attr_changes - change inode attributes.
>> * @inode: inode to change attributes for
>> @@ -1636,7 +1644,7 @@ static int ubifs_symlink_getattr(struct
>> mnt_idmap *idmap,
>> const struct address_space_operations ubifs_file_address_operations = {
>> .read_folio = ubifs_read_folio,
>> - .writepage = ubifs_writepage,
>> + .writepages = ubifs_writepages,
>> .write_begin = ubifs_write_begin,
>> .write_end = ubifs_write_end,
>> .invalidate_folio = ubifs_invalidate_folio,
>>
>
>
> .
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/4] ubifs: Convert ubifs_writepage to use a folio
2023-06-05 16:50 [PATCH 0/4] ubifs: Convert writeback to use folios Matthew Wilcox (Oracle)
2023-06-05 16:50 ` [PATCH 1/4] ubifs: Convert from writepage to writepages Matthew Wilcox (Oracle)
@ 2023-06-05 16:50 ` Matthew Wilcox (Oracle)
2023-06-07 14:48 ` Zhihao Cheng
2023-06-05 16:50 ` [PATCH 3/4] ubifs: Use a folio in do_truncation() Matthew Wilcox (Oracle)
2023-06-05 16:50 ` [PATCH 4/4] ubifs: Convert do_writepage() to take a folio Matthew Wilcox (Oracle)
3 siblings, 1 reply; 18+ messages in thread
From: Matthew Wilcox (Oracle) @ 2023-06-05 16:50 UTC (permalink / raw)
To: Richard Weinberger; +Cc: Matthew Wilcox (Oracle), linux-mtd, linux-fsdevel
We still pass the page down to do_writepage(), but ubifs_writepage()
itself is now large folio safe. It also contains far fewer hidden calls
to compound_head().
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/ubifs/file.c | 39 +++++++++++++++++----------------------
1 file changed, 17 insertions(+), 22 deletions(-)
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 8bb4cb9d528f..1c7a99c36906 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1006,21 +1006,18 @@ static int do_writepage(struct page *page, int len)
static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
void *data)
{
- struct page *page = &folio->page;
- struct inode *inode = page->mapping->host;
+ struct inode *inode = folio->mapping->host;
struct ubifs_info *c = inode->i_sb->s_fs_info;
struct ubifs_inode *ui = ubifs_inode(inode);
loff_t i_size = i_size_read(inode), synced_i_size;
- pgoff_t end_index = i_size >> PAGE_SHIFT;
- int err, len = i_size & (PAGE_SIZE - 1);
- void *kaddr;
+ int err, len = folio_size(folio);
dbg_gen("ino %lu, pg %lu, pg flags %#lx",
- inode->i_ino, page->index, page->flags);
- ubifs_assert(c, PagePrivate(page));
+ inode->i_ino, folio->index, folio->flags);
+ ubifs_assert(c, folio->private != NULL);
- /* Is the page fully outside @i_size? (truncate in progress) */
- if (page->index > end_index || (page->index == end_index && !len)) {
+ /* Is the folio fully outside @i_size? (truncate in progress) */
+ if (folio_pos(folio) >= i_size) {
err = 0;
goto out_unlock;
}
@@ -1029,9 +1026,9 @@ static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
synced_i_size = ui->synced_i_size;
spin_unlock(&ui->ui_lock);
- /* Is the page fully inside @i_size? */
- if (page->index < end_index) {
- if (page->index >= synced_i_size >> PAGE_SHIFT) {
+ /* Is the folio fully inside i_size? */
+ if (folio_pos(folio) + len < i_size) {
+ if (folio_pos(folio) >= synced_i_size) {
err = inode->i_sb->s_op->write_inode(inode, NULL);
if (err)
goto out_redirty;
@@ -1044,20 +1041,18 @@ static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
* with this.
*/
}
- return do_writepage(page, PAGE_SIZE);
+ return do_writepage(&folio->page, len);
}
/*
- * 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
* writes to that region are not written out to the file."
*/
- kaddr = kmap_atomic(page);
- memset(kaddr + len, 0, PAGE_SIZE - len);
- flush_dcache_page(page);
- kunmap_atomic(kaddr);
+ folio_zero_segment(folio, offset_in_folio(folio, i_size), len);
+ len = offset_in_folio(folio, i_size);
if (i_size > synced_i_size) {
err = inode->i_sb->s_op->write_inode(inode, NULL);
@@ -1065,16 +1060,16 @@ static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
goto out_redirty;
}
- return do_writepage(page, len);
+ return do_writepage(&folio->page, len);
out_redirty:
/*
- * redirty_page_for_writepage() won't call ubifs_dirty_inode() because
+ * folio_redirty_for_writepage() won't call ubifs_dirty_inode() because
* it passes I_DIRTY_PAGES flag while calling __mark_inode_dirty(), so
* there is no need to do space budget for dirty inode.
*/
- redirty_page_for_writepage(wbc, page);
+ folio_redirty_for_writepage(wbc, folio);
out_unlock:
- unlock_page(page);
+ folio_unlock(folio);
return err;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 2/4] ubifs: Convert ubifs_writepage to use a folio
2023-06-05 16:50 ` [PATCH 2/4] ubifs: Convert ubifs_writepage to use a folio Matthew Wilcox (Oracle)
@ 2023-06-07 14:48 ` Zhihao Cheng
0 siblings, 0 replies; 18+ messages in thread
From: Zhihao Cheng @ 2023-06-07 14:48 UTC (permalink / raw)
To: Matthew Wilcox (Oracle), Richard Weinberger; +Cc: linux-mtd, linux-fsdevel
在 2023/6/6 0:50, Matthew Wilcox (Oracle) 写道:
> We still pass the page down to do_writepage(), but ubifs_writepage()
> itself is now large folio safe. It also contains far fewer hidden calls
> to compound_head().
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> fs/ubifs/file.c | 39 +++++++++++++++++----------------------
> 1 file changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 8bb4cb9d528f..1c7a99c36906 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1006,21 +1006,18 @@ static int do_writepage(struct page *page, int len)
> static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
> void *data)
> {
> - struct page *page = &folio->page;
> - struct inode *inode = page->mapping->host;
> + struct inode *inode = folio->mapping->host;
> struct ubifs_info *c = inode->i_sb->s_fs_info;
> struct ubifs_inode *ui = ubifs_inode(inode);
> loff_t i_size = i_size_read(inode), synced_i_size;
> - pgoff_t end_index = i_size >> PAGE_SHIFT;
> - int err, len = i_size & (PAGE_SIZE - 1);
> - void *kaddr;
> + int err, len = folio_size(folio);
>
> dbg_gen("ino %lu, pg %lu, pg flags %#lx",
> - inode->i_ino, page->index, page->flags);
> - ubifs_assert(c, PagePrivate(page));
> + inode->i_ino, folio->index, folio->flags);
> + ubifs_assert(c, folio->private != NULL);
>
> - /* Is the page fully outside @i_size? (truncate in progress) */
> - if (page->index > end_index || (page->index == end_index && !len)) {
> + /* Is the folio fully outside @i_size? (truncate in progress) */
> + if (folio_pos(folio) >= i_size) {
> err = 0;
> goto out_unlock;
> }
> @@ -1029,9 +1026,9 @@ static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
> synced_i_size = ui->synced_i_size;
> spin_unlock(&ui->ui_lock);
>
> - /* Is the page fully inside @i_size? */
> - if (page->index < end_index) {
> - if (page->index >= synced_i_size >> PAGE_SHIFT) {
> + /* Is the folio fully inside i_size? */
> + if (folio_pos(folio) + len < i_size) {
if (folio_pos(folio) + len <= i_size) ?
> + if (folio_pos(folio) >= synced_i_size) {
> err = inode->i_sb->s_op->write_inode(inode, NULL);
> if (err)
> goto out_redirty;
> @@ -1044,20 +1041,18 @@ static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
> * with this.
> */
> }
> - return do_writepage(page, PAGE_SIZE);
> + return do_writepage(&folio->page, len);
> }
>
> /*
> - * 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
> * writes to that region are not written out to the file."
> */
> - kaddr = kmap_atomic(page);
> - memset(kaddr + len, 0, PAGE_SIZE - len);
> - flush_dcache_page(page);
> - kunmap_atomic(kaddr);
> + folio_zero_segment(folio, offset_in_folio(folio, i_size), len);
> + len = offset_in_folio(folio, i_size);
>
> if (i_size > synced_i_size) {
> err = inode->i_sb->s_op->write_inode(inode, NULL);
> @@ -1065,16 +1060,16 @@ static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
> goto out_redirty;
> }
>
> - return do_writepage(page, len);
> + return do_writepage(&folio->page, len);
> out_redirty:
> /*
> - * redirty_page_for_writepage() won't call ubifs_dirty_inode() because
> + * folio_redirty_for_writepage() won't call ubifs_dirty_inode() because
> * it passes I_DIRTY_PAGES flag while calling __mark_inode_dirty(), so
> * there is no need to do space budget for dirty inode.
> */
> - redirty_page_for_writepage(wbc, page);
> + folio_redirty_for_writepage(wbc, folio);
> out_unlock:
> - unlock_page(page);
> + folio_unlock(folio);
> return err;
> }
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/4] ubifs: Use a folio in do_truncation()
2023-06-05 16:50 [PATCH 0/4] ubifs: Convert writeback to use folios Matthew Wilcox (Oracle)
2023-06-05 16:50 ` [PATCH 1/4] ubifs: Convert from writepage to writepages Matthew Wilcox (Oracle)
2023-06-05 16:50 ` [PATCH 2/4] ubifs: Convert ubifs_writepage to use a folio Matthew Wilcox (Oracle)
@ 2023-06-05 16:50 ` Matthew Wilcox (Oracle)
2023-06-05 17:05 ` Matthew Wilcox
2023-06-05 16:50 ` [PATCH 4/4] ubifs: Convert do_writepage() to take a folio Matthew Wilcox (Oracle)
3 siblings, 1 reply; 18+ messages in thread
From: Matthew Wilcox (Oracle) @ 2023-06-05 16:50 UTC (permalink / raw)
To: Richard Weinberger; +Cc: Matthew Wilcox (Oracle), linux-mtd, linux-fsdevel
Convert from the old page APIs to the new folio APIs which saves
a few hidden calls to compound_head().
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/ubifs/file.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 1c7a99c36906..c0e68b3d7582 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1153,11 +1153,11 @@ static int do_truncation(struct ubifs_info *c, struct inode *inode,
if (offset) {
pgoff_t index = new_size >> PAGE_SHIFT;
- struct page *page;
+ struct folio *folio;
- page = find_lock_page(inode->i_mapping, index);
- if (page) {
- if (PageDirty(page)) {
+ folio = filemap_lock_folio(inode->i_mapping, index);
+ if (folio) {
+ if (folio_test_dirty(folio)) {
/*
* 'ubifs_jnl_truncate()' will try to truncate
* the last data node, but it contains
@@ -1166,14 +1166,14 @@ static int do_truncation(struct ubifs_info *c, struct inode *inode,
* 'ubifs_jnl_truncate()' will see an already
* truncated (and up to date) data node.
*/
- ubifs_assert(c, PagePrivate(page));
+ ubifs_assert(c, folio->private != NULL);
- clear_page_dirty_for_io(page);
+ folio_clear_dirty_for_io(folio);
if (UBIFS_BLOCKS_PER_PAGE_SHIFT)
- offset = new_size &
- (PAGE_SIZE - 1);
- err = do_writepage(page, offset);
- put_page(page);
+ offset = offset_in_folio(folio,
+ new_size);
+ err = do_writepage(&folio->page, offset);
+ folio_put(folio);
if (err)
goto out_budg;
/*
@@ -1186,8 +1186,8 @@ static int do_truncation(struct ubifs_info *c, struct inode *inode,
* to 'ubifs_jnl_truncate()' to save it from
* having to read it.
*/
- unlock_page(page);
- put_page(page);
+ folio_unlock(folio);
+ folio_put(folio);
}
}
}
--
2.39.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 3/4] ubifs: Use a folio in do_truncation()
2023-06-05 16:50 ` [PATCH 3/4] ubifs: Use a folio in do_truncation() Matthew Wilcox (Oracle)
@ 2023-06-05 17:05 ` Matthew Wilcox
2023-06-08 14:31 ` Zhihao Cheng
0 siblings, 1 reply; 18+ messages in thread
From: Matthew Wilcox @ 2023-06-05 17:05 UTC (permalink / raw)
To: Richard Weinberger; +Cc: linux-mtd, linux-fsdevel
On Mon, Jun 05, 2023 at 05:50:28PM +0100, Matthew Wilcox (Oracle) wrote:
> Convert from the old page APIs to the new folio APIs which saves
> a few hidden calls to compound_head().
Argh. This fix was supposed to be folded in.
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 1b2055d5ec5f..67cf5138ccc4 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1161,7 +1161,7 @@ static int do_truncation(struct ubifs_info *c, struct inode *inode,
struct folio *folio;
folio = filemap_lock_folio(inode->i_mapping, index);
- if (folio) {
+ if (!IS_ERR(folio)) {
if (folio_test_dirty(folio)) {
/*
* 'ubifs_jnl_truncate()' will try to truncate
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 3/4] ubifs: Use a folio in do_truncation()
2023-06-05 17:05 ` Matthew Wilcox
@ 2023-06-08 14:31 ` Zhihao Cheng
0 siblings, 0 replies; 18+ messages in thread
From: Zhihao Cheng @ 2023-06-08 14:31 UTC (permalink / raw)
To: Matthew Wilcox, Richard Weinberger; +Cc: linux-mtd, linux-fsdevel
在 2023/6/6 1:05, Matthew Wilcox 写道:
> On Mon, Jun 05, 2023 at 05:50:28PM +0100, Matthew Wilcox (Oracle) wrote:
>> Convert from the old page APIs to the new folio APIs which saves
>> a few hidden calls to compound_head().
>
> Argh. This fix was supposed to be folded in.
>
>
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 1b2055d5ec5f..67cf5138ccc4 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1161,7 +1161,7 @@ static int do_truncation(struct ubifs_info *c, struct inode *inode,
> struct folio *folio;
>
> folio = filemap_lock_folio(inode->i_mapping, index);
> - if (folio) {
> + if (!IS_ERR(folio)) {
> if (folio_test_dirty(folio)) {
> /*
> * 'ubifs_jnl_truncate()' will try to truncate
> .
>
Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/4] ubifs: Convert do_writepage() to take a folio
2023-06-05 16:50 [PATCH 0/4] ubifs: Convert writeback to use folios Matthew Wilcox (Oracle)
` (2 preceding siblings ...)
2023-06-05 16:50 ` [PATCH 3/4] ubifs: Use a folio in do_truncation() Matthew Wilcox (Oracle)
@ 2023-06-05 16:50 ` Matthew Wilcox (Oracle)
2023-06-05 19:28 ` kernel test robot
` (3 more replies)
3 siblings, 4 replies; 18+ messages in thread
From: Matthew Wilcox (Oracle) @ 2023-06-05 16:50 UTC (permalink / raw)
To: Richard Weinberger; +Cc: Matthew Wilcox (Oracle), linux-mtd, linux-fsdevel
Replace the call to SetPageError() with a call to mapping_set_error().
Support large folios by using kmap_local_folio() and remapping each time
we cross a page boundary. Saves a lot of hidden calls to compound_head().
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/ubifs/file.c | 53 +++++++++++++++++++++++++++----------------------
1 file changed, 29 insertions(+), 24 deletions(-)
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index c0e68b3d7582..1b2055d5ec5f 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -900,60 +900,65 @@ static int ubifs_read_folio(struct file *file, struct folio *folio)
return 0;
}
-static int do_writepage(struct page *page, int len)
+static int do_writepage(struct folio *folio, size_t len)
{
int err = 0, i, blen;
unsigned int block;
void *addr;
+ size_t offset = 0;
union ubifs_key key;
- struct inode *inode = page->mapping->host;
+ struct inode *inode = folio->mapping->host;
struct ubifs_info *c = inode->i_sb->s_fs_info;
#ifdef UBIFS_DEBUG
struct ubifs_inode *ui = ubifs_inode(inode);
spin_lock(&ui->ui_lock);
- ubifs_assert(c, page->index <= ui->synced_i_size >> PAGE_SHIFT);
+ ubifs_assert(c, folio->index <= ui->synced_i_size >> PAGE_SHIFT);
spin_unlock(&ui->ui_lock);
#endif
- /* Update radix tree tags */
- set_page_writeback(page);
+ folio_start_writeback(folio);
- addr = kmap(page);
- block = page->index << UBIFS_BLOCKS_PER_PAGE_SHIFT;
+ addr = kmap_local_folio(folio, offset);
+ block = folio->index << UBIFS_BLOCKS_PER_PAGE_SHIFT;
i = 0;
- while (len) {
- blen = min_t(int, len, UBIFS_BLOCK_SIZE);
+ for (;;) {
+ blen = min_t(size_t, len, UBIFS_BLOCK_SIZE);
data_key_init(c, &key, inode->i_ino, block);
err = ubifs_jnl_write_data(c, inode, &key, addr, blen);
if (err)
break;
- if (++i >= UBIFS_BLOCKS_PER_PAGE)
+ len -= blen;
+ if (!len)
break;
block += 1;
addr += blen;
- len -= blen;
+ if (folio_test_highmem(folio) && !offset_in_page(addr)) {
+ kunmap_local(addr - blen);
+ offset += PAGE_SIZE;
+ addr = kmap_local_folio(folio, offset);
+ }
}
+ kunmap_local(addr);
if (err) {
- SetPageError(page);
- ubifs_err(c, "cannot write page %lu of inode %lu, error %d",
- page->index, inode->i_ino, err);
+ mapping_set_error(folio->mapping, err);
+ ubifs_err(c, "cannot write folio %lu of inode %lu, error %d",
+ folio->index, inode->i_ino, err);
ubifs_ro_mode(c, err);
}
- ubifs_assert(c, PagePrivate(page));
- if (PageChecked(page))
+ ubifs_assert(c, folio->private != NULL);
+ if (folio_test_checked(folio))
release_new_page_budget(c);
else
release_existing_page_budget(c);
atomic_long_dec(&c->dirty_pg_cnt);
- detach_page_private(page);
- ClearPageChecked(page);
+ folio_detach_private(folio);
+ folio_clear_checked(folio);
- kunmap(page);
- unlock_page(page);
- end_page_writeback(page);
+ folio_unlock(folio);
+ folio_end_writeback(folio);
return err;
}
@@ -1041,7 +1046,7 @@ static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
* with this.
*/
}
- return do_writepage(&folio->page, len);
+ return do_writepage(folio, len);
}
/*
@@ -1060,7 +1065,7 @@ static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
goto out_redirty;
}
- return do_writepage(&folio->page, len);
+ return do_writepage(folio, len);
out_redirty:
/*
* folio_redirty_for_writepage() won't call ubifs_dirty_inode() because
@@ -1172,7 +1177,7 @@ static int do_truncation(struct ubifs_info *c, struct inode *inode,
if (UBIFS_BLOCKS_PER_PAGE_SHIFT)
offset = offset_in_folio(folio,
new_size);
- err = do_writepage(&folio->page, offset);
+ err = do_writepage(folio, offset);
folio_put(folio);
if (err)
goto out_budg;
--
2.39.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] ubifs: Convert do_writepage() to take a folio
2023-06-05 16:50 ` [PATCH 4/4] ubifs: Convert do_writepage() to take a folio Matthew Wilcox (Oracle)
@ 2023-06-05 19:28 ` kernel test robot
2023-06-05 21:37 ` Richard Weinberger
` (2 subsequent siblings)
3 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2023-06-05 19:28 UTC (permalink / raw)
To: Matthew Wilcox (Oracle), Richard Weinberger
Cc: oe-kbuild-all, Matthew Wilcox (Oracle), linux-mtd, linux-fsdevel
Hi Matthew,
kernel test robot noticed the following build warnings:
[auto build test WARNING on rw-ubifs/next]
[also build test WARNING on linus/master v6.4-rc5 next-20230605]
[cannot apply to rw-ubifs/fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Matthew-Wilcox-Oracle/ubifs-Convert-from-writepage-to-writepages/20230606-005309
base: https://git.kernel.org/pub/scm/linux/kernel/git/rw/ubifs.git next
patch link: https://lore.kernel.org/r/20230605165029.2908304-5-willy%40infradead.org
patch subject: [PATCH 4/4] ubifs: Convert do_writepage() to take a folio
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230606/202306060302.sCFYZsJ5-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4f6ac0962f61cc07c95177f78cf1f3b03e79d822
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Matthew-Wilcox-Oracle/ubifs-Convert-from-writepage-to-writepages/20230606-005309
git checkout 4f6ac0962f61cc07c95177f78cf1f3b03e79d822
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=alpha olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash fs/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306060302.sCFYZsJ5-lkp@intel.com/
All warnings (new ones prefixed by >>):
fs/ubifs/file.c: In function 'do_writepage':
>> fs/ubifs/file.c:905:22: warning: variable 'i' set but not used [-Wunused-but-set-variable]
905 | int err = 0, i, blen;
| ^
vim +/i +905 fs/ubifs/file.c
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 902
4f6ac0962f61cc Matthew Wilcox (Oracle 2023-06-05 903) static int do_writepage(struct folio *folio, size_t len)
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 904 {
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 @905 int err = 0, i, blen;
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 906 unsigned int block;
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 907 void *addr;
4f6ac0962f61cc Matthew Wilcox (Oracle 2023-06-05 908) size_t offset = 0;
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 909 union ubifs_key key;
4f6ac0962f61cc Matthew Wilcox (Oracle 2023-06-05 910) struct inode *inode = folio->mapping->host;
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 911 struct ubifs_info *c = inode->i_sb->s_fs_info;
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 912
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] ubifs: Convert do_writepage() to take a folio
2023-06-05 16:50 ` [PATCH 4/4] ubifs: Convert do_writepage() to take a folio Matthew Wilcox (Oracle)
2023-06-05 19:28 ` kernel test robot
@ 2023-06-05 21:37 ` Richard Weinberger
2023-06-06 3:22 ` Matthew Wilcox
2023-06-05 23:29 ` kernel test robot
2023-06-08 15:09 ` Zhihao Cheng
3 siblings, 1 reply; 18+ messages in thread
From: Richard Weinberger @ 2023-06-05 21:37 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-mtd, linux-fsdevel
Matthew,
----- Ursprüngliche Mail -----
> Von: "Matthew Wilcox" <willy@infradead.org>
> -static int do_writepage(struct page *page, int len)
> +static int do_writepage(struct folio *folio, size_t len)
> {
> int err = 0, i, blen;
> unsigned int block;
> void *addr;
> + size_t offset = 0;
> union ubifs_key key;
> - struct inode *inode = page->mapping->host;
> + struct inode *inode = folio->mapping->host;
> struct ubifs_info *c = inode->i_sb->s_fs_info;
>
> #ifdef UBIFS_DEBUG
> struct ubifs_inode *ui = ubifs_inode(inode);
> spin_lock(&ui->ui_lock);
> - ubifs_assert(c, page->index <= ui->synced_i_size >> PAGE_SHIFT);
> + ubifs_assert(c, folio->index <= ui->synced_i_size >> PAGE_SHIFT);
> spin_unlock(&ui->ui_lock);
> #endif
>
> - /* Update radix tree tags */
> - set_page_writeback(page);
> + folio_start_writeback(folio);
>
> - addr = kmap(page);
> - block = page->index << UBIFS_BLOCKS_PER_PAGE_SHIFT;
> + addr = kmap_local_folio(folio, offset);
> + block = folio->index << UBIFS_BLOCKS_PER_PAGE_SHIFT;
> i = 0;
> - while (len) {
> - blen = min_t(int, len, UBIFS_BLOCK_SIZE);
> + for (;;) {
This change will cause a file system corruption.
If len is zero (it can be) then a zero length data node will be written.
The while(len) made sure that upon zero length nothing is written.
Thanks,
//richard
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] ubifs: Convert do_writepage() to take a folio
2023-06-05 21:37 ` Richard Weinberger
@ 2023-06-06 3:22 ` Matthew Wilcox
2023-06-06 6:13 ` Richard Weinberger
0 siblings, 1 reply; 18+ messages in thread
From: Matthew Wilcox @ 2023-06-06 3:22 UTC (permalink / raw)
To: Richard Weinberger; +Cc: linux-mtd, linux-fsdevel
On Mon, Jun 05, 2023 at 11:37:00PM +0200, Richard Weinberger wrote:
> > - addr = kmap(page);
> > - block = page->index << UBIFS_BLOCKS_PER_PAGE_SHIFT;
> > + addr = kmap_local_folio(folio, offset);
> > + block = folio->index << UBIFS_BLOCKS_PER_PAGE_SHIFT;
> > i = 0;
> > - while (len) {
> > - blen = min_t(int, len, UBIFS_BLOCK_SIZE);
> > + for (;;) {
>
> This change will cause a file system corruption.
> If len is zero (it can be) then a zero length data node will be written.
> The while(len) made sure that upon zero length nothing is written.
I don't see how 'len' can be 0. len is modified each time around the
loop, and if it's decremented to 0, we break. So you must be referring
to a case where the caller of do_writepage passes 0.
There are three callers of do_writepage, two in ubifs_writepage():
int err, len = folio_size(folio);
...
if (folio_pos(folio) + len < i_size) {
...
return do_writepage(folio, len);
len is folio_size(), which is not 0.
len = offset_in_folio(folio, i_size);
Here, we know that len is not 0. We already tested earlier:
if (folio_pos(folio) >= i_size) {
so we know that i_size > folio_pos() and i_size < folio_pos() +
folio_size(). Actually, I should make this more explicit:
len = i_size - folio_pos(folio);
Now it should be clear that len cannot be zero.
The third caller is do_truncation():
loff_t old_size = inode->i_size, new_size = attr->ia_size;
int offset = new_size & (UBIFS_BLOCK_SIZE - 1), budgeted = 1;
if (offset) {
pgoff_t index = new_size >> PAGE_SHIFT;
offset = offset_in_folio(folio,
new_size);
err = do_writepage(folio, offset);
It's not large-folio-safe, but it's definitely not 0.
Did I miss something?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] ubifs: Convert do_writepage() to take a folio
2023-06-06 3:22 ` Matthew Wilcox
@ 2023-06-06 6:13 ` Richard Weinberger
2023-06-06 12:32 ` Matthew Wilcox
0 siblings, 1 reply; 18+ messages in thread
From: Richard Weinberger @ 2023-06-06 6:13 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-mtd, linux-fsdevel
Matthew,
----- Ursprüngliche Mail -----
> Von: "Matthew Wilcox" <willy@infradead.org>
> len is folio_size(), which is not 0.
>
> len = offset_in_folio(folio, i_size);
offset_in_folio(folio, i_size) can give 0.
Further it will call do_writepage() with len being 0.
I can actually trigger this case.
By adding the following ubifs_assert() I can catch the write side.
If the file length is a multiple of PAGE_SIZE (4k), it will trigger.
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 67cf5138ccc48..dc39ea368ca2b 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1059,6 +1059,8 @@ static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
folio_zero_segment(folio, offset_in_folio(folio, i_size), len);
len = offset_in_folio(folio, i_size);
+ ubifs_assert(c, len > 0);
+
if (i_size > synced_i_size) {
err = inode->i_sb->s_op->write_inode(inode, NULL);
if (err)
[ 44.569110] UBIFS error (ubi0:0 pid 59): ubifs_assert_failed: UBIFS assert failed: len > 0, in fs/ubifs/file.c:1062
[ 44.571359] UBIFS warning (ubi0:0 pid 59): ubifs_ro_mode.part.6: switched to read-only mode, error -22
[ 44.572998] CPU: 1 PID: 59 Comm: kworker/u8:2 Not tainted 6.4.0-rc5-00004-gd504b815b71c-dirty #19
[ 44.574139] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b-rebuilt.opensuse.org 04/01/2014
[ 44.574141] Workqueue: writeback wb_workfn (flush-ubifs_0_0)
[ 44.574148] Call Trace:
[ 44.574162] <TASK>
[ 44.574165] dump_stack_lvl+0x32/0x50
[ 44.574172] ubifs_writepage+0x25a/0x270
[ 44.578096] write_cache_pages+0x132/0x3a0
[ 44.578103] ? __pfx_ubifs_writepage+0x10/0x10
[ 44.578107] ? virtqueue_add_sgs+0x7b/0x90
[ 44.578113] do_writepages+0xd3/0x1a0
[ 44.578116] ? kvm_clock_read+0x14/0x30
[ 44.578121] ? kvm_sched_clock_read+0x5/0x20
[ 44.578125] __writeback_single_inode+0x3c/0x350
[ 44.578128] writeback_sb_inodes+0x1c9/0x460
[ 44.578133] __writeback_inodes_wb+0x5a/0xc0
[ 44.582508] wb_writeback+0x230/0x2c0
[ 44.582513] wb_workfn+0x301/0x430
[ 44.582515] ? kvm_clock_read+0x14/0x30
[ 44.582519] ? kvm_sched_clock_read+0x5/0x20
[ 44.582523] ? sched_clock_cpu+0xd/0x190
[ 44.582527] ? __smp_call_single_queue+0xa1/0x110
[ 44.582532] process_one_work+0x1f3/0x3f0
[ 44.582538] worker_thread+0x25/0x3b0
[ 44.586196] ? __pfx_worker_thread+0x10/0x10
[ 44.586201] kthread+0xde/0x110
[ 44.586204] ? __pfx_kthread+0x10/0x10
[ 44.586207] ret_from_fork+0x2c/0x50
[ 44.586212] </TASK>
Thanks,
//richard
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] ubifs: Convert do_writepage() to take a folio
2023-06-06 6:13 ` Richard Weinberger
@ 2023-06-06 12:32 ` Matthew Wilcox
2023-06-06 13:41 ` Richard Weinberger
0 siblings, 1 reply; 18+ messages in thread
From: Matthew Wilcox @ 2023-06-06 12:32 UTC (permalink / raw)
To: Richard Weinberger; +Cc: linux-mtd, linux-fsdevel
On Tue, Jun 06, 2023 at 08:13:55AM +0200, Richard Weinberger wrote:
> Matthew,
>
> ----- Ursprüngliche Mail -----
> > Von: "Matthew Wilcox" <willy@infradead.org>
> > len is folio_size(), which is not 0.
> >
> > len = offset_in_folio(folio, i_size);
>
> offset_in_folio(folio, i_size) can give 0.
Oh! There is a bug, because it shouldn't get here!
/* Is the folio fully inside i_size? */
if (folio_pos(folio) + len < i_size) {
should be:
/* Is the folio fully inside i_size? */
if (folio_pos(folio) + len <= i_size) {
right? Consider a file with i_size 4096. its single-page folio will
have a pos of 0 and a length of 4096. so it should be written back by
the first call to do_writepage(), not the case where the folio straddles
i_size.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] ubifs: Convert do_writepage() to take a folio
2023-06-06 12:32 ` Matthew Wilcox
@ 2023-06-06 13:41 ` Richard Weinberger
0 siblings, 0 replies; 18+ messages in thread
From: Richard Weinberger @ 2023-06-06 13:41 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-mtd, linux-fsdevel
----- Ursprüngliche Mail -----
> Von: "Matthew Wilcox" <willy@infradead.org>
> On Tue, Jun 06, 2023 at 08:13:55AM +0200, Richard Weinberger wrote:
>> Matthew,
>>
>> ----- Ursprüngliche Mail -----
>> > Von: "Matthew Wilcox" <willy@infradead.org>
>> > len is folio_size(), which is not 0.
>> >
>> > len = offset_in_folio(folio, i_size);
>>
>> offset_in_folio(folio, i_size) can give 0.
>
> Oh! There is a bug, because it shouldn't get here!
>
> /* Is the folio fully inside i_size? */
> if (folio_pos(folio) + len < i_size) {
>
> should be:
>
> /* Is the folio fully inside i_size? */
> if (folio_pos(folio) + len <= i_size) {
>
> right? Consider a file with i_size 4096. its single-page folio will
> have a pos of 0 and a length of 4096. so it should be written back by
> the first call to do_writepage(), not the case where the folio straddles
> i_size.
Indeed.
With that change I agree that do_writepage() cannot get called with zero len.
I'll run more tests, so far all is nice an shiny. :-)
Thanks,
//richard
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] ubifs: Convert do_writepage() to take a folio
2023-06-05 16:50 ` [PATCH 4/4] ubifs: Convert do_writepage() to take a folio Matthew Wilcox (Oracle)
2023-06-05 19:28 ` kernel test robot
2023-06-05 21:37 ` Richard Weinberger
@ 2023-06-05 23:29 ` kernel test robot
2023-06-08 15:09 ` Zhihao Cheng
3 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2023-06-05 23:29 UTC (permalink / raw)
To: Matthew Wilcox (Oracle), Richard Weinberger
Cc: llvm, oe-kbuild-all, Matthew Wilcox (Oracle), linux-mtd,
linux-fsdevel
Hi Matthew,
kernel test robot noticed the following build warnings:
[auto build test WARNING on rw-ubifs/next]
[also build test WARNING on linus/master v6.4-rc5 next-20230605]
[cannot apply to rw-ubifs/fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Matthew-Wilcox-Oracle/ubifs-Convert-from-writepage-to-writepages/20230606-005309
base: https://git.kernel.org/pub/scm/linux/kernel/git/rw/ubifs.git next
patch link: https://lore.kernel.org/r/20230605165029.2908304-5-willy%40infradead.org
patch subject: [PATCH 4/4] ubifs: Convert do_writepage() to take a folio
config: i386-randconfig-i001-20230605 (https://download.01.org/0day-ci/archive/20230606/202306060727.jUL92Co4-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4f6ac0962f61cc07c95177f78cf1f3b03e79d822
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Matthew-Wilcox-Oracle/ubifs-Convert-from-writepage-to-writepages/20230606-005309
git checkout 4f6ac0962f61cc07c95177f78cf1f3b03e79d822
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/ubifs/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306060727.jUL92Co4-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> fs/ubifs/file.c:905:15: warning: variable 'i' set but not used [-Wunused-but-set-variable]
int err = 0, i, blen;
^
1 warning generated.
vim +/i +905 fs/ubifs/file.c
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 902
4f6ac0962f61cc Matthew Wilcox (Oracle 2023-06-05 903) static int do_writepage(struct folio *folio, size_t len)
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 904 {
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 @905 int err = 0, i, blen;
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 906 unsigned int block;
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 907 void *addr;
4f6ac0962f61cc Matthew Wilcox (Oracle 2023-06-05 908) size_t offset = 0;
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 909 union ubifs_key key;
4f6ac0962f61cc Matthew Wilcox (Oracle 2023-06-05 910) struct inode *inode = folio->mapping->host;
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 911 struct ubifs_info *c = inode->i_sb->s_fs_info;
1e51764a3c2ac0 Artem Bityutskiy 2008-07-14 912
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] ubifs: Convert do_writepage() to take a folio
2023-06-05 16:50 ` [PATCH 4/4] ubifs: Convert do_writepage() to take a folio Matthew Wilcox (Oracle)
` (2 preceding siblings ...)
2023-06-05 23:29 ` kernel test robot
@ 2023-06-08 15:09 ` Zhihao Cheng
3 siblings, 0 replies; 18+ messages in thread
From: Zhihao Cheng @ 2023-06-08 15:09 UTC (permalink / raw)
To: Matthew Wilcox (Oracle), Richard Weinberger; +Cc: linux-mtd, linux-fsdevel
在 2023/6/6 0:50, Matthew Wilcox (Oracle) 写道:
Hi
> Replace the call to SetPageError() with a call to mapping_set_error().
> Support large folios by using kmap_local_folio() and remapping each time
> we cross a page boundary. Saves a lot of hidden calls to compound_head().
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> fs/ubifs/file.c | 53 +++++++++++++++++++++++++++----------------------
> 1 file changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index c0e68b3d7582..1b2055d5ec5f 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -900,60 +900,65 @@ static int ubifs_read_folio(struct file *file, struct folio *folio)
> return 0;
> }
>
> -static int do_writepage(struct page *page, int len)
> +static int do_writepage(struct folio *folio, size_t len)
> {
> int err = 0, i, blen;
> unsigned int block;
> void *addr;
> + size_t offset = 0;
> union ubifs_key key;
> - struct inode *inode = page->mapping->host;
> + struct inode *inode = folio->mapping->host;
> struct ubifs_info *c = inode->i_sb->s_fs_info;
>
> #ifdef UBIFS_DEBUG
> struct ubifs_inode *ui = ubifs_inode(inode);
> spin_lock(&ui->ui_lock);
> - ubifs_assert(c, page->index <= ui->synced_i_size >> PAGE_SHIFT);
> + ubifs_assert(c, folio->index <= ui->synced_i_size >> PAGE_SHIFT);
> spin_unlock(&ui->ui_lock);
> #endif
>
> - /* Update radix tree tags */
> - set_page_writeback(page);
> + folio_start_writeback(folio);
>
> - addr = kmap(page);
> - block = page->index << UBIFS_BLOCKS_PER_PAGE_SHIFT;
> + addr = kmap_local_folio(folio, offset);
> + block = folio->index << UBIFS_BLOCKS_PER_PAGE_SHIFT;
> i = 0;
> - while (len) {
> - blen = min_t(int, len, UBIFS_BLOCK_SIZE);
> + for (;;) {
> + blen = min_t(size_t, len, UBIFS_BLOCK_SIZE);
> data_key_init(c, &key, inode->i_ino, block);
> err = ubifs_jnl_write_data(c, inode, &key, addr, blen);
> if (err)
> break;
> - if (++i >= UBIFS_BLOCKS_PER_PAGE)
> + len -= blen;
> + if (!len)
> break;
> block += 1;
> addr += blen;
> - len -= blen;
> + if (folio_test_highmem(folio) && !offset_in_page(addr)) {
> + kunmap_local(addr - blen);
> + offset += PAGE_SIZE;
> + addr = kmap_local_folio(folio, offset);
I'm not sure whether it is a problem here, if we have a 64K PAGE_SIZE
environment, UBIFS_BLOCK_SIZE is 4K. Given len is 64K, after one
iteration, we might enter into this branch, ubifs has written 4K-size
data, and offset becomes 64K, ubifs will write from page pos 64K rather
4K in second iteration?
> + } > }
> + kunmap_local(addr);
> if (err) {
> - SetPageError(page);
> - ubifs_err(c, "cannot write page %lu of inode %lu, error %d",
> - page->index, inode->i_ino, err);
> + mapping_set_error(folio->mapping, err);
I rhink we can add mapping_set_error in ubifs_writepage's error
branch(eg, ->write_inode), just like I comment in patch 1.
> + ubifs_err(c, "cannot write folio %lu of inode %lu, error %d",
> + folio->index, inode->i_ino, err);
> ubifs_ro_mode(c, err);
> }
>
> - ubifs_assert(c, PagePrivate(page));
> - if (PageChecked(page))
> + ubifs_assert(c, folio->private != NULL);
> + if (folio_test_checked(folio))
> release_new_page_budget(c);
> else
> release_existing_page_budget(c);
>
> atomic_long_dec(&c->dirty_pg_cnt);
> - detach_page_private(page);
> - ClearPageChecked(page);
> + folio_detach_private(folio);
> + folio_clear_checked(folio);
>
> - kunmap(page);
> - unlock_page(page);
> - end_page_writeback(page);
> + folio_unlock(folio);
> + folio_end_writeback(folio);
> return err;
> }
>
> @@ -1041,7 +1046,7 @@ static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
> * with this.
> */
> }
> - return do_writepage(&folio->page, len);
> + return do_writepage(folio, len);
> }
>
> /*
> @@ -1060,7 +1065,7 @@ static int ubifs_writepage(struct folio *folio, struct writeback_control *wbc,
> goto out_redirty;
> }
>
> - return do_writepage(&folio->page, len);
> + return do_writepage(folio, len);
> out_redirty:
> /*
> * folio_redirty_for_writepage() won't call ubifs_dirty_inode() because
> @@ -1172,7 +1177,7 @@ static int do_truncation(struct ubifs_info *c, struct inode *inode,
> if (UBIFS_BLOCKS_PER_PAGE_SHIFT)
> offset = offset_in_folio(folio,
> new_size);
> - err = do_writepage(&folio->page, offset);
> + err = do_writepage(folio, offset);
> folio_put(folio);
> if (err)
> goto out_budg;
>
^ permalink raw reply [flat|nested] 18+ messages in thread