Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] fs: introduce is_dot_dotdot helper for cleanup
@ 2019-12-02 10:10 Tiezhu Yang
  2019-12-02 20:03 ` Matthew Wilcox
  0 siblings, 1 reply; 7+ messages in thread
From: Tiezhu Yang @ 2019-12-02 10:10 UTC (permalink / raw)
  To: Alexander Viro, Theodore Y. Ts'o, Jaegeuk Kim, Chao Yu,
	Eric Biggers, Tyler Hicks
  Cc: linux-fsdevel, ecryptfs, linux-fscrypt, linux-f2fs-devel, linux-kernel

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>
---
 fs/crypto/fname.c    | 15 ++-------------
 fs/ecryptfs/crypto.c | 13 ++-----------
 fs/f2fs/f2fs.h       | 11 -----------
 fs/libfs.c           | 12 ++++++++++++
 fs/namei.c           |  6 ++----
 include/linux/fs.h   |  2 ++
 6 files changed, 20 insertions(+), 39 deletions(-)

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/libfs.c b/fs/libfs.c
index 1463b03..876b1b6 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1291,3 +1291,15 @@ bool is_empty_dir_inode(struct inode *inode)
 	return (inode->i_fop == &empty_dir_operations) &&
 		(inode->i_op == &empty_dir_inode_operations);
 }
+
+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++;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c159a8b..e999826 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3627,4 +3627,6 @@ static inline int inode_drain_writes(struct inode *inode)
 	return filemap_write_and_wait(inode->i_mapping);
 }
 
+extern bool is_dot_dotdot(const struct qstr *str);
+
 #endif /* _LINUX_FS_H */
-- 
2.1.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fs: introduce is_dot_dotdot helper for cleanup
  2019-12-02 10:10 [PATCH] fs: introduce is_dot_dotdot helper for cleanup Tiezhu Yang
@ 2019-12-02 20:03 ` Matthew Wilcox
  2019-12-03  2:07   ` Tiezhu Yang
  0 siblings, 1 reply; 7+ messages in thread
From: Matthew Wilcox @ 2019-12-02 20:03 UTC (permalink / raw)
  To: Tiezhu Yang
  Cc: Alexander Viro, Theodore Y. Ts'o, Jaegeuk Kim, Chao Yu,
	Eric Biggers, Tyler Hicks, linux-fsdevel, ecryptfs,
	linux-fscrypt, linux-f2fs-devel, linux-kernel

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

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.

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

[-- Attachment #2: dotdotdot.c --]
[-- Type: text/plain, Size: 2053 bytes --]

#include <stdio.h>
#include <time.h>

typedef _Bool bool;
#define true 1
#define false 0
#define unlikely(x)	(x)
typedef unsigned int u32;
typedef unsigned long long u64;
#define HASH_LEN_DECLARE u32 hash; u32 len

struct qstr {
        union {
                struct {
                        HASH_LEN_DECLARE;
                };
                u64 hash_len;
        };
        const char *name;
};

bool is_dot_dotdot_1(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;
}

bool is_dot_dotdot_2(const struct qstr *str)
{
	if (unlikely(str->name[0] == '.')) {
		if (str->len < 2 || (str->len == 2 && str->name[1] == '.'))
			return false;
	}

	return true;
}

double time_sub(struct timespec *before, struct timespec *after)
{
	struct timespec diff = { .tv_sec  = after->tv_sec  - before->tv_sec,
				 .tv_nsec = after->tv_nsec - before->tv_nsec };
	if (diff.tv_nsec < 0) {
		diff.tv_nsec += 1000 * 1000 * 1000;
		diff.tv_sec -= 1;
	}

	return diff.tv_sec + diff.tv_nsec * 0.0000000001d;
}

bool res;

void mytime(const struct qstr *qstr)
{
	unsigned int i;
	struct timespec start, middle, end;

	clock_gettime(CLOCK_THREAD_CPUTIME_ID, &start);
	for (i = 0; i < 100000000; i++)
		res = is_dot_dotdot_1(qstr);
	clock_gettime(CLOCK_THREAD_CPUTIME_ID, &middle);
	for (i = 0; i < 100000000; i++)
		res = is_dot_dotdot_2(qstr);
	clock_gettime(CLOCK_THREAD_CPUTIME_ID, &end);

	printf("qstr %s time_1 %f time_2 %f\n", qstr->name,
			time_sub(&start, &middle), time_sub(&middle, &end));
}

int main(int argc, char **argv)
{
	struct qstr dot = { .len = 1, .name = "." };
	struct qstr dotdot = { .len = 2, .name = ".." };
	struct qstr a = { .len = 1, .name = "a" };
	struct qstr matthew = { .len = 7, .name = "matthew" };
	struct qstr dota = { .len = 2, .name = ".a" };
	struct qstr comma = { .len = 1, .name = "," };

	mytime(&dot);
	mytime(&dotdot);
	mytime(&a);
	mytime(&matthew);
	mytime(&dota);
	mytime(&comma);

	return 0;
}

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fs: introduce is_dot_dotdot helper for cleanup
  2019-12-02 20:03 ` Matthew Wilcox
