Re: [f2fs-dev] [PATCH v2] f2fs: fix sbi->extent_list corruption issue

2019-01-04 Thread Sahitya Tummala 
On Mon, Nov 26, 2018 at 10:17:20AM +0530, Sahitya Tummala wrote:
> When there is a failure in f2fs_fill_super() after/during
> the recovery of fsync'd nodes, it frees the current sbi and
> retries again. This time the mount is successful, but the files
> that got recovered before retry, still holds the extent tree,
> whose extent nodes list is corrupted since sbi and sbi->extent_list
> is freed up. The list_del corruption issue is observed when the
> file system is getting unmounted and when those recoverd files extent
> node is being freed up in the below context.
> 
> list_del corruption. prev->next should be fff1e1ef5480, but was (null)
> <...>
> kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
> task: fff1f46f2280 task.stack: ff8008068000
> lr : __list_del_entry_valid+0x94/0xb4
> pc : __list_del_entry_valid+0x94/0xb4
> <...>
> Call trace:
> __list_del_entry_valid+0x94/0xb4
> __release_extent_node+0xb0/0x114
> __free_extent_tree+0x58/0x7c
> f2fs_shrink_extent_tree+0xdc/0x3b0
> f2fs_leave_shrinker+0x28/0x7c
> f2fs_put_super+0xfc/0x1e0
> generic_shutdown_super+0x70/0xf4
> kill_block_super+0x2c/0x5c
> kill_f2fs_super+0x44/0x50
> deactivate_locked_super+0x60/0x8c
> deactivate_super+0x68/0x74
> cleanup_mnt+0x40/0x78
> __cleanup_mnt+0x1c/0x28
> task_work_run+0x48/0xd0
> do_notify_resume+0x678/0xe98
> work_pending+0x8/0x14
> 
> Fix this by cleaning up inodes, extent tree and nodes of those
> recovered files before freeing up sbi and before next retry.
> 
Hi Jaegeuk, Chao,

I have observed another scenario where the similar list corruption issue
can happen with sbi->inode_list as well. If recover_fsync_data()
fails at some point in write_checkpoint() due to some error and if
those recovered inodes are still dirty, then after the mount is
successful, this issue is observed when that dirty inode is under 
writeback.

[   90.400500] list_del corruption. prev->next should be ffed1f566208, but 
was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18

I think it is better to cleanup those inodes completely before freeing sbi
and before next retry as done in this patch. Would you like to re-consider
this patch for this new issue?

Please share your comments.

Thanks,

