Re: [PATCH] fs: introduce is_dot_dotdot helper for cleanup

From: Tiezhu Yang
Date: Tue Dec 03 2019 - 03:34:28 EST


On 12/03/2019 10:39 AM, Darrick J. Wong wrote:
On Tue, Dec 03, 2019 at 10:07:41AM +0800, Tiezhu Yang wrote:
On 12/03/2019 04:03 AM, Matthew Wilcox wrote:
On Mon, Dec 02, 2019 at 06:10:13PM +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.
The idea is good. The implementation is, I'm afraid, badly chosen.
Did you benchmark this change at all? In general, you should prefer the
core kernel implementation to that of some less-interesting filesystems.
I measured the performance with the attached test program on my laptop
(Core-i7 Kaby Lake):

qstr . time_1 0.020531 time_2 0.005786
qstr .. time_1 0.017892 time_2 0.008798
qstr a time_1 0.017633 time_2 0.003634
qstr matthew time_1 0.011820 time_2 0.003605
qstr .a time_1 0.017909 time_2 0.008710
qstr , time_1 0.017631 time_2 0.003619

The results are quite stable:

qstr . time_1 0.021137 time_2 0.005780
qstr .. time_1 0.017964 time_2 0.008675
qstr a time_1 0.017899 time_2 0.003654
qstr matthew time_1 0.011821 time_2 0.003620
qstr .a time_1 0.017889 time_2 0.008662
qstr , time_1 0.017764 time_2 0.003613

Feel free to suggest some different strings we could use for testing.
These seemed like interesting strings to test with. It's always possible
I've messed up something with this benchmark that causes it to not
accurately represent the performance of each algorithm, so please check
that too.
[Sorry to resend this email because the mail list server
was denied due to it is not plain text.]

Hi Matthew,

Thanks for your reply and suggestion. I measured the
performance with the test program, the following
implementation is better for various of test cases:

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;
}

I will send a v2 patch used with this implementation.
Can you make it a static inline since it's such a short function?

Thanks for your suggestion, I will make it static inline and
move it to include/linux/fs.h in the v2 patch.

Thanks,

Tiezhu Yang


--D

Thanks,

Tiezhu Yang

+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;
+}
+EXPORT_SYMBOL(is_dot_dotdot);
diff --git a/fs/namei.c b/fs/namei.c
index 2dda552..7730a3b 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 (unlikely(is_dot_dotdot(this)))
+ return -EACCES;
while (len--) {
unsigned int c = *(const unsigned char *)name++;