Re: [PATCH 2/2] erofs: decompress in endio if possible
Hi Huang,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on xiang-erofs/dev-test]
[cannot apply to v5.12-rc1 next-20210305]
[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]
url:
https://github.com/0day-ci/linux/commits/Huang-Jianan/erofs-avoid-memory-allocation-failure-during-rolling-decompression/20210305-142329
base: https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs.git dev-test
config: parisc-randconfig-r012-20210305 (attached as .config)
compiler: hppa64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/ecb1173ba28392f322cbb4c9cb1a66524b10dd78
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Huang-Jianan/erofs-avoid-memory-allocation-failure-during-rolling-decompression/20210305-142329
git checkout ecb1173ba28392f322cbb4c9cb1a66524b10dd78
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
fs/erofs/super.c: In function 'erofs_read_superblock':
>> fs/erofs/super.c:191:5: error: 'struct erofs_sb_info' has no member named
>> 'sync_decompress'
191 | sbi->sync_decompress = false;
| ^~
vim +191 fs/erofs/super.c
124
125 static int erofs_read_superblock(struct super_block *sb)
126 {
127 struct erofs_sb_info *sbi;
128 struct page *page;
129 struct erofs_super_block *dsb;
130 unsigned int blkszbits;
131 void *data;
132 int ret;
133
134 page = read_mapping_page(sb->s_bdev->bd_inode->i_mapping, 0,
NULL);
135 if (IS_ERR(page)) {
136 erofs_err(sb, "cannot read erofs superblock");
137 return PTR_ERR(page);
138 }
139
140 sbi = EROFS_SB(sb);
141
142 data = kmap(page);
143 dsb = (struct erofs_super_block *)(data + EROFS_SUPER_OFFSET);
144
145 ret = -EINVAL;
146 if (le32_to_cpu(dsb->magic) != EROFS_SUPER_MAGIC_V1) {
147 erofs_err(sb, "cannot find valid erofs superblock");
148 goto out;
149 }
150
151 sbi->feature_compat = le32_to_cpu(dsb->feature_compat);
152 if (sbi->feature_compat & EROFS_FEATURE_COMPAT_SB_CHKSUM) {
153 ret = erofs_superblock_csum_verify(sb, data);
154 if (ret)
155 goto out;
156 }
157
158 blkszbits = dsb->blkszbits;
159 /* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */
160 if (blkszbits != LOG_BLOCK_SIZE) {
161 erofs_err(sb, "blkszbits %u isn't supported on this
platform",
162blkszbits);
163 goto out;
164 }
165
166 if (!check_layout_compatibility(sb, dsb))
167 goto out;
168
169 sbi->blocks = le32_to_cpu(dsb->blocks);
170 sbi->meta_blkaddr = le32_to_cpu(dsb->meta_blkaddr);
171 #ifdef CONFIG_EROFS_FS_XATTR
172 sbi->xattr_blkaddr = le32_to_cpu(dsb->xattr_blkaddr);
173 #endif
174 sbi->islotbits = ilog2(sizeof(struct erofs_inode_compact));
175 sbi->root_nid = le16_to_cpu(dsb->root_nid);
176 sbi->inos = le64_to_cpu(dsb->inos);
177
178 sbi->build_time = le64_to_cpu(dsb->build_time);
179 sbi->build_time_nsec = le32_to_cpu(dsb->build_time_nsec);
180
181 memcpy(>s_uuid, dsb->uuid, sizeof(dsb->uuid));
182
183 ret = strscpy(sbi->volume_name, dsb->volume_name,
184sizeof(dsb->volume_name));
185 if (ret < 0) { /* -E2BIG */
186 erofs_err(sb, "bad volume name without NIL terminator");
187 ret = -EFSCORRUPTED;
188 goto out;
189 }
190
> 191 sbi->sync_decompress = false;
192 /* parse on-disk compression configurations */
193 z_erofs_load_lz4_config(sbi, dsb);
194 ret = 0;
195 out:
196 kunmap(page);
197 put_page(page);
198 return ret;
199 }
200
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH 2/2] erofs: decompress in endio if possible
On Fri, Mar 05, 2021 at 02:22:19PM +0800, Huang Jianan via Linux-erofs wrote:
> z_erofs_decompressqueue_endio may not be executed in the interrupt
> context, for example, when dm-verity is turned on. In this scenario,
> io should be decompressed directly to avoid additional scheduling
> overhead. Also there is no need to wait for endio to execute
> synchronous decompression.
z_erofs_decompressqueue_endio may not be executed in the atomic
context, for example, when dm-verity is turned on. In this scenario,
data can be decompressed directly to get rid of additional kworker
scheduling overhead. Also, it makes no sense to apply synchronous
decompression for such case.
>
> Signed-off-by: Huang Jianan
> Signed-off-by: Guo Weichao
> ---
> fs/erofs/internal.h | 3 ++
> fs/erofs/super.c| 1 +
> fs/erofs/zdata.c| 102
> 3 files changed, 60 insertions(+), 46 deletions(-)
>
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 77965490dced..a19bcbb681fc 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -71,6 +71,9 @@ struct erofs_sb_info {
> /* pseudo inode to manage cached pages */
> struct inode *managed_cache;
>
> + /* decide whether to decompress synchronously */
> + bool sync_decompress;
bool readahead_sync_decompress;
> +
> /* # of pages needed for EROFS lz4 rolling decompression */
> u16 lz4_max_distance_pages;
> #endif /* CONFIG_EROFS_FS_ZIP */
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 37f1cc9d28cc..5b9a21d10a30 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -188,6 +188,7 @@ static int erofs_read_superblock(struct super_block *sb)
> goto out;
> }
>
> + sbi->sync_decompress = false;
Ah, could you rebase the patch on the top of 5.12-rc1
rather than dev-test? since I've fold your
"erofs: support adjust lz4 history window size"
into a new patchset, see:
https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/log/?h=erofs/compr_cfgs
> /* parse on-disk compression configurations */
> z_erofs_load_lz4_config(sbi, dsb);
> ret = 0;
> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> index 6cb356c4217b..727dd01f55c1 100644
> --- a/fs/erofs/zdata.c
> +++ b/fs/erofs/zdata.c
> @@ -706,56 +706,11 @@ static int z_erofs_do_read_page(struct
> z_erofs_decompress_frontend *fe,
> goto out;
> }
>
> -static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
> -bool sync, int bios)
> -{
> - /* wake up the caller thread for sync decompression */
> - if (sync) {
> - unsigned long flags;
> -
> - spin_lock_irqsave(>u.wait.lock, flags);
> - if (!atomic_add_return(bios, >pending_bios))
> - wake_up_locked(>u.wait);
> - spin_unlock_irqrestore(>u.wait.lock, flags);
> - return;
> - }
> -
> - if (!atomic_add_return(bios, >pending_bios))
> - queue_work(z_erofs_workqueue, >u.work);
> -}
Is it necessary to move the code snippet?
> -
> 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);
> - struct z_erofs_decompressqueue *q = tagptr_unfold_ptr(t);
> - blk_status_t err = bio->bi_status;
> - struct bio_vec *bvec;
> - struct bvec_iter_all iter_all;
> -
> - bio_for_each_segment_all(bvec, bio, iter_all) {
> - struct page *page = bvec->bv_page;
> -
> - DBG_BUGON(PageUptodate(page));
> - DBG_BUGON(z_erofs_page_is_invalidated(page));
> -
> - if (err)
> - SetPageError(page);
> -
> - if (erofs_page_is_managed(EROFS_SB(q->sb), page)) {
> - if (!err)
> - SetPageUptodate(page);
> - unlock_page(page);
> - }
> - }
> - z_erofs_decompress_kickoff(q, tagptr_unfold_tags(t), -1);
> - bio_put(bio);
> -}
> -
> static int z_erofs_decompress_pcluster(struct super_block *sb,
> struct z_erofs_pcluster *pcl,
> struct list_head *pagepool)
> @@ -991,6 +946,60 @@ static void z_erofs_decompressqueue_work(struct
> work_struct *work)
> kvfree(bgq);
> }
>
> +static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
> +bool sync, int bios)
> +{
> + struct erofs_sb_info *const sbi = EROFS_SB(io->sb);
> +
> + /* wake up the caller thread for sync decompression */
> + if (sync) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(>u.wait.lock, flags);
> + if (!atomic_add_return(bios, >pending_bios))
> +
[PATCH 2/2] erofs: decompress in endio if possible
z_erofs_decompressqueue_endio may not be executed in the interrupt
context, for example, when dm-verity is turned on. In this scenario,
io should be decompressed directly to avoid additional scheduling
overhead. Also there is no need to wait for endio to execute
synchronous decompression.
Signed-off-by: Huang Jianan
Signed-off-by: Guo Weichao
---
fs/erofs/internal.h | 3 ++
fs/erofs/super.c| 1 +
fs/erofs/zdata.c| 102
3 files changed, 60 insertions(+), 46 deletions(-)
diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 77965490dced..a19bcbb681fc 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -71,6 +71,9 @@ struct erofs_sb_info {
/* pseudo inode to manage cached pages */
struct inode *managed_cache;
+ /* decide whether to decompress synchronously */
+ bool sync_decompress;
+
/* # of pages needed for EROFS lz4 rolling decompression */
u16 lz4_max_distance_pages;
#endif /* CONFIG_EROFS_FS_ZIP */
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 37f1cc9d28cc..5b9a21d10a30 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -188,6 +188,7 @@ static int erofs_read_superblock(struct super_block *sb)
goto out;
}
+ sbi->sync_decompress = false;
/* parse on-disk compression configurations */
z_erofs_load_lz4_config(sbi, dsb);
ret = 0;
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 6cb356c4217b..727dd01f55c1 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -706,56 +706,11 @@ static int z_erofs_do_read_page(struct
z_erofs_decompress_frontend *fe,
goto out;
}
-static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
- bool sync, int bios)
-{
- /* wake up the caller thread for sync decompression */
- if (sync) {
- unsigned long flags;
-
- spin_lock_irqsave(>u.wait.lock, flags);
- if (!atomic_add_return(bios, >pending_bios))
- wake_up_locked(>u.wait);
- spin_unlock_irqrestore(>u.wait.lock, flags);
- return;
- }
-
- if (!atomic_add_return(bios, >pending_bios))
- queue_work(z_erofs_workqueue, >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);
- struct z_erofs_decompressqueue *q = tagptr_unfold_ptr(t);
- blk_status_t err = bio->bi_status;
- struct bio_vec *bvec;
- struct bvec_iter_all iter_all;
-
- bio_for_each_segment_all(bvec, bio, iter_all) {
- struct page *page = bvec->bv_page;
-
- DBG_BUGON(PageUptodate(page));
- DBG_BUGON(z_erofs_page_is_invalidated(page));
-
- if (err)
- SetPageError(page);
-
- if (erofs_page_is_managed(EROFS_SB(q->sb), page)) {
- if (!err)
- SetPageUptodate(page);
- unlock_page(page);
- }
- }
- z_erofs_decompress_kickoff(q, tagptr_unfold_tags(t), -1);
- bio_put(bio);
-}
-
static int z_erofs_decompress_pcluster(struct super_block *sb,
struct z_erofs_pcluster *pcl,
struct list_head *pagepool)
@@ -991,6 +946,60 @@ static void z_erofs_decompressqueue_work(struct
work_struct *work)
kvfree(bgq);
}
+static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
+ bool sync, int bios)
+{
+ struct erofs_sb_info *const sbi = EROFS_SB(io->sb);
+
+ /* wake up the caller thread for sync decompression */
+ if (sync) {
+ unsigned long flags;
+
+ spin_lock_irqsave(>u.wait.lock, flags);
+ if (!atomic_add_return(bios, >pending_bios))
+ wake_up_locked(>u.wait);
+ spin_unlock_irqrestore(>u.wait.lock, flags);
+ return;
+ }
+
+ if (!atomic_add_return(bios, >pending_bios)) {
+ if (in_atomic() || irqs_disabled()) {
+ queue_work(z_erofs_workqueue, >u.work);
+ if (unlikely(!sbi->sync_decompress))
+ sbi->sync_decompress = true;
+ }
+ else
+ z_erofs_decompressqueue_work(>u.work);
+ }
+}
+
+static void z_erofs_decompressqueue_endio(struct bio *bio)
+{
+ tagptr1_t t = tagptr_init(tagptr1_t, bio->bi_private);
+ struct z_erofs_decompressqueue *q = tagptr_unfold_ptr(t);
+ blk_status_t err = bio->bi_status;
+ struct bio_vec *bvec;
+ struct bvec_iter_all
