[PATCH 0/4] Fixes for exfat driver

[PATCH 0/4] Fixes for exfat driver

[Pali Rohár]

This patch series contains small fixes for exfat driver. It removes
conversion from UTF-16 to UTF-16 at two places where it is not needed
and fixes discard support.

Patches are also in my exfat branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=exfat

Pali Rohár (4):
  exfat: Simplify exfat_utf8_d_hash() for code points above U+FFFF
  exfat: Simplify exfat_utf8_d_cmp() for code points above U+FFFF
  exfat: Remove unused functions exfat_high_surrogate() and
    exfat_low_surrogate()
  exfat: Fix discard support

 fs/exfat/exfat_fs.h |  2 --
 fs/exfat/namei.c    | 19 ++++---------------
 fs/exfat/nls.c      | 13 -------------
 fs/exfat/super.c    |  5 +++--
 4 files changed, 7 insertions(+), 32 deletions(-)

-- 
2.20.1

[PATCH 1/4] exfat: Simplify exfat_utf8_d_hash() for code points above U+FFFF

[Pali Rohár]

Function partial_name_hash() takes long type value into which can be stored
one Unicode code point. Therefore conversion from UTF-32 to UTF-16 is not
needed.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/exfat/namei.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index a8681d91f569..e0ec4ff366f5 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -147,16 +147,10 @@ static int exfat_utf8_d_hash(const struct dentry *dentry, struct qstr *qstr)
 			return charlen;
 
 		/*
-		 * Convert to UTF-16: code points above U+FFFF are encoded as
-		 * surrogate pairs.
 		 * exfat_toupper() works only for code points up to the U+FFFF.
 		 */
-		if (u > 0xFFFF) {
-			hash = partial_name_hash(exfat_high_surrogate(u), hash);
-			hash = partial_name_hash(exfat_low_surrogate(u), hash);
-		} else {
-			hash = partial_name_hash(exfat_toupper(sb, u), hash);
-		}
+		hash = partial_name_hash(u <= 0xFFFF ? exfat_toupper(sb, u) : u,
+					 hash);
 	}
 
 	qstr->hash = end_name_hash(hash);
-- 
2.20.1

Re: [PATCH 1/4] exfat: Simplify exfat_utf8_d_hash() for code points above U+FFFF

[Al Viro]

On Tue, Mar 17, 2020 at 11:25:52PM +0100, Pali Rohár wrote:
> Function partial_name_hash() takes long type value into which can be stored
> one Unicode code point. Therefore conversion from UTF-32 to UTF-16 is not
> needed.

Hmm...  You might want to update the comment in stringhash.h...

Re: [PATCH 1/4] exfat: Simplify exfat_utf8_d_hash() for code points above U+FFFF

[Pali Rohár]

On Wednesday 18 March 2020 00:09:25 Al Viro wrote:
> On Tue, Mar 17, 2020 at 11:25:52PM +0100, Pali Rohár wrote:
> > Function partial_name_hash() takes long type value into which can be stored
> > one Unicode code point. Therefore conversion from UTF-32 to UTF-16 is not
> > needed.
> 
> Hmm...  You might want to update the comment in stringhash.h...

Well, initially I have not looked at hashing functions deeply. Used
hashing function in stringhash.h is defined as:

static inline unsigned long
partial_name_hash(unsigned long c, unsigned long prevhash)
{
	return (prevhash + (c << 4) + (c >> 4)) * 11;
}

I guess it was designed for 8bit types, not for long (64bit types) and
I'm not sure how effective it is even for 16bit types for which it is
already used.

So question is, what should we do for either 21bit number (one Unicode
code point = equivalent of UTF-32) or for sequence of 16bit numbers
(UTF-16)?

Any opinion?

Re: [PATCH 1/4] exfat: Simplify exfat_utf8_d_hash() for code points above U+FFFF

[Pali Rohár]

On Wednesday 18 March 2020 10:32:51 Pali Rohár wrote:
> On Wednesday 18 March 2020 00:09:25 Al Viro wrote:
> > On Tue, Mar 17, 2020 at 11:25:52PM +0100, Pali Rohár wrote:
> > > Function partial_name_hash() takes long type value into which can be stored
> > > one Unicode code point. Therefore conversion from UTF-32 to UTF-16 is not
> > > needed.
> > 
> > Hmm...  You might want to update the comment in stringhash.h...
> 
> Well, initially I have not looked at hashing functions deeply. Used
> hashing function in stringhash.h is defined as:
> 
> static inline unsigned long
> partial_name_hash(unsigned long c, unsigned long prevhash)
> {
> 	return (prevhash + (c << 4) + (c >> 4)) * 11;
> }
> 
> I guess it was designed for 8bit types, not for long (64bit types) and
> I'm not sure how effective it is even for 16bit types for which it is
> already used.
> 
> So question is, what should we do for either 21bit number (one Unicode
> code point = equivalent of UTF-32) or for sequence of 16bit numbers
> (UTF-16)?
> 
> Any opinion?

