Re: [PATCH v10 00/14] add the latest exfat driver

Re: [PATCH v10 00/14] add the latest exfat driver

[Markus Elfring]

>  - Process utf16 surroage pair as one character.

Would you like to mention “surrogates” by this wording?

Regards,
Markus

Re: [PATCH v10 00/14] add the latest exfat driver

[Markus Elfring]

> We plan to treat this version as the future upstream for the code base
> once merged, and all new features and bug fixes will go upstream first.
>
> v10:

I find it interesting how the patch change log grew.


>  - Process length value as 1 when conversion is failed.

Would this description be nicer without the word “is”?


I pointed also further software development concerns out.
I hope that they will get corresponding constructive feedback finally.
Other contributors are similarly waiting for “effects” according to
their code review comments.
How will communication delays evolve for the clarification of
remaining open issues?

Regards,
Markus

[PATCH v10 00/14] add the latest exfat driver

[Namjae Jeon]

This adds the latest Samsung exfat driver to fs/exfat. This is an
implementation of the Microsoft exFAT specification. Previous versions
of this shipped with millions of Android phones, and a random previous
snaphot has been merged in drivers/staging/.

Compared to the sdfat driver shipped on the phones the following changes
have been made:

 - the support for vfat has been removed as that is already supported
   by fs/fat
 - driver has been renamed to exfat
 - the code has been refactored and clean up to fully integrate into
   the upstream Linux version and follow the Linux coding style
 - metadata operations like create, lookup and readdir have been further
   optimized
 - various major and minor bugs have been fixed

We plan to treat this version as the future upstream for the code base
once merged, and all new features and bug fixes will go upstream first.

v10:
 - Make PBR structures as packed structure.
 - Fix build error on 32 bit system.
 - Change L suffix of UNIX_SECS_2108 macro with LL suffix to work
   on both 32/64bit system.
 - Rework exfat time handling.
 - Don't warp exfat specification URLs.
 - Add _FS suffix to config name.
 - Remove case_sensitive mount option.
 - iocharset=utf8 mount option work as utf8 option.
 - Rename the misleading nls names to corresponding ones.
 - Fix wrong header guard name of exfat_fs.h.
 - Remove the unneeded braces of macros in exfat_fs.h.
 - Move the ondisk values to exfat_raw.h
 - Put the operators at the previous line in exfat_cluster_to_sector().
 - Braces of EXFAT_DELETE macro would outside the ~.
 - Directly use exfat dentry field name.
 - Add EXFAT_CLUSTERS_UNTRACKED macro.
 - Remove both sets of inner braces in exfat_set_vol_flags().
 - Replace is_reserved_cluster() with an explicit check
   for EXFAT_EOF_CLUSTER.
 - Initialize superblock s_time_gran/max/min.
 - Clean-up exfat_bmap and exfat_get_block().
 - Fix wrong boundlen to avoid potential buffer overflow
   in exfat_convert_char_to_ucs2().
 - Process length value as 1 when conversion is failed.
 - Replace union exfat_timezone with masking the valid bit.
 - Change exfat_cmp_uniname() with exfat_uniname_ncmp().
 - Remove struct exfat_timestamp.
 - Add atime update support.
 - Add time_offset mount option.
 - Remove unneeded CLUSTER_32 macro.
 - Process utf16 surroage pair as one character.
 - Rename MUST_ZERO_LEN to PBR64_RESERVED_LEN.
 - Simplify is_exfat function by just using memchr_inv().
 - Remove __exfat_init_name_hash.
 - Remove exfat_striptail_len.
 - Split dentry ops for the utf8 vs non-utf8 cases.

v9:
 - Add support time zone.
 - Fix data past EOF resulting from fsx testsuite.
 - Remove obsolete comments in __exfat_resolve_path().
 - Remove unused file attributes macros.
 - Remove unneeded #if BITS_PER_LONG.

v8:
 - Rearrange the function grouping in exfat_fs.h
   (exfat_count_dir_entries, exfat_get_dentry, exfat_get_dentry_set,
    exfat_find_location).
 - Mark exfat_extract_uni_name(), exfat_get_uniname_from_ext_entry() and
   exfat_mirror_bh() as static.

