Re: [PATCH] fork: Use helper function mapping_allow_writable() in dup_mmap()

Re: [PATCH] fork: Use helper function mapping_allow_writable() in dup_mmap()

[linmiaohe]

Christian Brauner <christian.brauner@ubuntu.com> wrote:
> On Sun, Sep 13, 2020 at 05:24:15AM -0400, Miaohe Lin wrote:
>> Use helper function mapping_allow_writable() to atomic_inc i_mmap_writable.
>> 
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>
>Hey Miaohe,
>
>Thanks for the patch!
>Per se there's nothing against using a proper helper when it exists.
>But it has already been pointed out that this needs a proper commit message with more rationale. But I'm otherwise happy to take this.
>

Many thanks for your reply.
Eric have kindly pointed this out. I think I should provide a proper commit message with more rationale in v2.
Thanks again.

>Thanks!
>Christian

Re: [PATCH] fork: Use helper function mapping_allow_writable() in dup_mmap()

[Christian Brauner]

On Sun, Sep 13, 2020 at 05:24:15AM -0400, Miaohe Lin wrote:
> Use helper function mapping_allow_writable() to atomic_inc i_mmap_writable.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---

Hey Miaohe,

Thanks for the patch!
Per se there's nothing against using a proper helper when it exists.
But it has already been pointed out that this needs a proper commit
message with more rationale. But I'm otherwise happy to take this.

Thanks!
Christian

Re: [PATCH] fork: Use helper function mapping_allow_writable() in dup_mmap()

[linmiaohe]

Eric W. Biederman <ebiederm@xmission.com> wrote:
> linmiaohe <linmiaohe@huawei.com> writes:
>> Eric W. Biederman <ebiederm@xmission.com> wrote:
>>> Miaohe Lin <linmiaohe@huawei.com> writes:
>>>
>>>> Use helper function mapping_allow_writable() to atomic_inc 
>>>> i_mmap_writable.
>>>
>>>Why?
>>>
>>
>> Because I think it's better to use the wrapper function instead of the 
>> open hard code.
>
>My point is there is no context in this commit message.
>
>What makes it better to use the wrapper function?
>What makes the wrapper function the appropriate function to use?
>Why just this location?
>Why wasn't this change made when the wrapper function was introduced?
>
>I could probably read through the code and figure these things out but the description of the change should really include these things.
>
>Eric

I see. Many thanks for your detailed explaination. :)

Re: [PATCH] fork: Use helper function mapping_allow_writable() in dup_mmap()

[Eric W. Biederman]

linmiaohe <linmiaohe@huawei.com> writes:

> Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Miaohe Lin <linmiaohe@huawei.com> writes:
>>
>>> Use helper function mapping_allow_writable() to atomic_inc 
>>> i_mmap_writable.
>>
>>Why?
>>
>
> Because I think it's better to use the wrapper function instead of the
> open hard code.

My point is there is no context in this commit message.

What makes it better to use the wrapper function?
What makes the wrapper function the appropriate function to use?
Why just this location?
Why wasn't this change made when the wrapper function was introduced?

I could probably read through the code and figure these things out
but the description of the change should really include these things.

Eric

Re: [PATCH] fork: Use helper function mapping_allow_writable() in dup_mmap()

[linmiaohe]

Eric W. Biederman <ebiederm@xmission.com> wrote:
> Miaohe Lin <linmiaohe@huawei.com> writes:
>
>> Use helper function mapping_allow_writable() to atomic_inc 
>> i_mmap_writable.
>
>Why?
>

Because I think it's better to use the wrapper function instead of the open hard code.
Thanks.

Re: [PATCH] fork: Use helper function mapping_allow_writable() in dup_mmap()

[Eric W. Biederman]

Miaohe Lin <linmiaohe@huawei.com> writes:

> Use helper function mapping_allow_writable() to atomic_inc
> i_mmap_writable.

Why?


> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  kernel/fork.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 4b328aecabb2..a0586716e327 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -558,7 +558,7 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>  				atomic_dec(&inode->i_writecount);
>  			i_mmap_lock_write(mapping);
>  			if (tmp->vm_flags & VM_SHARED)
> -				atomic_inc(&mapping->i_mmap_writable);
> +				mapping_allow_writable(mapping);
>  			flush_dcache_mmap_lock(mapping);
>  			/* insert tmp into the share list, just after mpnt */
>  			vma_interval_tree_insert_after(tmp, mpnt,

[PATCH] fork: Use helper function mapping_allow_writable() in dup_mmap()

[Miaohe Lin]

Use helper function mapping_allow_writable() to atomic_inc i_mmap_writable.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 kernel/fork.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 4b328aecabb2..a0586716e327 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -558,7 +558,7 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
 				atomic_dec(&inode->i_writecount);
 			i_mmap_lock_write(mapping);
 			if (tmp->vm_flags & VM_SHARED)
-				atomic_inc(&mapping->i_mmap_writable);
+				mapping_allow_writable(mapping);
 			flush_dcache_mmap_lock(mapping);
 			/* insert tmp into the share list, just after mpnt */
 			vma_interval_tree_insert_after(tmp, mpnt,
-- 
2.19.1