@ 2019-12-03  2:07   ` Tiezhu Yang
  2019-12-03  2:39     ` Darrick J. Wong
                       ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Tiezhu Yang @ 2019-12-03  2:07 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Alexander Viro, Theodore Y. Ts'o, Jaegeuk Kim, Chao Yu,
	Eric Biggers, Tyler Hicks, linux-fsdevel, ecryptfs,
	linux-fscrypt, linux-f2fs-devel, linux-kernel

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.

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fs: introduce is_dot_dotdot helper for cleanup
  2019-12-03  2:07   ` Tiezhu Yang
@ 2019-12-03  2:39     ` Darrick J. Wong
  2019-12-03  8:33       ` Tiezhu Yang
  2019-12-03  2:46     ` Matthew Wilcox
  2019-12-04 13:06     ` Jean-Louis Biasini
  2 siblings, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2019-12-03  2:39 UTC (permalink / raw)
  To: Tiezhu Yang
  Cc: Matthew Wilcox, Alexander Viro, Theodore Y. Ts'o,
	Jaegeuk Kim, Chao Yu, Eric Biggers, Tyler Hicks, linux-fsdevel,
	ecryptfs, linux-fscrypt, linux-f2fs-devel, linux-kernel

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?

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fs: introduce is_dot_dotdot helper for cleanup
  2019-12-03  2:07   ` Tiezhu Yang
  2019-12-03  2:39     ` Darrick J. Wong
@ 2019-12-03  2:46     ` Matthew Wilcox
  2019-12-04 13:06     ` Jean-Louis Biasini
  2 siblings, 0 replies; 7+ messages in thread
From: Matthew Wilcox @ 2019-12-03  2:46 UTC (permalink / raw)
  To: Tiezhu Yang
  Cc: Alexander Viro, Theodore Y. Ts'o, Jaegeuk Kim, Chao Yu,
	Eric Biggers, Tyler Hicks, linux-fsdevel, ecryptfs,
	linux-fscrypt, linux-f2fs-devel, linux-kernel

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
> 
> 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.

Well, hang on.  If you haven't done any benchmarking, please do so
before sending a v2.  In particular, you've now moved this to being a
function call.  That might slow things down, or it might speed things up.
I also don't know if passing a qstr is going to be the right API --
let's hear from the filesystems affected by the API change that they're
OK with this change.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fs: introduce is_dot_dotdot helper for cleanup
  2019-12-03  2:39     ` Darrick J. Wong
@ 2019-12-03  8:33       ` Tiezhu Yang
  0 siblings, 0 replies; 7+ messages in thread
From: Tiezhu Yang @ 2019-12-03  8:33 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Matthew Wilcox, Alexander Viro, Theodore Y. Ts'o,
	Jaegeuk Kim, Chao Yu, Eric Biggers, Tyler Hicks, linux-fsdevel,
	ecryptfs, linux-fscrypt, linux-f2fs-devel, linux-kernel

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] fs: introduce is_dot_dotdot helper for cleanup
  2019-12-03  2:07   ` Tiezhu Yang
  2019-12-03  2:39     ` Darrick J. Wong
  2019-12-03  2:46     ` Matthew Wilcox
@ 2019-12-04 13:06     ` Jean-Louis Biasini
  2 siblings, 0 replies; 7+ messages in thread
From: Jean-Louis Biasini @ 2019-12-04 13:06 UTC (permalink / raw)
  To: Tiezhu Yang, Matthew Wilcox
  Cc: Alexander Viro, Theodore Y. Ts'o, Jaegeuk Kim, Chao Yu,
	Eric Biggers, Tyler Hicks, linux-fsdevel, ecryptfs,
	linux-fscrypt, linux-f2fs-devel, linux-kernel

Please UNSUBSCRIBE ME from this list of tell how to!!!

Le 03/12/2019 à 03:07, Tiezhu Yang a écrit :
> 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.
>
> 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++;
>


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-02 10:10 [PATCH] fs: introduce is_dot_dotdot helper for cleanup Tiezhu Yang
2019-12-02 20:03 ` Matthew Wilcox
2019-12-03  2:07   ` Tiezhu Yang
2019-12-03  2:39     ` Darrick J. Wong
2019-12-03  8:33       ` Tiezhu Yang
2019-12-03  2:46     ` Matthew Wilcox
2019-12-04 13:06     ` Jean-Louis Biasini

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git