From: Zheng Liu <[email protected]>
This commit tries to fix a bug that we can't read symlink properly with
inline data feature when the length of symlink is greater than 60 bytes
but less than extra space.
The key issue is in ext4_inode_is_fast_symlink() that it doesn't check
whether or not an inode has inline data. When the user creates a new
symlink, an inode will be allocated with MAY_INLINE_DATA flag. Then
symlink will be stored in ->i_block and extended attribute space. In
the mean time, this inode is with inline data flag. After remounting
it, ext4_inode_is_fast_symlink() function thinks that this inode is a
fast symlink so that the data in ->i_block is copied to the user, and
the data in extra space is trimmed. In fact this inode should be as a
normal symlink.
The following script can hit this bug.
#!/bin/bash
cd ${MNT}
filename=ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789
rm -rf test
mkdir test
cd test
echo "hello" >$filename
ln -s $filename symlinkfile
cd
sudo umount /mnt/sda1
sudo mount -t ext4 /dev/sda1 /mnt/sda1
readlink /mnt/sda1/test/symlinkfile
After applying this patch, it will break the assumption in e2fsck
because the original implementation doesn't want to support symlink
with inline data.
Reported-by: "Darrick J. Wong" <[email protected]>
Reported-by: Ian Nartowicz <[email protected]>
Cc: Ian Nartowicz <[email protected]>
Cc: Tao Ma <[email protected]>
Cc: "Darrick J. Wong" <[email protected]>
Cc: Andreas Dilger <[email protected]>
Cc: "Theodore Ts'o" <[email protected]>
Signed-off-by: Zheng Liu <[email protected]>
---
fs/ext4/inode.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 55f999a..bc5e4c1 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -148,6 +148,9 @@ static int ext4_inode_is_fast_symlink(struct inode *inode)
int ea_blocks = EXT4_I(inode)->i_file_acl ?
EXT4_CLUSTER_SIZE(inode->i_sb) >> 9 : 0;
+ if (ext4_has_inline_data(inode))
+ return 0;
+
return (S_ISLNK(inode->i_mode) && inode->i_blocks - ea_blocks == 0);
}
--
1.7.9.7
On Mon, Jun 02, 2014 at 07:39:28PM +0800, Zheng Liu wrote:
> From: Zheng Liu <[email protected]>
>
> This commit tries to fix a bug that we can't read symlink properly with
> inline data feature when the length of symlink is greater than 60 bytes
> but less than extra space....
Applied, thanks.
> + if (ext4_has_inline_data(inode))
> + return 0;
> +
I am wondering though, as we add ext4_has_inline_data() to more and
more codepaths, whether we might be better off making this an inline
function in ext4.h. We should see how much such a change bloats the
text sizes, and whether there is any measurable CPU difference.
Cheers,
- Ted
On Mon, Jun 02, 2014 at 10:56:31AM -0400, Theodore Ts'o wrote:
> I am wondering though, as we add ext4_has_inline_data() to more and
> more codepaths, whether we might be better off making this an inline
> function in ext4.h. We should see how much such a change bloats the
> text sizes, and whether there is any measurable CPU difference.
Subject: [PATCH] ext4: make ext4_has_inline_data() as a inline function
From: Zheng Liu <[email protected]>
Now ext4_has_inline_data() is used in wide spread codepaths. So we need
to make it as a inline function to avoid burning some CPU cycles.
text size (bytes):
vanilla: 10350562
patched: 10384933 (+0.33%)
I use the following script to measure the CPU usage.
#!/bin/bash
shm_base='/dev/shm'
img=${shm_base}/ext4-img
mnt=/mnt/loop
e2fsprgs_base=$HOME/e2fsprogs
mkfs=${e2fsprgs_base}/misc/mke2fs
fsck=${e2fsprgs_base}/e2fsck/e2fsck
sudo umount $mnt
dd if=/dev/zero of=$img bs=4k count=3145728
${mkfs} -t ext4 -O inline_data -F $img
sudo mount -t ext4 -o loop $img $mnt
# start testing...
testdir="${mnt}/testdir"
mkdir $testdir
cd $testdir
echo "start testing..."
for ((cnt=0;cnt<100;cnt++)); do
for ((i=0;i<5;i++)); do
for ((j=0;j<5;j++)); do
for ((k=0;k<5;k++)); do
for ((l=0;l<5;l++)); do
mkdir -p $i/$j/$k/$l
echo "$i-$j-$k-$l" > $i/$j/$k/$l/testfile
done
done
done
done
ls -R $testdir > /dev/null
rm -rf $testdir/*
done
The result of `perf top -G -U` is as below.
vanilla:
13.92% [ext4] [k] ext4_do_update_inode
9.36% [ext4] [k] __ext4_get_inode_loc
4.07% [ext4] [k] ftrace_define_fields_ext4_writepages
3.83% [ext4] [k] __ext4_handle_dirty_metadata
3.42% [ext4] [k] ext4_get_inode_flags
2.71% [ext4] [k] ext4_mark_iloc_dirty
2.46% [ext4] [k] ftrace_define_fields_ext4_direct_IO_enter
2.26% [ext4] [k] ext4_get_inode_loc
2.22% [ext4] [k] ext4_has_inline_data
[...]
After applied the patch, we don't see ext4_has_inline_data() because it
has been inlined and perf couldn't sample it. Although it doesn't mean
that the CPU cycles can be saved but at least the overhead of function
calls can be eliminated. So IMHO we'd better inline this function.
Cc: Andreas Dilger <[email protected]>
Cc: "Theodore Ts'o" <[email protected]>
Signed-off-by: Zheng Liu <[email protected]>
---
fs/ext4/ext4.h | 7 ++++++-
fs/ext4/inline.c | 6 ------
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 0519715..0531df5 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2564,7 +2564,6 @@ extern const struct file_operations ext4_file_operations;
extern loff_t ext4_llseek(struct file *file, loff_t offset, int origin);
/* inline.c */
-extern int ext4_has_inline_data(struct inode *inode);
extern int ext4_get_max_inline_size(struct inode *inode);
extern int ext4_find_inline_data_nolock(struct inode *inode);
extern int ext4_init_inline_data(handle_t *handle, struct inode *inode,
@@ -2630,6 +2629,12 @@ extern void ext4_inline_data_truncate(struct inode *inode, int *has_inline);
extern int ext4_convert_inline_data(struct inode *inode);
+static inline int ext4_has_inline_data(struct inode *inode)
+{
+ return ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA) &&
+ EXT4_I(inode)->i_inline_off;
+}
+
/* namei.c */
extern const struct inode_operations ext4_dir_inode_operations;
extern const struct inode_operations ext4_special_inode_operations;
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 645205d..141b6ac 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -120,12 +120,6 @@ int ext4_get_max_inline_size(struct inode *inode)
return max_inline_size + EXT4_MIN_INLINE_DATA_SIZE;
}
-int ext4_has_inline_data(struct inode *inode)
-{
- return ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA) &&
- EXT4_I(inode)->i_inline_off;
-}
On Wed, Jun 04, 2014 at 03:11:11PM +0800, Zheng Liu wrote:
> Now ext4_has_inline_data() is used in wide spread codepaths. So we need
> to make it as a inline function to avoid burning some CPU cycles.
>
> text size (bytes):
> vanilla: 10350562
> patched: 10384933 (+0.33%)
This represents a difference of 34k, which was over 10% the text size
of ext4.o --- so this surprised me greatly. I just did my own
testing, and difference I see is 117 bytes:
text data bss dec hex filename
before: 326110 19258 5528 350896 55ab0 fs/ext4/ext4.o
after: 326227 19258 5528 351013 55b25 fs/ext4/ext4.o
So I'm going to adjust the patch include these numbers, which are much
less likely to cause embedded people to complain about a large
increase in code bloat. :-)
Thanks, applied.
- Ted