| Message ID | 20181210131903.4528-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: Convert open-coded "is inode open for write" check | expand |
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
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
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; }
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(-)