Re: [f2fs-dev] [RFC PATCH v3 3/5] f2fs-tools: unify the writeback of superblock

2018-09-10 Thread Junling Zheng
Hi, Jaegeuk

On 2018/9/11 10:43, Junling Zheng wrote:
> On 2018/9/8 6:23, Jaegeuk Kim wrote:
>> On 08/30, Junling Zheng wrote:
>>> diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h
>>> index 38a0da4..df46950 100644
>>> --- a/include/f2fs_fs.h
>>> +++ b/include/f2fs_fs.h
>>> @@ -1411,4 +1411,39 @@ static inline int parse_root_owner(char *ids,
>>> return 0;
>>>  }
>>>  
>>> +enum SB_ADDR {
>>> +   SB_ADDR_0,
>>> +   SB_ADDR_1,
>>> +   SB_ADDR_MAX,
>>> +};
>>> +
>>> +#define SB_FIRST   (1 << SB_ADDR_0)
>>> +#define SB_SECOND  (1 << SB_ADDR_1)
>>> +#define SB_ALL (SB_FIRST | SB_SECOND)
>>> +
>>> +static inline int __write_superblock(struct f2fs_super_block *sb, int 
>>> sb_mask)
>>> +{
>>> +   int index, ret;
>>> +   u_int8_t *buf;
>>> +
>>> +   buf = calloc(F2FS_BLKSIZE, 1);
>>> +   ASSERT(buf);
>>> +
>>> +   memcpy(buf + F2FS_SUPER_OFFSET, sb, sizeof(*sb));
>>> +   for (index = 0; index < SB_ADDR_MAX; index++) {
>>> +   if ((1 << index) & sb_mask) {
>>> +   ret = dev_write_block(buf, index);
>>
>> This doesn't look like a proper inline function.
>>
> 
> Ok, I'll remove the "inline" flag :)
> Any other flaws?
> 
> Thanks
> 

Sorry, here may be difficult to handle :(

This function is used in both mkfs and fsck, so I think it's proper to place it 
in
include/f2fs_fs.h.

Then, if the "inline" flag is removed it will generate some compile warnings as 
below:

In file included from libf2fs.c:11:0:
../include/f2fs_fs.h:1428:12: warning: '__write_superblock' defined but not 
used [-Wunused-function]
 static int __write_superblock(struct f2fs_super_block *sb, int sb_mask)
^

Besides, there're some other complex functions like get_best_overprovision(), 
parse_feature()
being defined as inline functions.

So, I think it's better to keep it inline. what do you think?

Thanks

>>> +   if (ret) {
>>> +   MSG(0, "\tError: While writing superblock %d "
>>> +   "to disk!!!\n", index);
>>> +   free(buf);
>>> +   return ret;
>>> +   }
>>> +   }
>>> +   }
>>> +
>>> +   free(buf);
>>> +   return 0;
>>> +}
>>> +
>>>  #endif /*__F2FS_FS_H */
>>> diff --git a/mkfs/f2fs_format.c b/mkfs/f2fs_format.c
>>> index 621126c..7e3846e 100644
>>> --- a/mkfs/f2fs_format.c
>>> +++ b/mkfs/f2fs_format.c
>>> @@ -979,24 +979,7 @@ free_cp:
>>>  
>>>  static int f2fs_write_super_block(void)
>>>  {
>>> -   int index;
>>> -   u_int8_t *zero_buff;
>>> -
>>> -   zero_buff = calloc(F2FS_BLKSIZE, 1);
>>> -
>>> -   memcpy(zero_buff + F2FS_SUPER_OFFSET, sb, sizeof(*sb));
>>> -   DBG(1, "\tWriting super block, at offset 0x%08x\n", 0);
>>> -   for (index = 0; index < 2; index++) {
>>> -   if (dev_write_block(zero_buff, index)) {
>>> -   MSG(1, "\tError: While while writing supe_blk "
>>> -   "on disk!!! index : %d\n", index);
>>> -   free(zero_buff);
>>> -   return -1;
>>> -   }
>>> -   }
>>> -
>>> -   free(zero_buff);
>>> -   return 0;
>>> +   return __write_superblock(sb, SB_ALL);
>>>  }
>>>  
>>>  #ifndef WITH_ANDROID
>>> -- 
>>> 2.18.0
>>
>> .
>>
> 
> 
> 
> 
> ___
> 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


[f2fs-dev] [Bug 200951] kernel NULL pointer dereference in update_sit_entry

2018-09-10 Thread bugzilla-daemon
https://bugzilla.kernel.org/show_bug.cgi?id=200951

Chao Yu (c...@kernel.org) changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |CODE_FIX

--- Comment #18 from Chao Yu (c...@kernel.org) ---
This issue has been fixed with patch listed in below link, so I tag this issue
as resolved one.

https://sourceforge.net/p/linux-f2fs/mailman/message/36407519/

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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


[f2fs-dev] f2fs page flag set with private but page.private pointer is NULL

2018-09-10 Thread Kassey
hi, Jaegeuk:

   we got some easy reproduced issue when doing reboot test.
   that the android init is sleep on a page to be writebback.
   from the ftrace we can confirm wb_workfn got running each 5s.
   but init has stuck for quite long time because below page can not
be writeback done.

   by check the page's member(a_ops) , we can see that is  f2fs page
and private is NULL.

   can you help to give some suggest  ?


   crash> kmem 0xFFBF01B6C840

  PAGE   PHYSICAL  MAPPING   INDEX CNT FLAGS

ffbf01b6c840 6db21000 ffc0964d1bc0   5c  2 1292c
referenced,uptodate,lru,owner_priv_1,private,writeback,mappedtodisk

crash> struct page  0xFFBF01B6C840
struct page {
  flags = 76076,
  {
mapping = 0xffc0964d1bc0,
s_mem = 0xffc0964d1bc0,
compound_mapcount = {
  counter = -1773331520
}
  },
  {
index = 92,
freelist = 0x5c
  },
  {
counters = 12884901887,
{
  {
_mapcount = {
  counter = -1
},
active = 4294967295,
{
  inuse = 65535,
  objects = 32767,
  frozen = 1
},
units = -1
  },
  _refcount = {
counter = 2
  }
}
  },
  {
lru = {
  next = 0xffbf02bd9520,
  prev = 0xffbf01b66960
},
pgmap = 0xffbf02bd9520,
{
  next = 0xffbf02bd9520,
  pages = 28731744,
  pobjects = -65
},
callback_head = {
  next = 0xffbf02bd9520,
  func = 0xffbf01b66960
},
{
  compound_head = 18446743794582656288,
  compound_dtor = 28731744,
  compound_order = 4294967231
}
  },
  {
private = 0,
ptl = 0x0,
slab_cache = 0x0
  }
}


init:


-000|__switch_to()

-001|__schedule()

-002|need_resched(inline)

-002|schedule()

-003|schedule_timeout()

-004|get_current(inline)

-004|io_schedule_timeout()

-005|bit_wait_io()

-006|__wait_on_bit()

-007|wait_on_page_bit()

-008|PageWriteback(inline)

-008|wait_on_page_writeback(inline)

-008|__filemap_fdatawait_range()

-009|filemap_fdatawait_keep_errors()

-010|sync_inodes_sb()

-011|__sync_filesystem(inline)

-011|sync_filesystem()

-012|generic_shutdown_super()

-013|kill_block_super()

-014|kill_f2fs_super()

-015|deactivate_locked_super()

-016|deactivate_super()

-017|mnt_free_id(inline)

-017|cleanup_mnt()

-018|__cleanup_mnt()

-019|task_work_run()

-020|do_notify_resume()

-021|work_pending(asm)

-->|exception

-022|NUX:0x539E58(asm)

---|end of frame


-- 
Best regards
Kassey


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


Re: [f2fs-dev] [RFC PATCH v3 3/5] f2fs-tools: unify the writeback of superblock

2018-09-10 Thread Junling Zheng
On 2018/9/8 6:23, Jaegeuk Kim wrote:
> On 08/30, Junling Zheng wrote:
>> Introduce __write_superblock() to support updating specified one
>> superblock or both, thus we can wrapper it in update_superblock() and
>> f2fs_write_super_block to unify all places where sb needs to be updated.
>>
>> Signed-off-by: Junling Zheng 
>> ---
>> v2 -> v3:
>>  - fix wrong condition of ASSERT in update_superblock.
>> v1 -> v2:
>>  - if dev_write_block failed, add some notes and free buf to avoid memory 
>> leak.
>>  fsck/fsck.h|  2 +-
>>  fsck/mount.c   | 74 +++---
>>  fsck/resize.c  | 20 ++---
>>  include/f2fs_fs.h  | 35 ++
>>  mkfs/f2fs_format.c | 19 +---
>>  5 files changed, 56 insertions(+), 94 deletions(-)
>>
>> diff --git a/fsck/fsck.h b/fsck/fsck.h
>> index 6042e68..e3490e6 100644
>> --- a/fsck/fsck.h
>> +++ b/fsck/fsck.h
>> @@ -178,7 +178,7 @@ extern void move_curseg_info(struct f2fs_sb_info *, u64, 
>> int);
>>  extern void write_curseg_info(struct f2fs_sb_info *);
>>  extern int find_next_free_block(struct f2fs_sb_info *, u64 *, int, int);
>>  extern void write_checkpoint(struct f2fs_sb_info *);
>> -extern void write_superblock(struct f2fs_super_block *);
>> +extern void update_superblock(struct f2fs_super_block *, int);
>>  extern void update_data_blkaddr(struct f2fs_sb_info *, nid_t, u16, block_t);
>>  extern void update_nat_blkaddr(struct f2fs_sb_info *, nid_t, nid_t, 
>> block_t);
>>  
>> diff --git a/fsck/mount.c b/fsck/mount.c
>> index 1daef75..2bc44ce 100644
>> --- a/fsck/mount.c
>> +++ b/fsck/mount.c
>> @@ -476,7 +476,7 @@ void print_sb_state(struct f2fs_super_block *sb)
>>  }
>>  
>>  static inline int sanity_check_area_boundary(struct f2fs_super_block *sb,
>> -u64 offset)
>> +enum SB_ADDR sb_addr)
>>  {
>>  u32 segment0_blkaddr = get_sb(segment0_blkaddr);
>>  u32 cp_blkaddr = get_sb(cp_blkaddr);
>> @@ -542,14 +542,11 @@ static inline int sanity_check_area_boundary(struct 
>> f2fs_super_block *sb,
>>  segment_count_main << log_blocks_per_seg);
>>  return -1;
>>  } else if (main_end_blkaddr < seg_end_blkaddr) {
>> -int err;
>> -
>>  set_sb(segment_count, (main_end_blkaddr -
>>  segment0_blkaddr) >> log_blocks_per_seg);
>>  
>> -err = dev_write(sb, offset, sizeof(struct f2fs_super_block));
>> -MSG(0, "Info: Fix alignment: %s, start(%u) end(%u) block(%u)\n",
>> -err ? "failed": "done",
>> +update_superblock(sb, 1 << sb_addr);
>> +MSG(0, "Info: Fix alignment: start(%u) end(%u) block(%u)\n",
>>  main_blkaddr,
>>  segment0_blkaddr +
>>  (segment_count << log_blocks_per_seg),
>> @@ -558,7 +555,7 @@ static inline int sanity_check_area_boundary(struct 
>> f2fs_super_block *sb,
>>  return 0;
>>  }
>>  
>> -int sanity_check_raw_super(struct f2fs_super_block *sb, u64 offset)
>> +int sanity_check_raw_super(struct f2fs_super_block *sb, enum SB_ADDR 
>> sb_addr)
>>  {
>>  unsigned int blocksize;
>>  
>> @@ -600,30 +597,24 @@ int sanity_check_raw_super(struct f2fs_super_block 
>> *sb, u64 offset)
>>  if (get_sb(segment_count) > F2FS_MAX_SEGMENT)
>>  return -1;
>>  
>> -if (sanity_check_area_boundary(sb, offset))
>> +if (sanity_check_area_boundary(sb, sb_addr))
>>  return -1;
>>  return 0;
>>  }
>>  
>> -int validate_super_block(struct f2fs_sb_info *sbi, int block)
>> +int validate_super_block(struct f2fs_sb_info *sbi, enum SB_ADDR sb_addr)
>>  {
>> -u64 offset;
>>  char buf[F2FS_BLKSIZE];
>>  
>>  sbi->raw_super = malloc(sizeof(struct f2fs_super_block));
>>  
>> -if (block == 0)
>> -offset = F2FS_SUPER_OFFSET;
>> -else
>> -offset = F2FS_BLKSIZE + F2FS_SUPER_OFFSET;
>> -
>> -if (dev_read_block(buf, block))
>> +if (dev_read_block(buf, sb_addr))
>>  return -1;
>>  
>>  memcpy(sbi->raw_super, buf + F2FS_SUPER_OFFSET,
>>  sizeof(struct f2fs_super_block));
>>  
>> -if (!sanity_check_raw_super(sbi->raw_super, offset)) {
>> +if (!sanity_check_raw_super(sbi->raw_super, sb_addr)) {
>>  /* get kernel version */
>>  if (c.kd >= 0) {
>>  dev_read_version(c.version, 0, VERSION_LEN);
>> @@ -642,13 +633,9 @@ int validate_super_block(struct f2fs_sb_info *sbi, int 
>> block)
>>  MSG(0, "Info: FSCK version\n  from \"%s\"\nto \"%s\"\n",
>>  c.sb_version, c.version);
>>  if (memcmp(c.sb_version, c.version, VERSION_LEN)) {
>> -int ret;
>> -
>>  memcpy(sbi->raw_super->version,
>>  

[f2fs-dev] [PATCH v3 2/2] f2fs: fix setattr project check upon fssetxattr ioctl

2018-09-10 Thread Wang Shilong
From: Wang Shilong 

Currently, project quota could be changed by fssetxattr
ioctl, and existed permission check inode_owner_or_capable()
is obviously not enough, just think that common users could
change project id of file, that could make users to
break project quota easily.

This patch try to follow same regular of xfs project
quota:

"Project Quota ID state is only allowed to change from
within the init namespace. Enforce that restriction only
if we are trying to change the quota ID state.
Everything else is allowed in user namespaces."

Besides that, check and set project id'state should
be an atomic operation, protect whole operation with
inode lock.

Signed-off-by: Wang Shilong 
---
v2->v3: remove inode_lock() and mnt_want_write_file()
inside f2fs_ioc_setproject()
v1->v2: rename f2fs_ioctl_setattr_check_projid()
to f2fs_ioctl_check_project()
 fs/f2fs/file.c | 61 --
 1 file changed, 38 insertions(+), 23 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 5474aaa..f5170e6 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2617,34 +2617,26 @@ static int f2fs_ioc_setproject(struct file *filp, __u32 
projid)
if (projid_eq(kprojid, F2FS_I(inode)->i_projid))
return 0;
 
-   err = mnt_want_write_file(filp);
-   if (err)
-   return err;
-
err = -EPERM;
-   inode_lock(inode);
-
/* Is it quota file? Do not allow user to mess with it */
if (IS_NOQUOTA(inode))
-   goto out_unlock;
+   return err;
 
ipage = f2fs_get_node_page(sbi, inode->i_ino);
-   if (IS_ERR(ipage)) {
-   err = PTR_ERR(ipage);
-   goto out_unlock;
-   }
+   if (IS_ERR(ipage))
+   return PTR_ERR(ipage);
 
if (!F2FS_FITS_IN_INODE(F2FS_INODE(ipage), fi->i_extra_isize,
i_projid)) {
err = -EOVERFLOW;
f2fs_put_page(ipage, 1);
-   goto out_unlock;
+   return err;
}
f2fs_put_page(ipage, 1);
 
err = dquot_initialize(inode);
if (err)
-   goto out_unlock;
+   return err;
 
transfer_to[PRJQUOTA] = dqget(sb, make_kqid_projid(kprojid));
if (!IS_ERR(transfer_to[PRJQUOTA])) {
@@ -2658,9 +2650,6 @@ static int f2fs_ioc_setproject(struct file *filp, __u32 
projid)
inode->i_ctime = current_time(inode);
 out_dirty:
f2fs_mark_inode_dirty_sync(inode, true);
-out_unlock:
-   inode_unlock(inode);
-   mnt_drop_write_file(filp);
return err;
 }
 #else
@@ -2736,6 +2725,31 @@ static int f2fs_ioc_fsgetxattr(struct file *filp, 
unsigned long arg)
return 0;
 }
 
+static int
+f2fs_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
+{
+   /*
+* Project Quota ID state is only allowed to change from within the init
+* namespace. Enforce that restriction only if we are trying to change
+* the quota ID state. Everything else is allowed in user namespaces.
+*/
+   if (current_user_ns() == _user_ns)
+   return 0;
+
+   if (__kprojid_val(F2FS_I(inode)->i_projid) != fa->fsx_projid)
+   return -EINVAL;
+
+   if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL) {
+   if (!(fa->fsx_xflags & FS_XFLAG_PROJINHERIT))
+   return -EINVAL;
+   } else {
+   if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
+   return -EINVAL;
+   }
+
+   return 0;
+}
+
 static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
 {
struct inode *inode = file_inode(filp);
@@ -2763,19 +2777,20 @@ static int f2fs_ioc_fssetxattr(struct file *filp, 
unsigned long arg)
return err;
 
inode_lock(inode);
+   err = f2fs_ioctl_check_project(inode, );
+   if (err)
+   goto out;
flags = (fi->i_flags & ~F2FS_FL_XFLAG_VISIBLE) |
(flags & F2FS_FL_XFLAG_VISIBLE);
err = __f2fs_ioc_setflags(inode, flags);
-   inode_unlock(inode);
-   mnt_drop_write_file(filp);
if (err)
-   return err;
+   goto out;
 
err = f2fs_ioc_setproject(filp, fa.fsx_projid);
-   if (err)
-   return err;
-
-   return 0;
+out:
+   inode_unlock(inode);
+   mnt_drop_write_file(filp);
+   return err;
 }
 
 int f2fs_pin_file_control(struct inode *inode, bool inc)
-- 
1.8.3.1



___
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: surround fault_injection ralted option parsing using CONFIG_F2FS_FAULT_INJECTION

2018-09-10 Thread Chengguang Xu
It's a little bit strange when fault_injection related
options fail with -EINVAL which were already disabled
from config, so surround all fault_injection related option
parsing code using CONFIG_F2FS_FAULT_INJECTION. Meanwhile,
slightly change warning message to keep consistency with
option POSIX_ACL and FS_XATTR.

Signed-off-by: Chengguang Xu 
---
v1->v2:
- modify warning message.

 fs/f2fs/super.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 896b885f504e..ea6952c62e9f 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -602,28 +602,31 @@ static int parse_options(struct super_block *sb, char 
*options)
}
F2FS_OPTION(sbi).write_io_size_bits = arg;
break;
+#ifdef CONFIG_F2FS_FAULT_INJECTION
case Opt_fault_injection:
if (args->from && match_int(args, ))
return -EINVAL;
-#ifdef CONFIG_F2FS_FAULT_INJECTION
f2fs_build_fault_attr(sbi, arg, F2FS_ALL_FAULT_TYPE);
set_opt(sbi, FAULT_INJECTION);
-#else
-   f2fs_msg(sb, KERN_INFO,
-   "FAULT_INJECTION was not selected");
-#endif
break;
+
case Opt_fault_type:
if (args->from && match_int(args, ))
return -EINVAL;
-#ifdef CONFIG_F2FS_FAULT_INJECTION
f2fs_build_fault_attr(sbi, 0, arg);
set_opt(sbi, FAULT_INJECTION);
+   break;
 #else
+   case Opt_fault_injection:
f2fs_msg(sb, KERN_INFO,
-   "FAULT_INJECTION was not selected");
-#endif
+   "fault_injection options not supported");
break;
+
+   case Opt_fault_type:
+   f2fs_msg(sb, KERN_INFO,
+   "fault_type options not supported");
+   break;
+#endif
case Opt_lazytime:
sb->s_flags |= SB_LAZYTIME;
break;
-- 
2.17.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] f2fs: surround fault_injection ralted option parsing using CONFIG_F2FS_FAULT_INJECTION

2018-09-10 Thread Chao Yu
On 2018/9/10 23:48, Chengguang Xu wrote:
> It's a little bit strange when fault_injection related
> options fail with -EINVAL which were already disabled
> from config, so surround all fault_injection related option
> parsing code using CONFIG_F2FS_FAULT_INJECTION. Meanwhile,
> slightly change warning message to keep consistency with
> option POSIX_ACL and FS_XATTR.
> 
> Signed-off-by: Chengguang Xu 
> ---
>  fs/f2fs/super.c | 19 +++
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 896b885f504e..cc49cc10d9f5 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -602,28 +602,31 @@ static int parse_options(struct super_block *sb, char 
> *options)
>   }
>   F2FS_OPTION(sbi).write_io_size_bits = arg;
>   break;
> +#ifdef CONFIG_F2FS_FAULT_INJECTION
>   case Opt_fault_injection:
>   if (args->from && match_int(args, ))
>   return -EINVAL;
> -#ifdef CONFIG_F2FS_FAULT_INJECTION
>   f2fs_build_fault_attr(sbi, arg, F2FS_ALL_FAULT_TYPE);
>   set_opt(sbi, FAULT_INJECTION);
> -#else
> - f2fs_msg(sb, KERN_INFO,
> - "FAULT_INJECTION was not selected");
> -#endif
>   break;
> +
>   case Opt_fault_type:
>   if (args->from && match_int(args, ))
>   return -EINVAL;
> -#ifdef CONFIG_F2FS_FAULT_INJECTION
>   f2fs_build_fault_attr(sbi, 0, arg);
>   set_opt(sbi, FAULT_INJECTION);
> + break;
>  #else
> + case Opt_fault_injection:
>   f2fs_msg(sb, KERN_INFO,
> - "FAULT_INJECTION was not selected");
> -#endif
> + "FAULT_INJECTION not supported");

"fault_injection options not supported"

>   break;
> +
> + case Opt_fault_type:
> + f2fs_msg(sb, KERN_INFO,
> + "FAULT_INJECTION not supported");

"fault_type options not supported"

Thanks,

> + break;
> +#endif
>   case Opt_lazytime:
>   sb->s_flags |= SB_LAZYTIME;
>   break;
> 


___
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: add new idle interval timing for discard and gc paths

2018-09-10 Thread Chao Yu
On 2018/9/10 11:47, Sahitya Tummala wrote:
> This helps to control the frequency of submission of discard and
> GC requests independently, based on the need. The sleep timing of
> GC thread is now aligned with this idle time when the dev is busy,
> to avoid unnecessary periodic wakeups.
> 
> Suggested-by: Chao Yu 
> Signed-off-by: Sahitya Tummala 
> ---
> v2:
> -fix __issue_discard_cmd_orderly() path
> 
>  Documentation/ABI/testing/sysfs-fs-f2fs | 17 -
>  fs/f2fs/f2fs.h  | 31 +++
>  fs/f2fs/gc.c|  6 --
>  fs/f2fs/segment.c   | 16 ++--
>  fs/f2fs/super.c |  2 ++
>  fs/f2fs/sysfs.c |  5 +
>  6 files changed, 60 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs 
> b/Documentation/ABI/testing/sysfs-fs-f2fs
> index 94a24ae..3ac4177 100644
> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
> @@ -121,7 +121,22 @@ What:/sys/fs/f2fs//idle_interval
>  Date:January 2016
>  Contact: "Jaegeuk Kim" 
>  Description:
> -  Controls the idle timing.
> +  Controls the idle timing for all paths other than
> +  discard and gc path.
> +
> +What:/sys/fs/f2fs//discard_idle_interval
> +Date:September 2018
> +Contact: "Chao Yu" 
> +Contact: "Sahitya Tummala" 
> +Description:
> +  Controls the idle timing for discard path.
> +
> +What:/sys/fs/f2fs//gc_idle_interval
> +Date:September 2018
> +Contact: "Chao Yu" 
> +Contact: "Sahitya Tummala" 
> +Description:
> +  Controls the idle timing for gc path.
>  
>  What:/sys/fs/f2fs//iostat_enable
>  Date:August 2017
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index abf9256..6070681 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1093,6 +1093,8 @@ enum {
>  enum {
>   CP_TIME,
>   REQ_TIME,
> + DISCARD_TIME,
> + GC_TIME,
>   MAX_TIME,
>  };
>  
> @@ -1347,14 +1349,35 @@ static inline void f2fs_update_time(struct 
> f2fs_sb_info *sbi, int type)
>   sbi->last_time[type] = jiffies;
>  }
>  
> -static inline bool f2fs_time_over(struct f2fs_sb_info *sbi, int type)
> +static inline bool f2fs_time_over_cp(struct f2fs_sb_info *sbi)
> +{
> + unsigned long interval = sbi->interval_time[CP_TIME] * HZ;
> +
> + return time_after(jiffies, sbi->last_time[CP_TIME] + interval);
> +}
> +
> +static inline bool f2fs_time_over_req(struct f2fs_sb_info *sbi, int type)
> +{
> + unsigned long interval = sbi->interval_time[type] * HZ;
> +
> + return time_after(jiffies, sbi->last_time[REQ_TIME] + interval);
> +}
> +
> +static inline unsigned int f2fs_get_wait_time(struct f2fs_sb_info *sbi,
> + int type)
>  {
>   unsigned long interval = sbi->interval_time[type] * HZ;
> + unsigned int wait_ms = 0;
> + long delta;
> +
> + delta = (sbi->last_time[REQ_TIME] + interval) - jiffies;
> + if (delta > 0)
> + wait_ms = jiffies_to_msecs(delta);
>  
> - return time_after(jiffies, sbi->last_time[type] + interval);
> + return wait_ms;
>  }
>  
> -static inline bool is_idle(struct f2fs_sb_info *sbi)
> +static inline bool is_idle(struct f2fs_sb_info *sbi, int type)
>  {
>   struct block_device *bdev = sbi->sb->s_bdev;
>   struct request_queue *q = bdev_get_queue(bdev);
> @@ -1363,7 +1386,7 @@ static inline bool is_idle(struct f2fs_sb_info *sbi)
>   if (rl->count[BLK_RW_SYNC] || rl->count[BLK_RW_ASYNC])
>   return false;
>  
> - return f2fs_time_over(sbi, REQ_TIME);
> + return f2fs_time_over_req(sbi, type);
>  }
>  
>  /*
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 5c8d004..c0bafea 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -83,8 +83,10 @@ static int gc_thread_func(void *data)
>   if (!mutex_trylock(>gc_mutex))
>   goto next;
>  
> - if (!is_idle(sbi)) {
> - increase_sleep_time(gc_th, _ms);
> + if (!is_idle(sbi, GC_TIME)) {
> + wait_ms = f2fs_get_wait_time(sbi, GC_TIME);

It seems this patch changes the method of increasing wait_ms here, if device is
busy, we may wake up GC thread earlier than before, not sure we should do this.

To Jaegeuk, how do you think of this?

Thanks,

> + if (!wait_ms)
> + increase_sleep_time(gc_th, _ms);
>   mutex_unlock(>gc_mutex);
>   goto next;
>   }
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index c5024f8..2d15733 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -511,7 +511,7 @@ void f2fs_balance_fs_bg(struct f2fs_sb_info *sbi)
>   

[f2fs-dev] [PATCH] f2fs: surround fault_injection ralted option parsing using CONFIG_F2FS_FAULT_INJECTION

2018-09-10 Thread Chengguang Xu
It's a little bit strange when fault_injection related
options fail with -EINVAL which were already disabled
from config, so surround all fault_injection related option
parsing code using CONFIG_F2FS_FAULT_INJECTION. Meanwhile,
slightly change warning message to keep consistency with
option POSIX_ACL and FS_XATTR.

Signed-off-by: Chengguang Xu 
---
 fs/f2fs/super.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 896b885f504e..cc49cc10d9f5 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -602,28 +602,31 @@ static int parse_options(struct super_block *sb, char 
*options)
}
F2FS_OPTION(sbi).write_io_size_bits = arg;
break;
+#ifdef CONFIG_F2FS_FAULT_INJECTION
case Opt_fault_injection:
if (args->from && match_int(args, ))
return -EINVAL;
-#ifdef CONFIG_F2FS_FAULT_INJECTION
f2fs_build_fault_attr(sbi, arg, F2FS_ALL_FAULT_TYPE);
set_opt(sbi, FAULT_INJECTION);
-#else
-   f2fs_msg(sb, KERN_INFO,
-   "FAULT_INJECTION was not selected");
-#endif
break;
+
case Opt_fault_type:
if (args->from && match_int(args, ))
return -EINVAL;
-#ifdef CONFIG_F2FS_FAULT_INJECTION
f2fs_build_fault_attr(sbi, 0, arg);
set_opt(sbi, FAULT_INJECTION);
+   break;
 #else
+   case Opt_fault_injection:
f2fs_msg(sb, KERN_INFO,
-   "FAULT_INJECTION was not selected");
-#endif
+   "FAULT_INJECTION not supported");
break;
+
+   case Opt_fault_type:
+   f2fs_msg(sb, KERN_INFO,
+   "FAULT_INJECTION not supported");
+   break;
+#endif
case Opt_lazytime:
sb->s_flags |= SB_LAZYTIME;
break;
-- 
2.17.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 2/2] f2fs: fix setattr project check upon fssetxattr ioctl

2018-09-10 Thread Chao Yu
On 2018/9/9 17:15, Wang Shilong wrote:
> From: Wang Shilong 
> 
> Currently, project quota could be changed by fssetxattr
> ioctl, and existed permission check inode_owner_or_capable()
> is obviously not enough, just think that common users could
> change project id of file, that could make users to
> break project quota easily.
> 
> This patch try to follow same regular of xfs project
> quota:
> 
> "Project Quota ID state is only allowed to change from
> within the init namespace. Enforce that restriction only
> if we are trying to change the quota ID state.
> Everything else is allowed in user namespaces."
> 
> Besides that, check and set project id'state should
> be an atomic operation, protect whole operation with
> inode lock.
> 
> Signed-off-by: Wang Shilong 

It looks good to me, thanks for the patch, Shilong. :)

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 v3] f2fs: avoid sleeping under spin_lock

2018-09-10 Thread Chao Yu
On 2018/9/10 16:18, Zhikang Zhang wrote:
> In the call trace below, we might sleep in function dput().
> 
> So in order to avoid sleeping under spin_lock, we remove 
> f2fs_mark_inode_dirty_sync
> from __try_update_largest_extent && __drop_largest_extent.
> 
> BUG: sleeping function called from invalid context at fs/dcache.c:796
> Call trace:
>   dump_backtrace+0x0/0x3f4
>   show_stack+0x24/0x30
>   dump_stack+0xe0/0x138
>   ___might_sleep+0x2a8/0x2c8
>   __might_sleep+0x78/0x10c
>   dput+0x7c/0x750
>   block_dump___mark_inode_dirty+0x120/0x17c
>   __mark_inode_dirty+0x344/0x11f0
>   f2fs_mark_inode_dirty_sync+0x40/0x50
>   __insert_extent_tree+0x2e0/0x2f4
>   f2fs_update_extent_tree_range+0xcf4/0xde8
>   f2fs_update_extent_cache+0x114/0x12c
>   f2fs_update_data_blkaddr+0x40/0x50
>   write_data_page+0x150/0x314
>   do_write_data_page+0x648/0x2318
>   __write_data_page+0xdb4/0x1640
>   f2fs_write_cache_pages+0x768/0xafc
>   __f2fs_write_data_pages+0x590/0x1218
>   f2fs_write_data_pages+0x64/0x74
>   do_writepages+0x74/0xe4
>   __writeback_single_inode+0xdc/0x15f0
>   writeback_sb_inodes+0x574/0xc98
>   __writeback_inodes_wb+0x190/0x204
>   wb_writeback+0x730/0xf14
>   wb_check_old_data_flush+0x1bc/0x1c8
>   wb_workfn+0x554/0xf74
>   process_one_work+0x440/0x118c
>   worker_thread+0xac/0x974
>   kthread+0x1a0/0x1c8
>   ret_from_fork+0x10/0x1c

Missed a Signed-off-by here?

Signed-off-by: Zhikang Zhang 
Reviewed-by: Chao Yu 

I think Jaegeuk can help to add them when merging this patch. :)

Thanks,

> ---
>  fs/f2fs/extent_cache.c | 51 
> +++---
>  fs/f2fs/f2fs.h |  7 ---
>  2 files changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index 231b77e..a70cd25 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -308,14 +308,13 @@ static unsigned int __free_extent_tree(struct 
> f2fs_sb_info *sbi,
>   return count - atomic_read(>node_cnt);
>  }
>  
> -static void __drop_largest_extent(struct inode *inode,
> +static void __drop_largest_extent(struct extent_tree *et,
>   pgoff_t fofs, unsigned int len)
>  {
> - struct extent_info *largest = _I(inode)->extent_tree->largest;
> -
> - if (fofs < largest->fofs + largest->len && fofs + len > largest->fofs) {
> - largest->len = 0;
> - f2fs_mark_inode_dirty_sync(inode, true);
> + if (fofs < et->largest.fofs + et->largest.len &&
> + fofs + len > et->largest.fofs) {
> + et->largest.len = 0;
> + et->largest_updated = true;
>   }
>  }
>  
> @@ -416,12 +415,11 @@ static bool f2fs_lookup_extent_tree(struct inode 
> *inode, pgoff_t pgofs,
>   return ret;
>  }
>  
> -static struct extent_node *__try_merge_extent_node(struct inode *inode,
> +static struct extent_node *__try_merge_extent_node(struct f2fs_sb_info *sbi,
>   struct extent_tree *et, struct extent_info *ei,
>   struct extent_node *prev_ex,
>   struct extent_node *next_ex)
>  {
> - struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>   struct extent_node *en = NULL;
>  
>   if (prev_ex && __is_back_mergeable(ei, _ex->ei)) {
> @@ -443,7 +441,7 @@ static struct extent_node *__try_merge_extent_node(struct 
> inode *inode,
>   if (!en)
>   return NULL;
>  
> - __try_update_largest_extent(inode, et, en);
> + __try_update_largest_extent(et, en);
>  
>   spin_lock(>extent_lock);
>   if (!list_empty(>list)) {
> @@ -454,12 +452,11 @@ static struct extent_node 
> *__try_merge_extent_node(struct inode *inode,
>   return en;
>  }
>  
> -static struct extent_node *__insert_extent_tree(struct inode *inode,
> +static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi,
>   struct extent_tree *et, struct extent_info *ei,
>   struct rb_node **insert_p,
>   struct rb_node *insert_parent)
>  {
> - struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>   struct rb_node **p;
>   struct rb_node *parent = NULL;
>   struct extent_node *en = NULL;
> @@ -476,7 +473,7 @@ static struct extent_node *__insert_extent_tree(struct 
> inode *inode,
>   if (!en)
>   return NULL;
>  
> - __try_update_largest_extent(inode, et, en);
> + __try_update_largest_extent(et, en);
>  
>   /* update in global extent list */
>   spin_lock(>extent_lock);
> @@ -497,6 +494,7 @@ static void f2fs_update_extent_tree_range(struct inode 
> *inode,
>   struct rb_node **insert_p = NULL, *insert_parent = NULL;
>   unsigned int end = fofs + len;
>   unsigned int pos = (unsigned int)fofs;
> + bool updated = false;
>  
>   if 

Re: [f2fs-dev] [PATCH] f2fs: submit bio after shutdown

2018-09-10 Thread Chao Yu
On 2018/9/8 5:28, Jaegeuk Kim wrote:
> Sometimes, some merged IOs could get a chance to be submitted, resulting in
> system hang in shutdown test. This issues IOs all the time after shutdown.
> 
> Signed-off-by: Jaegeuk Kim 

Reviewed-by: Chao Yu 

Thanks,

> ---
>  fs/f2fs/data.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 8c204f896c22..26f38b224bb2 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -533,6 +533,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>   if (fio->in_list)
>   goto next;
>  out:
> + if (is_sbi_flag_set(sbi, SBI_IS_SHUTDOWN))
> + __submit_merged_bio(io);
>   up_write(>io_rwsem);
>  }
>  
> 


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


[f2fs-dev] [PATCH v3] f2fs: avoid sleeping under spin_lock

2018-09-10 Thread Zhikang Zhang
In the call trace below, we might sleep in function dput().

So in order to avoid sleeping under spin_lock, we remove 
f2fs_mark_inode_dirty_sync
from __try_update_largest_extent && __drop_largest_extent.

BUG: sleeping function called from invalid context at fs/dcache.c:796
Call trace:
dump_backtrace+0x0/0x3f4
show_stack+0x24/0x30
dump_stack+0xe0/0x138
___might_sleep+0x2a8/0x2c8
__might_sleep+0x78/0x10c
dput+0x7c/0x750
block_dump___mark_inode_dirty+0x120/0x17c
__mark_inode_dirty+0x344/0x11f0
f2fs_mark_inode_dirty_sync+0x40/0x50
__insert_extent_tree+0x2e0/0x2f4
f2fs_update_extent_tree_range+0xcf4/0xde8
f2fs_update_extent_cache+0x114/0x12c
f2fs_update_data_blkaddr+0x40/0x50
write_data_page+0x150/0x314
do_write_data_page+0x648/0x2318
__write_data_page+0xdb4/0x1640
f2fs_write_cache_pages+0x768/0xafc
__f2fs_write_data_pages+0x590/0x1218
f2fs_write_data_pages+0x64/0x74
do_writepages+0x74/0xe4
__writeback_single_inode+0xdc/0x15f0
writeback_sb_inodes+0x574/0xc98
__writeback_inodes_wb+0x190/0x204
wb_writeback+0x730/0xf14
wb_check_old_data_flush+0x1bc/0x1c8
wb_workfn+0x554/0xf74
process_one_work+0x440/0x118c
worker_thread+0xac/0x974
kthread+0x1a0/0x1c8
ret_from_fork+0x10/0x1c
---
 fs/f2fs/extent_cache.c | 51 +++---
 fs/f2fs/f2fs.h |  7 ---
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
index 231b77e..a70cd25 100644
--- a/fs/f2fs/extent_cache.c
+++ b/fs/f2fs/extent_cache.c
@@ -308,14 +308,13 @@ static unsigned int __free_extent_tree(struct 
f2fs_sb_info *sbi,
return count - atomic_read(>node_cnt);
 }
 
-static void __drop_largest_extent(struct inode *inode,
+static void __drop_largest_extent(struct extent_tree *et,
pgoff_t fofs, unsigned int len)
 {
-   struct extent_info *largest = _I(inode)->extent_tree->largest;
-
-   if (fofs < largest->fofs + largest->len && fofs + len > largest->fofs) {
-   largest->len = 0;
-   f2fs_mark_inode_dirty_sync(inode, true);
+   if (fofs < et->largest.fofs + et->largest.len &&
+   fofs + len > et->largest.fofs) {
+   et->largest.len = 0;
+   et->largest_updated = true;
}
 }
 
@@ -416,12 +415,11 @@ static bool f2fs_lookup_extent_tree(struct inode *inode, 
pgoff_t pgofs,
return ret;
 }
 
-static struct extent_node *__try_merge_extent_node(struct inode *inode,
+static struct extent_node *__try_merge_extent_node(struct f2fs_sb_info *sbi,
struct extent_tree *et, struct extent_info *ei,
struct extent_node *prev_ex,
struct extent_node *next_ex)
 {
-   struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
struct extent_node *en = NULL;
 
if (prev_ex && __is_back_mergeable(ei, _ex->ei)) {
@@ -443,7 +441,7 @@ static struct extent_node *__try_merge_extent_node(struct 
inode *inode,
if (!en)
return NULL;
 
-   __try_update_largest_extent(inode, et, en);
+   __try_update_largest_extent(et, en);
 
spin_lock(>extent_lock);
if (!list_empty(>list)) {
@@ -454,12 +452,11 @@ static struct extent_node *__try_merge_extent_node(struct 
inode *inode,
return en;
 }
 
-static struct extent_node *__insert_extent_tree(struct inode *inode,
+static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi,
struct extent_tree *et, struct extent_info *ei,
struct rb_node **insert_p,
struct rb_node *insert_parent)
 {
-   struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
struct rb_node **p;
struct rb_node *parent = NULL;
struct extent_node *en = NULL;
@@ -476,7 +473,7 @@ static struct extent_node *__insert_extent_tree(struct 
inode *inode,
if (!en)
return NULL;
 
-   __try_update_largest_extent(inode, et, en);
+   __try_update_largest_extent(et, en);
 
/* update in global extent list */
spin_lock(>extent_lock);
@@ -497,6 +494,7 @@ static void f2fs_update_extent_tree_range(struct inode 
*inode,
struct rb_node **insert_p = NULL, *insert_parent = NULL;
unsigned int end = fofs + len;
unsigned int pos = (unsigned int)fofs;
+   bool updated = false;
 
if (!et)
return;
@@ -517,7 +515,7 @@ static void f2fs_update_extent_tree_range(struct inode 
*inode,
 * drop largest extent before lookup, in case it's already
 * been shrunk from extent tree
 */
-   __drop_largest_extent(inode, fofs, len);
+