Re: [PATCH] ext4: direct return when jinode allocate failed

Re: [PATCH] ext4: direct return when jinode allocate failed

[Andreas Dilger]

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


> On Oct 16, 2018, at 8:26 PM, liu.song11@zte.com.cn wrote:
> 
>> On Tue, Oct 16, 2018 at 10:55:26PM +0800, fishland wrote:
>>> The jinode does not need protected by *i_lock*, we can return
>>> directly if memory allocation fails.
>>> 
>> 
>> I don't see anything wrong with this patch, but at the same time, I'm
>> not sure I see the benefit, either.  Checking for the allocation
>> failure is cheap, and moving it out spinlock doesn't buy much; not to
>> mention that the allocation failure is going to be highly uncommon.
>> 
>> What inspired this commit?
>> 
>> - Ted
> 
> Hi, Ted
> I found this during code reading. All code logic is very smooth, when
> reading here, need to spin_unlock before "return -ENOMEM", which I
> feel this could more close to the allocate. I refer to other processing
> logic for this situation in "inode.c", found that they all return immediately
> after memory application failed. I agree with you that the benefit of this
> patch is almost none, however it can slightly improve the code in rare case.

Looking at the patch there are two effects that it has:
- optimize a very rare case where there is an allocation failure before locking
- return an unnecessary error if "ei->jinode" is allocated before locking

I don't think it is worthwhile to optimize this case, since allocation failures
will have a serious impact on the application, and I'd rather avoid the rare
case where we don't return an unnecessary error than make the error case faster.

Cheers, Andreas

> 
>>> Signed-off-by: Liu Song <liu.song11@zte.com.cn>
>>> Reviewed-by: Wang Yi <wang.yi59@zte.com.cn>
>>> ---
>>> fs/ext4/inode.c | 7 +++----
>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>>> index d767e993591d..67ba6f062de5 100644
>>> --- a/fs/ext4/inode.c
>>> +++ b/fs/ext4/inode.c
>>> @@ -4384,12 +4384,11 @@ int ext4_inode_attach_jinode(struct inode *inode)
>>>         return 0;
>>> 
>>>  	jinode = jbd2_alloc_inode(GFP_KERNEL);
>>> +	if (!jinode)
>>> +	return -ENOMEM;
>>> +
>>>  	spin_lock(&inode->i_lock);
>>>  	if (!ei->jinode) {
>>> -		if (!jinode) {
>>> -			spin_unlock(&inode->i_lock);
>>> -			return -ENOMEM;
>>> -		}
>>>  		ei->jinode = jinode;
>>>  		jbd2_journal_init_jbd_inode(ei->jinode, inode);
>>>  		jinode = NULL;
>>> --
>>> 2.17.1


Cheers, Andreas






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

Re: [PATCH] ext4: direct return when jinode allocate failed

[Theodore Y. Ts'o]

On Wed, Oct 17, 2018 at 05:28:55PM -0600, Andreas Dilger wrote:
> 
> Looking at the patch there are two effects that it has:
> - optimize a very rare case where there is an allocation failure before locking
> - return an unnecessary error if "ei->jinode" is allocated before locking
> 
> I don't think it is worthwhile to optimize this case, since allocation failures
> will have a serious impact on the application, and I'd rather avoid the rare
> case where we don't return an unnecessary error than make the error case faster.

To be fair, the "unnecessery error" case is also extremely rare.  In
order for that to happen, two processes would need to be racing to
allocate allocate the jinode structure for an inode (since we bail
earlier when we check for the ei->jinode == NULL case before we take
the lock), where the first process succeeds in allocating memory ---
but the second one fails.

Both of these are super-rare cases, and involve the system thrashing
due to super-high memory pressure --- at which point worrying about a
micro-optimization or an unnecessary failure is going to be the last
of the system's problem.

This is why I said, "the patch probably doesn't hurt, but I also don't
see it helping much".

Cheers,

						- Ted

Re: [PATCH] ext4: direct return when jinode allocate failed

[Theodore Y. Ts'o]

On Tue, Oct 16, 2018 at 10:55:26PM +0800, fishland wrote:
> The jinode does not need protected by *i_lock*, we can return
> directly if memory allocation fails.
> 

I don't see anything wrong with this patch, but at the same time, I'm
not sure I see the benefit, either.  Checking for the allocation
failure is cheap, and moving it out spinlock doesn't buy much; not to
mention that the allocation failure is going to be highly uncommon.

What inspired this commit?

						- Ted



> Signed-off-by: Liu Song <liu.song11@zte.com.cn>
> Reviewed-by: Wang Yi <wang.yi59@zte.com.cn>
> ---
>  fs/ext4/inode.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index d767e993591d..67ba6f062de5 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4384,12 +4384,11 @@ int ext4_inode_attach_jinode(struct inode *inode)
>  		return 0;
>  
>  	jinode = jbd2_alloc_inode(GFP_KERNEL);
> +	if (!jinode)
> +		return -ENOMEM;
> +
>  	spin_lock(&inode->i_lock);
>  	if (!ei->jinode) {
> -		if (!jinode) {
> -			spin_unlock(&inode->i_lock);
> -			return -ENOMEM;
> -		}
>  		ei->jinode = jinode;
>  		jbd2_journal_init_jbd_inode(ei->jinode, inode);
>  		jinode = NULL;
> -- 
> 2.17.1
>