> On Oct 16, 2018, at 8:26 PM, [email protected] 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 <[email protected]> >>> Reviewed-by: Wang Yi <[email protected]> >>> --- >>> 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
signature.asc
Description: Message signed with OpenPGP