So what to do with that hashing function?

Anyway, "[PATCH 4/4] exfat: Fix discard support" should be reviewed as
currently discard support in exfat is broken.

[PATCH 2/4] exfat: Simplify exfat_utf8_d_cmp() for code points above U+FFFF

[Pali Rohár]

If two Unicode code points represented in UTF-16 are different then also
their UTF-32 representation must be different. Therefore conversion from
UTF-32 to UTF-16 is not needed.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/exfat/namei.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index e0ec4ff366f5..f07cab5fcd28 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -179,14 +179,9 @@ static int exfat_utf8_d_cmp(const struct dentry *dentry, unsigned int len,
 		if (u_a <= 0xFFFF && u_b <= 0xFFFF) {
 			if (exfat_toupper(sb, u_a) != exfat_toupper(sb, u_b))
 				return 1;
-		} else if (u_a > 0xFFFF && u_b > 0xFFFF) {
-			if (exfat_low_surrogate(u_a) !=
-					exfat_low_surrogate(u_b) ||
-			    exfat_high_surrogate(u_a) !=
-					exfat_high_surrogate(u_b))
-				return 1;
 		} else {
-			return 1;
+			if (u_a != u_b)
+				return 1;
 		}
 	}
 
-- 
2.20.1

[PATCH 3/4] exfat: Remove unused functions exfat_high_surrogate() and exfat_low_surrogate()

[Pali Rohár]

After applying previous two patches, these functions are not used anymore.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/exfat/exfat_fs.h |  2 --
 fs/exfat/nls.c      | 13 -------------
 2 files changed, 15 deletions(-)

diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index 67d4e46fb810..8a176a803206 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -492,8 +492,6 @@ int exfat_nls_to_utf16(struct super_block *sb,
 		struct exfat_uni_name *uniname, int *p_lossy);
 int exfat_create_upcase_table(struct super_block *sb);
 void exfat_free_upcase_table(struct exfat_sb_info *sbi);
-unsigned short exfat_high_surrogate(unicode_t u);
-unsigned short exfat_low_surrogate(unicode_t u);
 
 /* exfat/misc.c */
 void __exfat_fs_error(struct super_block *sb, int report, const char *fmt, ...)
diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c
index 6d1c3ae130ff..e3a9f5e08f68 100644
--- a/fs/exfat/nls.c
+++ b/fs/exfat/nls.c
@@ -537,22 +537,9 @@ static int exfat_utf8_to_utf16(struct super_block *sb,
 	return unilen;
 }
 
-#define PLANE_SIZE	0x00010000
 #define SURROGATE_MASK	0xfffff800
 #define SURROGATE_PAIR	0x0000d800
 #define SURROGATE_LOW	0x00000400
