Re: [f2fs-dev] [PATCH V4] f2fs: Avoid double lock for cp_rwsem during checkpoint
On 2020/5/9 0:10, Jaegeuk Kim wrote:
> Hi Sayali,
>
> In order to address the perf regression, how about this?
>
>>From 48418af635884803ffb35972df7958a2e6649322 Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim
> Date: Fri, 8 May 2020 09:08:37 -0700
> Subject: [PATCH] f2fs: avoid double lock for cp_rwsem during checkpoint
>
> There could be a scenario where f2fs_sync_node_pages gets
> called during checkpoint, which in turn tries to flush
> inline data and calls iput(). This results in deadlock as
> iput() tries to hold cp_rwsem, which is already held at the
> beginning by checkpoint->block_operations().
>
> Call stack :
>
> Thread A Thread B
> f2fs_write_checkpoint()
> - block_operations(sbi)
> - f2fs_lock_all(sbi);
> - down_write(&sbi->cp_rwsem);
>
> - open()
> - igrab()
> - write() write inline data
> - unlink()
> - f2fs_sync_node_pages()
> - if (is_inline_node(page))
> - flush_inline_data()
>- ilookup()
> page = f2fs_pagecache_get_page()
> if (!page)
> goto iput_out;
> iput_out:
> -close()
> -iput()
>iput(inode);
>- f2fs_evict_inode()
> - f2fs_truncate_blocks()
> - f2fs_lock_op()
>- down_read(&sbi->cp_rwsem);
>
> Fixes: 2049d4fcb057 ("f2fs: avoid multiple node page writes due to
> inline_data")
> Signed-off-by: Sayali Lokhande
> Signed-off-by: Jaegeuk Kim
> ---
> fs/f2fs/node.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 1db8cabf727ef..626d7daca09de 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1870,8 +1870,8 @@ int f2fs_sync_node_pages(struct f2fs_sb_info *sbi,
> goto continue_unlock;
> }
>
> - /* flush inline_data */
> - if (is_inline_node(page)) {
> + /* flush inline_data, if it's not sync path. */
> + if (do_balance && is_inline_node(page)) {
IIRC, this flow was designed to avoid running out of free space issue
during checkpoint:
2049d4fcb057 ("f2fs: avoid multiple node page writes due to inline_data")
The sceanrio is:
1. create fully node blocks
2. flush node blocks
3. write inline_data for all the node blocks again
4. flush node blocks redundantly
I guess this may cause failing one case of fstest.
Since block_operations->f2fs_sync_inode_meta has synced inode cache to
inode page, so in block_operations->f2fs_sync_node_pages, could we
check nlink before flush_inline_data():
if (is_inline_node(page)) {
if (IS_INODE(page) && raw_inode_page->i_links) {
flush_inline_data()
}
}
> clear_inline_node(page);
> unlock_page(page);
> flush_inline_data(sbi, ino_of_node(page));
>
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: compress: allow lz4 to compress data partially
Hi Xiang,
On 2020/5/8 18:23, Gao Xiang wrote:
> Hi Chao,
>
> On Fri, May 08, 2020 at 05:47:09PM +0800, Chao Yu wrote:
>> For lz4 worst compress case, caller should allocate buffer with size
>> of LZ4_compressBound(inputsize) for target compressed data storing.
>>
>> However lz4 supports partial data compression, so we can get rid of
>> output buffer size limitation now, then we can avoid 2 * 4KB size
>> intermediate buffer allocation when log_cluster_size is 2, and avoid
>> unnecessary compressing work of compressor if we can not save at
>> least 4KB space.
>>
>> Suggested-by: Daeho Jeong
>> Signed-off-by: Chao Yu
>> ---
>> fs/f2fs/compress.c | 15 +--
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
>> index 5e4947250262..23825f559bcf 100644
>> --- a/fs/f2fs/compress.c
>> +++ b/fs/f2fs/compress.c
>> @@ -228,7 +228,12 @@ static int lz4_init_compress_ctx(struct compress_ctx
>> *cc)
>> if (!cc->private)
>> return -ENOMEM;
>>
>> -cc->clen = LZ4_compressBound(PAGE_SIZE << cc->log_cluster_size);
>> +/*
>> + * we do not change cc->clen to LZ4_compressBound(inputsize) to
>> + * adapt worst compress case, because lz4 algorithm supports partial
>> + * compression.
>
> Actually, in this case the lz4 compressed block is not valid (at least not
> ended
> in a valid lz4 EOF), and AFAIK the current public lz4 API cannot keep on
> compressing this block. so IMO "partial compression" for an invalid lz4
> block may be confusing.
Yes, comments could be improved to avoid confusing.
>
> I think some words like "because lz4 implementation can handle output buffer
> budget properly" or similar stuff could be better.
It's better, let me change to use this, thanks for the advice.
Thanks,
>
> The same to the patch title and the commit message.
>
> Thanks,
> Gao Xiang
>
>
>> + */
>> +cc->clen = cc->rlen - PAGE_SIZE - COMPRESS_HEADER_SIZE;
>> return 0;
>> }
>>
>> @@ -244,11 +249,9 @@ static int lz4_compress_pages(struct compress_ctx *cc)
>>
>> len = LZ4_compress_default(cc->rbuf, cc->cbuf->cdata, cc->rlen,
>> cc->clen, cc->private);
>> -if (!len) {
>> -printk_ratelimited("%sF2FS-fs (%s): lz4 compress failed\n",
>> -KERN_ERR, F2FS_I_SB(cc->inode)->sb->s_id);
>> -return -EIO;
>> -}
>> +if (!len)
>> +return -EAGAIN;
>> +
>> cc->clen = len;
>> return 0;
>> }
>> --
>> 2.18.0.rc1
> .
>
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH] f2fs: remove blk_plugging in block_operations
blk_plugging doesn't seem to give any benefit.
Signed-off-by: Jaegeuk Kim
---
fs/f2fs/checkpoint.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 97b6378554b40..cc9dd3bbf2188 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1169,8 +1169,6 @@ static int block_operations(struct f2fs_sb_info *sbi)
struct blk_plug plug;
int err = 0, cnt = 0;
- blk_start_plug(&plug);
-
retry_flush_quotas:
f2fs_lock_all(sbi);
if (__need_flush_quota(sbi)) {
@@ -1198,7 +1196,7 @@ static int block_operations(struct f2fs_sb_info *sbi)
f2fs_unlock_all(sbi);
err = f2fs_sync_dirty_inodes(sbi, DIR_INODE);
if (err)
- goto out;
+ return err;
cond_resched();
goto retry_flush_quotas;
}
@@ -1214,7 +1212,7 @@ static int block_operations(struct f2fs_sb_info *sbi)
f2fs_unlock_all(sbi);
err = f2fs_sync_inode_meta(sbi);
if (err)
- goto out;
+ return err;
cond_resched();
goto retry_flush_quotas;
}
@@ -1229,7 +1227,7 @@ static int block_operations(struct f2fs_sb_info *sbi)
err = f2fs_sync_node_pages(sbi, &wbc, false, FS_CP_NODE_IO);
atomic_dec(&sbi->wb_sync_req[NODE]);
if (err)
- goto out;
+ return err;
cond_resched();
goto retry_flush_quotas;
}
@@ -1240,8 +1238,6 @@ static int block_operations(struct f2fs_sb_info *sbi)
*/
__prepare_cp_block(sbi);
up_write(&sbi->node_change);
-out:
- blk_finish_plug(&plug);
return err;
}
--
2.26.2.645.ge9eca65c58-goog
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH V4] f2fs: Avoid double lock for cp_rwsem during checkpoint
Hi Sayali,
In order to address the perf regression, how about this?
>From 48418af635884803ffb35972df7958a2e6649322 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim
Date: Fri, 8 May 2020 09:08:37 -0700
Subject: [PATCH] f2fs: avoid double lock for cp_rwsem during checkpoint
There could be a scenario where f2fs_sync_node_pages gets
called during checkpoint, which in turn tries to flush
inline data and calls iput(). This results in deadlock as
iput() tries to hold cp_rwsem, which is already held at the
beginning by checkpoint->block_operations().
Call stack :
Thread AThread B
f2fs_write_checkpoint()
- block_operations(sbi)
- f2fs_lock_all(sbi);
- down_write(&sbi->cp_rwsem);
- open()
- igrab()
- write() write inline data
- unlink()
- f2fs_sync_node_pages()
- if (is_inline_node(page))
- flush_inline_data()
- ilookup()
page = f2fs_pagecache_get_page()
if (!page)
goto iput_out;
iput_out:
-close()
-iput()
iput(inode);
- f2fs_evict_inode()
- f2fs_truncate_blocks()
- f2fs_lock_op()
- down_read(&sbi->cp_rwsem);
Fixes: 2049d4fcb057 ("f2fs: avoid multiple node page writes due to inline_data")
Signed-off-by: Sayali Lokhande
Signed-off-by: Jaegeuk Kim
---
fs/f2fs/node.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 1db8cabf727ef..626d7daca09de 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1870,8 +1870,8 @@ int f2fs_sync_node_pages(struct f2fs_sb_info *sbi,
goto continue_unlock;
}
- /* flush inline_data */
- if (is_inline_node(page)) {
+ /* flush inline_data, if it's not sync path. */
+ if (do_balance && is_inline_node(page)) {
clear_inline_node(page);
unlock_page(page);
flush_inline_data(sbi, ino_of_node(page));
--
2.26.2.645.ge9eca65c58-goog
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks
On 05/08, Jaegeuk Kim wrote:
> Is this v2?
nvm. it seems the same version.
>
> On 05/08, Daeho Jeong wrote:
> > From: Daeho Jeong
> >
> > Now, if writing pages and releasing compress blocks occur
> > simultaneously, and releasing cblocks is executed more than one time
> > to a file, then total block count of filesystem and block count of the
> > file could be incorrect and damaged.
> >
> > We have to execute releasing compress blocks only one time for a file
> > without being interfered by writepages path.
> >
> > Signed-off-by: Daeho Jeong
> > Reviewed-by: Chao Yu
> > ---
> > fs/f2fs/file.c | 34 --
> > 1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index 4aab4b42d8ba..f7de2a1da528 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file
> > *filp, unsigned long arg)
> > pgoff_t page_idx = 0, last_idx;
> > unsigned int released_blocks = 0;
> > int ret;
> > + int writecount;
> >
> > if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
> > return -EOPNOTSUPP;
> > @@ -3502,20 +3503,33 @@ static int f2fs_release_compress_blocks(struct file
> > *filp, unsigned long arg)
> > if (ret)
> > return ret;
> >
> > - if (!F2FS_I(inode)->i_compr_blocks)
> > - goto out;
> > -
> > f2fs_balance_fs(F2FS_I_SB(inode), true);
> >
> > inode_lock(inode);
> >
> > - if (!IS_IMMUTABLE(inode)) {
> > - F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
> > - f2fs_set_inode_flags(inode);
> > - inode->i_ctime = current_time(inode);
> > - f2fs_mark_inode_dirty_sync(inode, true);
> > + writecount = atomic_read(&inode->i_writecount);
> > + if ((filp->f_mode & FMODE_WRITE && writecount != 1) || writecount) {
> > + ret = -EBUSY;
> > + goto out;
> > }
> >
> > + if (IS_IMMUTABLE(inode)) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
> > + if (ret)
> > + goto out;
> > +
> > + if (!F2FS_I(inode)->i_compr_blocks)
> > + goto out;
> > +
> > + F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
> > + f2fs_set_inode_flags(inode);
> > + inode->i_ctime = current_time(inode);
> > + f2fs_mark_inode_dirty_sync(inode, true);
> > +
> > down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> > down_write(&F2FS_I(inode)->i_mmap_sem);
> >
> > @@ -3554,9 +3568,9 @@ static int f2fs_release_compress_blocks(struct file
> > *filp, unsigned long arg)
> >
> > up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> > up_write(&F2FS_I(inode)->i_mmap_sem);
> > -
> > - inode_unlock(inode);
> > out:
> > + inode_unlock(inode);
> > +
> > mnt_drop_write_file(filp);
> >
> > if (ret >= 0) {
> > --
> > 2.26.2.645.ge9eca65c58-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
Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks
Is this v2?
On 05/08, Daeho Jeong wrote:
> From: Daeho Jeong
>
> Now, if writing pages and releasing compress blocks occur
> simultaneously, and releasing cblocks is executed more than one time
> to a file, then total block count of filesystem and block count of the
> file could be incorrect and damaged.
>
> We have to execute releasing compress blocks only one time for a file
> without being interfered by writepages path.
>
> Signed-off-by: Daeho Jeong
> Reviewed-by: Chao Yu
> ---
> fs/f2fs/file.c | 34 --
> 1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 4aab4b42d8ba..f7de2a1da528 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file
> *filp, unsigned long arg)
> pgoff_t page_idx = 0, last_idx;
> unsigned int released_blocks = 0;
> int ret;
> + int writecount;
>
> if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
> return -EOPNOTSUPP;
> @@ -3502,20 +3503,33 @@ static int f2fs_release_compress_blocks(struct file
> *filp, unsigned long arg)
> if (ret)
> return ret;
>
> - if (!F2FS_I(inode)->i_compr_blocks)
> - goto out;
> -
> f2fs_balance_fs(F2FS_I_SB(inode), true);
>
> inode_lock(inode);
>
> - if (!IS_IMMUTABLE(inode)) {
> - F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
> - f2fs_set_inode_flags(inode);
> - inode->i_ctime = current_time(inode);
> - f2fs_mark_inode_dirty_sync(inode, true);
> + writecount = atomic_read(&inode->i_writecount);
> + if ((filp->f_mode & FMODE_WRITE && writecount != 1) || writecount) {
> + ret = -EBUSY;
> + goto out;
> }
>
> + if (IS_IMMUTABLE(inode)) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
> + if (ret)
> + goto out;
> +
> + if (!F2FS_I(inode)->i_compr_blocks)
> + goto out;
> +
> + F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
> + f2fs_set_inode_flags(inode);
> + inode->i_ctime = current_time(inode);
> + f2fs_mark_inode_dirty_sync(inode, true);
> +
> down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> down_write(&F2FS_I(inode)->i_mmap_sem);
>
> @@ -3554,9 +3568,9 @@ static int f2fs_release_compress_blocks(struct file
> *filp, unsigned long arg)
>
> up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> up_write(&F2FS_I(inode)->i_mmap_sem);
> -
> - inode_unlock(inode);
> out:
> + inode_unlock(inode);
> +
> mnt_drop_write_file(filp);
>
> if (ret >= 0) {
> --
> 2.26.2.645.ge9eca65c58-goog
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks
Hi Daeho,
Please let me integrate this patch into the original patch since it is still in
the dev branch.
Thanks,
On 05/08, Daeho Jeong wrote:
> From: Daeho Jeong
>
> Now, if writing pages and releasing compress blocks occur
> simultaneously, and releasing cblocks is executed more than one time
> to a file, then total block count of filesystem and block count of the
> file could be incorrect and damaged.
>
> We have to execute releasing compress blocks only one time for a file
> without being interfered by writepages path.
>
> Signed-off-by: Daeho Jeong
> ---
> fs/f2fs/file.c | 34 --
> 1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 4aab4b42d8ba..f7de2a1da528 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file
> *filp, unsigned long arg)
> pgoff_t page_idx = 0, last_idx;
> unsigned int released_blocks = 0;
> int ret;
> + int writecount;
>
> if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
> return -EOPNOTSUPP;
> @@ -3502,20 +3503,33 @@ static int f2fs_release_compress_blocks(struct file
> *filp, unsigned long arg)
> if (ret)
> return ret;
>
> - if (!F2FS_I(inode)->i_compr_blocks)
> - goto out;
> -
> f2fs_balance_fs(F2FS_I_SB(inode), true);
>
> inode_lock(inode);
>
> - if (!IS_IMMUTABLE(inode)) {
> - F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
> - f2fs_set_inode_flags(inode);
> - inode->i_ctime = current_time(inode);
> - f2fs_mark_inode_dirty_sync(inode, true);
> + writecount = atomic_read(&inode->i_writecount);
> + if ((filp->f_mode & FMODE_WRITE && writecount != 1) || writecount) {
> + ret = -EBUSY;
> + goto out;
> }
>
> + if (IS_IMMUTABLE(inode)) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
> + if (ret)
> + goto out;
> +
> + if (!F2FS_I(inode)->i_compr_blocks)
> + goto out;
> +
> + F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
> + f2fs_set_inode_flags(inode);
> + inode->i_ctime = current_time(inode);
> + f2fs_mark_inode_dirty_sync(inode, true);
> +
> down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> down_write(&F2FS_I(inode)->i_mmap_sem);
>
> @@ -3554,9 +3568,9 @@ static int f2fs_release_compress_blocks(struct file
> *filp, unsigned long arg)
>
> up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> up_write(&F2FS_I(inode)->i_mmap_sem);
> -
> - inode_unlock(inode);
> out:
> + inode_unlock(inode);
> +
> mnt_drop_write_file(filp);
>
> if (ret >= 0) {
> --
> 2.26.2.645.ge9eca65c58-goog
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks
From: Daeho Jeong
Now, if writing pages and releasing compress blocks occur
simultaneously, and releasing cblocks is executed more than one time
to a file, then total block count of filesystem and block count of the
file could be incorrect and damaged.
We have to execute releasing compress blocks only one time for a file
without being interfered by writepages path.
Signed-off-by: Daeho Jeong
Reviewed-by: Chao Yu
---
fs/f2fs/file.c | 34 --
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 4aab4b42d8ba..f7de2a1da528 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file
*filp, unsigned long arg)
pgoff_t page_idx = 0, last_idx;
unsigned int released_blocks = 0;
int ret;
+ int writecount;
if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
return -EOPNOTSUPP;
@@ -3502,20 +3503,33 @@ static int f2fs_release_compress_blocks(struct file
*filp, unsigned long arg)
if (ret)
return ret;
- if (!F2FS_I(inode)->i_compr_blocks)
- goto out;
-
f2fs_balance_fs(F2FS_I_SB(inode), true);
inode_lock(inode);
- if (!IS_IMMUTABLE(inode)) {
- F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
- f2fs_set_inode_flags(inode);
- inode->i_ctime = current_time(inode);
- f2fs_mark_inode_dirty_sync(inode, true);
+ writecount = atomic_read(&inode->i_writecount);
+ if ((filp->f_mode & FMODE_WRITE && writecount != 1) || writecount) {
+ ret = -EBUSY;
+ goto out;
}
+ if (IS_IMMUTABLE(inode)) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
+ if (ret)
+ goto out;
+
+ if (!F2FS_I(inode)->i_compr_blocks)
+ goto out;
+
+ F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
+ f2fs_set_inode_flags(inode);
+ inode->i_ctime = current_time(inode);
+ f2fs_mark_inode_dirty_sync(inode, true);
+
down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
down_write(&F2FS_I(inode)->i_mmap_sem);
@@ -3554,9 +3568,9 @@ static int f2fs_release_compress_blocks(struct file
*filp, unsigned long arg)
up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
up_write(&F2FS_I(inode)->i_mmap_sem);
-
- inode_unlock(inode);
out:
+ inode_unlock(inode);
+
mnt_drop_write_file(filp);
if (ret >= 0) {
--
2.26.2.645.ge9eca65c58-goog
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: compress: allow lz4 to compress data partially
Hi Chao,
On Fri, May 08, 2020 at 05:47:09PM +0800, Chao Yu wrote:
> For lz4 worst compress case, caller should allocate buffer with size
> of LZ4_compressBound(inputsize) for target compressed data storing.
>
> However lz4 supports partial data compression, so we can get rid of
> output buffer size limitation now, then we can avoid 2 * 4KB size
> intermediate buffer allocation when log_cluster_size is 2, and avoid
> unnecessary compressing work of compressor if we can not save at
> least 4KB space.
>
> Suggested-by: Daeho Jeong
> Signed-off-by: Chao Yu
> ---
> fs/f2fs/compress.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 5e4947250262..23825f559bcf 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -228,7 +228,12 @@ static int lz4_init_compress_ctx(struct compress_ctx *cc)
> if (!cc->private)
> return -ENOMEM;
>
> - cc->clen = LZ4_compressBound(PAGE_SIZE << cc->log_cluster_size);
> + /*
> + * we do not change cc->clen to LZ4_compressBound(inputsize) to
> + * adapt worst compress case, because lz4 algorithm supports partial
> + * compression.
Actually, in this case the lz4 compressed block is not valid (at least not ended
in a valid lz4 EOF), and AFAIK the current public lz4 API cannot keep on
compressing this block. so IMO "partial compression" for an invalid lz4
block may be confusing.
I think some words like "because lz4 implementation can handle output buffer
budget properly" or similar stuff could be better.
The same to the patch title and the commit message.
Thanks,
Gao Xiang
> + */
> + cc->clen = cc->rlen - PAGE_SIZE - COMPRESS_HEADER_SIZE;
> return 0;
> }
>
> @@ -244,11 +249,9 @@ static int lz4_compress_pages(struct compress_ctx *cc)
>
> len = LZ4_compress_default(cc->rbuf, cc->cbuf->cdata, cc->rlen,
> cc->clen, cc->private);
> - if (!len) {
> - printk_ratelimited("%sF2FS-fs (%s): lz4 compress failed\n",
> - KERN_ERR, F2FS_I_SB(cc->inode)->sb->s_id);
> - return -EIO;
> - }
> + if (!len)
> + return -EAGAIN;
> +
> cc->clen = len;
> return 0;
> }
> --
> 2.18.0.rc1
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks
On 2020/5/8 17:29, Daeho Jeong wrote: > From: Daeho Jeong > > Now, if writing pages and releasing compress blocks occur > simultaneously, and releasing cblocks is executed more than one time > to a file, then total block count of filesystem and block count of the > file could be incorrect and damaged. > > We have to execute releasing compress blocks only one time for a file > without being interfered by writepages path. > > Signed-off-by: Daeho Jeong Reviewed-by: Chao Yu Thanks, ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH v2] f2fs: shrink spinlock coverage
In f2fs_try_to_free_nids(), .nid_list_lock spinlock critical region will
increase as expected shrink number increase, to avoid spining other CPUs
for long time, we change to release nid caches with small batch each time
under .nid_list_lock coverage.
Signed-off-by: Chao Yu
---
v2:
- shrink free nid caches in batch under spinlock coverage.
fs/f2fs/node.c | 25 +++--
fs/f2fs/node.h | 3 +++
2 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 4da0d8713df5..1db8cabf727e 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -2488,7 +2488,6 @@ void f2fs_alloc_nid_failed(struct f2fs_sb_info *sbi,
nid_t nid)
int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi, int nr_shrink)
{
struct f2fs_nm_info *nm_i = NM_I(sbi);
- struct free_nid *i, *next;
int nr = nr_shrink;
if (nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
@@ -2497,17 +2496,23 @@ int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi, int
nr_shrink)
if (!mutex_trylock(&nm_i->build_lock))
return 0;
- spin_lock(&nm_i->nid_list_lock);
- list_for_each_entry_safe(i, next, &nm_i->free_nid_list, list) {
- if (nr_shrink <= 0 ||
- nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
- break;
+ while (nr_shrink && nm_i->nid_cnt[FREE_NID] > MAX_FREE_NIDS) {
+ struct free_nid *i, *next;
+ unsigned int batch = SHRINK_NID_BATCH_SIZE;
- __remove_free_nid(sbi, i, FREE_NID);
- kmem_cache_free(free_nid_slab, i);
- nr_shrink--;
+ spin_lock(&nm_i->nid_list_lock);
+ list_for_each_entry_safe(i, next, &nm_i->free_nid_list, list) {
+ if (!nr_shrink || !batch ||
+ nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
+ break;
+ __remove_free_nid(sbi, i, FREE_NID);
+ kmem_cache_free(free_nid_slab, i);
+ nr_shrink--;
+ batch--;
+ }
+ spin_unlock(&nm_i->nid_list_lock);
}
- spin_unlock(&nm_i->nid_list_lock);
+
mutex_unlock(&nm_i->build_lock);
return nr - nr_shrink;
diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
index e05af5df5648..33d677f83569 100644
--- a/fs/f2fs/node.h
+++ b/fs/f2fs/node.h
@@ -15,6 +15,9 @@
#define FREE_NID_PAGES 8
#define MAX_FREE_NIDS (NAT_ENTRY_PER_BLOCK * FREE_NID_PAGES)
+/* size of free nid batch when shrinking */
+#define SHRINK_NID_BATCH_SIZE 8
+
#define DEF_RA_NID_PAGES 0 /* # of nid pages to be readaheaded */
/* maximum readahead size for node during getting data blocks */
--
2.18.0.rc1
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH] f2fs: compress: allow lz4 to compress data partially
For lz4 worst compress case, caller should allocate buffer with size
of LZ4_compressBound(inputsize) for target compressed data storing.
However lz4 supports partial data compression, so we can get rid of
output buffer size limitation now, then we can avoid 2 * 4KB size
intermediate buffer allocation when log_cluster_size is 2, and avoid
unnecessary compressing work of compressor if we can not save at
least 4KB space.
Suggested-by: Daeho Jeong
Signed-off-by: Chao Yu
---
fs/f2fs/compress.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index 5e4947250262..23825f559bcf 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -228,7 +228,12 @@ static int lz4_init_compress_ctx(struct compress_ctx *cc)
if (!cc->private)
return -ENOMEM;
- cc->clen = LZ4_compressBound(PAGE_SIZE << cc->log_cluster_size);
+ /*
+* we do not change cc->clen to LZ4_compressBound(inputsize) to
+* adapt worst compress case, because lz4 algorithm supports partial
+* compression.
+*/
+ cc->clen = cc->rlen - PAGE_SIZE - COMPRESS_HEADER_SIZE;
return 0;
}
@@ -244,11 +249,9 @@ static int lz4_compress_pages(struct compress_ctx *cc)
len = LZ4_compress_default(cc->rbuf, cc->cbuf->cdata, cc->rlen,
cc->clen, cc->private);
- if (!len) {
- printk_ratelimited("%sF2FS-fs (%s): lz4 compress failed\n",
- KERN_ERR, F2FS_I_SB(cc->inode)->sb->s_id);
- return -EIO;
- }
+ if (!len)
+ return -EAGAIN;
+
cc->clen = len;
return 0;
}
--
2.18.0.rc1
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks
From: Daeho Jeong
Now, if writing pages and releasing compress blocks occur
simultaneously, and releasing cblocks is executed more than one time
to a file, then total block count of filesystem and block count of the
file could be incorrect and damaged.
We have to execute releasing compress blocks only one time for a file
without being interfered by writepages path.
Signed-off-by: Daeho Jeong
---
fs/f2fs/file.c | 34 --
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 4aab4b42d8ba..f7de2a1da528 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file
*filp, unsigned long arg)
pgoff_t page_idx = 0, last_idx;
unsigned int released_blocks = 0;
int ret;
+ int writecount;
if (!f2fs_sb_has_compression(F2FS_I_SB(inode)))
return -EOPNOTSUPP;
@@ -3502,20 +3503,33 @@ static int f2fs_release_compress_blocks(struct file
*filp, unsigned long arg)
if (ret)
return ret;
- if (!F2FS_I(inode)->i_compr_blocks)
- goto out;
-
f2fs_balance_fs(F2FS_I_SB(inode), true);
inode_lock(inode);
- if (!IS_IMMUTABLE(inode)) {
- F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
- f2fs_set_inode_flags(inode);
- inode->i_ctime = current_time(inode);
- f2fs_mark_inode_dirty_sync(inode, true);
+ writecount = atomic_read(&inode->i_writecount);
+ if ((filp->f_mode & FMODE_WRITE && writecount != 1) || writecount) {
+ ret = -EBUSY;
+ goto out;
}
+ if (IS_IMMUTABLE(inode)) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
+ if (ret)
+ goto out;
+
+ if (!F2FS_I(inode)->i_compr_blocks)
+ goto out;
+
+ F2FS_I(inode)->i_flags |= F2FS_IMMUTABLE_FL;
+ f2fs_set_inode_flags(inode);
+ inode->i_ctime = current_time(inode);
+ f2fs_mark_inode_dirty_sync(inode, true);
+
down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
down_write(&F2FS_I(inode)->i_mmap_sem);
@@ -3554,9 +3568,9 @@ static int f2fs_release_compress_blocks(struct file
*filp, unsigned long arg)
up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
up_write(&F2FS_I(inode)->i_mmap_sem);
-
- inode_unlock(inode);
out:
+ inode_unlock(inode);
+
mnt_drop_write_file(filp);
if (ret >= 0) {
--
2.26.2.645.ge9eca65c58-goog
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 1/4] f2fs: don't leak filename in f2fs_try_convert_inline_dir()
On 2020/5/7 15:59, Eric Biggers wrote:
> From: Eric Biggers
>
> We need to call fscrypt_free_filename() to free the memory allocated by
> fscrypt_setup_filename().
>
> Fixes: b06af2aff28b ("f2fs: convert inline_dir early before starting rename")
Thanks for fixing this.
> Cc: # v5.6+
> Signed-off-by: Eric Biggers
Reviewed-by: Chao Yu
Thanks,
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks
Oops, I will re-check it. Thanks, 2020년 5월 8일 (금) 오후 4:09, Chao Yu 님이 작성: > > On 2020/5/8 14:58, Daeho Jeong wrote: > > I moved checking i_compr_blocks phrase after calling inode_lock() > > already, because we should check this after writing pages. > > > > So, if it fails to check i_compr_blocks, we need to call inode_unlock(). > > > > Am I missing something? > > After applying this patch, I get this: > > ret = mnt_want_write_file(filp); > if (ret) > return ret; > > if (!F2FS_I(inode)->i_compr_blocks) > goto out; > > f2fs_balance_fs(F2FS_I_SB(inode), true); > > inode_lock(inode); > > > > > 2020년 5월 8일 (금) 오후 3:50, Chao Yu 님이 작성: > >> > >> On 2020/5/8 12:25, Daeho Jeong wrote: > >>> From: Daeho Jeong > >>> > >>> Now, if writing pages and releasing compress blocks occur > >>> simultaneously, and releasing cblocks is executed more than one time > >>> to a file, then total block count of filesystem and block count of the > >>> file could be incorrect and damaged. > >>> > >>> We have to execute releasing compress blocks only one time for a file > >>> without being interfered by writepages path. > >>> > >>> Signed-off-by: Daeho Jeong > >>> --- > >>> fs/f2fs/file.c | 31 --- > >>> 1 file changed, 24 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >>> index 4aab4b42d8ba..a92bc51b9b28 100644 > >>> --- a/fs/f2fs/file.c > >>> +++ b/fs/f2fs/file.c > >>> @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file > >>> *filp, unsigned long arg) > >>> pgoff_t page_idx = 0, last_idx; > >>> unsigned int released_blocks = 0; > >>> int ret; > >>> + int writecount; > >>> > >>> if (!f2fs_sb_has_compression(F2FS_I_SB(inode))) > >>> return -EOPNOTSUPP; > >> > >> Before inode_lock(), there is one case we may jump to out label, in > >> this case, we may unlock inode incorrectly. > >> > >> if (!F2FS_I(inode)->i_compr_blocks) > >> goto out; > >> > >>> - > >>> - inode_unlock(inode); > >>> out: > >>> + inode_unlock(inode); > >>> + > >>> mnt_drop_write_file(filp); > >> > >> Thanks, > > . > > ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks
On 2020/5/8 14:58, Daeho Jeong wrote: > I moved checking i_compr_blocks phrase after calling inode_lock() > already, because we should check this after writing pages. > > So, if it fails to check i_compr_blocks, we need to call inode_unlock(). > > Am I missing something? After applying this patch, I get this: ret = mnt_want_write_file(filp); if (ret) return ret; if (!F2FS_I(inode)->i_compr_blocks) goto out; f2fs_balance_fs(F2FS_I_SB(inode), true); inode_lock(inode); > > 2020년 5월 8일 (금) 오후 3:50, Chao Yu 님이 작성: >> >> On 2020/5/8 12:25, Daeho Jeong wrote: >>> From: Daeho Jeong >>> >>> Now, if writing pages and releasing compress blocks occur >>> simultaneously, and releasing cblocks is executed more than one time >>> to a file, then total block count of filesystem and block count of the >>> file could be incorrect and damaged. >>> >>> We have to execute releasing compress blocks only one time for a file >>> without being interfered by writepages path. >>> >>> Signed-off-by: Daeho Jeong >>> --- >>> fs/f2fs/file.c | 31 --- >>> 1 file changed, 24 insertions(+), 7 deletions(-) >>> >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>> index 4aab4b42d8ba..a92bc51b9b28 100644 >>> --- a/fs/f2fs/file.c >>> +++ b/fs/f2fs/file.c >>> @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file >>> *filp, unsigned long arg) >>> pgoff_t page_idx = 0, last_idx; >>> unsigned int released_blocks = 0; >>> int ret; >>> + int writecount; >>> >>> if (!f2fs_sb_has_compression(F2FS_I_SB(inode))) >>> return -EOPNOTSUPP; >> >> Before inode_lock(), there is one case we may jump to out label, in >> this case, we may unlock inode incorrectly. >> >> if (!F2FS_I(inode)->i_compr_blocks) >> goto out; >> >>> - >>> - inode_unlock(inode); >>> out: >>> + inode_unlock(inode); >>> + >>> mnt_drop_write_file(filp); >> >> Thanks, > . > ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: remove race condition in releasing cblocks
I moved checking i_compr_blocks phrase after calling inode_lock() already, because we should check this after writing pages. So, if it fails to check i_compr_blocks, we need to call inode_unlock(). Am I missing something? 2020년 5월 8일 (금) 오후 3:50, Chao Yu 님이 작성: > > On 2020/5/8 12:25, Daeho Jeong wrote: > > From: Daeho Jeong > > > > Now, if writing pages and releasing compress blocks occur > > simultaneously, and releasing cblocks is executed more than one time > > to a file, then total block count of filesystem and block count of the > > file could be incorrect and damaged. > > > > We have to execute releasing compress blocks only one time for a file > > without being interfered by writepages path. > > > > Signed-off-by: Daeho Jeong > > --- > > fs/f2fs/file.c | 31 --- > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > index 4aab4b42d8ba..a92bc51b9b28 100644 > > --- a/fs/f2fs/file.c > > +++ b/fs/f2fs/file.c > > @@ -3488,6 +3488,7 @@ static int f2fs_release_compress_blocks(struct file > > *filp, unsigned long arg) > > pgoff_t page_idx = 0, last_idx; > > unsigned int released_blocks = 0; > > int ret; > > + int writecount; > > > > if (!f2fs_sb_has_compression(F2FS_I_SB(inode))) > > return -EOPNOTSUPP; > > Before inode_lock(), there is one case we may jump to out label, in > this case, we may unlock inode incorrectly. > > if (!F2FS_I(inode)->i_compr_blocks) > goto out; > > > - > > - inode_unlock(inode); > > out: > > + inode_unlock(inode); > > + > > mnt_drop_write_file(filp); > > Thanks, ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
