| Message ID | 1575377810-3574-1-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fs: introduce is_dot_dotdot helper for cleanup | expand |
On Tue, Dec 03, 2019 at 08:56:50PM +0800, Tiezhu Yang wrote: > There exists many similar and duplicate codes to check "." and "..", > so introduce is_dot_dotdot helper to make the code more clean. > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > --- > > v2: > - use the better performance implementation of is_dot_dotdot > - make it static inline and move it to include/linux/fs.h Ugh, not more crap in fs.h. $ ls -l --sort=size include/linux |head -rw-r--r-- 1 willy willy 154148 Nov 29 22:35 netdevice.h -rw-r--r-- 1 willy willy 130488 Nov 29 22:35 skbuff.h -rw-r--r-- 1 willy willy 123540 Nov 29 22:35 pci_ids.h -rw-r--r-- 1 willy willy 118991 Nov 29 22:35 fs.h I think this probably fits well in namei.h. And I think it works better with bare 'name' and 'len' arguments, rather than taking a qstr. And, as I asked twice in the last round of review, did you benchmark this change?
On 12/03/2019 09:56 PM, Matthew Wilcox wrote: > On Tue, Dec 03, 2019 at 08:56:50PM +0800, Tiezhu Yang wrote: >> There exists many similar and duplicate codes to check "." and "..", >> so introduce is_dot_dotdot helper to make the code more clean. >> >> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> >> --- >> >> v2: >> - use the better performance implementation of is_dot_dotdot >> - make it static inline and move it to include/linux/fs.h > Ugh, not more crap in fs.h. > > $ ls -l --sort=size include/linux |head > -rw-r--r-- 1 willy willy 154148 Nov 29 22:35 netdevice.h > -rw-r--r-- 1 willy willy 130488 Nov 29 22:35 skbuff.h > -rw-r--r-- 1 willy willy 123540 Nov 29 22:35 pci_ids.h > -rw-r--r-- 1 willy willy 118991 Nov 29 22:35 fs.h > > I think this probably fits well in namei.h. And I think it works > better with bare 'name' and 'len' arguments, rather than taking a qstr. According to the definition of struct qstr: "quick string" -- eases parameter passing, but more importantly saves "metadata" about the string (ie length and the hash), it seems there is no need to use qstr to only check "." and "..", I will use "name" and "len" as arguments in the v3 patch and move it to namei.h: static inline bool is_dot_dotdot(const unsigned char *name, size_t len) { if (unlikely(name[0] == '.')) { if (len < 2 || (len == 2 && name[1] == '.')) return true; } return false; } > > And, as I asked twice in the last round of review, did you benchmark > this change? Before sending this v2 patch, I have done the test used with your test program and already pointed out the following implementation is better: bool is_dot_dotdot(const struct qstr *str) { if (unlikely(str->name[0] == '.')) { if (str->len < 2 || (str->len == 2 && str->name[1] == '.')) return true; } return false; } [enjoy@linux ~]$ lscpu | grep "Model name" Model name: Intel(R) Core(TM) i5-7200U CPU @ 2.50GHz [enjoy@linux ~]$ ./test qstr . time_1 0.025204 time_2 0.023717 qstr .. time_1 0.036542 time_2 0.034330 qstr a time_1 0.028123 time_2 0.022514 qstr matthew time_1 0.024282 time_2 0.022617 qstr .a time_1 0.037132 time_2 0.034118 qstr , time_1 0.028121 time_2 0.022527 [enjoy@linux ~]$ ./test qstr . time_1 0.025200 time_2 0.023666 qstr .. time_1 0.036486 time_2 0.034275 qstr a time_1 0.028113 time_2 0.022514 qstr matthew time_1 0.024289 time_2 0.022515 qstr .a time_1 0.037058 time_2 0.034063 qstr , time_1 0.028117 time_2 0.022523 [enjoy@linux ~]$ ./test qstr . time_1 0.025128 time_2 0.023692 qstr .. time_1 0.036687 time_2 0.034284 qstr a time_1 0.028133 time_2 0.022527 qstr matthew time_1 0.024246 time_2 0.022589 qstr .a time_1 0.037063 time_2 0.034106 qstr , time_1 0.028127 time_2 0.022522 Additionally, by declaring is_dot_dotdot inline, we can make execution faster by eliminating the function-call overhead, so the performance influence is very small with this patch. If you have any more suggestion, please let me know. Thanks, Tiezhu Yang
On Thu, Dec 05, 2019 at 08:56:07AM +0800, Tiezhu Yang wrote: > > And, as I asked twice in the last round of review, did you benchmark > > this change? > > Before sending this v2 patch, I have done the test used with your test > program and already pointed out the following implementation is better: I didn't mean "have you run the test program i wrote". I meant "have you booted a kernel with this change and done some performance measurements to see if you've changed anything".
On 12/05/2019 03:06 PM, Matthew Wilcox wrote: > On Thu, Dec 05, 2019 at 08:56:07AM +0800, Tiezhu Yang wrote: >>> And, as I asked twice in the last round of review, did you benchmark >>> this change? >> Before sending this v2 patch, I have done the test used with your test >> program and already pointed out the following implementation is better: > I didn't mean "have you run the test program i wrote". I meant "have you > booted a kernel with this change and done some performance measurements > to see if you've changed anything". Oh, no, it is hard to measure the performance influence with this patch. Based on the above analysis, I think the performance influence is very small due to is_dot_dotdot() is a such short static inline function. Thanks, Tiezhu Yang
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c index 3da3707..36be864 100644 --- a/fs/crypto/fname.c +++ b/fs/crypto/fname.c @@ -15,17 +15,6 @@ #include <crypto/skcipher.h> #include "fscrypt_private.h" -static inline bool fscrypt_is_dot_dotdot(const struct qstr *str) -{ - if (str->len == 1 && str->name[0] == '.') - return true; - - if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.') - return true; - - return false; -} - /** * fname_encrypt() - encrypt a filename * @@ -255,7 +244,7 @@ int fscrypt_fname_disk_to_usr(struct inode *inode, const struct qstr qname = FSTR_TO_QSTR(iname); struct fscrypt_digested_name digested_name; - if (fscrypt_is_dot_dotdot(&qname)) { + if (is_dot_dotdot(&qname)) { oname->name[0] = '.'; oname->name[iname->len - 1] = '.'; oname->len = iname->len; @@ -323,7 +312,7 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname, memset(fname, 0, sizeof(struct fscrypt_name)); fname->usr_fname = iname; - if (!IS_ENCRYPTED(dir) || fscrypt_is_dot_dotdot(iname)) { + if (!IS_ENCRYPTED(dir) || is_dot_dotdot(iname)) { fname->disk_name.name = (unsigned char *)iname->name; fname->disk_name.len = iname->len; return 0; diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c index f91db24..6f4db74 100644 --- a/fs/ecryptfs/crypto.c +++ b/fs/ecryptfs/crypto.c @@ -1991,16 +1991,6 @@ int ecryptfs_encrypt_and_encode_filename( return rc; } -static bool is_dot_dotdot(const char *name, size_t name_size) -{ - if (name_size == 1 && name[0] == '.') - return true; - else if (name_size == 2 && name[0] == '.' && name[1] == '.') - return true; - - return false; -} - /** * ecryptfs_decode_and_decrypt_filename - converts the encoded cipher text name to decoded plaintext * @plaintext_name: The plaintext name @@ -2020,6 +2010,7 @@ int ecryptfs_decode_and_decrypt_filename(char **plaintext_name, { struct ecryptfs_mount_crypt_stat *mount_crypt_stat = &ecryptfs_superblock_to_private(sb)->mount_crypt_stat; + const struct qstr file_name = {.name = name, .len = name_size}; char *decoded_name; size_t decoded_name_size; size_t packet_size; @@ -2027,7 +2018,7 @@ int ecryptfs_decode_and_decrypt_filename(char **plaintext_name, if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES) && !(mount_crypt_stat->flags & ECRYPTFS_ENCRYPTED_VIEW_ENABLED)) { - if (is_dot_dotdot(name, name_size)) { + if (is_dot_dotdot(&file_name)) { rc = ecryptfs_copy_filename(plaintext_name, plaintext_name_size, name, name_size); diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 5a888a0..3d5e684 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -2767,17 +2767,6 @@ static inline bool f2fs_cp_error(struct f2fs_sb_info *sbi) return is_set_ckpt_flags(sbi, CP_ERROR_FLAG); } -static inline bool is_dot_dotdot(const struct qstr *str) -{ - if (str->len == 1 && str->name[0] == '.') - return true; - - if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.') - return true; - - return false; -} - static inline bool f2fs_may_extent_tree(struct inode *inode) { struct f2fs_sb_info *sbi = F2FS_I_SB(inode); diff --git a/fs/namei.c b/fs/namei.c index 2dda552..babe7e8 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2458,10 +2458,8 @@ static int lookup_one_len_common(const char *name, struct dentry *base, if (!len) return -EACCES; - if (unlikely(name[0] == '.')) { - if (len < 2 || (len == 2 && name[1] == '.')) - return -EACCES; - } + if (is_dot_dotdot(this)) + return -EACCES; while (len--) { unsigned int c = *(const unsigned char *)name++; diff --git a/include/linux/fs.h b/include/linux/fs.h index 98e0349..78a2932 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -3627,4 +3627,14 @@ static inline int inode_drain_writes(struct inode *inode) return filemap_write_and_wait(inode->i_mapping); } +static inline bool is_dot_dotdot(const struct qstr *str) +{ + if (unlikely(str->name[0] == '.')) { + if (str->len < 2 || (str->len == 2 && str->name[1] == '.')) + return true; + } + + return false; +} + #endif /* _LINUX_FS_H */
There exists many similar and duplicate codes to check "." and "..", so introduce is_dot_dotdot helper to make the code more clean. Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- v2: - use the better performance implementation of is_dot_dotdot - make it static inline and move it to include/linux/fs.h fs/crypto/fname.c | 15 ++------------- fs/ecryptfs/crypto.c | 13 ++----------- fs/f2fs/f2fs.h | 11 ----------- fs/namei.c | 6 ++---- include/linux/fs.h | 10 ++++++++++ 5 files changed, 16 insertions(+), 39 deletions(-)