-#define SURROGATE_BITS	0x000003ff
-
-unsigned short exfat_high_surrogate(unicode_t u)
-{
-	return ((u - PLANE_SIZE) >> 10) + SURROGATE_PAIR;
-}
-
-unsigned short exfat_low_surrogate(unicode_t u)
-{
-	return ((u - PLANE_SIZE) & SURROGATE_BITS) | SURROGATE_PAIR |
-		SURROGATE_LOW;
-}
 
 static int __exfat_utf16_to_nls(struct super_block *sb,
 		struct exfat_uni_name *p_uniname, unsigned char *p_cstring,
-- 
2.20.1

[PATCH 4/4] exfat: Fix discard support

[Pali Rohár]

Discard support was always unconditionally disabled. Now it is disabled
only in the case when blk_queue_discard() returns false.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/exfat/super.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 16ed202ef527..30e914ad17b5 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -531,10 +531,11 @@ static int exfat_fill_super(struct super_block *sb, struct fs_context *fc)
 	if (opts->discard) {
 		struct request_queue *q = bdev_get_queue(sb->s_bdev);
 
-		if (!blk_queue_discard(q))
+		if (!blk_queue_discard(q)) {
 			exfat_msg(sb, KERN_WARNING,
 				"mounting with \"discard\" option, but the device does not support discard");
-		opts->discard = 0;
+			opts->discard = 0;
+		}
 	}
 
 	sb->s_flags |= SB_NODIRATIME;
-- 
2.20.1

RE: [PATCH 0/4] Fixes for exfat driver

[Namjae Jeon]

> This patch series contains small fixes for exfat driver. It removes
> conversion from UTF-16 to UTF-16 at two places where it is not needed and
> fixes discard support.
Looks good to me.
Acked-by: Namjae Jeon <namjae.jeon@samsung.com>

Hi Al,

Could you please push these patches into your #for-next ?
Thanks!

> 
> Patches are also in my exfat branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=exfa
> t
> 
> Pali Rohár (4):
>   exfat: Simplify exfat_utf8_d_hash() for code points above U+FFFF
>   exfat: Simplify exfat_utf8_d_cmp() for code points above U+FFFF
>   exfat: Remove unused functions exfat_high_surrogate() and
>     exfat_low_surrogate()
>   exfat: Fix discard support
> 
>  fs/exfat/exfat_fs.h |  2 --
>  fs/exfat/namei.c    | 19 ++++---------------
>  fs/exfat/nls.c      | 13 -------------
>  fs/exfat/super.c    |  5 +++--
>  4 files changed, 7 insertions(+), 32 deletions(-)
> 
> --
> 2.20.1

Re: [PATCH 0/4] Fixes for exfat driver

[Pali Rohár]

On Wednesday 18 March 2020 08:20:04 Namjae Jeon wrote:
> > This patch series contains small fixes for exfat driver. It removes
> > conversion from UTF-16 to UTF-16 at two places where it is not needed and
> > fixes discard support.
> Looks good to me.
> Acked-by: Namjae Jeon <namjae.jeon@samsung.com>
> 
> Hi Al,
> 
> Could you please push these patches into your #for-next ?
> Thanks!

Al, could you please take this patch series? Based on feedback current
hashing code is good enough. And we do not want to have broken discard
support in upcoming Linux kernel version.

> > 
> > Patches are also in my exfat branch:
> > https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=exfa
> > t
> > 
> > Pali Rohár (4):
> >   exfat: Simplify exfat_utf8_d_hash() for code points above U+FFFF
> >   exfat: Simplify exfat_utf8_d_cmp() for code points above U+FFFF
> >   exfat: Remove unused functions exfat_high_surrogate() and
> >     exfat_low_surrogate()
> >   exfat: Fix discard support
> > 
> >  fs/exfat/exfat_fs.h |  2 --
> >  fs/exfat/namei.c    | 19 ++++---------------
> >  fs/exfat/nls.c      | 13 -------------
> >  fs/exfat/super.c    |  5 +++--
> >  4 files changed, 7 insertions(+), 32 deletions(-)
> > 
> > --
> > 2.20.1
> 
> 
> 

RE: [PATCH 0/4] Fixes for exfat driver

[Namjae Jeon]

> On Wednesday 18 March 2020 08:20:04 Namjae Jeon wrote:
> > > This patch series contains small fixes for exfat driver. It removes
> > > conversion from UTF-16 to UTF-16 at two places where it is not
> > > needed and fixes discard support.
> > Looks good to me.
> > Acked-by: Namjae Jeon <namjae.jeon@samsung.com>
> >
> > Hi Al,
> >
> > Could you please push these patches into your #for-next ?
> > Thanks!
> 
> Al, could you please take this patch series? Based on feedback current
> hashing code is good enough. And we do not want to have broken discard
> support in upcoming Linux kernel version.
Hi Pali,

I will push them to exfat git tree.

Thanks for your work!
> 
> > >
> > > Patches are also in my exfat branch:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?
> > > h=exfa
> > > t
> > >
> > > Pali Rohár (4):
> > >   exfat: Simplify exfat_utf8_d_hash() for code points above U+FFFF
> > >   exfat: Simplify exfat_utf8_d_cmp() for code points above U+FFFF
> > >   exfat: Remove unused functions exfat_high_surrogate() and
> > >     exfat_low_surrogate()
> > >   exfat: Fix discard support
> > >
> > >  fs/exfat/exfat_fs.h |  2 --
> > >  fs/exfat/namei.c    | 19 ++++---------------
> > >  fs/exfat/nls.c      | 13 -------------
> > >  fs/exfat/super.c    |  5 +++--
> > >  4 files changed, 7 insertions(+), 32 deletions(-)
> > >
> > > --
> > > 2.20.1
> >
> >
> >