> Signed-off-by: Sahitya Tummala 
> ---
> v2:
> -call evict_inodes() and f2fs_shrink_extent_tree() to cleanup inodes
> 
>  fs/f2fs/f2fs.h |  1 +
>  fs/f2fs/shrinker.c |  2 +-
>  fs/f2fs/super.c| 13 -
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 1e03197..aaee63b 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3407,6 +3407,7 @@ struct rb_entry *f2fs_lookup_rb_tree_ret(struct 
> rb_root_cached *root,
>  bool f2fs_check_rb_tree_consistence(struct f2fs_sb_info *sbi,
>   struct rb_root_cached *root);
>  unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int 
> nr_shrink);
> +unsigned long __count_extent_cache(struct f2fs_sb_info *sbi);
>  bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent *i_ext);
>  void f2fs_drop_extent_tree(struct inode *inode);
>  unsigned int f2fs_destroy_extent_node(struct inode *inode);
> diff --git a/fs/f2fs/shrinker.c b/fs/f2fs/shrinker.c
> index 9e13db9..7e3c13b 100644
> --- a/fs/f2fs/shrinker.c
> +++ b/fs/f2fs/shrinker.c
> @@ -30,7 +30,7 @@ static unsigned long __count_free_nids(struct f2fs_sb_info 
> *sbi)
>   return count > 0 ? count : 0;
>  }
>  
> -static unsigned long __count_extent_cache(struct f2fs_sb_info *sbi)
> +unsigned long __count_extent_cache(struct f2fs_sb_info *sbi)
>  {
>   return atomic_read(&sbi->total_zombie_tree) +
>   atomic_read(&sbi->total_ext_node);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index af58b2c..769e7b1 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -3016,6 +3016,16 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info 
> *sbi)
>   sbi->readdir_ra = 1;
>  }
>  
> +static void f2fs_cleanup_inodes(struct f2fs_sb_info *sbi)
> +{
> + struct super_block *sb = sbi->sb;
> +
> + sync_filesystem(sb);
> + shrink_dcache_sb(sb);
> + evict_inodes(sb);
> + f2fs_shrink_extent_tree(sbi, __count_extent_cache(sbi));
> +}
> +
>  static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  {
>   struct f2fs_sb_info *sbi;
> @@ -3402,6 +3412,8 @@ static int f2fs_fill_super(struct sup

Re: [f2fs-dev] [PATCH] f2fs: export pin_file flag to user

2019-01-04 Thread Chao Yu 
On 2019/1/4 12:19, Jaegeuk Kim wrote:
> This exports pin_file status to user.

Semantics of pin_file flag is the same as nocow flag which is more widely
used in lsattr/chattr and vfs now.

#define FS_NOCOW_FL 0x0080 /* Do not cow file */

lsattr/chattr
no copy on write (C)

How about exporting it via F2FS_NOCOW_FL(0x0080)?

Thanks,

> 
> Signed-off-by: Jaegeuk Kim 
> ---
>  fs/f2fs/f2fs.h | 3 ++-
>  fs/f2fs/file.c | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 12fabd6735dd..036fd62010c1 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2303,9 +2303,10 @@ static inline void f2fs_change_bit(unsigned int nr, 
> char *addr)
>  #define F2FS_EOFBLOCKS_FL0x0040 /* Blocks allocated beyond 
> EOF */
>  #define F2FS_INLINE_DATA_FL  0x1000 /* Inode has inline data. */
>  #define F2FS_PROJINHERIT_FL  0x2000 /* Create with parents 
> projid */
> +#define F2FS_PIN_FILE_FL 0x4000 /* pin_file status */
>  #define F2FS_RESERVED_FL 0x8000 /* reserved for ext4 lib */
>  
> -#define F2FS_FL_USER_VISIBLE 0x304BDFFF /* User visible flags */
> +#define F2FS_FL_USER_VISIBLE 0x704BDFFF /* User visible flags */
>  #define F2FS_FL_USER_MODIFIABLE  0x204BC0FF /* User modifiable 
> flags */
>  
>  /* Flags we can manipulate with through F2FS_IOC_FSSETXATTR */
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index ae2b45e75847..eb099f1ee937 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1651,6 +1651,8 @@ static int f2fs_ioc_getflags(struct file *filp, 
> unsigned long arg)
>   flags |= F2FS_ENCRYPT_FL;
>   if (f2fs_has_inline_data(inode) || f2fs_has_inline_dentry(inode))
>   flags |= F2FS_INLINE_DATA_FL;
> + if (is_inode_flag_set(inode, FI_PIN_FILE))
> + flags |= F2FS_PIN_FILE_FL;
>  
>   flags &= F2FS_FL_USER_VISIBLE;
>  
> 



___
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/2] f2fs: wait on atomic writes to count F2FS_CP_WB_DATA

2019-01-04 Thread Chao Yu 
On 2019/1/4 12:20, Jaegeuk Kim wrote:
> Otherwise, we can get wrong counts incurring checkpoint hang.
> 
> IO_W (CP:  -24, Data:   24, Flush: (   001), Discard: (   00))
> 
> Cc: 
> Signed-off-by: Jaegeuk Kim 

Good catch! ;)

I can understand this condition, but for other new developer who reads this
commit, it will be a little hard to understand situation here.

How about explaining a little more about problem here, maybe:

Thread AThread B
- f2fs_write_data_pages
 -  __write_data_page
  - f2fs_submit_page_write
   - inc_page_count(F2FS_WB_DATA)
 type is F2FS_WB_DATA due to file is non-atomic one
- f2fs_ioc_start_atomic_write
 - set_inode_flag(FI_ATOMIC_FILE)
- f2fs_write_end_io
 - dec_page_count(F2FS_WB_CP_DATA)
   type is F2FS_WB_DATA due to file becomes
   atomic one

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 2/2] f2fs: don't access node/meta inode mapping after iput