v7:
 - Add the helpers macros for bitmap and fat entry to improve readability.
 - Rename exfat_test_bitmap to exfat_find_free_bitmap.
 - Merge exfat_get_num_entries into exfat_calc_num_entries.
 - Add EXFAT_DATA_CLUSTERS and EXFAT_RESERVED_CLUSTERS macro.
 - Add the macros for EXFAT BIOS block(JUMP_BOOT_LEN, OEM_NAME_LEN,
   MUST_BE_ZERO_LEN).
 - Add the macros for EXFAT entry type (IS_EXFAT_CRITICAL_PRI,
   IS_EXFAT_BENIGN_PRI, IS_EXFAT_CRITICAL_SEC).
 - Add EXFAT_FILE_NAME_LEN macro.
 - Change the data type of is_dir with bool in __exfat_write_inode().
 - Change the data type of sync with bool in exfat_set_vol_flags().
 - Merge __exfat_set_vol_flags into exfat_set_vol_flags.
 - Fix wrong statfs->f_namelen.

v6:
 - Fix always false comparison due to limited range of allow_utime's data
   type.
 - Move bh into loop in exfat_find_dir_entry().
 - Move entry_uniname and unichar variables into
   an if "entry_type == TYPE_EXTEND" branch.

v5:
 - Remove a blank line between the message and the error code in
   exfat_load_upcase_table.
 - Move brelse to the end of the while loop and rename release_bh label
   to free_table in exfat_load_upcase_table.
 - Move an error code assignment after a failed function call.
 - Rename labels and directly return instead of goto.
 - Improve the exception handling in exfat_get_dentry_set().
 - Remove ->d_time leftover.
 - fix boolreturn.cocci warnings.

v4:
 - Declare ALLOC_FAT_CHAIN and ALLOC_NO_FAT_CHAIN macros.
 - Rename labels with proper name.
 - Remove blank lines.
 - Remove pointer check for bh.
 - Move ep into loop in exfat_load_bitmap().
 - Replace READ/WRITE_ONCE() with test_and_clear_bit() and set_bit().
 - Change exfat_allow_set_time return type with bool.

v3:
 - fix wrong sbi->s_dirt set.

v2:
 - Check the bitmap count up to the total clusters.
 - Rename goto labels in several places.
 - Change time mode type with enumeration.
 - Directly return error instead of goto at first error check.
 - Combine seq_printf calls into a single one.

Namjae Jeon (14):
  exfat: add in-memory and on-disk structures and headers
  exfat: add super block operations
  exfat: add inode operations
  exfat: add directory operations
  exfat: add file operations
  exfat: add fat entry operations
  exfat: add bitmap operations
  exfat: add exfat cache
  exfat: add misc operations
  exfat: add nls operations
  exfat: add Kconfig and Makefile
  exfat: add exfat in fs/Kconfig and fs/Makefile
  MAINTAINERS: add exfat filesystem
  staging: exfat: make staging/exfat and fs/exfat mutually exclusive

 MAINTAINERS                   |    7 +
 drivers/staging/exfat/Kconfig |    2 +-
 fs/Kconfig                    |    3 +-
 fs/Makefile                   |    1 +
 fs/exfat/Kconfig              |   21 +
 fs/exfat/Makefile             |    8 +
 fs/exfat/balloc.c             |  282 +++++++
 fs/exfat/cache.c              |  325 ++++++++
 fs/exfat/dir.c                | 1244 ++++++++++++++++++++++++++++
 fs/exfat/exfat_fs.h           |  520 ++++++++++++
 fs/exfat/exfat_raw.h          |  184 +++++
 fs/exfat/fatent.c             |  463 +++++++++++
 fs/exfat/file.c               |  355 ++++++++
 fs/exfat/inode.c              |  667 +++++++++++++++
 fs/exfat/misc.c               |  162 ++++
 fs/exfat/namei.c              | 1442 +++++++++++++++++++++++++++++++++
 fs/exfat/nls.c                |  834 +++++++++++++++++++
 fs/exfat/super.c              |  724 +++++++++++++++++
 18 files changed, 7242 insertions(+), 2 deletions(-)
 create mode 100644 fs/exfat/Kconfig
 create mode 100644 fs/exfat/Makefile
 create mode 100644 fs/exfat/balloc.c
 create mode 100644 fs/exfat/cache.c
 create mode 100644 fs/exfat/dir.c
 create mode 100644 fs/exfat/exfat_fs.h
 create mode 100644 fs/exfat/exfat_raw.h
 create mode 100644 fs/exfat/fatent.c
 create mode 100644 fs/exfat/file.c
 create mode 100644 fs/exfat/inode.c
 create mode 100644 fs/exfat/misc.c
 create mode 100644 fs/exfat/namei.c
 create mode 100644 fs/exfat/nls.c
 create mode 100644 fs/exfat/super.c

