fs: Convert open-coded "is inode open for write" check

Message ID 20181210131903.4528-1-nborisov@suse.com
State New
Headers show
Series
  • fs: Convert open-coded "is inode open for write" check
Related show

Commit Message

Nikolay Borisov Dec. 10, 2018, 1:19 p.m.
There is a generic helper inode_is_open_for_write that could be used
when checking i_writecount. Use it instead of opencoding
if (i_writecount > 0) check. Also the checks in ext4 seem to be wrong since 
they will also evaluate to true when i_writecount is MMAP_DENY_WRITE (< 0)

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

Andrew, 

Please take this patch after the ext4 people have sent their RB/Acks. 
Alternatively I can split the ext4 changes in a separate patch and send you just
the rest. 

 fs/ext4/inode.c                    | 2 +-
 fs/ext4/mballoc.c                  | 7 +++----
 fs/locks.c                         | 2 +-
 fs/notify/fanotify/fanotify_user.c | 2 +-
 security/integrity/ima/ima_main.c  | 2 +-
 5 files changed, 7 insertions(+), 8 deletions(-)

Comments

kbuild test robot Dec. 11, 2018, 12:56 a.m. | #1
Hi Nikolay,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on ext4/dev]
[also build test ERROR on v4.20-rc6 next-20181207]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nikolay-Borisov/fs-Convert-open-coded-is-inode-open-for-write-check/20181211-023543
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: i386-randconfig-x000-201849 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   fs/ext4/mballoc.c: In function 'ext4_mb_group_or_file':
>> fs/ext4/mballoc.c:4180:34: error: 'inode' undeclared (first use in this function)
         && !inode_is_open_for_write(inode) {
                                     ^~~~~
   fs/ext4/mballoc.c:4180:34: note: each undeclared identifier is reported only once for each function it appears in
>> fs/ext4/mballoc.c:4180:41: error: expected ')' before '{' token
         && !inode_is_open_for_write(inode) {
                                            ^
>> fs/ext4/mballoc.c:4210:1: error: expected expression before '}' token
    }
    ^
   In file included from include/linux/kernel.h:14:0,
                    from include/linux/list.h:9,
                    from include/linux/wait.h:7,
                    from include/linux/wait_bit.h:8,
                    from include/linux/fs.h:6,
                    from fs/ext4/ext4_jbd2.h:15,
                    from fs/ext4/mballoc.c:12:
   fs/ext4/mballoc.c: In function 'ext4_mb_initialize_context':
>> fs/ext4/mballoc.c:4260:28: error: passing argument 1 of 'inode_is_open_for_write' from incompatible pointer type [-Werror=incompatible-pointer-types]
       inode_is_open_for_write(&ar->inode) ? "" : "non-");
                               ^
   include/linux/printk.h:136:17: note: in definition of macro 'no_printk'
      printk(fmt, ##__VA_ARGS__);  \
                    ^~~~~~~~~~~
>> fs/ext4/mballoc.c:4254:2: note: in expansion of macro 'mb_debug'
     mb_debug(1, "init ac: %u blocks @ %u, goal %u, flags %x, 2^%d, "
     ^~~~~~~~
   In file included from fs/ext4/ext4_jbd2.h:15:0,
                    from fs/ext4/mballoc.c:12:
   include/linux/fs.h:2861:20: note: expected 'const struct inode *' but argument is of type 'struct inode **'
    static inline bool inode_is_open_for_write(const struct inode *inode)
                       ^~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/inode +4180 fs/ext4/mballoc.c

  4155	
  4156	/*
  4157	 * We use locality group preallocation for small size file. The size of the
  4158	 * file is determined by the current size or the resulting size after
  4159	 * allocation which ever is larger
  4160	 *
  4161	 * One can tune this size via /sys/fs/ext4/<partition>/mb_stream_req
  4162	 */
  4163	static void ext4_mb_group_or_file(struct ext4_allocation_context *ac)
  4164	{
  4165		struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
  4166		int bsbits = ac->ac_sb->s_blocksize_bits;
  4167		loff_t size, isize;
  4168	
  4169		if (!(ac->ac_flags & EXT4_MB_HINT_DATA))
  4170			return;
  4171	
  4172		if (unlikely(ac->ac_flags & EXT4_MB_HINT_GOAL_ONLY))
  4173			return;
  4174	
  4175		size = ac->ac_o_ex.fe_logical + EXT4_C2B(sbi, ac->ac_o_ex.fe_len);
  4176		isize = (i_size_read(ac->ac_inode) + ac->ac_sb->s_blocksize - 1)
  4177			>> bsbits;
  4178	
  4179		if ((size == isize) && !ext4_fs_is_busy(sbi)
> 4180		    && !inode_is_open_for_write(inode) {
  4181			ac->ac_flags |= EXT4_MB_HINT_NOPREALLOC;
  4182			return;
  4183		}
  4184	
  4185		if (sbi->s_mb_group_prealloc <= 0) {
  4186			ac->ac_flags |= EXT4_MB_STREAM_ALLOC;
  4187			return;
  4188		}
  4189	
  4190		/* don't use group allocation for large files */
  4191		size = max(size, isize);
  4192		if (size > sbi->s_mb_stream_request) {
  4193			ac->ac_flags |= EXT4_MB_STREAM_ALLOC;
  4194			return;
  4195		}
  4196	
  4197		BUG_ON(ac->ac_lg != NULL);
  4198		/*
  4199		 * locality group prealloc space are per cpu. The reason for having
  4200		 * per cpu locality group is to reduce the contention between block
  4201		 * request from multiple CPUs.
  4202		 */
  4203		ac->ac_lg = raw_cpu_ptr(sbi->s_locality_groups);
  4204	
  4205		/* we're going to use group allocation */
  4206		ac->ac_flags |= EXT4_MB_HINT_GROUP_ALLOC;
  4207	
  4208		/* serialize all allocations in the group */
  4209		mutex_lock(&ac->ac_lg->lg_mutex);
> 4210	}
  4211	
  4212	static noinline_for_stack int
  4213	ext4_mb_initialize_context(struct ext4_allocation_context *ac,
  4214					struct ext4_allocation_request *ar)
  4215	{
  4216		struct super_block *sb = ar->inode->i_sb;
  4217		struct ext4_sb_info *sbi = EXT4_SB(sb);
  4218		struct ext4_super_block *es = sbi->s_es;
  4219		ext4_group_t group;
  4220		unsigned int len;
  4221		ext4_fsblk_t goal;
  4222		ext4_grpblk_t block;
  4223	
  4224		/* we can't allocate > group size */
  4225		len = ar->len;
  4226	
  4227		/* just a dirty hack to filter too big requests  */
  4228		if (len >= EXT4_CLUSTERS_PER_GROUP(sb))
  4229			len = EXT4_CLUSTERS_PER_GROUP(sb);
  4230	
  4231		/* start searching from the goal */
  4232		goal = ar->goal;
  4233		if (goal < le32_to_cpu(es->s_first_data_block) ||
  4234				goal >= ext4_blocks_count(es))
  4235			goal = le32_to_cpu(es->s_first_data_block);
  4236		ext4_get_group_no_and_offset(sb, goal, &group, &block);
  4237	
  4238		/* set up allocation goals */
  4239		ac->ac_b_ex.fe_logical = EXT4_LBLK_CMASK(sbi, ar->logical);
  4240		ac->ac_status = AC_STATUS_CONTINUE;
  4241		ac->ac_sb = sb;
  4242		ac->ac_inode = ar->inode;
  4243		ac->ac_o_ex.fe_logical = ac->ac_b_ex.fe_logical;
  4244		ac->ac_o_ex.fe_group = group;
  4245		ac->ac_o_ex.fe_start = block;
  4246		ac->ac_o_ex.fe_len = len;
  4247		ac->ac_g_ex = ac->ac_o_ex;
  4248		ac->ac_flags = ar->flags;
  4249	
  4250		/* we have to define context: we'll we work with a file or
  4251		 * locality group. this is a policy, actually */
  4252		ext4_mb_group_or_file(ac);
  4253	
> 4254		mb_debug(1, "init ac: %u blocks @ %u, goal %u, flags %x, 2^%d, "
  4255				"left: %u/%u, right %u/%u to %swritable\n",
  4256				(unsigned) ar->len, (unsigned) ar->logical,
  4257				(unsigned) ar->goal, ac->ac_flags, ac->ac_2order,
  4258				(unsigned) ar->lleft, (unsigned) ar->pleft,
  4259				(unsigned) ar->lright, (unsigned) ar->pright,
> 4260				inode_is_open_for_write(&ar->inode) ? "" : "non-");
  4261		return 0;
  4262	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kbuild test robot Dec. 11, 2018, 1:12 a.m. | #2
Hi Nikolay,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on ext4/dev]
[also build test ERROR on v4.20-rc6 next-20181207]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nikolay-Borisov/fs-Convert-open-coded-is-inode-open-for-write-check/20181211-023543
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: i386-randconfig-x006-201849 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   fs//ext4/mballoc.c: In function 'ext4_mb_group_or_file':
>> fs//ext4/mballoc.c:5362:0: error: unterminated argument list invoking macro "if"
    }
    
>> fs//ext4/mballoc.c:4179:2: error: expected '(' at end of input
     if ((size == isize) && !ext4_fs_is_busy(sbi)
     ^~
>> fs//ext4/mballoc.c:4179:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
   fs//ext4/mballoc.c:5362:0: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
    }
    
>> fs//ext4/mballoc.c:4179:2: error: expected declaration or statement at end of input
     if ((size == isize) && !ext4_fs_is_busy(sbi)
     ^~
   At top level:
   fs//ext4/mballoc.c:4163:13: warning: 'ext4_mb_group_or_file' defined but not used [-Wunused-function]
    static void ext4_mb_group_or_file(struct ext4_allocation_context *ac)
                ^~~~~~~~~~~~~~~~~~~~~
   fs//ext4/mballoc.c:4092:13: warning: 'ext4_mb_show_ac' defined but not used [-Wunused-function]
    static void ext4_mb_show_ac(struct ext4_allocation_context *ac)
                ^~~~~~~~~~~~~~~
   fs//ext4/mballoc.c:3876:1: warning: 'ext4_mb_discard_group_preallocations' defined but not used [-Wunused-function]
    ext4_mb_discard_group_preallocations(struct super_block *sb,
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs//ext4/mballoc.c:3774:12: warning: 'ext4_mb_new_preallocation' defined but not used [-Wunused-function]
    static int ext4_mb_new_preallocation(struct ext4_allocation_context *ac)
               ^~~~~~~~~~~~~~~~~~~~~~~~~
   fs//ext4/mballoc.c:3563:13: warning: 'ext4_mb_put_pa' defined but not used [-Wunused-function]
    static void ext4_mb_put_pa(struct ext4_allocation_context *ac,
                ^~~~~~~~~~~~~~
   fs//ext4/mballoc.c:3399:1: warning: 'ext4_mb_use_preallocated' defined but not used [-Wunused-function]
    ext4_mb_use_preallocated(struct ext4_allocation_context *ac)
    ^~~~~~~~~~~~~~~~~~~~~~~~
   fs//ext4/mballoc.c:3282:13: warning: 'ext4_discard_allocated_blocks' defined but not used [-Wunused-function]
    static void ext4_discard_allocated_blocks(struct ext4_allocation_context *ac)
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs//ext4/mballoc.c:3253:13: warning: 'ext4_mb_collect_stats' defined but not used [-Wunused-function]
    static void ext4_mb_collect_stats(struct ext4_allocation_context *ac)
                ^~~~~~~~~~~~~~~~~~~~~
   fs//ext4/mballoc.c:3059:1: warning: 'ext4_mb_normalize_request' defined but not used [-Wunused-function]
    ext4_mb_normalize_request(struct ext4_allocation_context *ac,
    ^~~~~~~~~~~~~~~~~~~~~~~~~
   fs//ext4/mballoc.c:2921:1: warning: 'ext4_mb_mark_diskspace_used' defined but not used [-Wunused-function]
    ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs//ext4/mballoc.c:2099:1: warning: 'ext4_mb_regular_allocator' defined but not used [-Wunused-function]
    ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
    ^~~~~~~~~~~~~~~~~~~~~~~~~

vim +/if +5362 fs//ext4/mballoc.c

0c9ec4be Darrick J. Wong 2017-04-30  5314  
0c9ec4be Darrick J. Wong 2017-04-30  5315  /* Iterate all the free extents in the group. */
0c9ec4be Darrick J. Wong 2017-04-30  5316  int
0c9ec4be Darrick J. Wong 2017-04-30  5317  ext4_mballoc_query_range(
0c9ec4be Darrick J. Wong 2017-04-30  5318  	struct super_block		*sb,
0c9ec4be Darrick J. Wong 2017-04-30  5319  	ext4_group_t			group,
0c9ec4be Darrick J. Wong 2017-04-30  5320  	ext4_grpblk_t			start,
0c9ec4be Darrick J. Wong 2017-04-30  5321  	ext4_grpblk_t			end,
0c9ec4be Darrick J. Wong 2017-04-30  5322  	ext4_mballoc_query_range_fn	formatter,
0c9ec4be Darrick J. Wong 2017-04-30  5323  	void				*priv)
0c9ec4be Darrick J. Wong 2017-04-30  5324  {
0c9ec4be Darrick J. Wong 2017-04-30  5325  	void				*bitmap;
0c9ec4be Darrick J. Wong 2017-04-30  5326  	ext4_grpblk_t			next;
0c9ec4be Darrick J. Wong 2017-04-30  5327  	struct ext4_buddy		e4b;
0c9ec4be Darrick J. Wong 2017-04-30  5328  	int				error;
0c9ec4be Darrick J. Wong 2017-04-30  5329  
0c9ec4be Darrick J. Wong 2017-04-30  5330  	error = ext4_mb_load_buddy(sb, group, &e4b);
0c9ec4be Darrick J. Wong 2017-04-30  5331  	if (error)
0c9ec4be Darrick J. Wong 2017-04-30  5332  		return error;
0c9ec4be Darrick J. Wong 2017-04-30  5333  	bitmap = e4b.bd_bitmap;
0c9ec4be Darrick J. Wong 2017-04-30  5334  
0c9ec4be Darrick J. Wong 2017-04-30  5335  	ext4_lock_group(sb, group);
0c9ec4be Darrick J. Wong 2017-04-30  5336  
0c9ec4be Darrick J. Wong 2017-04-30  5337  	start = (e4b.bd_info->bb_first_free > start) ?
0c9ec4be Darrick J. Wong 2017-04-30  5338  		e4b.bd_info->bb_first_free : start;
0c9ec4be Darrick J. Wong 2017-04-30  5339  	if (end >= EXT4_CLUSTERS_PER_GROUP(sb))
0c9ec4be Darrick J. Wong 2017-04-30  5340  		end = EXT4_CLUSTERS_PER_GROUP(sb) - 1;
0c9ec4be Darrick J. Wong 2017-04-30  5341  
0c9ec4be Darrick J. Wong 2017-04-30  5342  	while (start <= end) {
0c9ec4be Darrick J. Wong 2017-04-30  5343  		start = mb_find_next_zero_bit(bitmap, end + 1, start);
0c9ec4be Darrick J. Wong 2017-04-30  5344  		if (start > end)
0c9ec4be Darrick J. Wong 2017-04-30  5345  			break;
0c9ec4be Darrick J. Wong 2017-04-30  5346  		next = mb_find_next_bit(bitmap, end + 1, start);
0c9ec4be Darrick J. Wong 2017-04-30  5347  
0c9ec4be Darrick J. Wong 2017-04-30  5348  		ext4_unlock_group(sb, group);
0c9ec4be Darrick J. Wong 2017-04-30  5349  		error = formatter(sb, group, start, next - start, priv);
0c9ec4be Darrick J. Wong 2017-04-30  5350  		if (error)
0c9ec4be Darrick J. Wong 2017-04-30  5351  			goto out_unload;
0c9ec4be Darrick J. Wong 2017-04-30  5352  		ext4_lock_group(sb, group);
0c9ec4be Darrick J. Wong 2017-04-30  5353  
0c9ec4be Darrick J. Wong 2017-04-30  5354  		start = next + 1;
0c9ec4be Darrick J. Wong 2017-04-30  5355  	}
0c9ec4be Darrick J. Wong 2017-04-30  5356  
0c9ec4be Darrick J. Wong 2017-04-30  5357  	ext4_unlock_group(sb, group);
0c9ec4be Darrick J. Wong 2017-04-30  5358  out_unload:
0c9ec4be Darrick J. Wong 2017-04-30  5359  	ext4_mb_unload_buddy(&e4b);
0c9ec4be Darrick J. Wong 2017-04-30  5360  
0c9ec4be Darrick J. Wong 2017-04-30  5361  	return error;
0c9ec4be Darrick J. Wong 2017-04-30 @5362  }

:::::: The code at line 5362 was first introduced by commit
:::::: 0c9ec4beecac94cb450c8abb2ac8b7e8a79240ea ext4: support GETFSMAP ioctls

:::::: TO: Darrick J. Wong <darrick.wong@oracle.com>
:::::: CC: Theodore Ts'o <tytso@mit.edu>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Patch

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 22a9d8159720..04e5bc0b5c8d 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -391,7 +391,7 @@  void ext4_da_update_reserve_space(struct inode *inode,
 	 * inode's preallocations.
 	 */
 	if ((ei->i_reserved_data_blocks == 0) &&
-	    (atomic_read(&inode->i_writecount) == 0))
+	    !inode_is_open_for_write(inode))
 		ext4_discard_preallocations(inode);
 }
 
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index e2248083cdca..7b656ec9a333 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4176,9 +4176,8 @@  static void ext4_mb_group_or_file(struct ext4_allocation_context *ac)
 	isize = (i_size_read(ac->ac_inode) + ac->ac_sb->s_blocksize - 1)
 		>> bsbits;
 
-	if ((size == isize) &&
-	    !ext4_fs_is_busy(sbi) &&
-	    (atomic_read(&ac->ac_inode->i_writecount) == 0)) {
+	if ((size == isize) && !ext4_fs_is_busy(sbi)
+	    && !inode_is_open_for_write(inode) {
 		ac->ac_flags |= EXT4_MB_HINT_NOPREALLOC;
 		return;
 	}
@@ -4258,7 +4257,7 @@  ext4_mb_initialize_context(struct ext4_allocation_context *ac,
 			(unsigned) ar->goal, ac->ac_flags, ac->ac_2order,
 			(unsigned) ar->lleft, (unsigned) ar->pleft,
 			(unsigned) ar->lright, (unsigned) ar->pright,
-			atomic_read(&ar->inode->i_writecount) ? "" : "non-");
+			inode_is_open_for_write(&ar->inode) ? "" : "non-");
 	return 0;
 
 }
diff --git a/fs/locks.c b/fs/locks.c
index 2ecb4db8c840..f71142269dd3 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1646,7 +1646,7 @@  check_conflicting_open(const struct dentry *dentry, const long arg, int flags)
 	if (flags & FL_LAYOUT)
 		return 0;
 
-	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
+	if ((arg == F_RDLCK) && inode_is_open_for_write(&inode))
 		return -EAGAIN;
 
 	if ((arg == F_WRLCK) && ((d_count(dentry) > 1) ||
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index e03be5071362..98b0769e4cf2 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -669,7 +669,7 @@  static int fanotify_add_inode_mark(struct fsnotify_group *group,
 	 */
 	if ((flags & FAN_MARK_IGNORED_MASK) &&
 	    !(flags & FAN_MARK_IGNORED_SURV_MODIFY) &&
-	    (atomic_read(&inode->i_writecount) > 0))
+	    inode_is_open_for_write(inode))
 		return 0;
 
 	return fanotify_add_mark(group, &inode->i_fsnotify_marks,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 1b88d58e1325..05fbf8a2fa42 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -103,7 +103,7 @@  static void ima_rdwr_violation_check(struct file *file,
 	} else {
 		if (must_measure)
 			set_bit(IMA_MUST_MEASURE, &iint->atomic_flags);
-		if ((atomic_read(&inode->i_writecount) > 0) && must_measure)
+		if (inode_is_open_for_write(inode) && must_measure)
 			send_writers = true;
 	}