2019-01-04 Thread Chao Yu 
On 2019/1/4 12:20, Jaegeuk Kim wrote:
> This fixes wrong access of address spaces of node and meta inodes after iput.
> 
> Fixes: 60aa4d5536ab ("f2fs: fix use-after-free issue when accessing 
> sbi->stat_info")
> Signed-off-by: Jaegeuk Kim 

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] f2fs: check inject_rate validity during configuring

2019-01-04 Thread Chao Yu 
Type of inject_rate is unsigned int, let's check new value's
validity during configuring.

Signed-off-by: Chao Yu 
---
 fs/f2fs/sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index 0575edbe3ed6..02d4012a9183 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -222,6 +222,8 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
 #ifdef CONFIG_F2FS_FAULT_INJECTION
if (a->struct_type == FAULT_INFO_TYPE && t >= (1 << FAULT_MAX))
return -EINVAL;
+   if (a->struct_type == FAULT_INFO_RATE && t >= UINT_MAX)
+   return -EINVAL;
 #endif
if (a->struct_type == RESERVED_BLOCKS) {
spin_lock(&sbi->stat_lock);
-- 
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 v2 1/2] f2fs-dev: support multi-device direct IO

2019-01-04 Thread sunqiuyang 
From: Qiuyang Sun 

Changelog v1 ==> v2:
1. Modify the definition of update_device_state(),
   and call it in direct write;
2. Move some local variables into branches where they are used.