-- 
2.17.1

Re: [PATCH v10 00/14] add the latest exfat driver

[Pali Rohár]

Hello! I have reviewed all changes in time when v10 has been preparing.

There is just a small issue with description of EXFAT_DEFAULT_IOCHARSET
option (see email). Otherwise it looks good you can add my Reviewed-by
on whole patch series.

Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

Next steps for future:

* De-duplicate cache code between fat and exfat. Currently fs/exfat
  cache code is heavily copy-paste of fs/fat cache code.

* De-duplicate UTF-16 functions. Currently fs/exfat has e.g. helper
  functions for surrogate pairs copy-paste from fs/nls.

* Unify EXFAT_DEFAULT_IOCHARSET and FAT_DEFAULT_IOCHARSET. Or maybe
  unify it with other filesystems too.

* After applying this patch series, remote staging exfat implementation.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v10 00/14] add the latest exfat driver

[Namjae Jeon]

2020-01-15 18:47 GMT+09:00, Pali Rohár <pali.rohar@gmail.com>:
> Hello! I have reviewed all changes in time when v10 has been preparing.
>
> There is just a small issue with description of EXFAT_DEFAULT_IOCHARSET
> option (see email). Otherwise it looks good you can add my Reviewed-by
> on whole patch series.
>
> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
Thanks for your review and help.
I will add your reviewed-by tag on next version.
>
> Next steps for future:
>
> * De-duplicate cache code between fat and exfat. Currently fs/exfat
>   cache code is heavily copy-paste of fs/fat cache code.
>
> * De-duplicate UTF-16 functions. Currently fs/exfat has e.g. helper
>   functions for surrogate pairs copy-paste from fs/nls.
>
> * Unify EXFAT_DEFAULT_IOCHARSET and FAT_DEFAULT_IOCHARSET. Or maybe
>   unify it with other filesystems too.
>
> * After applying this patch series, remote staging exfat implementation.
Yep, I will check them.
>
> --
> Pali Rohár
> pali.rohar@gmail.com
>

Re: [PATCH v10 00/14] add the latest exfat driver

[Christoph Hellwig]

On Wed, Jan 15, 2020 at 10:47:32AM +0100, Pali Rohár wrote:
> Next steps for future:
> 
> * De-duplicate cache code between fat and exfat. Currently fs/exfat
>   cache code is heavily copy-paste of fs/fat cache code.

As said before I don't think this should be a merge blocker.  I actually
see this more of an experiment as the sharing might make things worse.
But at least it is worth giving it a try.

> * De-duplicate UTF-16 functions. Currently fs/exfat has e.g. helper
>   functions for surrogate pairs copy-paste from fs/nls.

If you looked into that can you post a list of suspected duplicates?

> 
> * Unify EXFAT_DEFAULT_IOCHARSET and FAT_DEFAULT_IOCHARSET. Or maybe
>   unify it with other filesystems too.

For the initial merge I think they should be kept separate, as
referencing other file systems Kconfig variable is confusing.
Investingating if we could a single common one sounds like a good idea,
though.

> * After applying this patch series, remote staging exfat implementation.

I think Greg wants to do that separately.  I still hope we can do that
in the same merge window, though.

Re: [PATCH v10 00/14] add the latest exfat driver

[Pali Rohár]

On Thursday 16 January 2020 11:51:08 Christoph Hellwig wrote:
> On Wed, Jan 15, 2020 at 10:47:32AM +0100, Pali Rohár wrote:
> > Next steps for future:

I mean all points to be next future steps after merging. Not something
for this patch series. Sorry for a confusion.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v10 00/14] add the latest exfat driver

[Greg KH]

On Thu, Jan 16, 2020 at 11:51:08AM +0100, Christoph Hellwig wrote:
> > * After applying this patch series, remote staging exfat implementation.
> 
> I think Greg wants to do that separately.  I still hope we can do that
> in the same merge window, though.

I will be glad to do it in the same merge window, just let me know when
this gets accepted and I'll drop the staging version.

thanks,

greg k-h