Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
Hi Jaegeuk,
On 2019-12-11 9:27, Jaegeuk Kim wrote:
Hi Chao,
Let me know, if it's okay to integrate compression patch all together.
I don't have a critical bug to fix w/ them now.
Cool, let me send a new RFC with below fix applied.
Thanks,
Another fix:
---
fs/f2fs/compress.c | 101 -
fs/f2fs/data.c | 15 ---
fs/f2fs/f2fs.h | 1 -
3 files changed, 72 insertions(+), 45 deletions(-)
diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index 7ebd2bc018bd..af23ed6deffd 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -73,6 +73,17 @@ static void f2fs_put_compressed_page(struct page *page)
put_page(page);
}
+static void f2fs_put_rpages(struct compress_ctx *cc)
+{
+ unsigned int i;
+
+ for (i = 0; i < cc->cluster_size; i++) {
+ if (!cc->rpages[i])
+ continue;
+ put_page(cc->rpages[i]);
+ }
+}
+
struct page *f2fs_compress_control_page(struct page *page)
{
return ((struct compress_io_ctx *)page_private(page))->rpages[0];
@@ -93,7 +104,10 @@ int f2fs_init_compress_ctx(struct compress_ctx *cc)
void f2fs_destroy_compress_ctx(struct compress_ctx *cc)
{
kfree(cc->rpages);
- f2fs_reset_compress_ctx(cc);
+ cc->rpages = NULL;
+ cc->nr_rpages = 0;
+ cc->nr_cpages = 0;
+ cc->cluster_idx = NULL_CLUSTER;
}
void f2fs_compress_ctx_add_page(struct compress_ctx *cc, struct page *page)
@@ -536,14 +550,6 @@ static bool cluster_may_compress(struct compress_ctx *cc)
return __cluster_may_compress(cc);
}
-void f2fs_reset_compress_ctx(struct compress_ctx *cc)
-{
- cc->rpages = NULL;
- cc->nr_rpages = 0;
- cc->nr_cpages = 0;
- cc->cluster_idx = NULL_CLUSTER;
-}
-
static void set_cluster_writeback(struct compress_ctx *cc)
{
int i;
@@ -602,13 +608,13 @@ static int prepare_compress_overwrite(struct compress_ctx
*cc,
ret = f2fs_read_multi_pages(cc, , cc->cluster_size,
_block_in_bio, false);
if (ret)
- return ret;
+ goto release_pages;
if (bio)
f2fs_submit_bio(sbi, bio, DATA);
ret = f2fs_init_compress_ctx(cc);
if (ret)
- return ret;
+ goto release_pages;
}
for (i = 0; i < cc->cluster_size; i++) {
@@ -638,9 +644,11 @@ static int prepare_compress_overwrite(struct compress_ctx
*cc,
for (i = cc->cluster_size - 1; i > 0; i--) {
ret = f2fs_get_block(, start_idx + i);
- if (ret)
+ if (ret) {
/* TODO: release preallocate blocks */
- goto release_pages;
+ i = cc->cluster_size;
+ goto unlock_pages;
+ }
if (dn.data_blkaddr != NEW_ADDR)
break;
@@ -769,7 +777,11 @@ static int f2fs_write_compressed_pages(struct compress_ctx
*cc,
cic->magic = F2FS_COMPRESSED_PAGE_MAGIC;
cic->inode = inode;
refcount_set(>ref, 1);
- cic->rpages = cc->rpages;
+ cic->rpages = f2fs_kzalloc(sbi, sizeof(struct page *) <<
+ cc->log_cluster_size, GFP_NOFS);
+ if (!cic->rpages)
+ goto out_put_cic;
+
cic->nr_rpages = cc->cluster_size;
for (i = 0; i < cc->nr_cpages; i++) {
@@ -793,7 +805,7 @@ static int f2fs_write_compressed_pages(struct compress_ctx
*cc,
blkaddr = datablock_addr(dn.inode, dn.node_page,
dn.ofs_in_node);
- fio.page = cc->rpages[i];
+ fio.page = cic->rpages[i] = cc->rpages[i];
fio.old_blkaddr = blkaddr;
/* cluster header */
@@ -819,7 +831,6 @@ static int f2fs_write_compressed_pages(struct compress_ctx
*cc,
f2fs_bug_on(fio.sbi, blkaddr == NULL_ADDR);
-
if (fio.encrypted)
fio.encrypted_page = cc->cpages[i - 1];
else if (fio.compressed)
@@ -859,17 +870,22 @@ static int f2fs_write_compressed_pages(struct
compress_ctx *cc,
fi->last_disk_size = psize;
up_write(>i_sem);
}
- f2fs_reset_compress_ctx(cc);
+ f2fs_put_rpages(cc);
+ f2fs_destroy_compress_ctx(cc);
return 0;
out_destroy_crypt:
- for (i -= 1; i >= 0; i--)
+ kfree(cic->rpages);
+
+ for (--i; i >= 0; i--)
fscrypt_finalize_bounce_page(>cpages[i]);
for (i = 0; i < cc->nr_cpages; i++) {
if (!cc->cpages[i])
continue;
f2fs_put_page(cc->cpages[i], 1);
}
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
Hi Chao,
Let me know, if it's okay to integrate compression patch all together.
I don't have a critical bug to fix w/ them now.
Another fix:
---
fs/f2fs/compress.c | 101 -
fs/f2fs/data.c | 15 ---
fs/f2fs/f2fs.h | 1 -
3 files changed, 72 insertions(+), 45 deletions(-)
diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index 7ebd2bc018bd..af23ed6deffd 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -73,6 +73,17 @@ static void f2fs_put_compressed_page(struct page *page)
put_page(page);
}
+static void f2fs_put_rpages(struct compress_ctx *cc)
+{
+ unsigned int i;
+
+ for (i = 0; i < cc->cluster_size; i++) {
+ if (!cc->rpages[i])
+ continue;
+ put_page(cc->rpages[i]);
+ }
+}
+
struct page *f2fs_compress_control_page(struct page *page)
{
return ((struct compress_io_ctx *)page_private(page))->rpages[0];
@@ -93,7 +104,10 @@ int f2fs_init_compress_ctx(struct compress_ctx *cc)
void f2fs_destroy_compress_ctx(struct compress_ctx *cc)
{
kfree(cc->rpages);
- f2fs_reset_compress_ctx(cc);
+ cc->rpages = NULL;
+ cc->nr_rpages = 0;
+ cc->nr_cpages = 0;
+ cc->cluster_idx = NULL_CLUSTER;
}
void f2fs_compress_ctx_add_page(struct compress_ctx *cc, struct page *page)
@@ -536,14 +550,6 @@ static bool cluster_may_compress(struct compress_ctx *cc)
return __cluster_may_compress(cc);
}
-void f2fs_reset_compress_ctx(struct compress_ctx *cc)
-{
- cc->rpages = NULL;
- cc->nr_rpages = 0;
- cc->nr_cpages = 0;
- cc->cluster_idx = NULL_CLUSTER;
-}
-
static void set_cluster_writeback(struct compress_ctx *cc)
{
int i;
@@ -602,13 +608,13 @@ static int prepare_compress_overwrite(struct compress_ctx
*cc,
ret = f2fs_read_multi_pages(cc, , cc->cluster_size,
_block_in_bio, false);
if (ret)
- return ret;
+ goto release_pages;
if (bio)
f2fs_submit_bio(sbi, bio, DATA);
ret = f2fs_init_compress_ctx(cc);
if (ret)
- return ret;
+ goto release_pages;
}
for (i = 0; i < cc->cluster_size; i++) {
@@ -638,9 +644,11 @@ static int prepare_compress_overwrite(struct compress_ctx
*cc,
for (i = cc->cluster_size - 1; i > 0; i--) {
ret = f2fs_get_block(, start_idx + i);
- if (ret)
+ if (ret) {
/* TODO: release preallocate blocks */
- goto release_pages;
+ i = cc->cluster_size;
+ goto unlock_pages;
+ }
if (dn.data_blkaddr != NEW_ADDR)
break;
@@ -769,7 +777,11 @@ static int f2fs_write_compressed_pages(struct compress_ctx
*cc,
cic->magic = F2FS_COMPRESSED_PAGE_MAGIC;
cic->inode = inode;
refcount_set(>ref, 1);
- cic->rpages = cc->rpages;
+ cic->rpages = f2fs_kzalloc(sbi, sizeof(struct page *) <<
+ cc->log_cluster_size, GFP_NOFS);
+ if (!cic->rpages)
+ goto out_put_cic;
+
cic->nr_rpages = cc->cluster_size;
for (i = 0; i < cc->nr_cpages; i++) {
@@ -793,7 +805,7 @@ static int f2fs_write_compressed_pages(struct compress_ctx
*cc,
blkaddr = datablock_addr(dn.inode, dn.node_page,
dn.ofs_in_node);
- fio.page = cc->rpages[i];
+ fio.page = cic->rpages[i] = cc->rpages[i];
fio.old_blkaddr = blkaddr;
/* cluster header */
@@ -819,7 +831,6 @@ static int f2fs_write_compressed_pages(struct compress_ctx
*cc,
f2fs_bug_on(fio.sbi, blkaddr == NULL_ADDR);
-
if (fio.encrypted)
fio.encrypted_page = cc->cpages[i - 1];
else if (fio.compressed)
@@ -859,17 +870,22 @@ static int f2fs_write_compressed_pages(struct
compress_ctx *cc,
fi->last_disk_size = psize;
up_write(>i_sem);
}
- f2fs_reset_compress_ctx(cc);
+ f2fs_put_rpages(cc);
+ f2fs_destroy_compress_ctx(cc);
return 0;
out_destroy_crypt:
- for (i -= 1; i >= 0; i--)
+ kfree(cic->rpages);
+
+ for (--i; i >= 0; i--)
fscrypt_finalize_bounce_page(>cpages[i]);
for (i = 0; i < cc->nr_cpages; i++) {
if (!cc->cpages[i])
continue;
f2fs_put_page(cc->cpages[i], 1);
}
+out_put_cic:
+ kfree(cic);
out_put_dnode:
f2fs_put_dnode();
out_unlock_op:
@@ -963,37 +979,39 @@
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
Fix having my additional fixes:
---
fs/f2fs/compress.c | 114 ++--
fs/f2fs/data.c | 158 ++---
fs/f2fs/f2fs.h | 29 +++--
fs/f2fs/file.c | 25 +++
fs/f2fs/inode.c| 7 +-
fs/f2fs/namei.c| 7 +-
6 files changed, 208 insertions(+), 132 deletions(-)
diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index e9f633c30942..7ebd2bc018bd 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
#include
#include
@@ -86,15 +87,13 @@ int f2fs_init_compress_ctx(struct compress_ctx *cc)
cc->rpages = f2fs_kzalloc(sbi, sizeof(struct page *) <<
cc->log_cluster_size, GFP_NOFS);
- if (!cc->rpages)
- return -ENOMEM;
- return 0;
+ return cc->rpages ? 0 : -ENOMEM;
}
void f2fs_destroy_compress_ctx(struct compress_ctx *cc)
{
- f2fs_reset_compress_ctx(cc);
kfree(cc->rpages);
+ f2fs_reset_compress_ctx(cc);
}
void f2fs_compress_ctx_add_page(struct compress_ctx *cc, struct page *page)
@@ -378,7 +377,7 @@ void f2fs_decompress_pages(struct bio *bio, struct page
*page, bool verity)
dec_page_count(sbi, F2FS_RD_DATA);
- if (bio->bi_status)
+ if (bio->bi_status || PageError(page))
dic->failed = true;
if (refcount_dec_not_one(>ref))
@@ -420,10 +419,14 @@ void f2fs_decompress_pages(struct bio *bio, struct page
*page, bool verity)
out_vunmap_rbuf:
vunmap(dic->rbuf);
out_free_dic:
- f2fs_set_cluster_uptodate(dic->rpages, dic->cluster_size, ret, verity);
+ if (!verity)
+ f2fs_decompress_end_io(dic->rpages, dic->cluster_size,
+ ret, false);
+
trace_f2fs_decompress_pages_end(dic->inode, dic->cluster_idx,
- dic->clen, ret);
- f2fs_free_dic(dic);
+ dic->clen, ret);
+ if (!verity)
+ f2fs_free_dic(dic);
}
static bool is_page_in_cluster(struct compress_ctx *cc, pgoff_t index)
@@ -470,22 +473,18 @@ static bool __cluster_may_compress(struct compress_ctx
*cc)
/* beyond EOF */
if (page->index >= nr_pages)
return false;
- if (page->index != start_idx_of_cluster(cc) + i)
- return false;
}
return true;
}
-int is_compressed_cluster(struct compress_ctx *cc, pgoff_t index)
+static int is_compressed_cluster(struct compress_ctx *cc)
{
struct dnode_of_data dn;
- unsigned int start_idx = cluster_idx(cc, index) <<
- cc->log_cluster_size;
int ret;
- int i;
set_new_dnode(, cc->inode, NULL, NULL, 0);
- ret = f2fs_get_dnode_of_data(, start_idx, LOOKUP_NODE);
+ ret = f2fs_get_dnode_of_data(, start_idx_of_cluster(cc),
+ LOOKUP_NODE);
if (ret) {
if (ret == -ENOENT)
ret = 0;
@@ -493,6 +492,8 @@ int is_compressed_cluster(struct compress_ctx *cc, pgoff_t
index)
}
if (dn.data_blkaddr == COMPRESS_ADDR) {
+ int i;
+
ret = CLUSTER_IS_FULL;
for (i = 1; i < cc->cluster_size; i++) {
block_t blkaddr;
@@ -516,9 +517,10 @@ int f2fs_is_compressed_cluster(struct inode *inode,
pgoff_t index)
.inode = inode,
.log_cluster_size = F2FS_I(inode)->i_log_cluster_size,
.cluster_size = F2FS_I(inode)->i_cluster_size,
+ .cluster_idx = index >> F2FS_I(inode)->i_log_cluster_size,
};
- return is_compressed_cluster(, index);
+ return is_compressed_cluster();
}
static bool cluster_may_compress(struct compress_ctx *cc)
@@ -536,6 +538,7 @@ static bool cluster_may_compress(struct compress_ctx *cc)
void f2fs_reset_compress_ctx(struct compress_ctx *cc)
{
+ cc->rpages = NULL;
cc->nr_rpages = 0;
cc->nr_cpages = 0;
cc->cluster_idx = NULL_CLUSTER;
@@ -565,19 +568,18 @@ static int prepare_compress_overwrite(struct compress_ctx
*cc,
bool prealloc)
{
struct f2fs_sb_info *sbi = F2FS_I_SB(cc->inode);
- struct bio *bio = NULL;
struct address_space *mapping = cc->inode->i_mapping;
struct page *page;
struct dnode_of_data dn;
sector_t last_block_in_bio;
unsigned fgp_flag = FGP_LOCK | FGP_WRITE | FGP_CREAT;
- unsigned int start_idx = cluster_idx(cc, index) << cc->log_cluster_size;
+ unsigned int start_idx = start_idx_of_cluster(cc);
int i, idx;
int ret;
ret = f2fs_init_compress_ctx(cc);
if (ret)
- goto out;
+
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
On 11/18, Jaegeuk Kim wrote:
> On 11/13, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > I've split workqueue for fsverity, please test compression based on last
> > patch.
> >
> > I shutdown F2FS_FS_COMPRESSION config, it looks all verity testcases can
> > pass, will
> > do more test for compress/encrypt/fsverity combination later.
>
> Thanks, I applied and start some tests.
I modified below to fix wrong compression check in read path.
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -1007,6 +1007,7 @@ struct decompress_io_ctx *f2fs_alloc_dic(struct
compress_ctx *cc)
if (!dic)
return ERR_PTR(-ENOMEM);
+ dic->magic = F2FS_COMPRESSED_PAGE_MAGIC;
dic->inode = cc->inode;
refcount_set(>ref, 1);
dic->cluster_idx = cc->cluster_idx;
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 02a2e7261b457..399ba883632a0 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1255,6 +1255,7 @@ struct compress_io_ctx {
/* decompress io context for read IO path */
struct decompress_io_ctx {
+ u32 magic; /* magic number to indicate page is
compressed */
struct inode *inode;/* inode the context belong to */
unsigned int cluster_idx; /* cluster index number */
unsigned int cluster_size; /* page count in cluster */
>
> >
> > The diff is as below, code base is last g-dev-test branch:
> >
> > >From 5b51682bc3013b8de6dee4906865181c3ded435f Mon Sep 17 00:00:00 2001
> > From: Chao Yu
> > Date: Tue, 12 Nov 2019 10:03:21 +0800
> > Subject: [PATCH INCREMENT] f2fs: compress: split workqueue for fsverity
> >
> > Signed-off-by: Chao Yu
> > ---
> > fs/f2fs/compress.c | 16 +---
> > fs/f2fs/data.c | 94 +++---
> > fs/f2fs/f2fs.h | 2 +-
> > 3 files changed, 84 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> > index f4ce825f12b4..254275325890 100644
> > --- a/fs/f2fs/compress.c
> > +++ b/fs/f2fs/compress.c
> > @@ -377,7 +377,7 @@ void f2fs_decompress_pages(struct bio *bio, struct page
> > *page, bool verity)
> >
> > dec_page_count(sbi, F2FS_RD_DATA);
> >
> > - if (bio->bi_status)
> > + if (bio->bi_status || PageError(page))
> > dic->failed = true;
> >
> > if (refcount_dec_not_one(>ref))
> > @@ -419,10 +419,14 @@ void f2fs_decompress_pages(struct bio *bio, struct
> > page *page, bool verity)
> > out_vunmap_rbuf:
> > vunmap(dic->rbuf);
> > out_free_dic:
> > - f2fs_set_cluster_uptodate(dic->rpages, dic->cluster_size, ret, verity);
> > + if (!verity)
> > + f2fs_decompress_end_io(dic->rpages, dic->cluster_size,
> > + ret, false);
> > +
> > trace_f2fs_decompress_pages_end(dic->inode, dic->cluster_idx,
> > - dic->clen, ret);
> > - f2fs_free_dic(dic);
> > + dic->clen, ret);
> > + if (!verity)
> > + f2fs_free_dic(dic);
> > }
> >
> > static bool is_page_in_cluster(struct compress_ctx *cc, pgoff_t index)
> > @@ -1086,7 +1090,7 @@ void f2fs_free_dic(struct decompress_io_ctx *dic)
> > kfree(dic);
> > }
> >
> > -void f2fs_set_cluster_uptodate(struct page **rpages,
> > +void f2fs_decompress_end_io(struct page **rpages,
> > unsigned int cluster_size, bool err, bool verity)
> > {
> > int i;
> > @@ -1108,4 +1112,4 @@ void f2fs_set_cluster_uptodate(struct page **rpages,
> > }
> > unlock_page(rpage);
> > }
> > -}
> > +}
> > \ No newline at end of file
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index c9362a53f8a1..2d64c6ffee84 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -98,7 +98,7 @@ static void __read_end_io(struct bio *bio, bool compr,
> > bool verity)
> > page = bv->bv_page;
> >
> > #ifdef CONFIG_F2FS_FS_COMPRESSION
> > - if (compr && PagePrivate(page)) {
> > + if (compr && f2fs_is_compressed_page(page)) {
> > f2fs_decompress_pages(bio, page, verity);
> > continue;
> > }
> > @@ -115,9 +115,14 @@ static void __read_end_io(struct bio *bio, bool compr,
> > bool verity)
> > dec_page_count(F2FS_P_SB(page), __read_io_type(page));
> > unlock_page(page);
> > }
> > - if (bio->bi_private)
> > - mempool_free(bio->bi_private, bio_post_read_ctx_pool);
> > - bio_put(bio);
> > +}
> > +
> > +static void f2fs_release_read_bio(struct bio *bio);
> > +static void __f2fs_read_end_io(struct bio *bio, bool compr, bool verity)
> > +{
> > + if (!compr)
> > + __read_end_io(bio, false, verity);
> > + f2fs_release_read_bio(bio);
> > }
> >
> > static void f2fs_decompress_bio(struct bio *bio, bool verity)
> > @@ -127,19 +132,50 @@ static void f2fs_decompress_bio(struct bio *bio, bool
>
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
On 11/13, Chao Yu wrote:
> Hi Jaegeuk,
>
> I've split workqueue for fsverity, please test compression based on last
> patch.
>
> I shutdown F2FS_FS_COMPRESSION config, it looks all verity testcases can
> pass, will
> do more test for compress/encrypt/fsverity combination later.
Thanks, I applied and start some tests.
>
> The diff is as below, code base is last g-dev-test branch:
>
> >From 5b51682bc3013b8de6dee4906865181c3ded435f Mon Sep 17 00:00:00 2001
> From: Chao Yu
> Date: Tue, 12 Nov 2019 10:03:21 +0800
> Subject: [PATCH INCREMENT] f2fs: compress: split workqueue for fsverity
>
> Signed-off-by: Chao Yu
> ---
> fs/f2fs/compress.c | 16 +---
> fs/f2fs/data.c | 94 +++---
> fs/f2fs/f2fs.h | 2 +-
> 3 files changed, 84 insertions(+), 28 deletions(-)
>
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index f4ce825f12b4..254275325890 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -377,7 +377,7 @@ void f2fs_decompress_pages(struct bio *bio, struct page
> *page, bool verity)
>
> dec_page_count(sbi, F2FS_RD_DATA);
>
> - if (bio->bi_status)
> + if (bio->bi_status || PageError(page))
> dic->failed = true;
>
> if (refcount_dec_not_one(>ref))
> @@ -419,10 +419,14 @@ void f2fs_decompress_pages(struct bio *bio, struct page
> *page, bool verity)
> out_vunmap_rbuf:
> vunmap(dic->rbuf);
> out_free_dic:
> - f2fs_set_cluster_uptodate(dic->rpages, dic->cluster_size, ret, verity);
> + if (!verity)
> + f2fs_decompress_end_io(dic->rpages, dic->cluster_size,
> + ret, false);
> +
> trace_f2fs_decompress_pages_end(dic->inode, dic->cluster_idx,
> - dic->clen, ret);
> - f2fs_free_dic(dic);
> + dic->clen, ret);
> + if (!verity)
> + f2fs_free_dic(dic);
> }
>
> static bool is_page_in_cluster(struct compress_ctx *cc, pgoff_t index)
> @@ -1086,7 +1090,7 @@ void f2fs_free_dic(struct decompress_io_ctx *dic)
> kfree(dic);
> }
>
> -void f2fs_set_cluster_uptodate(struct page **rpages,
> +void f2fs_decompress_end_io(struct page **rpages,
> unsigned int cluster_size, bool err, bool verity)
> {
> int i;
> @@ -1108,4 +1112,4 @@ void f2fs_set_cluster_uptodate(struct page **rpages,
> }
> unlock_page(rpage);
> }
> -}
> +}
> \ No newline at end of file
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index c9362a53f8a1..2d64c6ffee84 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -98,7 +98,7 @@ static void __read_end_io(struct bio *bio, bool compr, bool
> verity)
> page = bv->bv_page;
>
> #ifdef CONFIG_F2FS_FS_COMPRESSION
> - if (compr && PagePrivate(page)) {
> + if (compr && f2fs_is_compressed_page(page)) {
> f2fs_decompress_pages(bio, page, verity);
> continue;
> }
> @@ -115,9 +115,14 @@ static void __read_end_io(struct bio *bio, bool compr,
> bool verity)
> dec_page_count(F2FS_P_SB(page), __read_io_type(page));
> unlock_page(page);
> }
> - if (bio->bi_private)
> - mempool_free(bio->bi_private, bio_post_read_ctx_pool);
> - bio_put(bio);
> +}
> +
> +static void f2fs_release_read_bio(struct bio *bio);
> +static void __f2fs_read_end_io(struct bio *bio, bool compr, bool verity)
> +{
> + if (!compr)
> + __read_end_io(bio, false, verity);
> + f2fs_release_read_bio(bio);
> }
>
> static void f2fs_decompress_bio(struct bio *bio, bool verity)
> @@ -127,19 +132,50 @@ static void f2fs_decompress_bio(struct bio *bio, bool
> verity)
>
> static void bio_post_read_processing(struct bio_post_read_ctx *ctx);
>
> -static void decrypt_work(struct bio_post_read_ctx *ctx)
> +static void f2fs_decrypt_work(struct bio_post_read_ctx *ctx)
> {
> fscrypt_decrypt_bio(ctx->bio);
> }
>
> -static void decompress_work(struct bio_post_read_ctx *ctx, bool verity)
> +static void f2fs_decompress_work(struct bio_post_read_ctx *ctx)
> +{
> + f2fs_decompress_bio(ctx->bio, ctx->enabled_steps & (1 << STEP_VERITY));
> +}
> +
> +#ifdef CONFIG_F2FS_FS_COMPRESSION
> +void f2fs_verify_pages(struct page **rpages, unsigned int cluster_size)
> {
> - f2fs_decompress_bio(ctx->bio, verity);
> + f2fs_decompress_end_io(rpages, cluster_size, false, true);
> }
>
> -static void verity_work(struct bio_post_read_ctx *ctx)
> +static void f2fs_verify_bio(struct bio *bio)
> {
> + struct page *page = bio_first_page_all(bio);
> + struct decompress_io_ctx *dic =
> + (struct decompress_io_ctx *)page_private(page);
> +
> + f2fs_verify_pages(dic->rpages, dic->cluster_size);
> + f2fs_free_dic(dic);
> +}
> +#endif
> +
> +static void
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
Hi Jaegeuk,
I've split workqueue for fsverity, please test compression based on last patch.
I shutdown F2FS_FS_COMPRESSION config, it looks all verity testcases can pass,
will
do more test for compress/encrypt/fsverity combination later.
The diff is as below, code base is last g-dev-test branch:
>From 5b51682bc3013b8de6dee4906865181c3ded435f Mon Sep 17 00:00:00 2001
From: Chao Yu
Date: Tue, 12 Nov 2019 10:03:21 +0800
Subject: [PATCH INCREMENT] f2fs: compress: split workqueue for fsverity
Signed-off-by: Chao Yu
---
fs/f2fs/compress.c | 16 +---
fs/f2fs/data.c | 94 +++---
fs/f2fs/f2fs.h | 2 +-
3 files changed, 84 insertions(+), 28 deletions(-)
diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index f4ce825f12b4..254275325890 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -377,7 +377,7 @@ void f2fs_decompress_pages(struct bio *bio, struct page
*page, bool verity)
dec_page_count(sbi, F2FS_RD_DATA);
- if (bio->bi_status)
+ if (bio->bi_status || PageError(page))
dic->failed = true;
if (refcount_dec_not_one(>ref))
@@ -419,10 +419,14 @@ void f2fs_decompress_pages(struct bio *bio, struct page
*page, bool verity)
out_vunmap_rbuf:
vunmap(dic->rbuf);
out_free_dic:
- f2fs_set_cluster_uptodate(dic->rpages, dic->cluster_size, ret, verity);
+ if (!verity)
+ f2fs_decompress_end_io(dic->rpages, dic->cluster_size,
+ ret, false);
+
trace_f2fs_decompress_pages_end(dic->inode, dic->cluster_idx,
- dic->clen, ret);
- f2fs_free_dic(dic);
+ dic->clen, ret);
+ if (!verity)
+ f2fs_free_dic(dic);
}
static bool is_page_in_cluster(struct compress_ctx *cc, pgoff_t index)
@@ -1086,7 +1090,7 @@ void f2fs_free_dic(struct decompress_io_ctx *dic)
kfree(dic);
}
-void f2fs_set_cluster_uptodate(struct page **rpages,
+void f2fs_decompress_end_io(struct page **rpages,
unsigned int cluster_size, bool err, bool verity)
{
int i;
@@ -1108,4 +1112,4 @@ void f2fs_set_cluster_uptodate(struct page **rpages,
}
unlock_page(rpage);
}
-}
+}
\ No newline at end of file
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index c9362a53f8a1..2d64c6ffee84 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -98,7 +98,7 @@ static void __read_end_io(struct bio *bio, bool compr, bool
verity)
page = bv->bv_page;
#ifdef CONFIG_F2FS_FS_COMPRESSION
- if (compr && PagePrivate(page)) {
+ if (compr && f2fs_is_compressed_page(page)) {
f2fs_decompress_pages(bio, page, verity);
continue;
}
@@ -115,9 +115,14 @@ static void __read_end_io(struct bio *bio, bool compr,
bool verity)
dec_page_count(F2FS_P_SB(page), __read_io_type(page));
unlock_page(page);
}
- if (bio->bi_private)
- mempool_free(bio->bi_private, bio_post_read_ctx_pool);
- bio_put(bio);
+}
+
+static void f2fs_release_read_bio(struct bio *bio);
+static void __f2fs_read_end_io(struct bio *bio, bool compr, bool verity)
+{
+ if (!compr)
+ __read_end_io(bio, false, verity);
+ f2fs_release_read_bio(bio);
}
static void f2fs_decompress_bio(struct bio *bio, bool verity)
@@ -127,19 +132,50 @@ static void f2fs_decompress_bio(struct bio *bio, bool
verity)
static void bio_post_read_processing(struct bio_post_read_ctx *ctx);
-static void decrypt_work(struct bio_post_read_ctx *ctx)
+static void f2fs_decrypt_work(struct bio_post_read_ctx *ctx)
{
fscrypt_decrypt_bio(ctx->bio);
}
-static void decompress_work(struct bio_post_read_ctx *ctx, bool verity)
+static void f2fs_decompress_work(struct bio_post_read_ctx *ctx)
+{
+ f2fs_decompress_bio(ctx->bio, ctx->enabled_steps & (1 << STEP_VERITY));
+}
+
+#ifdef CONFIG_F2FS_FS_COMPRESSION
+void f2fs_verify_pages(struct page **rpages, unsigned int cluster_size)
{
- f2fs_decompress_bio(ctx->bio, verity);
+ f2fs_decompress_end_io(rpages, cluster_size, false, true);
}
-static void verity_work(struct bio_post_read_ctx *ctx)
+static void f2fs_verify_bio(struct bio *bio)
{
+ struct page *page = bio_first_page_all(bio);
+ struct decompress_io_ctx *dic =
+ (struct decompress_io_ctx *)page_private(page);
+
+ f2fs_verify_pages(dic->rpages, dic->cluster_size);
+ f2fs_free_dic(dic);
+}
+#endif
+
+static void f2fs_verity_work(struct work_struct *work)
+{
+ struct bio_post_read_ctx *ctx =
+ container_of(work, struct bio_post_read_ctx, work);
+
+#ifdef CONFIG_F2FS_FS_COMPRESSION
+ /* previous step is decompression */
+ if (ctx->enabled_steps &
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
Hi Jaegeuk,
On 2019/10/31 23:35, Jaegeuk Kim wrote:
> Hi Chao,
>
> On 10/31, Chao Yu wrote:
>> On 2019/10/31 0:50, Eric Biggers wrote:
>>> No, just use kvmalloc(). The whole point of kvmalloc() is that it tries
>>> kmalloc() and then falls back to vmalloc() if it fails.
>>
>> Okay, it's fine to me, let me fix this in another patch.
>
> I've fixed some bugs. (e.g., mmap) Please apply this in your next patch, so
> that
> I can continue to test new version as early as possible.
Applied with some fixes as below comments.
>
> With this patch, I could boot up a device and install some apps successfully
> with "compress_extension=*".
Ah, '*' can trigger big pressure on compression paths.
>
> ---
> fs/f2fs/compress.c | 229 +++--
> fs/f2fs/data.c | 109 +
> fs/f2fs/f2fs.h | 22 +++--
> fs/f2fs/file.c | 71 +-
> fs/f2fs/namei.c| 20 +++-
> 5 files changed, 264 insertions(+), 187 deletions(-)
>
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index f276d82a67aa..e03d57396ea2 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -77,8 +77,9 @@ int f2fs_init_compress_ctx(struct compress_ctx *cc)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(cc->inode);
>
> - if (cc->rpages)
> + if (cc->nr_rpages)
> return 0;
> +
> cc->rpages = f2fs_kzalloc(sbi, sizeof(struct page *) * cc->cluster_size,
> GFP_KERNEL);
> if (!cc->rpages)
> @@ -88,7 +89,9 @@ int f2fs_init_compress_ctx(struct compress_ctx *cc)
>
> void f2fs_destroy_compress_ctx(struct compress_ctx *cc)
> {
> - kvfree(cc->rpages);
> + f2fs_reset_compress_ctx(cc);
> + WARN_ON(cc->nr_rpages);
f2fs_reset_compress_ctx() will reset cc->nr_rpages to zero, I removed it for
now.
> + kfree(cc->rpages);
> }
>
> int f2fs_compress_ctx_add_page(struct compress_ctx *cc, struct page *page)
> @@ -224,16 +227,6 @@ static const struct f2fs_compress_ops f2fs_lz4_ops = {
> .decompress_pages = lz4_decompress_pages,
> };
>
> -static void f2fs_release_cluster_pages(struct compress_ctx *cc)
> -{
> - int i;
> -
> - for (i = 0; i < cc->nr_rpages; i++) {
> - inode_dec_dirty_pages(cc->inode);
> - unlock_page(cc->rpages[i]);
> - }
> -}
> -
> static struct page *f2fs_grab_page(void)
> {
> struct page *page;
> @@ -321,6 +314,7 @@ static int f2fs_compress_pages(struct compress_ctx *cc)
> trace_f2fs_compress_pages_end(cc->inode, cc->cluster_idx,
> cc->clen, ret);
> return 0;
> +
> out_vunmap_cbuf:
> vunmap(cc->cbuf);
> out_vunmap_rbuf:
> @@ -393,10 +387,9 @@ void f2fs_decompress_pages(struct bio *bio, struct page
> *page, bool verity)
> vunmap(dic->rbuf);
> out_free_dic:
> f2fs_set_cluster_uptodate(dic->rpages, dic->cluster_size, ret, verity);
> - f2fs_free_dic(dic);
> -
> trace_f2fs_decompress_pages_end(dic->inode, dic->cluster_idx,
> dic->clen, ret);
> + f2fs_free_dic(dic);
> }
>
> static bool is_page_in_cluster(struct compress_ctx *cc, pgoff_t index)
> @@ -443,51 +436,25 @@ static bool __cluster_may_compress(struct compress_ctx
> *cc)
> return false;
> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
> return false;
> - if (f2fs_is_drop_cache(cc->inode))
> - return false;
> - if (f2fs_is_volatile_file(cc->inode))
> - return false;
>
> offset = i_size & (PAGE_SIZE - 1);
> if ((page->index > end_index) ||
> (page->index == end_index && !offset))
> return false;
> + if (page->index != start_idx_of_cluster(cc) + i)
> + return false;
Should this be a bug?
> }
> return true;
> }
>
> -int f2fs_is_cluster_existed(struct compress_ctx *cc)
> -{
> - struct dnode_of_data dn;
> - unsigned int start_idx = start_idx_of_cluster(cc);
> - int ret;
> - int i;
> -
> - set_new_dnode(, cc->inode, NULL, NULL, 0);
> - ret = f2fs_get_dnode_of_data(, start_idx, LOOKUP_NODE);
> - if (ret)
> - return ret;
> -
> - for (i = 0; i < cc->cluster_size; i++, dn.ofs_in_node++) {
> - block_t blkaddr = datablock_addr(dn.inode, dn.node_page,
> - dn.ofs_in_node);
> - if (blkaddr == COMPRESS_ADDR) {
> - ret = 1;
> - break;
> - }
> - if (__is_valid_data_blkaddr(blkaddr)) {
> - ret = 2;
> - break;
> - }
> - }
> - f2fs_put_dnode();
> - return ret;
> -}
> -
> static bool
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
Hi Chao,
On 10/31, Chao Yu wrote:
> On 2019/10/31 0:50, Eric Biggers wrote:
> > No, just use kvmalloc(). The whole point of kvmalloc() is that it tries
> > kmalloc() and then falls back to vmalloc() if it fails.
>
> Okay, it's fine to me, let me fix this in another patch.
I've fixed some bugs. (e.g., mmap) Please apply this in your next patch, so that
I can continue to test new version as early as possible.
With this patch, I could boot up a device and install some apps successfully
with "compress_extension=*".
---
fs/f2fs/compress.c | 229 +++--
fs/f2fs/data.c | 109 +
fs/f2fs/f2fs.h | 22 +++--
fs/f2fs/file.c | 71 +-
fs/f2fs/namei.c| 20 +++-
5 files changed, 264 insertions(+), 187 deletions(-)
diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index f276d82a67aa..e03d57396ea2 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -77,8 +77,9 @@ int f2fs_init_compress_ctx(struct compress_ctx *cc)
{
struct f2fs_sb_info *sbi = F2FS_I_SB(cc->inode);
- if (cc->rpages)
+ if (cc->nr_rpages)
return 0;
+
cc->rpages = f2fs_kzalloc(sbi, sizeof(struct page *) * cc->cluster_size,
GFP_KERNEL);
if (!cc->rpages)
@@ -88,7 +89,9 @@ int f2fs_init_compress_ctx(struct compress_ctx *cc)
void f2fs_destroy_compress_ctx(struct compress_ctx *cc)
{
- kvfree(cc->rpages);
+ f2fs_reset_compress_ctx(cc);
+ WARN_ON(cc->nr_rpages);
+ kfree(cc->rpages);
}
int f2fs_compress_ctx_add_page(struct compress_ctx *cc, struct page *page)
@@ -224,16 +227,6 @@ static const struct f2fs_compress_ops f2fs_lz4_ops = {
.decompress_pages = lz4_decompress_pages,
};
-static void f2fs_release_cluster_pages(struct compress_ctx *cc)
-{
- int i;
-
- for (i = 0; i < cc->nr_rpages; i++) {
- inode_dec_dirty_pages(cc->inode);
- unlock_page(cc->rpages[i]);
- }
-}
-
static struct page *f2fs_grab_page(void)
{
struct page *page;
@@ -321,6 +314,7 @@ static int f2fs_compress_pages(struct compress_ctx *cc)
trace_f2fs_compress_pages_end(cc->inode, cc->cluster_idx,
cc->clen, ret);
return 0;
+
out_vunmap_cbuf:
vunmap(cc->cbuf);
out_vunmap_rbuf:
@@ -393,10 +387,9 @@ void f2fs_decompress_pages(struct bio *bio, struct page
*page, bool verity)
vunmap(dic->rbuf);
out_free_dic:
f2fs_set_cluster_uptodate(dic->rpages, dic->cluster_size, ret, verity);
- f2fs_free_dic(dic);
-
trace_f2fs_decompress_pages_end(dic->inode, dic->cluster_idx,
dic->clen, ret);
+ f2fs_free_dic(dic);
}
static bool is_page_in_cluster(struct compress_ctx *cc, pgoff_t index)
@@ -443,51 +436,25 @@ static bool __cluster_may_compress(struct compress_ctx
*cc)
return false;
if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
return false;
- if (f2fs_is_drop_cache(cc->inode))
- return false;
- if (f2fs_is_volatile_file(cc->inode))
- return false;
offset = i_size & (PAGE_SIZE - 1);
if ((page->index > end_index) ||
(page->index == end_index && !offset))
return false;
+ if (page->index != start_idx_of_cluster(cc) + i)
+ return false;
}
return true;
}
-int f2fs_is_cluster_existed(struct compress_ctx *cc)
-{
- struct dnode_of_data dn;
- unsigned int start_idx = start_idx_of_cluster(cc);
- int ret;
- int i;
-
- set_new_dnode(, cc->inode, NULL, NULL, 0);
- ret = f2fs_get_dnode_of_data(, start_idx, LOOKUP_NODE);
- if (ret)
- return ret;
-
- for (i = 0; i < cc->cluster_size; i++, dn.ofs_in_node++) {
- block_t blkaddr = datablock_addr(dn.inode, dn.node_page,
- dn.ofs_in_node);
- if (blkaddr == COMPRESS_ADDR) {
- ret = 1;
- break;
- }
- if (__is_valid_data_blkaddr(blkaddr)) {
- ret = 2;
- break;
- }
- }
- f2fs_put_dnode();
- return ret;
-}
-
static bool cluster_may_compress(struct compress_ctx *cc)
{
if (!f2fs_compressed_file(cc->inode))
return false;
+ if (f2fs_is_atomic_file(cc->inode))
+ return false;
+ if (f2fs_is_mmap_file(cc->inode))
+ return false;
if (!f2fs_cluster_is_full(cc))
return false;
return __cluster_may_compress(cc);
@@ -495,19 +462,59 @@ static bool
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
On 2019/10/31 1:02, Eric Biggers wrote:
> On Wed, Oct 30, 2019 at 04:43:52PM +0800, Chao Yu wrote:
>> static void bio_post_read_processing(struct bio_post_read_ctx *ctx)
>> {
>> -/*
>> - * We use different work queues for decryption and for verity
>> because
>> - * verity may require reading metadata pages that need
>> decryption, and
>> - * we shouldn't recurse to the same workqueue.
>> - */
>
> Why is it okay (i.e., no deadlocks) to no longer use different work
> queues for
> decryption and for verity? See the comment above which is being deleted.
Could you explain more about how deadlock happen? or share me a link
address if
you have described that case somewhere?
>>>
>>> The verity work can read pages from the file which require decryption. I'm
>>> concerned that it could deadlock if the work is scheduled on the same
>>> workqueue.
>>
>> I assume you've tried one workqueue, and suffered deadlock..
>>
>>> Granted, I'm not an expert in Linux workqueues, so if you've investigated
>>> this
>>> and determined that it's safe, can you explain why?
>>
>> I'm not familiar with workqueue... I guess it may not safe that if the work
>> is
>> scheduled to the same cpu in where verity was waiting for data? if the work
>> is
>> scheduled to other cpu, it may be safe.
>>
>> I can check that before splitting the workqueue for verity and
>> decrypt/decompress.
>>
>
> Yes this is a real problem, try 'kvm-xfstests -c f2fs/encrypt generic/579'.
> The worker thread gets deadlocked in f2fs_read_merkle_tree_page() waiting for
> the Merkle tree page to be decrypted. This is with the v2 compression patch;
> it works fine on current mainline.
Oh, alright...
Let me split them, thanks very much for all the comments and test anyway.
Thanks,
>
> INFO: task kworker/u5:0:61 blocked for more than 30 seconds.
> Not tainted 5.4.0-rc1-00119-g464e31ba60d0 #13
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> kworker/u5:0D061 2 0x80004000
> Workqueue: f2fs_post_read_wq f2fs_post_read_work
> Call Trace:
> context_switch kernel/sched/core.c:3384 [inline]
> __schedule+0x299/0x6c0 kernel/sched/core.c:4069
> schedule+0x44/0xd0 kernel/sched/core.c:4136
> io_schedule+0x11/0x40 kernel/sched/core.c:5780
> wait_on_page_bit_common mm/filemap.c:1174 [inline]
> wait_on_page_bit mm/filemap.c:1223 [inline]
> wait_on_page_locked include/linux/pagemap.h:527 [inline]
> wait_on_page_locked include/linux/pagemap.h:524 [inline]
> wait_on_page_read mm/filemap.c:2767 [inline]
> do_read_cache_page+0x407/0x660 mm/filemap.c:2810
> read_cache_page+0xd/0x10 mm/filemap.c:2894
> f2fs_read_merkle_tree_page+0x2e/0x30 include/linux/pagemap.h:396
> verify_page+0x110/0x560 fs/verity/verify.c:120
> fsverity_verify_bio+0xe6/0x1a0 fs/verity/verify.c:239
> verity_work fs/f2fs/data.c:142 [inline]
> f2fs_post_read_work+0x36/0x50 fs/f2fs/data.c:160
> process_one_work+0x225/0x550 kernel/workqueue.c:2269
> worker_thread+0x4b/0x3c0 kernel/workqueue.c:2415
> kthread+0x125/0x140 kernel/kthread.c:255
> ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
> INFO: task kworker/u5:1:1140 blocked for more than 30 seconds.
> Not tainted 5.4.0-rc1-00119-g464e31ba60d0 #13
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> kworker/u5:1D0 1140 2 0x80004000
> Workqueue: f2fs_post_read_wq f2fs_post_read_work
> Call Trace:
> context_switch kernel/sched/core.c:3384 [inline]
> __schedule+0x299/0x6c0 kernel/sched/core.c:4069
> schedule+0x44/0xd0 kernel/sched/core.c:4136
> io_schedule+0x11/0x40 kernel/sched/core.c:5780
> wait_on_page_bit_common mm/filemap.c:1174 [inline]
> wait_on_page_bit mm/filemap.c:1223 [inline]
> wait_on_page_locked include/linux/pagemap.h:527 [inline]
> wait_on_page_locked include/linux/pagemap.h:524 [inline]
> wait_on_page_read mm/filemap.c:2767 [inline]
> do_read_cache_page+0x407/0x660 mm/filemap.c:2810
> read_cache_page+0xd/0x10 mm/filemap.c:2894
> f2fs_read_merkle_tree_page+0x2e/0x30 include/linux/pagemap.h:396
> verify_page+0x110/0x560 fs/verity/verify.c:120
> fsverity_verify_bio+0xe6/0x1a0 fs/verity/verify.c:239
> verity_work fs/f2fs/data.c:142 [inline]
> f2fs_post_read_work+0x36/0x50 fs/f2fs/data.c:160
> process_one_work+0x225/0x550 kernel/workqueue.c:2269
> worker_thread+0x4b/0x3c0 kernel/workqueue.c:2415
> kthread+0x125/0x140 kernel/kthread.c:255
> ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
>
> Showing all locks held in the system:
> 1 lock held by khungtaskd/21:
> #0: 82250520 (rcu_read_lock){}, at:
> rcu_lock_acquire.constprop.0+0x0/0x30 include/trace/events/lock.h:13
> 2 locks held by kworker/u5:0/61:
> #0: 88807b78eb28 ((wq_completion)f2fs_post_read_wq){+.+.}, at:
> set_work_data kernel/workqueue.c:619 [inline]
> #0: 88807b78eb28
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
On 2019/10/31 0:50, Eric Biggers wrote: > No, just use kvmalloc(). The whole point of kvmalloc() is that it tries > kmalloc() and then falls back to vmalloc() if it fails. Okay, it's fine to me, let me fix this in another patch. Thanks, > > - Eric > . > ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
On 10/30, Eric Biggers wrote:
> On Wed, Oct 30, 2019 at 04:43:52PM +0800, Chao Yu wrote:
> > On 2019/10/30 10:55, Eric Biggers wrote:
> > > On Tue, Oct 29, 2019 at 04:33:36PM +0800, Chao Yu wrote:
> > >> On 2019/10/28 6:50, Eric Biggers wrote:
> > +bool f2fs_is_compressed_page(struct page *page)
> > +{
> > + if (!page_private(page))
> > + return false;
> > + if (IS_ATOMIC_WRITTEN_PAGE(page) || IS_DUMMY_WRITTEN_PAGE(page))
> > + return false;
> > + return *((u32 *)page_private(page)) ==
> > F2FS_COMPRESSED_PAGE_MAGIC;
> > +}
> > >>>
> > >>> This code implies that there can be multiple page private structures
> > >>> each of
> > >>> which has a different magic number. But I only see
> > >>> F2FS_COMPRESSED_PAGE_MAGIC.
> > >>> Where in the code is the other one(s)?
> > >>
> > >> I'm not sure I understood you correctly, did you mean it needs to
> > >> introduce
> > >> f2fs_is_atomic_written_page() and f2fs_is_dummy_written_page() like
> > >> f2fs_is_compressed_page()?
> > >>
> > >
> > > No, I'm asking what is the case where the line
> > >
> > > *((u32 *)page_private(page)) == F2FS_COMPRESSED_PAGE_MAGIC
> > >
> > > returns false?
> >
> > Should be this?
> >
> > if (!page_private(page))
> > return false;
> > f2fs_bug_on(*((u32 *)page_private(page)) != F2FS_COMPRESSED_PAGE_MAGIC)
> > return true;
>
> Yes, that makes more sense, unless there are other cases.
>
> >
> > >
> > >>>
> > +
> > +static void f2fs_set_compressed_page(struct page *page,
> > + struct inode *inode, pgoff_t index, void *data,
> > refcount_t *r)
> > +{
> > + SetPagePrivate(page);
> > + set_page_private(page, (unsigned long)data);
> > +
> > + /* i_crypto_info and iv index */
> > + page->index = index;
> > + page->mapping = inode->i_mapping;
> > + if (r)
> > + refcount_inc(r);
> > +}
> > >>>
> > >>> It isn't really appropriate to create fake pagecache pages like this.
> > >>> Did you
> > >>> consider changing f2fs to use fscrypt_decrypt_block_inplace() instead?
> > >>
> > >> We need to store i_crypto_info and iv index somewhere, in order to pass
> > >> them to
> > >> fscrypt_decrypt_block_inplace(), where did you suggest to store them?
> > >>
> > >
> > > The same place where the pages are stored.
> >
> > Still we need allocate space for those fields, any strong reason to do so?
> >
>
> page->mapping set implies that the page is a pagecache page. Faking it could
> cause problems with code elsewhere.
I've checked it with minchan, and it seems to be fine that filesystem uses
this page internally only, not in pagecache.
>
> > >
> > +
> > +void f2fs_destroy_compress_ctx(struct compress_ctx *cc)
> > +{
> > + kvfree(cc->rpages);
> > +}
> > >>>
> > >>> The memory is allocated with kzalloc(), so why is it freed with
> > >>> kvfree() and not
> > >>> just kfree()?
> > >>
> > >> It was allocated by f2fs_*alloc() which will fallback to kvmalloc() once
> > >> kmalloc() failed.
> > >
> > > This seems to be a bug in f2fs_kmalloc() -- it inappropriately falls back
> > > to
> > > kvmalloc(). As per its name, it should only use kmalloc().
> > > f2fs_kvmalloc()
> > > already exists, so it can be used when the fallback is wanted.
> >
> > We can introduce f2fs_memalloc() to wrap f2fs_kmalloc() and f2fs_kvmalloc()
> > as
> > below:
> >
> > f2fs_memalloc()
> > {
> > mem = f2fs_kmalloc();
> > if (mem)
> > return mem;
> > return f2fs_kvmalloc();
> > }
> >
> > It can be used in specified place where we really need it, like the place
> > descirbied in 5222595d093e ("f2fs: use kvmalloc, if kmalloc is failed") in
> > where
> > we introduced original logic.
>
> No, just use kvmalloc(). The whole point of kvmalloc() is that it tries
> kmalloc() and then falls back to vmalloc() if it fails.
>
> - Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
Hi Eric, (add some mm folks...) On Wed, Oct 30, 2019 at 09:50:56AM -0700, Eric Biggers wrote: > > >>> > > >>> It isn't really appropriate to create fake pagecache pages like this. > > >>> Did you > > >>> consider changing f2fs to use fscrypt_decrypt_block_inplace() instead? > > >> > > >> We need to store i_crypto_info and iv index somewhere, in order to pass > > >> them to > > >> fscrypt_decrypt_block_inplace(), where did you suggest to store them? > > >> > > > > > > The same place where the pages are stored. > > > > Still we need allocate space for those fields, any strong reason to do so? > > > > page->mapping set implies that the page is a pagecache page. Faking it could > cause problems with code elsewhere. Not very related with this patch. Faking page->mapping was used in zsmalloc before nonLRU migration (see material [1]) and use in erofs now (page->mapping to indicate nonLRU short lifetime temporary page type, page->private is used for per-page information), as far as I know, NonLRU page without PAGE_MAPPING_MOVABLE set is safe for most mm code. On the other hands, I think NULL page->mapping will waste such field in precious page structure... And we can not get such page type directly only by a NULL -- a truncated file page or just allocated page or some type internal temporary pages... So I have some proposal is to use page->mapping to indicate specific page type for such nonLRU pages (by some common convention, e.g. some real structure, rather than just zero out to waste 8 bytes, it's also natural to indicate some page type by its `mapping' naming )... Since my English is not very well, I delay it util now... [1] https://elixir.bootlin.com/linux/v3.18.140/source/mm/zsmalloc.c#L379 https://lore.kernel.org/linux-mm/1459321935-3655-7-git-send-email-minc...@kernel.org and some not very related topic: https://lwn.net/Articles/752564/ Thanks, Gao Xiang ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
On Wed, Oct 30, 2019 at 04:43:52PM +0800, Chao Yu wrote:
> static void bio_post_read_processing(struct bio_post_read_ctx *ctx)
> {
> -/*
> - * We use different work queues for decryption and for verity
> because
> - * verity may require reading metadata pages that need
> decryption, and
> - * we shouldn't recurse to the same workqueue.
> - */
> >>>
> >>> Why is it okay (i.e., no deadlocks) to no longer use different work
> >>> queues for
> >>> decryption and for verity? See the comment above which is being deleted.
> >>
> >> Could you explain more about how deadlock happen? or share me a link
> >> address if
> >> you have described that case somewhere?
> >>
> >
> > The verity work can read pages from the file which require decryption. I'm
> > concerned that it could deadlock if the work is scheduled on the same
> > workqueue.
>
> I assume you've tried one workqueue, and suffered deadlock..
>
> > Granted, I'm not an expert in Linux workqueues, so if you've investigated
> > this
> > and determined that it's safe, can you explain why?
>
> I'm not familiar with workqueue... I guess it may not safe that if the work
> is
> scheduled to the same cpu in where verity was waiting for data? if the work is
> scheduled to other cpu, it may be safe.
>
> I can check that before splitting the workqueue for verity and
> decrypt/decompress.
>
Yes this is a real problem, try 'kvm-xfstests -c f2fs/encrypt generic/579'.
The worker thread gets deadlocked in f2fs_read_merkle_tree_page() waiting for
the Merkle tree page to be decrypted. This is with the v2 compression patch;
it works fine on current mainline.
INFO: task kworker/u5:0:61 blocked for more than 30 seconds.
Not tainted 5.4.0-rc1-00119-g464e31ba60d0 #13
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u5:0D061 2 0x80004000
Workqueue: f2fs_post_read_wq f2fs_post_read_work
Call Trace:
context_switch kernel/sched/core.c:3384 [inline]
__schedule+0x299/0x6c0 kernel/sched/core.c:4069
schedule+0x44/0xd0 kernel/sched/core.c:4136
io_schedule+0x11/0x40 kernel/sched/core.c:5780
wait_on_page_bit_common mm/filemap.c:1174 [inline]
wait_on_page_bit mm/filemap.c:1223 [inline]
wait_on_page_locked include/linux/pagemap.h:527 [inline]
wait_on_page_locked include/linux/pagemap.h:524 [inline]
wait_on_page_read mm/filemap.c:2767 [inline]
do_read_cache_page+0x407/0x660 mm/filemap.c:2810
read_cache_page+0xd/0x10 mm/filemap.c:2894
f2fs_read_merkle_tree_page+0x2e/0x30 include/linux/pagemap.h:396
verify_page+0x110/0x560 fs/verity/verify.c:120
fsverity_verify_bio+0xe6/0x1a0 fs/verity/verify.c:239
verity_work fs/f2fs/data.c:142 [inline]
f2fs_post_read_work+0x36/0x50 fs/f2fs/data.c:160
process_one_work+0x225/0x550 kernel/workqueue.c:2269
worker_thread+0x4b/0x3c0 kernel/workqueue.c:2415
kthread+0x125/0x140 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
INFO: task kworker/u5:1:1140 blocked for more than 30 seconds.
Not tainted 5.4.0-rc1-00119-g464e31ba60d0 #13
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u5:1D0 1140 2 0x80004000
Workqueue: f2fs_post_read_wq f2fs_post_read_work
Call Trace:
context_switch kernel/sched/core.c:3384 [inline]
__schedule+0x299/0x6c0 kernel/sched/core.c:4069
schedule+0x44/0xd0 kernel/sched/core.c:4136
io_schedule+0x11/0x40 kernel/sched/core.c:5780
wait_on_page_bit_common mm/filemap.c:1174 [inline]
wait_on_page_bit mm/filemap.c:1223 [inline]
wait_on_page_locked include/linux/pagemap.h:527 [inline]
wait_on_page_locked include/linux/pagemap.h:524 [inline]
wait_on_page_read mm/filemap.c:2767 [inline]
do_read_cache_page+0x407/0x660 mm/filemap.c:2810
read_cache_page+0xd/0x10 mm/filemap.c:2894
f2fs_read_merkle_tree_page+0x2e/0x30 include/linux/pagemap.h:396
verify_page+0x110/0x560 fs/verity/verify.c:120
fsverity_verify_bio+0xe6/0x1a0 fs/verity/verify.c:239
verity_work fs/f2fs/data.c:142 [inline]
f2fs_post_read_work+0x36/0x50 fs/f2fs/data.c:160
process_one_work+0x225/0x550 kernel/workqueue.c:2269
worker_thread+0x4b/0x3c0 kernel/workqueue.c:2415
kthread+0x125/0x140 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Showing all locks held in the system:
1 lock held by khungtaskd/21:
#0: 82250520 (rcu_read_lock){}, at:
rcu_lock_acquire.constprop.0+0x0/0x30 include/trace/events/lock.h:13
2 locks held by kworker/u5:0/61:
#0: 88807b78eb28 ((wq_completion)f2fs_post_read_wq){+.+.}, at:
set_work_data kernel/workqueue.c:619 [inline]
#0: 88807b78eb28 ((wq_completion)f2fs_post_read_wq){+.+.}, at:
set_work_pool_and_clear_pending kernel/workqueue.c:647 [inline]
#0: 88807b78eb28 ((wq_completion)f2fs_post_read_wq){+.+.}, at:
process_one_work+0x1ad/0x550 kernel/workqueue.c:2240
#1: c9253e50 ((work_completion)(>work)){+.+.}, at: set_work_data
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
On Wed, Oct 30, 2019 at 04:43:52PM +0800, Chao Yu wrote:
> On 2019/10/30 10:55, Eric Biggers wrote:
> > On Tue, Oct 29, 2019 at 04:33:36PM +0800, Chao Yu wrote:
> >> On 2019/10/28 6:50, Eric Biggers wrote:
> +bool f2fs_is_compressed_page(struct page *page)
> +{
> +if (!page_private(page))
> +return false;
> +if (IS_ATOMIC_WRITTEN_PAGE(page) || IS_DUMMY_WRITTEN_PAGE(page))
> +return false;
> +return *((u32 *)page_private(page)) ==
> F2FS_COMPRESSED_PAGE_MAGIC;
> +}
> >>>
> >>> This code implies that there can be multiple page private structures each
> >>> of
> >>> which has a different magic number. But I only see
> >>> F2FS_COMPRESSED_PAGE_MAGIC.
> >>> Where in the code is the other one(s)?
> >>
> >> I'm not sure I understood you correctly, did you mean it needs to introduce
> >> f2fs_is_atomic_written_page() and f2fs_is_dummy_written_page() like
> >> f2fs_is_compressed_page()?
> >>
> >
> > No, I'm asking what is the case where the line
> >
> > *((u32 *)page_private(page)) == F2FS_COMPRESSED_PAGE_MAGIC
> >
> > returns false?
>
> Should be this?
>
> if (!page_private(page))
> return false;
> f2fs_bug_on(*((u32 *)page_private(page)) != F2FS_COMPRESSED_PAGE_MAGIC)
> return true;
Yes, that makes more sense, unless there are other cases.
>
> >
> >>>
> +
> +static void f2fs_set_compressed_page(struct page *page,
> +struct inode *inode, pgoff_t index, void *data,
> refcount_t *r)
> +{
> +SetPagePrivate(page);
> +set_page_private(page, (unsigned long)data);
> +
> +/* i_crypto_info and iv index */
> +page->index = index;
> +page->mapping = inode->i_mapping;
> +if (r)
> +refcount_inc(r);
> +}
> >>>
> >>> It isn't really appropriate to create fake pagecache pages like this.
> >>> Did you
> >>> consider changing f2fs to use fscrypt_decrypt_block_inplace() instead?
> >>
> >> We need to store i_crypto_info and iv index somewhere, in order to pass
> >> them to
> >> fscrypt_decrypt_block_inplace(), where did you suggest to store them?
> >>
> >
> > The same place where the pages are stored.
>
> Still we need allocate space for those fields, any strong reason to do so?
>
page->mapping set implies that the page is a pagecache page. Faking it could
cause problems with code elsewhere.
> >
> +
> +void f2fs_destroy_compress_ctx(struct compress_ctx *cc)
> +{
> +kvfree(cc->rpages);
> +}
> >>>
> >>> The memory is allocated with kzalloc(), so why is it freed with kvfree()
> >>> and not
> >>> just kfree()?
> >>
> >> It was allocated by f2fs_*alloc() which will fallback to kvmalloc() once
> >> kmalloc() failed.
> >
> > This seems to be a bug in f2fs_kmalloc() -- it inappropriately falls back to
> > kvmalloc(). As per its name, it should only use kmalloc(). f2fs_kvmalloc()
> > already exists, so it can be used when the fallback is wanted.
>
> We can introduce f2fs_memalloc() to wrap f2fs_kmalloc() and f2fs_kvmalloc() as
> below:
>
> f2fs_memalloc()
> {
> mem = f2fs_kmalloc();
> if (mem)
> return mem;
> return f2fs_kvmalloc();
> }
>
> It can be used in specified place where we really need it, like the place
> descirbied in 5222595d093e ("f2fs: use kvmalloc, if kmalloc is failed") in
> where
> we introduced original logic.
No, just use kvmalloc(). The whole point of kvmalloc() is that it tries
kmalloc() and then falls back to vmalloc() if it fails.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
On 2019/10/30 10:55, Eric Biggers wrote:
> On Tue, Oct 29, 2019 at 04:33:36PM +0800, Chao Yu wrote:
>> On 2019/10/28 6:50, Eric Biggers wrote:
+bool f2fs_is_compressed_page(struct page *page)
+{
+ if (!page_private(page))
+ return false;
+ if (IS_ATOMIC_WRITTEN_PAGE(page) || IS_DUMMY_WRITTEN_PAGE(page))
+ return false;
+ return *((u32 *)page_private(page)) == F2FS_COMPRESSED_PAGE_MAGIC;
+}
>>>
>>> This code implies that there can be multiple page private structures each of
>>> which has a different magic number. But I only see
>>> F2FS_COMPRESSED_PAGE_MAGIC.
>>> Where in the code is the other one(s)?
>>
>> I'm not sure I understood you correctly, did you mean it needs to introduce
>> f2fs_is_atomic_written_page() and f2fs_is_dummy_written_page() like
>> f2fs_is_compressed_page()?
>>
>
> No, I'm asking what is the case where the line
>
> *((u32 *)page_private(page)) == F2FS_COMPRESSED_PAGE_MAGIC
>
> returns false?
Should be this?
if (!page_private(page))
return false;
f2fs_bug_on(*((u32 *)page_private(page)) != F2FS_COMPRESSED_PAGE_MAGIC)
return true;
>
>>>
+
+static void f2fs_set_compressed_page(struct page *page,
+ struct inode *inode, pgoff_t index, void *data, refcount_t *r)
+{
+ SetPagePrivate(page);
+ set_page_private(page, (unsigned long)data);
+
+ /* i_crypto_info and iv index */
+ page->index = index;
+ page->mapping = inode->i_mapping;
+ if (r)
+ refcount_inc(r);
+}
>>>
>>> It isn't really appropriate to create fake pagecache pages like this. Did
>>> you
>>> consider changing f2fs to use fscrypt_decrypt_block_inplace() instead?
>>
>> We need to store i_crypto_info and iv index somewhere, in order to pass them
>> to
>> fscrypt_decrypt_block_inplace(), where did you suggest to store them?
>>
>
> The same place where the pages are stored.
Still we need allocate space for those fields, any strong reason to do so?
>
+
+void f2fs_destroy_compress_ctx(struct compress_ctx *cc)
+{
+ kvfree(cc->rpages);
+}
>>>
>>> The memory is allocated with kzalloc(), so why is it freed with kvfree()
>>> and not
>>> just kfree()?
>>
>> It was allocated by f2fs_*alloc() which will fallback to kvmalloc() once
>> kmalloc() failed.
>
> This seems to be a bug in f2fs_kmalloc() -- it inappropriately falls back to
> kvmalloc(). As per its name, it should only use kmalloc(). f2fs_kvmalloc()
> already exists, so it can be used when the fallback is wanted.
We can introduce f2fs_memalloc() to wrap f2fs_kmalloc() and f2fs_kvmalloc() as
below:
f2fs_memalloc()
{
mem = f2fs_kmalloc();
if (mem)
return mem;
return f2fs_kvmalloc();
}
It can be used in specified place where we really need it, like the place
descirbied in 5222595d093e ("f2fs: use kvmalloc, if kmalloc is failed") in where
we introduced original logic.
>
>>
+static int lzo_compress_pages(struct compress_ctx *cc)
+{
+ int ret;
+
+ ret = lzo1x_1_compress(cc->rbuf, cc->rlen, cc->cbuf->cdata,
+ >clen, cc->private);
+ if (ret != LZO_E_OK) {
+ printk_ratelimited("%sF2FS-fs: lzo compress failed, ret:%d\n",
+ KERN_ERR, ret);
+ return -EIO;
+ }
+ return 0;
+}
>>>
>>> Why not using f2fs_err()? Same in lots of other places.
>>
>> We use printk_ratelimited at some points where we can afford to lose logs,
>> otherwise we use f2fs_{err,warn...} to record info as much as possible for
>> troubleshoot.
>>
>
> It used to be the case that f2fs_msg() was ratelimited. What stops it from
> spamming the logs now?
https://lore.kernel.org/patchwork/patch/973837/
>
> The problem with a bare printk is that it doesn't show which filesystem
> instance
> the message is coming from.
We can add to print sbi->sb->s_id like f2fs_printk().
>
+
+ ret = cops->compress_pages(cc);
+ if (ret)
+ goto out_vunmap_cbuf;
+
+ max_len = PAGE_SIZE * (cc->cluster_size - 1) - COMPRESS_HEADER_SIZE;
+
+ if (cc->clen > max_len) {
+ ret = -EAGAIN;
+ goto out_vunmap_cbuf;
+ }
>>>
>>> Since we already know the max length we're willing to compress to (the max
>>> length for any space to be saved), why is more space than that being
>>> allocated?
>>> LZ4_compress_default() will return an error if there isn't enough space, so
>>> that
>>> error could just be used as the indication to store the data uncompressed.
>>
>> AFAIK, there is no such common error code returned from all compression
>> algorithms indicating there is no room for limited target size, however we
>> need
>> that information to fallback to write raw pages. Any better idea?
>>
>
> "Not enough room" is the only reasonable way for compression to
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
On Tue, Oct 29, 2019 at 04:33:36PM +0800, Chao Yu wrote:
> On 2019/10/28 6:50, Eric Biggers wrote:
> >> +bool f2fs_is_compressed_page(struct page *page)
> >> +{
> >> + if (!page_private(page))
> >> + return false;
> >> + if (IS_ATOMIC_WRITTEN_PAGE(page) || IS_DUMMY_WRITTEN_PAGE(page))
> >> + return false;
> >> + return *((u32 *)page_private(page)) == F2FS_COMPRESSED_PAGE_MAGIC;
> >> +}
> >
> > This code implies that there can be multiple page private structures each of
> > which has a different magic number. But I only see
> > F2FS_COMPRESSED_PAGE_MAGIC.
> > Where in the code is the other one(s)?
>
> I'm not sure I understood you correctly, did you mean it needs to introduce
> f2fs_is_atomic_written_page() and f2fs_is_dummy_written_page() like
> f2fs_is_compressed_page()?
>
No, I'm asking what is the case where the line
*((u32 *)page_private(page)) == F2FS_COMPRESSED_PAGE_MAGIC
returns false?
> >
> >> +
> >> +static void f2fs_set_compressed_page(struct page *page,
> >> + struct inode *inode, pgoff_t index, void *data, refcount_t *r)
> >> +{
> >> + SetPagePrivate(page);
> >> + set_page_private(page, (unsigned long)data);
> >> +
> >> + /* i_crypto_info and iv index */
> >> + page->index = index;
> >> + page->mapping = inode->i_mapping;
> >> + if (r)
> >> + refcount_inc(r);
> >> +}
> >
> > It isn't really appropriate to create fake pagecache pages like this. Did
> > you
> > consider changing f2fs to use fscrypt_decrypt_block_inplace() instead?
>
> We need to store i_crypto_info and iv index somewhere, in order to pass them
> to
> fscrypt_decrypt_block_inplace(), where did you suggest to store them?
>
The same place where the pages are stored.
> >> +
> >> +void f2fs_destroy_compress_ctx(struct compress_ctx *cc)
> >> +{
> >> + kvfree(cc->rpages);
> >> +}
> >
> > The memory is allocated with kzalloc(), so why is it freed with kvfree()
> > and not
> > just kfree()?
>
> It was allocated by f2fs_*alloc() which will fallback to kvmalloc() once
> kmalloc() failed.
This seems to be a bug in f2fs_kmalloc() -- it inappropriately falls back to
kvmalloc(). As per its name, it should only use kmalloc(). f2fs_kvmalloc()
already exists, so it can be used when the fallback is wanted.
>
> >> +static int lzo_compress_pages(struct compress_ctx *cc)
> >> +{
> >> + int ret;
> >> +
> >> + ret = lzo1x_1_compress(cc->rbuf, cc->rlen, cc->cbuf->cdata,
> >> + >clen, cc->private);
> >> + if (ret != LZO_E_OK) {
> >> + printk_ratelimited("%sF2FS-fs: lzo compress failed, ret:%d\n",
> >> + KERN_ERR, ret);
> >> + return -EIO;
> >> + }
> >> + return 0;
> >> +}
> >
> > Why not using f2fs_err()? Same in lots of other places.
>
> We use printk_ratelimited at some points where we can afford to lose logs,
> otherwise we use f2fs_{err,warn...} to record info as much as possible for
> troubleshoot.
>
It used to be the case that f2fs_msg() was ratelimited. What stops it from
spamming the logs now?
The problem with a bare printk is that it doesn't show which filesystem instance
the message is coming from.
> >> +
> >> + ret = cops->compress_pages(cc);
> >> + if (ret)
> >> + goto out_vunmap_cbuf;
> >> +
> >> + max_len = PAGE_SIZE * (cc->cluster_size - 1) - COMPRESS_HEADER_SIZE;
> >> +
> >> + if (cc->clen > max_len) {
> >> + ret = -EAGAIN;
> >> + goto out_vunmap_cbuf;
> >> + }
> >
> > Since we already know the max length we're willing to compress to (the max
> > length for any space to be saved), why is more space than that being
> > allocated?
> > LZ4_compress_default() will return an error if there isn't enough space, so
> > that
> > error could just be used as the indication to store the data uncompressed.
>
> AFAIK, there is no such common error code returned from all compression
> algorithms indicating there is no room for limited target size, however we
> need
> that information to fallback to write raw pages. Any better idea?
>
"Not enough room" is the only reasonable way for compression to fail, so all
that's needed is the ability for compression to report errors at all. What
specifically prevents this approach from working?
> >> static void bio_post_read_processing(struct bio_post_read_ctx *ctx)
> >> {
> >> - /*
> >> - * We use different work queues for decryption and for verity because
> >> - * verity may require reading metadata pages that need decryption, and
> >> - * we shouldn't recurse to the same workqueue.
> >> - */
> >
> > Why is it okay (i.e., no deadlocks) to no longer use different work queues
> > for
> > decryption and for verity? See the comment above which is being deleted.
>
> Could you explain more about how deadlock happen? or share me a link address
> if
> you have described that case somewhere?
>
The verity work can read pages from the file which require decryption. I'm
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
On 2019/10/28 6:50, Eric Biggers wrote:
>> +bool f2fs_is_compressed_page(struct page *page)
>> +{
>> +if (!page_private(page))
>> +return false;
>> +if (IS_ATOMIC_WRITTEN_PAGE(page) || IS_DUMMY_WRITTEN_PAGE(page))
>> +return false;
>> +return *((u32 *)page_private(page)) == F2FS_COMPRESSED_PAGE_MAGIC;
>> +}
>
> This code implies that there can be multiple page private structures each of
> which has a different magic number. But I only see
> F2FS_COMPRESSED_PAGE_MAGIC.
> Where in the code is the other one(s)?
I'm not sure I understood you correctly, did you mean it needs to introduce
f2fs_is_atomic_written_page() and f2fs_is_dummy_written_page() like
f2fs_is_compressed_page()?
>
>> +
>> +static void f2fs_set_compressed_page(struct page *page,
>> +struct inode *inode, pgoff_t index, void *data, refcount_t *r)
>> +{
>> +SetPagePrivate(page);
>> +set_page_private(page, (unsigned long)data);
>> +
>> +/* i_crypto_info and iv index */
>> +page->index = index;
>> +page->mapping = inode->i_mapping;
>> +if (r)
>> +refcount_inc(r);
>> +}
>
> It isn't really appropriate to create fake pagecache pages like this. Did you
> consider changing f2fs to use fscrypt_decrypt_block_inplace() instead?
We need to store i_crypto_info and iv index somewhere, in order to pass them to
fscrypt_decrypt_block_inplace(), where did you suggest to store them?
>> +
>> +void f2fs_destroy_compress_ctx(struct compress_ctx *cc)
>> +{
>> +kvfree(cc->rpages);
>> +}
>
> The memory is allocated with kzalloc(), so why is it freed with kvfree() and
> not
> just kfree()?
It was allocated by f2fs_*alloc() which will fallback to kvmalloc() once
kmalloc() failed.
>> +static int lzo_compress_pages(struct compress_ctx *cc)
>> +{
>> +int ret;
>> +
>> +ret = lzo1x_1_compress(cc->rbuf, cc->rlen, cc->cbuf->cdata,
>> +>clen, cc->private);
>> +if (ret != LZO_E_OK) {
>> +printk_ratelimited("%sF2FS-fs: lzo compress failed, ret:%d\n",
>> +KERN_ERR, ret);
>> +return -EIO;
>> +}
>> +return 0;
>> +}
>
> Why not using f2fs_err()? Same in lots of other places.
We use printk_ratelimited at some points where we can afford to lose logs,
otherwise we use f2fs_{err,warn...} to record info as much as possible for
troubleshoot.
>> +
>> +ret = cops->compress_pages(cc);
>> +if (ret)
>> +goto out_vunmap_cbuf;
>> +
>> +max_len = PAGE_SIZE * (cc->cluster_size - 1) - COMPRESS_HEADER_SIZE;
>> +
>> +if (cc->clen > max_len) {
>> +ret = -EAGAIN;
>> +goto out_vunmap_cbuf;
>> +}
>
> Since we already know the max length we're willing to compress to (the max
> length for any space to be saved), why is more space than that being
> allocated?
> LZ4_compress_default() will return an error if there isn't enough space, so
> that
> error could just be used as the indication to store the data uncompressed.
AFAIK, there is no such common error code returned from all compression
algorithms indicating there is no room for limited target size, however we need
that information to fallback to write raw pages. Any better idea?
>
>> +
>> +cc->cbuf->clen = cpu_to_le32(cc->clen);
>> +cc->cbuf->chksum = 0;
>
> What is the point of the chksum field? It's always set to 0 and never
> checked.
When I written initial codes, I doubt that I may lose to check some SPO corner
cases, in where we missed to write whole cluster, so I added that to help to
recall that case, however I didn't have time to cover those cases, resulting
leaving unfinished code there... :(, I'm okay to delete it in a formal version.
BTW, for data checksum feature, I guess we need to reconstruct dnode layout to
cover both compressed/non-compressed data.
>
>> +
>> +static bool __cluster_may_compress(struct compress_ctx *cc)
>> +{
>> +struct f2fs_sb_info *sbi = F2FS_I_SB(cc->inode);
>> +loff_t i_size = i_size_read(cc->inode);
>> +const pgoff_t end_index = ((unsigned long long)i_size)
>> +>> PAGE_SHIFT;
>> +unsigned offset;
>> +int i;
>> +
>> +for (i = 0; i < cc->cluster_size; i++) {
>> +struct page *page = cc->rpages[i];
>> +
>> +f2fs_bug_on(sbi, !page);
>> +
>> +if (unlikely(f2fs_cp_error(sbi)))
>> +return false;
>> +if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
>> +return false;
>> +if (f2fs_is_drop_cache(cc->inode))
>> +return false;
>> +if (f2fs_is_volatile_file(cc->inode))
>> +return false;
>> +
>> +offset = i_size & (PAGE_SIZE - 1);
>> +if ((page->index > end_index) ||
>> +(page->index == end_index && !offset))
>> +return false;
>
> No need to
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
On Tue, Oct 22, 2019 at 10:16:02AM -0700, Jaegeuk Kim wrote: > diff --git a/Documentation/filesystems/f2fs.txt > b/Documentation/filesystems/f2fs.txt > index 29020af0cff9..d1accf665c86 100644 > --- a/Documentation/filesystems/f2fs.txt > +++ b/Documentation/filesystems/f2fs.txt > @@ -235,6 +235,13 @@ checkpoint=%s[:%u[%]] Set to "disable" to turn off > checkpointing. Set to "en > hide up to all remaining free space. The actual space > that > would be unusable can be viewed at > /sys/fs/f2fs//unusable > This space is reclaimed once checkpoint=enable. > +compress_algorithm=%s Control compress algorithm, currently f2fs supports > "lzo" > + and "lz4" algorithm. > +compress_log_size=%u Support configuring compress cluster size, the size > will > + be 4kb * (1 << %u), 16kb is minimum size, also it's > + default size. kb means kilobits, not kilobytes. > +compress_extension=%s Support adding specified extension, so that f2fs can > + enable compression on those corresponding file. What does "Support adding specified extension" mean? And does "so that f2fs can enable compression on those corresponding file" mean that f2fs can't enable compression on other files? Please be clear about what this option does. > > > > DEBUGFS ENTRIES > @@ -837,3 +844,44 @@ zero or random data, which is useful to the below > scenario where: > 4. address = fibmap(fd, offset) > 5. open(blkdev) > 6. write(blkdev, address) > + > +Compression implementation > +-- > + > +- New term named cluster is defined as basic unit of compression, file can > +be divided into multiple clusters logically. One cluster includes 4 << n > +(n >= 0) logical pages, compression size is also cluster size, each of > +cluster can be compressed or not. > + > +- In cluster metadata layout, one special flag is used to indicate cluster > +is compressed one or normal one, for compressed cluster, following metadata > +maps cluster to [1, 4 << n - 1] physical blocks, in where f2fs stores > +data including compress header and compressed data. In the code it's actually a special block address, not a "flag". > + > +- In order to eliminate write amplification during overwrite, F2FS only > +support compression on write-once file, data can be compressed only when > +all logical blocks in file are valid and cluster compress ratio is lower > +than specified threshold. > + > +- To enable compression on regular inode, there are three ways: > +* chattr +c file > +* chattr +c dir; touch dir/file > +* mount w/ -o compress_extension=ext; touch file.ext > + > +Compress metadata layout: > + [Dnode Structure] > + +---+ > + | cluster 1 | cluster 2 | . | cluster N | > + +---+ > + . . . . > + . .. . > + . Compressed Cluster ..Normal Cluster > . > ++--+-+-+-+ > +-+-+-+-+ > +|compr flag| block 1 | block 2 | block 3 | | block 1 | block 2 | block 3 | > block 4 | > ++--+-+-+-+ > +-+-+-+-+ > + . . > + . . > + . . > + +-+-+--++ > + | data length | data chksum | reserved | compressed data | > + +-+-+--++ > diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig > index 652fd2e2b23d..c12854c3b1a1 100644 > --- a/fs/f2fs/Kconfig > +++ b/fs/f2fs/Kconfig > @@ -6,6 +6,10 @@ config F2FS_FS > select CRYPTO > select CRYPTO_CRC32 > select F2FS_FS_XATTR if FS_ENCRYPTION > + select LZO_COMPRESS > + select LZO_DECOMPRESS > + select LZ4_COMPRESS > + select LZ4_DECOMPRESS As someone else suggested, there's not much need to support LZO, since LZ4 is usually better. Also, compression support should be behind a kconfig option, so it doesn't cause bloat or extra attack surface for people who don't want it. > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c > new file mode 100644 > index ..f276d82a67aa > --- /dev/null > +++ b/fs/f2fs/compress.c > @@ -0,0 +1,1066 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * f2fs compress support > + * > + * Copyright (c) 2019 Chao Yu > + */ > + > +#include > +#include > +#include > +#include >
Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression
Hi Jaegeuk and Chao,
Nice to see this finally getting into shape :) Great work
I'm excited to see possible use-cases for this in the future.
Would f2fs compress files automatically like how btrfs' "compress" option works?
Or is it per-extension basis for now?
On Wed, Oct 23, 2019 at 2:16 AM Jaegeuk Kim wrote:
> +compress_algorithm=%s Control compress algorithm, currently f2fs supports
> "lzo"
> + and "lz4" algorithm.
I see absolutely no reason to support regular lzo variant at this time.
Everyone should use lz4 instead of lzo. If one wants zlib-level
compression, they should use zstd.
However, there's recent conversation on new lzo-rle and how it could
be a better candidate than lz4.
Since the mainline now have lz4, zstd and lzo-rle, I don't think
supporting lzo is a good idea.
> diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
> index 652fd2e2b23d..c12854c3b1a1 100644
> --- a/fs/f2fs/Kconfig
> +++ b/fs/f2fs/Kconfig
> @@ -6,6 +6,10 @@ config F2FS_FS
> select CRYPTO
> select CRYPTO_CRC32
> select F2FS_FS_XATTR if FS_ENCRYPTION
> + select LZO_COMPRESS
> + select LZO_DECOMPRESS
> + select LZ4_COMPRESS
> + select LZ4_DECOMPRESS
This is a bad idea.
This unnecessarily increases kernel binary image when no the user
intends to change the defaults.
For example, my Android kernel doesn't use lzo anywhere and this
wouldn't be welcome.
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> new file mode 100644
> index ..f276d82a67aa
> --- /dev/null
> +++ b/fs/f2fs/compress.c
> @@ -0,0 +1,1066 @@
> +static unsigned int offset_in_cluster(struct compress_ctx *cc, pgoff_t index)
> +static unsigned int cluster_idx(struct compress_ctx *cc, pgoff_t index)
> +static unsigned int start_idx_of_cluster(struct compress_ctx *cc)
Looks like these would be better if they were explicitly marked as inline.
> +static void f2fs_init_compress_ops(struct f2fs_sb_info *sbi)
> +{
> + sbi->cops[COMPRESS_LZO] = _lzo_ops;
> + sbi->cops[COMPRESS_LZ4] = _lz4_ops;
> +}
Would it be possible for f2fs to use generic crypto compression APIs?
Hardcoding for lzo/lz4 would make it harder to venture future different options.
Have a look at mm/zswap.c:__zswap_pool_create_fallback().
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index c681f51e351b..775c96291490 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -155,6 +163,7 @@ struct f2fs_mount_info {
> #define F2FS_FEATURE_VERITY0x0400
> #define F2FS_FEATURE_SB_CHKSUM 0x0800
> #define F2FS_FEATURE_CASEFOLD 0x1000
> +#define F2FS_FEATURE_COMPRESSION 0x2000
How would older versions of f2fs behave if an image was used by the
latest f2fs and have compressed pages?
I hope fail-safes are in place.
Thanks.
> --
> 2.19.0.605.g01d371f741-goog
>
>
>
> ___
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