Signed-off-by: Qiuyang Sun 
---
 fs/f2fs/data.c| 25 -
 fs/f2fs/f2fs.h|  3 +--
 fs/f2fs/segment.c | 11 +--
 3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index e5cd3fd..010300c 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1076,6 +1076,7 @@ int f2fs_map_blocks(struct inode *inode, struct 
f2fs_map_blocks *map,
struct extent_info ei = {0,0,0};
block_t blkaddr;
unsigned int start_pgofs;
+   block_t end_blk;
 
if (!maxblocks)
return 0;
@@ -1207,8 +1208,17 @@ int f2fs_map_blocks(struct inode *inode, struct 
f2fs_map_blocks *map,
 
map->m_pblk = blkaddr;
map->m_len = 1;
+
+   if (sbi->s_ndevs && blkaddr != NEW_ADDR &&
+   blkaddr != NULL_ADDR) {
+   int devi;
+
+   devi = f2fs_target_device_index(sbi, blkaddr);
+   end_blk = FDEV(devi).end_blk;
+   }
} else if ((map->m_pblk != NEW_ADDR &&
-   blkaddr == (map->m_pblk + ofs)) ||
+   blkaddr == (map->m_pblk + ofs) &&
+   (!sbi->s_ndevs || blkaddr <= end_blk)) ||
(map->m_pblk == NEW_ADDR && blkaddr == NEW_ADDR) ||
flag == F2FS_GET_BLOCK_PRE_DIO) {
ofs++;
@@ -1322,6 +1332,7 @@ static int __get_data_block(struct inode *inode, sector_t 
iblock,
 {
struct f2fs_map_blocks map;
int err;
+   struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 
map.m_lblk = iblock;
map.m_len = bh->b_size >> inode->i_blkbits;
@@ -1333,6 +1344,18 @@ static int __get_data_block(struct inode *inode, 
sector_t iblock,
err = f2fs_map_blocks(inode, &map, create, flag);
if (!err) {
map_bh(bh, inode->i_sb, map.m_pblk);
+   if (sbi->s_ndevs) {
+   int devi;
+
+   devi = f2fs_target_device_index(sbi, map.m_pblk);
+   if (devi) {
+   bh->b_bdev = FDEV(devi).bdev;
+   bh->b_blocknr -= FDEV(devi).start_blk;
+   }
+   if (may_write)
+   update_device_state(sbi, inode->i_ino,
+   map.m_pblk);
+   }
bh->b_state = (bh->b_state & ~F2FS_MAP_FLAGS) | map.m_flags;
bh->b_size = (u64)map.m_len << inode->i_blkbits;
}
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index eeede26..659e1e0 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3056,6 +3056,7 @@ int f2fs_lookup_journal_in_cursum(struct f2fs_journal 
*journal, int type,
 int f2fs_rw_hint_to_seg_type(enum rw_hint hint);
 enum rw_hint f2fs_io_type_to_rw_hint(struct f2fs_sb_info *sbi,
enum page_type type, enum temp_type temp);
+void update_device_state(struct f2fs_sb_info *sbi, nid_t ino, block_t blkaddr);
 
 /*
  * checkpoint.c
@@ -3595,8 +3596,6 @@ static inline bool f2fs_force_buffered_io(struct inode 
*inode,
 
if (f2fs_post_read_required(inode))
return true;
-   if (sbi->s_ndevs)
-   return true;
/*
 * for blkzoned device, fallback direct IO to buffered IO, so
 * all IOs can be serialized by log-structured write.
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index a361d61..eec5db1 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -3050,18 +3050,17 @@ void f2fs_allocate_data_block(struct f2fs_sb_info *sbi, 
struct page *page,
up_read(&SM_I(sbi)->curseg_lock);
 }
 
-static void update_device_state(struct f2fs_io_info *fio)
+void update_device_state(struct f2fs_sb_info *sbi, nid_t ino, block_t blkaddr)
 {
-   struct f2fs_sb_info *sbi = fio->sbi;
unsigned int devidx;
 
if (!sbi->s_ndevs)
return;
 
-   devidx = f2fs_target_device_index(sbi, fio->new_blkaddr);
+   devidx = f2fs_target_device_index(sbi, blkaddr);
 
/* update device state for fsync */
-   f2fs_set_dirty_device(sbi, fio->ino, devidx, FLUSH_INO);
+   f2fs_set_dirty_device(sbi, ino, devidx, FLUSH_INO);
 
/* update device state for checkpoint */
if (!f2fs_test_bit(devidx, (char *)&sbi->dirty_device)) {
@@ -3092,7 +3091,7 @@ static void do_write_page(struct f2fs_summary *sum, 
struct f2fs_io_info *fio)
goto reallocate;
}
 
-   update_device_state(fio);
+   update_device_state(fio->sbi, fio->ino, fio->new_blkaddr);
 
if (keep_order)
up_read(&fio->sbi->io_order_lock);
@@ -3168,7

[f2fs-dev] [PATCH] f2fs: check if file namelen exceeds max value

2019-01-04 Thread Sheng Yong 
Dentry bitmap is not enough to detect incorrect dentries. So this patch
also checks the namelen value of a dentry.

Signed-off-by: Gong Chen 
Signed-off-by: Sheng Yong 
---
 fs/f2fs/dir.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index c0c845da12fa..07cfd826f090 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -810,7 +810,8 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct 
f2fs_dentry_ptr *d,
 
/* check memory boundary before moving forward */
bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
-   if (unlikely(bit_pos > d->max)) {
+   if (unlikely(bit_pos > d->max) ||
+   le16_to_cpu(de->name_len > F2FS_NAME_LEN)) {
f2fs_msg(sbi->sb, KERN_WARNING,
"%s: corrupted namelen=%d, run fsck to fix.",
__func__, le16_to_cpu(de->name_len));
-- 
2.17.1



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH] f2fs: no need to check return value of debugfs_create functions

2019-01-04 Thread Greg Kroah-Hartman 
When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Cc: Jaegeuk Kim 
Cc: Chao Yu 
Cc: linux-f2fs-devel@lists.sourceforge.net
Signed-off-by: Greg Kroah-Hartman 
---
 fs/f2fs/debug.c | 20 +++-
 fs/f2fs/f2fs.h  |  4 ++--
 fs/f2fs/super.c |  5 +
 3 files changed, 6 insertions(+), 23 deletions(-)

diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
index ebcc121920ba..fd7f170e2f2d 100644
--- a/fs/f2fs/debug.c
+++ b/fs/f2fs/debug.c
@@ -506,30 +506,16 @@ void f2fs_destroy_stats(struct f2fs_sb_info *sbi)
kvfree(si);
 }
 
-int __init f2fs_create_root_stats(void)
+void __init f2fs_create_root_stats(void)
 {
-   struct dentry *file;
-
f2fs_debugfs_root = debugfs_create_dir("f2fs", NULL);
-   if (!f2fs_debugfs_root)
-   return -ENOMEM;
 
-   file = debugfs_create_file("status", S_IRUGO, f2fs_debugfs_root,
-   NULL, &stat_fops);
-   if (!file) {
-   debugfs_remove(f2fs_debugfs_root);
-   f2fs_debugfs_root = NULL;
-   return -ENOMEM;
-   }
-
-   return 0;
+   debugfs_create_file("status", S_IRUGO, f2fs_debugfs_root, NULL,
+   &stat_fops);
 }
 
 void f2fs_destroy_root_stats(void)
 {
-   if (!f2fs_debugfs_root)
-   return;
-
debugfs_remove_recursive(f2fs_debugfs_root);
f2fs_debugfs_root = NULL;
 }
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 12fabd6735dd..8f23ee6e8eb9 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3328,7 +3328,7 @@ static inline struct f2fs_stat_info *F2FS_STAT(struct 
f2fs_sb_info *sbi)
 
 int f2fs_build_stats(struct f2fs_sb_info *sbi);
 void f2fs_destroy_stats(struct f2fs_sb_info *sbi);
-int __init f2fs_create_root_stats(void);
+void __init f2fs_create_root_stats(void);
 void f2fs_destroy_root_stats(void);
 #else
 #define stat_inc_cp_count(si)  do { } while (0)
@@ -3366,7 +3366,7 @@ void f2fs_destroy_root_stats(void);
 
 static inline int f2fs_build_stats(struct f2fs_sb_info *sbi) { return 0; }
 static inline void f2fs_destroy_stats(struct f2fs_sb_info *sbi) { }
-static inline int __init f2fs_create_root_stats(void) { return 0; }
+static inline void __init f2fs_create_root_stats(void) { }
 static inline void f2fs_destroy_root_stats(void) { }
 #endif
 
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index c46a1d4318d4..3d3ce9eb6d13 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3545,9 +3545,7 @@ static int __init init_f2fs_fs(void)
err = register_filesystem(&f2fs_fs_type);
if (err)
goto free_shrinker;
-   err = f2fs_create_root_stats();
-   if (err)
-   goto free_filesystem;
+   f2fs_create_root_stats();
err = f2fs_init_post_read_processing();
if (err)
goto free_root_stats;
@@ -3555,7 +3553,6 @@ static int __init init_f2fs_fs(void)
 
 free_root_stats:
f2fs_destroy_root_stats();
-free_filesystem:
unregister_filesystem(&f2fs_fs_type);
 free_shrinker:
unregister_shrinker(&f2fs_shrinker_info);
-- 
2.20.1



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2] f2fs: fix sbi->extent_list corruption issue

2019-01-04 Thread Jaegeuk Kim 
On 01/04, Sahitya Tummala wrote:
> On Mon, Nov 26, 2018 at 10:17:20AM +0530, Sahitya Tummala wrote:
> > When there is a failure in f2fs_fill_super() after/during
> > the recovery of fsync'd nodes, it frees the current sbi and
> > retries again. This time the mount is successful, but the files
> > that got recovered before retry, still holds the extent tree,
> > whose extent nodes list is corrupted since sbi and sbi->extent_list
> > is freed up. The list_del corruption issue is observed when the
> > file system is getting unmounted and when those recoverd files extent
> > node is being freed up in the below context.
> > 
> > list_del corruption. prev->next should be fff1e1ef5480, but was (null)
> > <...>
> > kernel BUG at kernel/msm-4.14/lib/list_debug.c:53!
> > task: fff1f46f2280 task.stack: ff8008068000
> > lr : __list_del_entry_valid+0x94/0xb4
> > pc : __list_del_entry_valid+0x94/0xb4
> > <...>
> > Call trace:
> > __list_del_entry_valid+0x94/0xb4
> > __release_extent_node+0xb0/0x114
> > __free_extent_tree+0x58/0x7c
> > f2fs_shrink_extent_tree+0xdc/0x3b0
> > f2fs_leave_shrinker+0x28/0x7c
> > f2fs_put_super+0xfc/0x1e0
> > generic_shutdown_super+0x70/0xf4
> > kill_block_super+0x2c/0x5c
> > kill_f2fs_super+0x44/0x50
> > deactivate_locked_super+0x60/0x8c
> > deactivate_super+0x68/0x74
> > cleanup_mnt+0x40/0x78
> > __cleanup_mnt+0x1c/0x28
> > task_work_run+0x48/0xd0
> > do_notify_resume+0x678/0xe98
> > work_pending+0x8/0x14
> > 
> > Fix this by cleaning up inodes, extent tree and nodes of those
> > recovered files before freeing up sbi and before next retry.
> > 
> Hi Jaegeuk, Chao,
> 
> I have observed another scenario where the similar list corruption issue
> can happen with sbi->inode_list as well. If recover_fsync_data()
> fails at some point in write_checkpoint() due to some error and if
> those recovered inodes are still dirty, then after the mount is
> successful, this issue is observed when that dirty inode is under 
> writeback.

recover_fsync_data() does iget/iput in pair, and destroy_fsync_dnodes() drops
its dirty list and call iput(), when there is an error. So, after then, there'd
be no dirty inodes. If there's no error, checkpoint() flushes quota/dentry pages
in dirty inodes as well. Can we check where this dirty inode came from?

Oh, one sceanrio can be an error by f2fs_disable_checkpoint() which will do GC.

> 
> [   90.400500] list_del corruption. prev->next should be ffed1f566208, 
> but was (null)
> [   90.675349] Call trace:
> [   90.677869]  __list_del_entry_valid+0x94/0xb4
> [   90.682351]  remove_dirty_inode+0xac/0x114
> [   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
> [   90.691302]  f2fs_write_data_pages+0x40/0x4c
> [   90.695695]  do_writepages+0x80/0xf0
> [   90.699372]  __writeback_single_inode+0xdc/0x4ac
> [   90.704113]  writeback_sb_inodes+0x280/0x440
> [   90.708501]  wb_writeback+0x1b8/0x3d0
> [   90.712267]  wb_workfn+0x1a8/0x4d4
> [   90.715765]  process_one_work+0x1c0/0x3d4
> [   90.719883]  worker_thread+0x224/0x344
> [   90.723739]  kthread+0x120/0x130
> [   90.727055]  ret_from_fork+0x10/0x18
> 
> I think it is better to cleanup those inodes completely before freeing sbi
> and before next retry as done in this patch. Would you like to re-consider
> this patch for this new issue?

The patch was merged in mainline already.
Could you take a look at this patch?

>From cb1d20e640402beed300c2bdce79311ee8a781ad Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim 
Date: Fri, 4 Jan 2019 12:29:00 -0800
Subject: [PATCH] f2fs: sync filesystem after roll-forward recovery

Some works after roll-forward recovery can get an error which will release
all the data structures. Let's flush them in order to make it clean.

One possible corruption came from:

[   90.400500] list_del corruption. prev->next should be ffed1f566208, but 
was (null)
[   90.675349] Call trace:
[   90.677869]  __list_del_entry_valid+0x94/0xb4
[   90.682351]  remove_dirty_inode+0xac/0x114
[   90.686563]  __f2fs_write_data_pages+0x6a8/0x6c8
[   90.691302]  f2fs_write_data_pages+0x40/0x4c
[   90.695695]  do_writepages+0x80/0xf0
[   90.699372]  __writeback_single_inode+0xdc/0x4ac
[   90.704113]  writeback_sb_inodes+0x280/0x440
[   90.708501]  wb_writeback+0x1b8/0x3d0
[   90.712267]  wb_workfn+0x1a8/0x4d4
[   90.715765]  process_one_work+0x1c0/0x3d4
[   90.719883]  worker_thread+0x224/0x344
[   90.723739]  kthread+0x120/0x130
[   90.727055]  ret_from_fork+0x10/0x18

Reported-by: Sahitya Tummala 
Signed-off-by: Jaegeuk Kim 
---
 fs/f2fs/super.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 547cb7459be7..bb02186293a3 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3357,7 +3357,7 @@ static int f2fs_fill_super(struct super_block *sb, void 
*data, int silent)
if (test_opt(sbi, DISABLE_CHECKPOINT)) {
err = f2fs_disable_checkpoint(sbi);
if (err)
-   goto free_meta;
+

Re: [f2fs-dev] [PATCH] f2fs: export pin_file flag to user

2019-01-04 Thread Jaegeuk Kim 
On 01/04, Chao Yu wrote:
> On 2019/1/4 12:19, Jaegeuk Kim wrote:
> > This exports pin_file status to user.
> 
> Semantics of pin_file flag is the same as nocow flag which is more widely
> used in lsattr/chattr and vfs now.
> 
> #define FS_NOCOW_FL 0x0080 /* Do not cow file */
> 
> lsattr/chattr
> no copy on write (C)
> 
> How about exporting it via F2FS_NOCOW_FL(0x0080)?

Yeah, let me send v2.

> 
> Thanks,
> 
> > 
> > Signed-off-by: Jaegeuk Kim 
> > ---
> >  fs/f2fs/f2fs.h | 3 ++-
> >  fs/f2fs/file.c | 2 ++
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 12fabd6735dd..036fd62010c1 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -2303,9 +2303,10 @@ static inline void f2fs_change_bit(unsigned int nr, 
> > char *addr)
> >  #define F2FS_EOFBLOCKS_FL  0x0040 /* Blocks allocated beyond 
> > EOF */
> >  #define F2FS_INLINE_DATA_FL0x1000 /* Inode has inline 
> > data. */
> >  #define F2FS_PROJINHERIT_FL0x2000 /* Create with 
> > parents projid */
> > +#define F2FS_PIN_FILE_FL   0x4000 /* pin_file status */
> >  #define F2FS_RESERVED_FL   0x8000 /* reserved for ext4 lib */
> >  
> > -#define F2FS_FL_USER_VISIBLE   0x304BDFFF /* User visible 
> > flags */
> > +#define F2FS_FL_USER_VISIBLE   0x704BDFFF /* User visible 
> > flags */
> >  #define F2FS_FL_USER_MODIFIABLE0x204BC0FF /* User modifiable 
> > flags */
> >  
> >  /* Flags we can manipulate with through F2FS_IOC_FSSETXATTR */
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index ae2b45e75847..eb099f1ee937 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -1651,6 +1651,8 @@ static int f2fs_ioc_getflags(struct file *filp, 
> > unsigned long arg)
> > flags |= F2FS_ENCRYPT_FL;
> > if (f2fs_has_inline_data(inode) || f2fs_has_inline_dentry(inode))
> > flags |= F2FS_INLINE_DATA_FL;
> > +   if (is_inode_flag_set(inode, FI_PIN_FILE))
> > +   flags |= F2FS_PIN_FILE_FL;
> >  
> > flags &= F2FS_FL_USER_VISIBLE;
> >  
> > 


___
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/2] f2fs: wait on atomic writes to count F2FS_CP_WB_DATA

2019-01-04 Thread Jaegeuk Kim 
On 01/04, Chao Yu wrote:
> On 2019/1/4 12:20, Jaegeuk Kim wrote:
> > Otherwise, we can get wrong counts incurring checkpoint hang.
> > 
> > IO_W (CP:  -24, Data:   24, Flush: (   001), Discard: (   00))
> > 
> > Cc: 
> > Signed-off-by: Jaegeuk Kim 
> 
> Good catch! ;)
> 
> I can understand this condition, but for other new developer who reads this
> commit, it will be a little hard to understand situation here.
> 
> How about explaining a little more about problem here, maybe:
> 
> Thread A  Thread B
> - f2fs_write_data_pages
>  -  __write_data_page
>   - f2fs_submit_page_write
>- inc_page_count(F2FS_WB_DATA)
>  type is F2FS_WB_DATA due to file is non-atomic one
> - f2fs_ioc_start_atomic_write
>  - set_inode_flag(FI_ATOMIC_FILE)
>   - f2fs_write_end_io
>- dec_page_count(F2FS_WB_CP_DATA)
>  type is F2FS_WB_DATA due to file becomes
>  atomic one
> 
> Reviewed-by: Chao Yu 

Thanks, added the comment. :P

> 
> 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 v2 01/12] fs-verity: add a documentation file

2019-01-04 Thread Daniel Colascione via Linux-f2fs-devel 
On Sat, Dec 22, 2018 at 8:46 PM Theodore Y. Ts'o  wrote:
>
> On Sat, Dec 22, 2018 at 08:10:07PM -0800, Matthew Wilcox wrote:
> > Pretty much every file format has the ability to put arbitrary blocks
> > of information into a file somewhere the tools which don't know about
> > it will skip it.  For example, ZIP "includes an extra field facility
> > within file headers, which can be used to store extra data not defined
> > by existing ZIP specifications, and which allow compliant archivers that
> > do not recognize the fields to safely skip them. Header IDs 0–31 are
> > reserved for use by PKWARE. The remaining IDs can be used by third-party
> > vendors for proprietary usage. " (Wikipedia)
> >
> > ELF, PNG, PDF and many other formats have the ability to put data
> > _somewhere_.  It might not be at the tail of the file, but there's
> > somewhere to do it.
> >
> > (I appreciate this isn't what Linus is asking for, but I'm pointing out
> > that this is by no means as intractable as you make it sound.)
>
> That design would require the fs-verity code to know the type of eacho
> file, and where to find the in-band Merkle tree for each file type
> that we wanted to support.  And if you wanted to use fs-verity to
> protect a sudoers text configuration file (for example), we'd have to
> teach sudo how to ignore the userspace visible Merkle tree.

I'm pretty late to the game, but I just want to bring up one approach
that I'm not sure people have previously considered. You can't put the
verification blob in an xattr due to xattr size limits, but you *can*
put a filename in an xattr. What if, at open time, fs-verity looked
for a specially-named xattr attached to a file, resolved that name
like a symlink target, opened the pointed-to file, and just used
*that* as the authentication blob? It'd also be possible to teach
unlink to delete the pointed-to file when the pointer file is deleted
--- sort of like a simple and stupid kind of data fork.

For example, if you wanted to secure /usr/bin/emacs, you could set an
security.fsverify.verification_file xattr (in the system namespace
because the xattr has special semantics) to
"/.verification-blobs/@usr@bin@emacs.hashtree" or something like that.
Then, open(2) on /usr/bin/emacs would, internally to VFS, also open
/.verification-blobs/@usr@bin@emacs.hashtree and read verification
data from it, transparently to both users and the underlying
filesystem. If someone deleted /usr/bin/emacs, VFS would automatically
delete /.verification-blobs/@usr@bin@emacs.hashtree. If
/.verification-blobs/@usr@bin@emacs.hashtree didn't exist at time of
open(2) of /usr/bin/emacs, or couldn't be opened for whatever reason,
the open(2) of /usr/bin/emacs would fail.

ISTM that a scheme like this would give you some of the advantages of
jumbo xattrs, but with much less implementation complexity. If
someone's proposed something like this before, sorry for the noise.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2] f2fs: export FS_NOCOW_FL flag to user

2019-01-04 Thread Jaegeuk Kim 
This exports pin_file status to user.

Signed-off-by: Jaegeuk Kim 
---
v2:
 - use F2FS_NOCOW_FL (aka FS_NOCOW_FL)

 fs/f2fs/f2fs.h | 3 ++-
 fs/f2fs/file.c | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 12fabd6735dd..9286ec381453 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2301,11 +2301,12 @@ static inline void f2fs_change_bit(unsigned int nr, 
char *addr)
 #define F2FS_EXTENTS_FL0x0008 /* Inode uses 
extents */
 #define F2FS_EA_INODE_FL   0x0020 /* Inode used for large EA */
 #define F2FS_EOFBLOCKS_FL  0x0040 /* Blocks allocated beyond 
EOF */
+#define F2FS_NOCOW_FL  0x0080 /* Do not cow file */
 #define F2FS_INLINE_DATA_FL0x1000 /* Inode has inline data. */
 #define F2FS_PROJINHERIT_FL0x2000 /* Create with parents 
projid */
 #define F2FS_RESERVED_FL   0x8000 /* reserved for ext4 lib */
 
-#define F2FS_FL_USER_VISIBLE   0x304BDFFF /* User visible flags */
+#define F2FS_FL_USER_VISIBLE   0x30CBDFFF /* User visible flags */
 #define F2FS_FL_USER_MODIFIABLE0x204BC0FF /* User modifiable 
flags */
 
 /* Flags we can manipulate with through F2FS_IOC_FSSETXATTR */
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index ae2b45e75847..0d461321edfc 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1651,6 +1651,8 @@ static int f2fs_ioc_getflags(struct file *filp, unsigned 
long arg)
flags |= F2FS_ENCRYPT_FL;
if (f2fs_has_inline_data(inode) || f2fs_has_inline_dentry(inode))
flags |= F2FS_INLINE_DATA_FL;
+   if (is_inode_flag_set(inode, FI_PIN_FILE))
+   flags |= F2FS_NOCOW_FL;
 
flags &= F2FS_FL_USER_VISIBLE;
 
-- 
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