linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] fs: Convert open-coded "is inode open for write" check
@ 2018-12-10 13:19 Nikolay Borisov
  2018-12-10 13:25 ` [PATCH v2] " Nikolay Borisov
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Nikolay Borisov @ 2018-12-10 13:19 UTC (permalink / raw)
  To: linux-fsdevel
  Cc: jack, linux-integrity, zohar, amir73il, jmorris, jlayton, tytso,
	linux-ext4, akpm, adilger.kernel, Nikolay Borisov

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(-)

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;
 	}
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2] fs: Convert open-coded "is inode open for write" check
  2018-12-10 13:19 [PATCH] fs: Convert open-coded "is inode open for write" check Nikolay Borisov
@ 2018-12-10 13:25 ` Nikolay Borisov
  2018-12-10 13:45   ` Jan Kara
  2018-12-10 20:18   ` Andreas Dilger
  2018-12-11  0:56 ` [PATCH] " kbuild test robot
  2018-12-11  1:12 ` kbuild test robot
  2 siblings, 2 replies; 6+ messages in thread
From: Nikolay Borisov @ 2018-12-10 13:25 UTC (permalink / raw)
  To: linux-fsdevel
  Cc: jack, linux-integrity, zohar, amir73il, jmorris, jlayton, tytso,
	linux-ext4, akpm, adilger.kernel, Nikolay Borisov

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.

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

Changes v2: 
 * This time actually compile-tested it, doh...


 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(-)

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..b811a9cb896e 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(ac->ac_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..16ea7d89a67d 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;
 	}
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] fs: Convert open-coded "is inode open for write" check
  2018-12-10 13:25 ` [PATCH v2] " Nikolay Borisov
@ 2018-12-10 13:45   ` Jan Kara
  2018-12-10 20:18   ` Andreas Dilger
  1 sibling, 0 replies; 6+ messages in thread
From: Jan Kara @ 2018-12-10 13:45 UTC (permalink / raw)
  To: Nikolay Borisov
  Cc: linux-fsdevel, jack, linux-integrity, zohar, amir73il, jmorris,
	jlayton, tytso, linux-ext4, akpm, adilger.kernel

On Mon 10-12-18 15:25:00, Nikolay Borisov wrote:
> 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.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> Changes v2: 
>  * This time actually compile-tested it, doh...

You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

for the fanotify and ext4 bits. But I think you may have better chances of
getting this applied if you split the patch by subsystem - i.e., ext4,
fanotify, security, locks - and let each maintainer merge the patch on his
own...

								Honza

> 
> 
>  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(-)
> 
> 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..b811a9cb896e 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(ac->ac_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..16ea7d89a67d 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;
>  	}
>  
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] fs: Convert open-coded "is inode open for write" check
  2018-12-10 13:25 ` [PATCH v2] " Nikolay Borisov
  2018-12-10 13:45   ` Jan Kara
@ 2018-12-10 20:18   ` Andreas Dilger
  1 sibling, 0 replies; 6+ messages in thread
From: Andreas Dilger @ 2018-12-10 20:18 UTC (permalink / raw)
  To: Nikolay Borisov
  Cc: linux-fsdevel, Jan Kara, linux-integrity, Mimi Zohar,
	Amir Goldstein, jmorris, jlayton, Theodore Y. Ts'o,
	Ext4 Developers List, Andrew Morton

[-- Attachment #1: Type: text/plain, Size: 3970 bytes --]

On Dec 10, 2018, at 6:25 AM, Nikolay Borisov <nborisov@suse.com> wrote:
> 
> 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.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> Changes v2:
> * This time actually compile-tested it, doh...
> 
> 
> 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(-)
> 
> 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..b811a9cb896e 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(ac->ac_inode)) {

If you will note the other cases in this patch, the "&&" operator should go
at the end of the previous line, instead of the start of the continued line.

Cheers, Andreas

> 		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..16ea7d89a67d 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;
> 	}
> 
> --
> 2.17.1
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs: Convert open-coded "is inode open for write" check
  2018-12-10 13:19 [PATCH] fs: Convert open-coded "is inode open for write" check Nikolay Borisov
  2018-12-10 13:25 ` [PATCH v2] " Nikolay Borisov
@ 2018-12-11  0:56 ` kbuild test robot
  2018-12-11  1:12 ` kbuild test robot
  2 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2018-12-11  0:56 UTC (permalink / raw)
  To: Nikolay Borisov
  Cc: kbuild-all, linux-fsdevel, jack, linux-integrity, zohar,
	amir73il, jmorris, jlayton, tytso, linux-ext4, akpm,
	adilger.kernel, Nikolay Borisov

[-- Attachment #1: Type: text/plain, Size: 6780 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38028 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs: Convert open-coded "is inode open for write" check
  2018-12-10 13:19 [PATCH] fs: Convert open-coded "is inode open for write" check Nikolay Borisov
  2018-12-10 13:25 ` [PATCH v2] " Nikolay Borisov
  2018-12-11  0:56 ` [PATCH] " kbuild test robot
@ 2018-12-11  1:12 ` kbuild test robot
  2 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2018-12-11  1:12 UTC (permalink / raw)
  To: Nikolay Borisov
  Cc: kbuild-all, linux-fsdevel, jack, linux-integrity, zohar,
	amir73il, jmorris, jlayton, tytso, linux-ext4, akpm,
	adilger.kernel, Nikolay Borisov

[-- Attachment #1: Type: text/plain, Size: 7302 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29372 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-12-11  1:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-10 13:19 [PATCH] fs: Convert open-coded "is inode open for write" check Nikolay Borisov
2018-12-10 13:25 ` [PATCH v2] " Nikolay Borisov
2018-12-10 13:45   ` Jan Kara
2018-12-10 20:18   ` Andreas Dilger
2018-12-11  0:56 ` [PATCH] " kbuild test robot
2018-12-11  1:12 ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).