Re: [PATCH] staging: exfat: add exfat filesystem code to staging

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Greg Kroah-Hartman]

On Wed, Aug 28, 2019 at 06:08:17PM +0200, Greg Kroah-Hartman wrote:
> From: Valdis Klētnieks <valdis.kletnieks@vt.edu>
> 
> The exfat code needs a lot of work to get it into "real" shape for
> the fs/ part of the kernel, so put it into drivers/staging/ for now so
> that it can be worked on by everyone in the community.
> 
> The full specification of the filesystem can be found at:
>   https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification
> 
> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu>
> Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  MAINTAINERS                          |    5 +
>  drivers/staging/Kconfig              |    2 +
>  drivers/staging/Makefile             |    1 +
>  drivers/staging/exfat/Kconfig        |   39 +
>  drivers/staging/exfat/Makefile       |   10 +
>  drivers/staging/exfat/TODO           |   12 +
>  drivers/staging/exfat/exfat.h        |  973 ++++++
>  drivers/staging/exfat/exfat_blkdev.c |  136 +
>  drivers/staging/exfat/exfat_cache.c  |  722 +++++
>  drivers/staging/exfat/exfat_core.c   | 3704 +++++++++++++++++++++++
>  drivers/staging/exfat/exfat_nls.c    |  404 +++
>  drivers/staging/exfat/exfat_super.c  | 4137 ++++++++++++++++++++++++++
>  drivers/staging/exfat/exfat_upcase.c |  740 +++++
>  13 files changed, 10885 insertions(+)
>  create mode 100644 drivers/staging/exfat/Kconfig
>  create mode 100644 drivers/staging/exfat/Makefile
>  create mode 100644 drivers/staging/exfat/TODO
>  create mode 100644 drivers/staging/exfat/exfat.h
>  create mode 100644 drivers/staging/exfat/exfat_blkdev.c
>  create mode 100644 drivers/staging/exfat/exfat_cache.c
>  create mode 100644 drivers/staging/exfat/exfat_core.c
>  create mode 100644 drivers/staging/exfat/exfat_nls.c
>  create mode 100644 drivers/staging/exfat/exfat_super.c
>  create mode 100644 drivers/staging/exfat/exfat_upcase.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e3242687cd19..a484b36e5117 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6097,6 +6097,11 @@ F:	include/trace/events/mdio.h
>  F:	include/uapi/linux/mdio.h
>  F:	include/uapi/linux/mii.h
>  
> +EXFAT FILE SYSTEM
> +M:	Valdis Kletnieks <valdis.kletnieks@vt.edu>
> +S:	Maintained
> +F:	fs/exfat/

Oops, I messed this line up.  I moved this to drivers/staging/ and I
forgot to update this line.  I'll do it in a follow-on patch.

thanks,

greg k-h

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Christoph Hellwig]

Can we please just review the damn thing and get it into the proper
tree?  That whole concept of staging file systems just has been one
fricking disaster, including Greg just moving not fully reviewed ones
over like erofs just because he feels like it.  I'm getting sick and
tired of this scheme.

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Greg Kroah-Hartman]

On Wed, Aug 28, 2019 at 11:23:40PM -0700, Christoph Hellwig wrote:
> Can we please just review the damn thing and get it into the proper
> tree?  That whole concept of staging file systems just has been one
> fricking disaster, including Greg just moving not fully reviewed ones
> over like erofs just because he feels like it.  I'm getting sick and
> tired of this scheme.

For this filesystem, it's going to be a _lot_ of work before that can
happen, and I'd really like to have lots of people help out with it
instead of it living in random github trees for long periods of time.

Putting it in the kernel lets all of the people that have been spending
time on this work together in one place.

Given that the vfs apis change so infrequently, I don't really
understand the objection here.  If you change apis in the rest of the
kernel, don't worry about anything in drivers/staging/ I will fix that
up.  This code has a much smaller api-interaction-level than lots of the
drivers that are currently living in staging.

thanks,

greg k-h

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Gao Xiang]

Hi Christoph,

On Wed, Aug 28, 2019 at 11:23:40PM -0700, Christoph Hellwig wrote:
> Can we please just review the damn thing and get it into the proper
> tree?  That whole concept of staging file systems just has been one
> fricking disaster, including Greg just moving not fully reviewed ones
> over like erofs just because he feels like it.  I'm getting sick and
> tired of this scheme.

I just want to a word on EROFS stuff (I'm not suitable to comment
on the current exfat implementation). Since EROFS stuff has been
in staging tree for more than a year, anyone who wants to review
it can review this filesystem at any time.

EROFS is not just a homebrew or experimental fs for now, it has been
widely used for many commerical smartphones, and we will upstream it
to AOSP for more Android smartphones after it gets merged to upstream.
I personally cc-ed you for a month, and I didn't get any objection
from others (including Linus) for about 2 months. That isn't because
of someone likes it, rather we cannot make no progress compared with
some exist fs community because this is our love work.

And it's self-contained driver at least, and it's disabled by default,
It cannot be stayed in staging tree to do a lot of EROFS feature
improvements itself forever (since it is cleaned enough).
It has proven its validity as well.

Thanks,
Gao Xiang

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Gao Xiang]

Hi Christoph,

On Thu, Aug 29, 2019 at 03:01:49PM +0800, Gao Xiang wrote:
> Hi Christoph,
> 
> On Wed, Aug 28, 2019 at 11:23:40PM -0700, Christoph Hellwig wrote:
> > Can we please just review the damn thing and get it into the proper
> > tree?  That whole concept of staging file systems just has been one
> > fricking disaster, including Greg just moving not fully reviewed ones
> > over like erofs just because he feels like it.  I'm getting sick and
> > tired of this scheme.
> 
> I just want to a word on EROFS stuff (I'm not suitable to comment
> on the current exfat implementation). Since EROFS stuff has been
> in staging tree for more than a year, anyone who wants to review
> it can review this filesystem at any time.
> 
> EROFS is not just a homebrew or experimental fs for now, it has been
> widely used for many commerical smartphones, and we will upstream it
> to AOSP for more Android smartphones after it gets merged to upstream.
> I personally cc-ed you for a month, and I didn't get any objection
> from others (including Linus) for about 2 months. That isn't because
> of someone likes it, rather we cannot make no progress compared with
> some exist fs community because this is our love work.
> 
> And it's self-contained driver at least, and it's disabled by default,
> It cannot be stayed in staging tree to do a lot of EROFS feature
> improvements itself forever (since it is cleaned enough).
> It has proven its validity as well.

It seems I misunderstood your idea, sorry about that... EROFS
properly uses vfs interfaces (e.g. we also considered RCU symlink
lookup path at the very beginning of our design as Al said [1],
except for mount interface as Al mentioned [2] (thanks him for
taking some time on it), it was used for our debugging use),
and it didn't cause any extra burden to vfs or other subsystems.

[1] https://lore.kernel.org/r/20190325045744.GK2217@ZenIV.linux.org.uk/
[2] https://lore.kernel.org/r/20190720224955.GD17978@ZenIV.linux.org.uk/

Thanks,
Gao Xiang

> 
> Thanks,
> Gao Xiang
> 

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 08:39:55AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Aug 28, 2019 at 11:23:40PM -0700, Christoph Hellwig wrote:
> > Can we please just review the damn thing and get it into the proper
> > tree?  That whole concept of staging file systems just has been one
> > fricking disaster, including Greg just moving not fully reviewed ones
> > over like erofs just because he feels like it.  I'm getting sick and
> > tired of this scheme.
> 
> For this filesystem, it's going to be a _lot_ of work before that can
> happen, and I'd really like to have lots of people help out with it
> instead of it living in random github trees for long periods of time.

Did you actually look at the thing instead of blindly applying some
pile of crap?

It basically is a reimplementation of fs/fat/ not up to kernel standards
with a few indirections thrown in to also support exfat.  So no amount
of work on this codebase is really going to bring us forward.  Instead
someone how can spend a couple days on this and actually has file
systems to test it just needs to bring the low-level format bits over
to our well tested fs/fat codebase instead of duplicating it.

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Greg Kroah-Hartman]

On Thu, Aug 29, 2019 at 02:41:36AM -0700, Christoph Hellwig wrote:
> On Thu, Aug 29, 2019 at 08:39:55AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Aug 28, 2019 at 11:23:40PM -0700, Christoph Hellwig wrote:
> > > Can we please just review the damn thing and get it into the proper
> > > tree?  That whole concept of staging file systems just has been one
> > > fricking disaster, including Greg just moving not fully reviewed ones
> > > over like erofs just because he feels like it.  I'm getting sick and
> > > tired of this scheme.
> > 
> > For this filesystem, it's going to be a _lot_ of work before that can
> > happen, and I'd really like to have lots of people help out with it
> > instead of it living in random github trees for long periods of time.
> 
> Did you actually look at the thing instead of blindly applying some
> pile of crap?
> 
> It basically is a reimplementation of fs/fat/ not up to kernel standards
> with a few indirections thrown in to also support exfat.  So no amount
> of work on this codebase is really going to bring us forward.  Instead
> someone how can spend a couple days on this and actually has file
> systems to test it just needs to bring the low-level format bits over
> to our well tested fs/fat codebase instead of duplicating it.

I did try just that, a few years ago, and gave up on it.  I don't think
it can be added to the existing vfat code base but I am willing to be
proven wrong.

Now that we have the specs, it might be easier, and the vfat spec is a
subset of the exfat spec, but to get stuff working today, for users,
it's good to have it in staging.  We can do the normal, "keep it in
stable, get a clean-room implementation merged like usual, and then
delete the staging version" three step process like we have done a
number of times already as well.

I know the code is horrible, but I will gladly take horrible code into
staging.  If it bothers you, just please ignore it.  That's what staging
is there for :)

thanks,

greg k-h

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 04:24:09PM +0800, Gao Xiang wrote:
> It seems I misunderstood your idea, sorry about that... EROFS
> properly uses vfs interfaces (e.g. we also considered RCU symlink
> lookup path at the very beginning of our design as Al said [1],
> except for mount interface as Al mentioned [2] (thanks him for
> taking some time on it), it was used for our debugging use),
> and it didn't cause any extra burden to vfs or other subsystems.

It would still have been a lot less effort for everyone without
the idiotic staging detour.

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 11:50:19AM +0200, Greg Kroah-Hartman wrote:
> I did try just that, a few years ago, and gave up on it.  I don't think
> it can be added to the existing vfat code base but I am willing to be
> proven wrong.

And what exactly was the problem?

> 
> Now that we have the specs, it might be easier, and the vfat spec is a
> subset of the exfat spec, but to get stuff working today, for users,
> it's good to have it in staging.  We can do the normal, "keep it in
> stable, get a clean-room implementation merged like usual, and then
> delete the staging version" three step process like we have done a
> number of times already as well.
> 
> I know the code is horrible, but I will gladly take horrible code into
> staging.  If it bothers you, just please ignore it.  That's what staging
> is there for :)

And then after a while you decide it's been long enough and force move
it out of staging like the POS erofs code?

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Gao Xiang]

On Thu, Aug 29, 2019 at 03:37:49AM -0700, Christoph Hellwig wrote:
> On Thu, Aug 29, 2019 at 11:50:19AM +0200, Greg Kroah-Hartman wrote:
> > I did try just that, a few years ago, and gave up on it.  I don't think
> > it can be added to the existing vfat code base but I am willing to be
> > proven wrong.
> 
> And what exactly was the problem?
> 
> > 
> > Now that we have the specs, it might be easier, and the vfat spec is a
> > subset of the exfat spec, but to get stuff working today, for users,
> > it's good to have it in staging.  We can do the normal, "keep it in
> > stable, get a clean-room implementation merged like usual, and then
> > delete the staging version" three step process like we have done a
> > number of times already as well.
> > 
> > I know the code is horrible, but I will gladly take horrible code into
> > staging.  If it bothers you, just please ignore it.  That's what staging
> > is there for :)
> 
> And then after a while you decide it's been long enough and force move
> it out of staging like the POS erofs code?

The problem is that EROFS has been there for a year and
I sent v1-v8 patches here, You didn't review or reply it
once until now.

And I have no idea what is the relationship between EROFS
and the current exfat implementation.

Thanks,
Gao Xiang

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Greg Kroah-Hartman]

On Thu, Aug 29, 2019 at 03:37:49AM -0700, Christoph Hellwig wrote:
> On Thu, Aug 29, 2019 at 11:50:19AM +0200, Greg Kroah-Hartman wrote:
> > I did try just that, a few years ago, and gave up on it.  I don't think
> > it can be added to the existing vfat code base but I am willing to be
> > proven wrong.
> 
> And what exactly was the problem?

At the time, I just couldn't figure out how to do it as I had no spec,
and only a bad code-base to go off of.  I'm sure someone else might be
able to do to it :)

> > Now that we have the specs, it might be easier, and the vfat spec is a
> > subset of the exfat spec, but to get stuff working today, for users,
> > it's good to have it in staging.  We can do the normal, "keep it in
> > stable, get a clean-room implementation merged like usual, and then
> > delete the staging version" three step process like we have done a
> > number of times already as well.
> > 
> > I know the code is horrible, but I will gladly take horrible code into
> > staging.  If it bothers you, just please ignore it.  That's what staging
> > is there for :)
> 
> And then after a while you decide it's been long enough and force move
> it out of staging like the POS erofs code?

Hey, that's not nice, erofs isn't a POS.  It could always use more
review, which the developers asked for numerous times.

There's nothing different from a filesystem compared to a driver.  If
its stand-alone, and touches nothing else, all issues with it are
self-contained and do not bother anyone else in the kernel.  We merge
drivers all the time that need more work because our review cycles are
low.  And review cycles for vfs developers are even more scarce than
driver reviewers.

thanks,

greg k-h

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Greg Kroah-Hartman]

On Thu, Aug 29, 2019 at 07:04:43PM +0800, Gao Xiang wrote:
> On Thu, Aug 29, 2019 at 03:37:49AM -0700, Christoph Hellwig wrote:
> > On Thu, Aug 29, 2019 at 11:50:19AM +0200, Greg Kroah-Hartman wrote:
> > > I did try just that, a few years ago, and gave up on it.  I don't think
> > > it can be added to the existing vfat code base but I am willing to be
> > > proven wrong.
> > 
> > And what exactly was the problem?
> > 
> > > 
> > > Now that we have the specs, it might be easier, and the vfat spec is a
> > > subset of the exfat spec, but to get stuff working today, for users,
> > > it's good to have it in staging.  We can do the normal, "keep it in
> > > stable, get a clean-room implementation merged like usual, and then
> > > delete the staging version" three step process like we have done a
> > > number of times already as well.
> > > 
> > > I know the code is horrible, but I will gladly take horrible code into
> > > staging.  If it bothers you, just please ignore it.  That's what staging
> > > is there for :)
> > 
> > And then after a while you decide it's been long enough and force move
> > it out of staging like the POS erofs code?
> 
> The problem is that EROFS has been there for a year and
> I sent v1-v8 patches here, You didn't review or reply it
> once until now.
> 
> And I have no idea what is the relationship between EROFS
> and the current exfat implementation.

There isn't any, other than it too is a filesystem that was in the
staging directory :)

greg k-h

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

On Wednesday 28 August 2019 18:08:17 Greg Kroah-Hartman wrote:
> The full specification of the filesystem can be found at:
>   https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification

This is not truth. This specification is not "full". There are missing
important details, like how is TexFAT implemented. In that specification
is just reference to other unpublished documents. So it is classic MS
way, they release something incomplete and incompatible with their own
NT implementation, like with FAT32 vs fastfat.sys.

I would be very very careful about existence of such documentation until
somebody implement it and do some testing against MS own implementation.

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

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Valdis Klētnieks]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 2092 bytes --]

On Thu, 29 Aug 2019 14:14:35 +0200, Pali Rohár said:
> On Wednesday 28 August 2019 18:08:17 Greg Kroah-Hartman wrote:
> > The full specification of the filesystem can be found at:
> >   https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification
>
> This is not truth. This specification is not "full". There are missing
> important details, like how is TexFAT implemented. 

Well..given that the spec says it's an extension used by Windows CE...

> 1.5 Windows CE and TexFAT

> TexFAT is an extension to exFAT that adds transaction-safe operational
> semantics on top of the base file system. TexFAT is used by Windows CE. TexFAT
> requires the use of the two FATs and allocation bitmaps for use in
> transactions. It also defines several additional structures including padding
> descriptors and security descriptors.

And these two pieces of info:

> 3.1.13.1 ActiveFat Field

> The ActiveFat field shall describe which FAT and Allocation Bitmap are active
> (and implementations shall use), as follows:

> 0, which means the First FAT and First Allocation Bitmap are active

> 1, which means the Second FAT and Second Allocation Bitmap are active and is
> possible only when the NumberOfFats field contains the value 2

> Implementations shall consider the inactive FAT and Allocation Bitmap as stale.
> Only TexFAT-aware implementations shall switch the active FAT and Allocation
> Bitmaps (see Section 7.1).

> 3.1.16 NumberOfFats Field
> The NumberOfFats field shall describe the number of FATs and Allocation Bitmaps
> the volume contains.

> The valid range of values for this field shall be:

> 1, which indicates the volume only contains the First FAT and First Allocation Bitmap

> 2, which indicates the volume contains the First FAT, Second FAT, First
> Allocation Bitmap, and Second Allocation Bitmap; this value is only valid for
> TexFAT volumes

I think we're OK if we just set ActiveFat to 0 and NumberOfFats to 1.

Unless somebody has actual evidence of a non-WindowsCE extfat that has
NumberOfFats == 2....

[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

On Thursday 29 August 2019 08:34:04 Valdis Klētnieks wrote:
> On Thu, 29 Aug 2019 14:14:35 +0200, Pali Roh?r said:
> > On Wednesday 28 August 2019 18:08:17 Greg Kroah-Hartman wrote:
> > > The full specification of the filesystem can be found at:
> > >   https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification
> >
> > This is not truth. This specification is not "full". There are missing
> > important details, like how is TexFAT implemented. 
> 
> Well..given that the spec says it's an extension used by Windows CE...

It is extension which provides more error-prone write operations to
minimize damage on filesystem.

> > 1.5 Windows CE and TexFAT
> 
> > TexFAT is an extension to exFAT that adds transaction-safe operational
> > semantics on top of the base file system. TexFAT is used by Windows CE. TexFAT
> > requires the use of the two FATs and allocation bitmaps for use in
> > transactions. It also defines several additional structures including padding
> > descriptors and security descriptors.
> 
> And these two pieces of info:
> 
> > 3.1.13.1 ActiveFat Field
> 
> > The ActiveFat field shall describe which FAT and Allocation Bitmap are active
> > (and implementations shall use), as follows:
> 
> > 0, which means the First FAT and First Allocation Bitmap are active
> 
> > 1, which means the Second FAT and Second Allocation Bitmap are active and is
> > possible only when the NumberOfFats field contains the value 2
> 
> > Implementations shall consider the inactive FAT and Allocation Bitmap as stale.
> > Only TexFAT-aware implementations shall switch the active FAT and Allocation
> > Bitmaps (see Section 7.1).
> 
> > 3.1.16 NumberOfFats Field
> > The NumberOfFats field shall describe the number of FATs and Allocation Bitmaps
> > the volume contains.
> 
> > The valid range of values for this field shall be:
> 
> > 1, which indicates the volume only contains the First FAT and First Allocation Bitmap
> 
> > 2, which indicates the volume contains the First FAT, Second FAT, First
> > Allocation Bitmap, and Second Allocation Bitmap; this value is only valid for
> > TexFAT volumes
> 
> I think we're OK if we just set ActiveFat to 0 and NumberOfFats to 1.

But this degrades whole FS. Even FAT32 uses two FAT tables due to big
factor of brokenness and fsck takes care of it when repairing.

There is not too much sense to use exFAT with just one FAT if we have
already working FAT32 with redundancy of FAT table.

> Unless somebody has actual evidence of a non-WindowsCE extfat that has
> NumberOfFats == 2....

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

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Markus Elfring]

> +++ b/drivers/staging/exfat/exfat.h
> @@ -0,0 +1,973 @@
…
> +/* file types */
> +#define TYPE_UNUSED		0x0000
> +#define TYPE_DELETED		0x0001
…
> +/* time modes */
> +#define TM_CREATE		0
> +#define TM_MODIFY		1

Will it be helpful to work with enumerations at such places?

Regards,
Markus

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Dan Carpenter]

On Thu, Aug 29, 2019 at 01:18:10PM +0200, Greg Kroah-Hartman wrote:
> It could always use more review, which the developers asked for
> numerous times.

I stopped commenting on erofs style because all the likely/unlikely()
nonsense is a pet peeve.

regards,
dan carpenter

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Gao Xiang]

Hi Dan,

On Thu, Aug 29, 2019 at 06:11:44PM +0300, Dan Carpenter wrote:
> On Thu, Aug 29, 2019 at 01:18:10PM +0200, Greg Kroah-Hartman wrote:
> > It could always use more review, which the developers asked for
> > numerous times.
> 
> I stopped commenting on erofs style because all the likely/unlikely()
> nonsense is a pet peeve.

I don't know what is wrong for EROFS to use unlikely for all error
handling path (that is what IS_ERR implys).

If you like, I will delete them all.
p.s. There are 2947 (un)likely places in fs/ directory.

Thanks,
Gao Xiang

> 
> regards,
> dan carpenter
> 

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Dan Carpenter]

> p.s. There are 2947 (un)likely places in fs/ directory.

I was complaining about you adding new pointless ones, not existing
ones.  The likely/unlikely annotations are supposed to be functional and
not decorative.  I explained this very clearly.

Probably most of the annotations in fs/ are wrong but they are also
harmless except for the slight messiness.  However there are definitely
some which are important so removing them all isn't a good idea.

> If you like, I will delete them all.

But for erofs, I don't think that any of the likely/unlikely calls have
been thought about so I'm fine with removing all of them in one go.

regards,
dan carpenter

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Markus Elfring]

> +++ b/drivers/staging/exfat/exfat_core.c
> @@ -0,0 +1,3704 @@
…
> +static s32 __load_upcase_table(struct super_block *sb, sector_t sector,
> +			       u32 num_sectors, u32 utbl_checksum)
> +{
…
> +error:

An other label would be nicer, wouldn't it?


> +	if (tmp_bh)
> +		brelse(tmp_bh);

This inline function tolerates the passing of null pointers.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/buffer_head.h?id=9cf6b756cdf2cd38b8b0dac2567f7c6daf5e79d5#n292
https://elixir.bootlin.com/linux/v5.3-rc6/source/include/linux/buffer_head.h#L292

Thus I suggest to omit the extra pointer check at affected places.


> +++ b/drivers/staging/exfat/exfat_super.c
> @@ -0,0 +1,4137 @@
…
> +// FIXME use commented lines
> +// static int exfat_default_codepage = CONFIG_EXFAT_DEFAULT_CODEPAGE;

Is such information still relevant anyhow?


> +static int exfat_fill_super(struct super_block *sb, void *data, int silent)
> +{
…
> +out_fail:
> +	if (root_inode)
> +		iput(root_inode);

I am informed in the way that this function tolerates the passing
of null pointers.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/inode.c?id=9cf6b756cdf2cd38b8b0dac2567f7c6daf5e79d5#n1564
https://elixir.bootlin.com/linux/v5.3-rc6/source/fs/inode.c#L1564

Thus I suggest to omit the extra pointer check also at this place.

Regards,
Markus

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Gao Xiang]

Hi Dan,

On Thu, Aug 29, 2019 at 06:43:46PM +0300, Dan Carpenter wrote:
> > p.s. There are 2947 (un)likely places in fs/ directory.
> 
> I was complaining about you adding new pointless ones, not existing
> ones.  The likely/unlikely annotations are supposed to be functional and
> not decorative.  I explained this very clearly.

I don't think that is mostly pointless. I think it has functional use
because all error handling paths are rarely happened, or can you remove
the unlikely in IS_ERR as well?

> 
> Probably most of the annotations in fs/ are wrong but they are also
> harmless except for the slight messiness.  However there are definitely
> some which are important so removing them all isn't a good idea.
> 
> > If you like, I will delete them all.
> 
> But for erofs, I don't think that any of the likely/unlikely calls have
> been thought about so I'm fine with removing all of them in one go.

Maybe some misuse but rare, I will show you case by case. Wait a minute.

Thanks,
Gao Xiang

> 
> regards,
> dan carpenter
> 

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Gao Xiang]

Hi Dan,

On Thu, Aug 29, 2019 at 11:51:27PM +0800, Gao Xiang wrote:
> Hi Dan,
> 
> On Thu, Aug 29, 2019 at 06:43:46PM +0300, Dan Carpenter wrote:
> > > p.s. There are 2947 (un)likely places in fs/ directory.
> > 
> > I was complaining about you adding new pointless ones, not existing
> > ones.  The likely/unlikely annotations are supposed to be functional and
> > not decorative.  I explained this very clearly.
> 
> I don't think that is mostly pointless. I think it has functional use
> because all error handling paths are rarely happened, or can you remove
> the unlikely in IS_ERR as well?
> 
> > 
> > Probably most of the annotations in fs/ are wrong but they are also
> > harmless except for the slight messiness.  However there are definitely
> > some which are important so removing them all isn't a good idea.
> > 
> > > If you like, I will delete them all.
> > 
> > But for erofs, I don't think that any of the likely/unlikely calls have
> > been thought about so I'm fine with removing all of them in one go.
> 
> Maybe some misuse but rare, I will show you case by case. Wait a minute.

Anyway, I'm fine to delete them all if you like, but I think majority of these
are meaningful.

data.c-		/* page is already locked */
data.c-		DBG_BUGON(PageUptodate(page));
data.c-
data.c:		if (unlikely(err))
data.c-			SetPageError(page);
data.c-		else
data.c-			SetPageUptodate(page);
--
IO error

data.c-
data.c-repeat:
data.c-	page = find_or_create_page(mapping, blkaddr, gfp);
data.c:	if (unlikely(!page)) {
data.c-		DBG_BUGON(nofail);
data.c-		return ERR_PTR(-ENOMEM);
data.c-	}
--
NO MEM


data.c-		}
data.c-
data.c-		err = bio_add_page(bio, page, PAGE_SIZE, 0);
data.c:		if (unlikely(err != PAGE_SIZE)) {
data.c-			err = -EFAULT;
data.c-			goto err_out;

Internal error (since it is a single bio with one page).

data.c-		}
---

data.c-		lock_page(page);
data.c-
data.c-		/* this page has been truncated by others */
data.c:		if (unlikely(page->mapping != mapping)) {
data.c-unlock_repeat:
data.c-			unlock_page(page);
data.c-			put_page(page);
data.c-			goto repeat;
data.c-		}

truncated

data.c-
data.c:		/* more likely a read error */
data.c:		if (unlikely(!PageUptodate(page))) {
data.c-			if (io_retries) {
data.c-				--io_retries;
data.c-				goto unlock_repeat;

IO err

--
data.c-	nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
data.c-	lastblk = nblocks - is_inode_flat_inline(inode);
data.c-
data.c:	if (unlikely(offset >= inode->i_size)) {
data.c-		/* leave out-of-bound access unmapped */
data.c-		map->m_flags = 0;
data.c-		map->m_plen = 0;
--

FS corrupted

data.c-int erofs_map_blocks(struct inode *inode,
data.c-		     struct erofs_map_blocks *map, int flags)
data.c-{
data.c:	if (unlikely(is_inode_layout_compression(inode))) {

for compressed files, should call z_erofs_map_blocks_iter since it
behaves in iterative way. but I think that can be deleted.

data.c-		int err = z_erofs_map_blocks_iter(inode, map, flags);
data.c-
data.c-		if (map->mpage) {
--
data.c-		unsigned int blkoff;
data.c-
data.c-		err = erofs_map_blocks(inode, &map, EROFS_GET_BLOCKS_RAW);
data.c:		if (unlikely(err))
data.c-			goto err_out;

Error

data.c-
data.c-		/* zero out the holed page */
data.c:		if (unlikely(!(map.m_flags & EROFS_MAP_MAPPED))) {

no hole in erofs.

data.c-			zero_user_segment(page, 0, PAGE_SIZE);
data.c-			SetPageUptodate(page);
data.c-
--
data.c-submit_bio_out:
data.c-		__submit_bio(bio, REQ_OP_READ, 0);
data.c-
data.c:	return unlikely(err) ? ERR_PTR(err) : NULL;

err

data.c-}
data.c-
data.c-/*
--
data.c-	DBG_BUGON(!list_empty(pages));
data.c-
data.c-	/* the rare case (end in gaps) */
data.c:	if (unlikely(bio))
data.c-		__submit_bio(bio, REQ_OP_READ, 0);
data.c-	return 0;
data.c-}


decompressor.c-			get_page(victim);
decompressor.c-		} else {
decompressor.c-			victim = erofs_allocpage(pagepool, GFP_KERNEL, false);
decompressor.c:			if (unlikely(!victim))
decompressor.c-				return -ENOMEM;

nomem

decompressor.c-			victim->mapping = Z_EROFS_MAPPING_STAGING;
decompressor.c-		}


dir.c-			de_namelen = le16_to_cpu(de[1].nameoff) - nameoff;
dir.c-
dir.c-		/* a corrupted entry is found */
dir.c:		if (unlikely(nameoff + de_namelen > maxsize ||
dir.c-			     de_namelen > EROFS_NAME_LEN)) {

corrupted

dir.c-			errln("bogus dirent @ nid %llu", EROFS_V(dir)->nid);
dir.c-			DBG_BUGON(1);
--
dir.c-
dir.c-		nameoff = le16_to_cpu(de->nameoff);
dir.c-
dir.c:		if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
dir.c-			     nameoff >= PAGE_SIZE)) {

corrupted

dir.c-			errln("%s, invalid de[0].nameoff %u @ nid %llu",
dir.c-			      __func__, nameoff, EROFS_V(dir)->nid);
--
dir.c-				dirsize - ctx->pos + ofs, PAGE_SIZE);
dir.c-
dir.c-		/* search dirents at the arbitrary position */
dir.c:		if (unlikely(initial)) {

as comments said

dir.c-			initial = false;
dir.c-
dir.c-			ofs = roundup(ofs, sizeof(struct erofs_dirent));
dir.c:			if (unlikely(ofs >= nameoff))
dir.c-				goto skip_this;
dir.c-		}
dir.c-
--
dir.c-
dir.c-		ctx->pos = blknr_to_addr(i) + ofs;
dir.c-
dir.c:		if (unlikely(err))

err

dir.c-			break;
dir.c-		++i;
dir.c-		ofs = 0;

inode.c-
inode.c-	vi->datamode = __inode_data_mapping(advise);
inode.c-
inode.c:	if (unlikely(vi->datamode >= EROFS_INODE_LAYOUT_MAX)) {
inode.c-		errln("unsupported data mapping %u of nid %llu",
inode.c-		      vi->datamode, vi->nid);
inode.c-		DBG_BUGON(1);

err

--
inode.c-	if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) {
inode.c-		char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL);
inode.c-
inode.c:		if (unlikely(!lnk))
inode.c-			return -ENOMEM;
inode.c-
inode.c-		m_pofs += vi->inode_isize + vi->xattr_isize;
inode.c-
inode.c-		/* inline symlink data shouldn't across page boundary as well */
inode.c:		if (unlikely(m_pofs + inode->i_size > PAGE_SIZE)) {
inode.c-			kfree(lnk);
inode.c-			errln("inline data cross block boundary @ nid %llu",
inode.c-			      vi->nid);

err

--
inode.c-{
inode.c-	struct inode *inode = erofs_iget_locked(sb, nid);
inode.c-
inode.c:	if (unlikely(!inode))
inode.c-		return ERR_PTR(-ENOMEM);

err

inode.c-
inode.c-	if (inode->i_state & I_NEW) {
--
inode.c-		vi->nid = nid;
inode.c-
inode.c-		err = fill_inode(inode, isdir);
inode.c:		if (likely(!err))
inode.c-			unlock_new_inode(inode);
inode.c-		else {

err

inode.c-			iget_failed(inode);

I will stop here.

Thanks,
Gao Xiang

> 
> Thanks,
> Gao Xiang
> 
> > 
> > regards,
> > dan carpenter
> > 

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Gao Xiang]

Hi Dan,

On Thu, Aug 29, 2019 at 11:43:46PM +0800, Dan Carpenter wrote:
> > p.s. There are 2947 (un)likely places in fs/ directory.
> 
> I was complaining about you adding new pointless ones, not existing
> ones.  The likely/unlikely annotations are supposed to be functional and
> not decorative.  I explained this very clearly.
> 
> Probably most of the annotations in fs/ are wrong but they are also
> harmless except for the slight messiness.  However there are definitely
> some which are important so removing them all isn't a good idea.
> 
> > If you like, I will delete them all.
> 
> But for erofs, I don't think that any of the likely/unlikely calls have
> been thought about so I'm fine with removing all of them in one go.

Anyway, I have removed them all in
https://lore.kernel.org/r/20190829163827.203274-1-gaoxiang25@huawei.com/

Does it look good to you?

Thanks,
Gao Xiang

> 
> regards,
> dan carpenter
> 

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Joe Perches]

On Fri, 2019-08-30 at 00:44 +0800, Gao Xiang wrote:
> Hi Dan,
> 
> On Thu, Aug 29, 2019 at 11:43:46PM +0800, Dan Carpenter wrote:
> > > p.s. There are 2947 (un)likely places in fs/ directory.
> > 
> > I was complaining about you adding new pointless ones, not existing
> > ones.  The likely/unlikely annotations are supposed to be functional and
> > not decorative.  I explained this very clearly.
> > 
> > Probably most of the annotations in fs/ are wrong but they are also
> > harmless except for the slight messiness.  However there are definitely
> > some which are important so removing them all isn't a good idea.
> > 
> > > If you like, I will delete them all.
> > 
> > But for erofs, I don't think that any of the likely/unlikely calls have
> > been thought about so I'm fine with removing all of them in one go.
> 
> Anyway, I have removed them all in
> https://lore.kernel.org/r/20190829163827.203274-1-gaoxiang25@huawei.com/
> 
> Does it look good to you?

Unrelated bikeshed from a trivial look:

There's a block there that looks like:

diff --git a/fs/erofs/data.c b/fs/erofs/data.c
[]
@@ -70,7 +70,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
 		}
 
 		err = bio_add_page(bio, page, PAGE_SIZE, 0);
-		if (unlikely(err != PAGE_SIZE)) {
+		if (err != PAGE_SIZE) {
 			err = -EFAULT;
 			goto err_out;
 		}

The initial assignment to err is odd as it's not
actually an error value -E<FOO> but a int size
from a unsigned int len.

Here the return is either 0 or PAGE_SIZE.

This would be more legible to me as:

		if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
			err = -EFAULT;
			goto err_out;
		}

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Gao Xiang]

Hi Joe,

On Thu, Aug 29, 2019 at 09:59:21AM -0700, Joe Perches wrote:
> On Fri, 2019-08-30 at 00:44 +0800, Gao Xiang wrote:
> > Hi Dan,
> > 
> > On Thu, Aug 29, 2019 at 11:43:46PM +0800, Dan Carpenter wrote:
> > > > p.s. There are 2947 (un)likely places in fs/ directory.
> > > 
> > > I was complaining about you adding new pointless ones, not existing
> > > ones.  The likely/unlikely annotations are supposed to be functional and
> > > not decorative.  I explained this very clearly.
> > > 
> > > Probably most of the annotations in fs/ are wrong but they are also
> > > harmless except for the slight messiness.  However there are definitely
> > > some which are important so removing them all isn't a good idea.
> > > 
> > > > If you like, I will delete them all.
> > > 
> > > But for erofs, I don't think that any of the likely/unlikely calls have
> > > been thought about so I'm fine with removing all of them in one go.
> > 
> > Anyway, I have removed them all in
> > https://lore.kernel.org/r/20190829163827.203274-1-gaoxiang25@huawei.com/
> > 
> > Does it look good to you?
> 
> Unrelated bikeshed from a trivial look:
> 
> There's a block there that looks like:
> 
> diff --git a/fs/erofs/data.c b/fs/erofs/data.c
> []
> @@ -70,7 +70,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>  		}
>  
>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
> -		if (unlikely(err != PAGE_SIZE)) {
> +		if (err != PAGE_SIZE) {
>  			err = -EFAULT;
>  			goto err_out;
>  		}
> 
> The initial assignment to err is odd as it's not
> actually an error value -E<FOO> but a int size
> from a unsigned int len.
> 
> Here the return is either 0 or PAGE_SIZE.
> 
> This would be more legible to me as:
> 
> 		if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
> 			err = -EFAULT;
> 			goto err_out;
> 		}

Okay, that is more reasonable, I will update the original patch as you suggested.

Thanks,
Gao Xiang

> 
> 

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

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

On Wednesday 28 August 2019 18:08:17 Greg Kroah-Hartman wrote:
> From: Valdis Klētnieks <valdis.kletnieks@vt.edu>
> 
> The exfat code needs a lot of work to get it into "real" shape for
> the fs/ part of the kernel, so put it into drivers/staging/ for now so
> that it can be worked on by everyone in the community.
> 
> The full specification of the filesystem can be found at:
>   https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification
> 
> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu>
> Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Hi Greg!

I'm not really sure if this exfat implementation is fully suitable for
mainline linux kernel.

In my opinion, proper way should be to implement exFAT support into
existing fs/fat/ code instead of replacing whole vfat/msdosfs by this
new (now staging) fat implementation.

In linux kernel we really do not need two different implementation of
VFAT32.

So I'm a bit sceptical about usefulness of this exfat code/driver, if it
makes any sense to have it even in staging.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Valdis Klētnieks]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 698 bytes --]

On Thu, 29 Aug 2019 22:56:31 +0200, Pali Rohár said:

> I'm not really sure if this exfat implementation is fully suitable for
> mainline linux kernel.
>
> In my opinion, proper way should be to implement exFAT support into
> existing fs/fat/ code instead of replacing whole vfat/msdosfs by this
> new (now staging) fat implementation.

> In linux kernel we really do not need two different implementation of
> VFAT32.

This patch however does have one major advantage over "patch vfat to
support exfat" - which is that the patch exists.

If somebody comes forward with an actual "extend vfat to do exfat" patch,
we should at that point have a discussion about relative merits....

[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Sasha Levin]

On Thu, Aug 29, 2019 at 07:18:16PM -0400, Valdis Klētnieks wrote:
>On Thu, 29 Aug 2019 22:56:31 +0200, Pali Roh?r said:
>
>> I'm not really sure if this exfat implementation is fully suitable for
>> mainline linux kernel.
>>
>> In my opinion, proper way should be to implement exFAT support into
>> existing fs/fat/ code instead of replacing whole vfat/msdosfs by this
>> new (now staging) fat implementation.
>
>> In linux kernel we really do not need two different implementation of
>> VFAT32.
>
>This patch however does have one major advantage over "patch vfat to
>support exfat" - which is that the patch exists.
>
>If somebody comes forward with an actual "extend vfat to do exfat" patch,
>we should at that point have a discussion about relative merits....

This patch going into staging doesn't necessarily mean that one day
it'll get moved to fs/exfat/. It's very possible that the approach would
instead be to use the staging code for reference, build this
functionality in fs/fat/, and kill off the staging code when it's not
needed anymore.

With regards to missing specs/docs/whatever - our main concern with this
release was that we want full interoperability, which is why the spec
was made public as-is without modifications from what was used
internally. There's no "secret sauce" that Microsoft is hiding here.

How about we give this spec/code time to get soaked and reviewed for a
bit, and if folks still feel (in a month or so?) that there are missing
bits of information related to exfat, I'll be happy to go back and try
to get them out as well.

--
Thanks,
Sasha

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Chao Yu]

On 2019/8/29 23:43, Dan Carpenter wrote:
>> p.s. There are 2947 (un)likely places in fs/ directory.
> 
> I was complaining about you adding new pointless ones, not existing
> ones.  The likely/unlikely annotations are supposed to be functional and
> not decorative.  I explained this very clearly.
> 
> Probably most of the annotations in fs/ are wrong but they are also
> harmless except for the slight messiness.  However there are definitely
> some which are important so removing them all isn't a good idea.

Hi Dan,

Could you please pick up one positive example using likely and unlikely
correctly? so we can follow the example, rather than removing them all blindly.

Thanks,

> 
>> If you like, I will delete them all.
> 
> But for erofs, I don't think that any of the likely/unlikely calls have
> been thought about so I'm fine with removing all of them in one go.
> 
> regards,
> dan carpenter
> 
> .
> 

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Gao Xiang]

Hi Dan,

On Fri, Aug 30, 2019 at 10:06:25AM +0800, Chao Yu wrote:
> On 2019/8/29 23:43, Dan Carpenter wrote:
> >> p.s. There are 2947 (un)likely places in fs/ directory.
> > 
> > I was complaining about you adding new pointless ones, not existing
> > ones.  The likely/unlikely annotations are supposed to be functional and
> > not decorative.  I explained this very clearly.
> > 
> > Probably most of the annotations in fs/ are wrong but they are also
> > harmless except for the slight messiness.  However there are definitely
> > some which are important so removing them all isn't a good idea.
> 
> Hi Dan,
> 
> Could you please pick up one positive example using likely and unlikely
> correctly? so we can follow the example, rather than removing them all blindly.

I'm also curious about that, what is the filesystem or kernel standard about
likely/unlikely use (since I didn't find some documented standard so I used
in my personal way, I think it is reasonable at least to cover all error
handling paths), maybe I'm an _idiot_ as some earlier unfriendly word said
somewhere so I'm too stupid to understand the implicit meaning of some document.

> 
> Thanks,
> 
> > 
> >> If you like, I will delete them all.
> > 
> > But for erofs, I don't think that any of the likely/unlikely calls have
> > been thought about so I'm fine with removing all of them in one go.

Add a word (just a note), I don't think such kind of "any", "few", "all"
words are meaningful without some explicit evidence. (e.g. such as EROFS
have few error handling path. I don't think that is true and worth to
give many details since EROFS code is here for more than one year.)

Yes, EROFS is not prefectly, I have to admit, and I said similar words
on other threads for many times if you decide to check each likely/unlikely
line by line, I cannot say all unlikely/likely cases I wrote are reasonable
(just as bug-free, I think no one can make such guarantee even for new code),
but I can say the majority of them are reasonable in my personal understanding
of likely/unlikely. And I can fix all your reports in time (but maybe some
are not urgent at all.)

In addition there will be endless discussions on detailed code since there are
many personal tendencies from various people in it, as the saying goes "There
are a thousand Hamlets in a thousand people's eyes. "

Anyway, I have sent a patch to kill them all blindly as you like, so I think
we can come to an agreement on it, but I still don't fully agree with your
"for EROFS, I don't think that any of the likely/unlikely calls have been
thought about" conclusion.

Thanks,
Gao Xiang

> > 
> > regards,
> > dan carpenter
> > 
> > .
> > 

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

Hello, thank you for input!

On Thursday 29 August 2019 19:35:06 Sasha Levin wrote:
> On Thu, Aug 29, 2019 at 07:18:16PM -0400, Valdis Klētnieks wrote:
> > On Thu, 29 Aug 2019 22:56:31 +0200, Pali Roh?r said:
> > 
> > > I'm not really sure if this exfat implementation is fully suitable for
> > > mainline linux kernel.
> > > 
> > > In my opinion, proper way should be to implement exFAT support into
> > > existing fs/fat/ code instead of replacing whole vfat/msdosfs by this
> > > new (now staging) fat implementation.
> > 
> > > In linux kernel we really do not need two different implementation of
> > > VFAT32.
> > 
> > This patch however does have one major advantage over "patch vfat to
> > support exfat" - which is that the patch exists.
> > 
> > If somebody comes forward with an actual "extend vfat to do exfat" patch,
> > we should at that point have a discussion about relative merits....
> 
> This patch going into staging doesn't necessarily mean that one day
> it'll get moved to fs/exfat/. It's very possible that the approach would
> instead be to use the staging code for reference, build this
> functionality in fs/fat/, and kill off the staging code when it's not
> needed anymore.

So, if current exfat code is just going to be "reference" how to
implement exfat properly in fs/fat, is this correct usage of "staging"?

I was in impression that staging is there for drivers which needs
cleanup as they are not ready to be part of mainline. But not that it is
for "random" implementation of something which is going to be just a
"code example" or "reference" for final implementation which would be
different.

Maybe other people could clarify state of staging, if this is how
staging should be used or not.

> With regards to missing specs/docs/whatever - our main concern with this
> release was that we want full interoperability, which is why the spec
> was made public as-is without modifications from what was used
> internally. There's no "secret sauce" that Microsoft is hiding here.

Ok, if it was just drop of "current version" of documentation then it
makes sense.

> How about we give this spec/code time to get soaked and reviewed for a
> bit, and if folks still feel (in a month or so?) that there are missing
> bits of information related to exfat, I'll be happy to go back and try
> to get them out as well.

Basically external references in that released exFAT specification are
unknown / not released yet. Like TexFAT. So if you have an input in MS
could you forward request about these missing bits?

Also, MS already released source code of FAT32 kernel driver
(fastfat.sys) and from code can be seen that it does not match MS FAT
specification, and include some "secret sauce" bits. Source code is on:
https://github.com/microsoft/Windows-driver-samples/tree/master/filesys/fastfat

In fastfat.sys there is e.g. usage of undocumented bits in directory
entry or usage of undocumented bits for marking filesystem as "dirty" --
incorrectly unmounted.

Would it be possible for MS to release in same way code of exFAT NT
driver? I'm really a bit sceptical that exFAT MS implementation is
really according to standard as I already known that FAT32 is not.

Just to note that I did some investigation in FAT32 level how are
handled volume labels and I found more bugs in current implementations
(mtools, dosfstools, util-linux and even in fastfat.sys). Some
references are on: https://www.spinics.net/lists/kernel/msg2640891.html

Fixes for mtools, dosfstools and util-linux were already applied and fix
for MS's fastfat.sys is pending in opened pull request:
https://github.com/microsoft/Windows-driver-samples/pull/303

Could you please forward my pull request for fastfat.sys fixes to
appropriate people/team in MS?

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

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

On Thursday 29 August 2019 19:18:16 Valdis Klētnieks wrote:
> On Thu, 29 Aug 2019 22:56:31 +0200, Pali Roh?r said:
> 
> > I'm not really sure if this exfat implementation is fully suitable for
> > mainline linux kernel.
> >
> > In my opinion, proper way should be to implement exFAT support into
> > existing fs/fat/ code instead of replacing whole vfat/msdosfs by this
> > new (now staging) fat implementation.
> 
> > In linux kernel we really do not need two different implementation of
> > VFAT32.
> 
> This patch however does have one major advantage over "patch vfat to
> support exfat" - which is that the patch exists.

I understand that this is advantage...

> If somebody comes forward with an actual "extend vfat to do exfat" patch,
> we should at that point have a discussion about relative merits....

... but is this advantage such big that it should be merged even due to
"horrible" code quality and lot of code/functionality duplication?
In similar way there should be discussion about these pros and cons.

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

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Dan Carpenter]

On Fri, Aug 30, 2019 at 12:04:41AM +0800, Gao Xiang wrote:
> Anyway, I'm fine to delete them all if you like, but I think majority of these
> are meaningful.
> 
> data.c-		/* page is already locked */
> data.c-		DBG_BUGON(PageUptodate(page));
> data.c-
> data.c:		if (unlikely(err))
> data.c-			SetPageError(page);
> data.c-		else
> data.c-			SetPageUptodate(page);

If we cared about speed here then we would delete the DBG_BUGON() check
because that's going to be expensive.  The likely/unlikely annotations
should be used in places a reasonable person thinks it will make a
difference to benchmarks.

regards,
dan carpenter

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Gao Xiang]

Hi Dan,

On Fri, Aug 30, 2019 at 11:34:45AM +0300, Dan Carpenter wrote:
> On Fri, Aug 30, 2019 at 12:04:41AM +0800, Gao Xiang wrote:
> > Anyway, I'm fine to delete them all if you like, but I think majority of these
> > are meaningful.
> > 
> > data.c-		/* page is already locked */
> > data.c-		DBG_BUGON(PageUptodate(page));
> > data.c-
> > data.c:		if (unlikely(err))
> > data.c-			SetPageError(page);
> > data.c-		else
> > data.c-			SetPageUptodate(page);
> 
> If we cared about speed here then we would delete the DBG_BUGON() check
> because that's going to be expensive.  The likely/unlikely annotations
> should be used in places a reasonable person thinks it will make a
> difference to benchmarks.

DBG_BUGON will be a no-op ((void)x) in non-debugging mode,
I discussed related stuffs with Greg many months before [1] and
other filesystems also have similar functions...

p.s. I think we come to an agreement here...
I killed all unlikely/likely.

[1] http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128951.html
sorry about no lore here.

Thanks,
Gao Xiang

> 
> regards,
> dan carpenter
> 

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Dan Carpenter]

On Fri, Aug 30, 2019 at 04:43:33PM +0800, Gao Xiang wrote:
> Hi Dan,
> 
> On Fri, Aug 30, 2019 at 11:34:45AM +0300, Dan Carpenter wrote:
> > On Fri, Aug 30, 2019 at 12:04:41AM +0800, Gao Xiang wrote:
> > > Anyway, I'm fine to delete them all if you like, but I think majority of these
> > > are meaningful.
> > > 
> > > data.c-		/* page is already locked */
> > > data.c-		DBG_BUGON(PageUptodate(page));
> > > data.c-
> > > data.c:		if (unlikely(err))
> > > data.c-			SetPageError(page);
> > > data.c-		else
> > > data.c-			SetPageUptodate(page);
> > 
> > If we cared about speed here then we would delete the DBG_BUGON() check
> > because that's going to be expensive.  The likely/unlikely annotations
> > should be used in places a reasonable person thinks it will make a
> > difference to benchmarks.
> 
> DBG_BUGON will be a no-op ((void)x) in non-debugging mode,

It expands to:

	((void)PageUptodate(page));

Calling PageUptodate() doesn't do anything, but it isn't free.  The
time it takes to do that function call completely negates any speed up
from using likely/unlikely.

I'm really not trying to be a jerk...

regards,
dan carpenter

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[David Sterba]

On Fri, Aug 30, 2019 at 10:06:25AM +0800, Chao Yu wrote:
> On 2019/8/29 23:43, Dan Carpenter wrote:
> >> p.s. There are 2947 (un)likely places in fs/ directory.
> > 
> > I was complaining about you adding new pointless ones, not existing
> > ones.  The likely/unlikely annotations are supposed to be functional and
> > not decorative.  I explained this very clearly.
> > 
> > Probably most of the annotations in fs/ are wrong but they are also
> > harmless except for the slight messiness.  However there are definitely
> > some which are important so removing them all isn't a good idea.
> 
> Hi Dan,
> 
> Could you please pick up one positive example using likely and unlikely
> correctly? so we can follow the example, rather than removing them all blindly.

Remove all of them and re-add with explanation if and how each is going
to make things better. If you can't reason about, prove by benchmarks or
point to inefficient asm code generated, then don't add them again.

The feedback I got from CPU and compiler people over the years is not to
bother using the annotations. CPUs are doing dynamic branch prediction
and compilers are applying tons of optimizations.

GCC docs state about the builtin: "In general, you should prefer to use
actual profile feedback for this (-fprofile-arcs), as programmers are
notoriously bad at predicting how their programs actually perform."
(https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html)

Re: Checking usage of likeliness annotations

[Markus Elfring]

> I'm also curious about that, what is the filesystem or kernel standard about
> likely/unlikely use (since I didn't find some documented standard
> so I used in my personal way,

Such information is helpful.


> I think it is reasonable at least to cover all error handling paths),

I hope so, too.

I imagine that the likeliness annotation usage could depend also on
software build parameters.
Would you occasionally like to influence corresponding probabilities
any more by system settings?


> maybe I'm an _idiot_

I hope not.


> as some earlier unfriendly word said somewhere
> so I'm too stupid to understand the implicit meaning of some document.

Can the software development discussion be continued in more constructive ways?

Regards,
Markus

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Gao Xiang]

Hi Dan,

On Fri, Aug 30, 2019 at 02:26:12PM +0300, Dan Carpenter wrote:
> On Fri, Aug 30, 2019 at 04:43:33PM +0800, Gao Xiang wrote:
> > Hi Dan,
> > 
> > On Fri, Aug 30, 2019 at 11:34:45AM +0300, Dan Carpenter wrote:
> > > On Fri, Aug 30, 2019 at 12:04:41AM +0800, Gao Xiang wrote:
> > > > Anyway, I'm fine to delete them all if you like, but I think majority of these
> > > > are meaningful.
> > > > 
> > > > data.c-		/* page is already locked */
> > > > data.c-		DBG_BUGON(PageUptodate(page));
> > > > data.c-
> > > > data.c:		if (unlikely(err))
> > > > data.c-			SetPageError(page);
> > > > data.c-		else
> > > > data.c-			SetPageUptodate(page);
> > > 
> > > If we cared about speed here then we would delete the DBG_BUGON() check
> > > because that's going to be expensive.  The likely/unlikely annotations
> > > should be used in places a reasonable person thinks it will make a
> > > difference to benchmarks.
> > 
> > DBG_BUGON will be a no-op ((void)x) in non-debugging mode,
> 
> It expands to:
> 
> 	((void)PageUptodate(page));
> 
> Calling PageUptodate() doesn't do anything, but it isn't free.  The
> time it takes to do that function call completely negates any speed up
> from using likely/unlikely.
> 
> I'm really not trying to be a jerk...

You are right, I recalled that PageUptodate is not as simple as it implys.
Yes, those are all removed now... I am ok with that,
thanks for your suggestion :)

Thanks,
Gao Xiang

> 
> regards,
> dan carpenter
>
 

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 01:18:10PM +0200, Greg Kroah-Hartman wrote:
> Hey, that's not nice, erofs isn't a POS.  It could always use more
> review, which the developers asked for numerous times.
> 
> There's nothing different from a filesystem compared to a driver.  If
> its stand-alone, and touches nothing else, all issues with it are
> self-contained and do not bother anyone else in the kernel.  We merge
> drivers all the time that need more work because our review cycles are
> low.  And review cycles for vfs developers are even more scarce than
> driver reviewers.

A lot of the issue that are trivial to pick are really just very basic
issue that don't even require file system know how.  Or in other ways
just a little less lazy developer that looks out for similar code
outside their own little fiefdom.

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 10:56:31PM +0200, Pali Rohár wrote:
> In my opinion, proper way should be to implement exFAT support into
> existing fs/fat/ code instead of replacing whole vfat/msdosfs by this
> new (now staging) fat implementation.
> 
> In linux kernel we really do not need two different implementation of
> VFAT32.

Not only not useful, but having another one is actively harmful, as
people might actually accidentally used it for classic fat.

But what I'm really annoyed at is this whole culture of just dumping
some crap into staging and hoping it'll sort itself out.  Which it
won't.  We'll need a dedidcated developer spending some time on it
and just get it into shape, and having it in staging does not help
with that at all - it will get various random cleanup that could
be trivially scripted, but that is rarely the main issue with any
codebase.

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

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

On Friday 30 August 2019 08:40:06 Christoph Hellwig wrote:
> On Thu, Aug 29, 2019 at 10:56:31PM +0200, Pali Rohár wrote:
> > In my opinion, proper way should be to implement exFAT support into
> > existing fs/fat/ code instead of replacing whole vfat/msdosfs by this
> > new (now staging) fat implementation.
> > 
> > In linux kernel we really do not need two different implementation of
> > VFAT32.
> 
> Not only not useful, but having another one is actively harmful, as
> people might actually accidentally used it for classic fat.
> 
> But what I'm really annoyed at is this whole culture of just dumping
> some crap into staging and hoping it'll sort itself out.  Which it
> won't.  We'll need a dedidcated developer spending some time on it
> and just get it into shape, and having it in staging does not help
> with that at all - it will get various random cleanup that could
> be trivially scripted, but that is rarely the main issue with any
> codebase.

Exactly. Somebody should take this code and work on it. Otherwise we can
say it is dead code and would happen same thing as with other staging
drivers -- ready to be removed.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Dave Chinner]

On Thu, Aug 29, 2019 at 01:18:10PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 29, 2019 at 03:37:49AM -0700, Christoph Hellwig wrote:
> > On Thu, Aug 29, 2019 at 11:50:19AM +0200, Greg Kroah-Hartman wrote:
> > > I know the code is horrible, but I will gladly take horrible code into
> > > staging.  If it bothers you, just please ignore it.  That's what staging
> > > is there for :)
> > 
> > And then after a while you decide it's been long enough and force move
> > it out of staging like the POS erofs code?
> 
> Hey, that's not nice, erofs isn't a POS.  It could always use more
> review, which the developers asked for numerous times.
> 
> There's nothing different from a filesystem compared to a driver.  If
> its stand-alone, and touches nothing else, all issues with it are
> self-contained and do not bother anyone else in the kernel.  We merge

I whole-heartedly disagree with that statement.

The major difference between filesystems and the rest of the kernel
that seems to be missed by most kernel developers is that
filesystems maintain persistent data - you can't fix a problem/bug
by rebooting or power cycling. Once the filesystem code screws up,
the user is left with a mess they have to fix and that invariably
results in data loss.

Users remember when a filesystem eats their data - they don't tend
to want to have anything to do with that filesystem ever again if it
happens to them. We still get people saying "XFS ate my data back in
2002, I dont trust it and I'll never use it again".

Users put up with shit hardware and drivers - it's an inconvenience
more than anything. They don't put up with buggy filesystems that
lose their data - that is completely unacceptible to users.  As a
result, the quality and stability standard for merging a new
filesystem needs to be far higher that what is acceptible for
merging a new driver.

The correct place for new filesystem review is where all the
experienced filesystem developers hang out - that's linux-fsdevel,
not the driver staging tree.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Chao Yu]

On 2019/8/30 19:51, David Sterba wrote:
> On Fri, Aug 30, 2019 at 10:06:25AM +0800, Chao Yu wrote:
>> On 2019/8/29 23:43, Dan Carpenter wrote:
>>>> p.s. There are 2947 (un)likely places in fs/ directory.
>>>
>>> I was complaining about you adding new pointless ones, not existing
>>> ones.  The likely/unlikely annotations are supposed to be functional and
>>> not decorative.  I explained this very clearly.
>>>
>>> Probably most of the annotations in fs/ are wrong but they are also
>>> harmless except for the slight messiness.  However there are definitely
>>> some which are important so removing them all isn't a good idea.
>>
>> Hi Dan,
>>
>> Could you please pick up one positive example using likely and unlikely
>> correctly? so we can follow the example, rather than removing them all blindly.
> 
> Remove all of them and re-add with explanation if and how each is going
> to make things better. If you can't reason about, prove by benchmarks or
> point to inefficient asm code generated, then don't add them again.

It seems the result of it is strongly related to arch and compiler, if we readd
it after we just prove its gain only in one combination, I doubt we may suffer
regression in other combination in between archs and comilers. It looks like we
don't have any cheaper way to readd it? instead of verifying all/most combinations.

> 
> The feedback I got from CPU and compiler people over the years is not to
> bother using the annotations. CPUs are doing dynamic branch prediction
> and compilers are applying tons of optimizations.
> 
> GCC docs state about the builtin: "In general, you should prefer to use
> actual profile feedback for this (-fprofile-arcs), as programmers are
> notoriously bad at predicting how their programs actually perform."
> (https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html)

Yes, I agree with this. Thanks a lot for sharing your experience. :)

The removal change has done and been merged into Greg's tree, let's consider to
readd it later if possible as you suggested.

thanks,

> .
> 

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Valdis Klētnieks]

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

On Sat, 31 Aug 2019 07:54:10 +1000, Dave Chinner said:

> The correct place for new filesystem review is where all the
> experienced filesystem developers hang out - that's linux-fsdevel,
> not the driver staging tree.

So far everything's been cc'ed to linux-fsdevel, which has been spending
more time discussing unlikely() usage in a different filesystem.



[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Dave Chinner]

On Sat, Aug 31, 2019 at 06:31:45AM -0400, Valdis Klētnieks wrote:
> On Sat, 31 Aug 2019 07:54:10 +1000, Dave Chinner said:
> 
> > The correct place for new filesystem review is where all the
> > experienced filesystem developers hang out - that's linux-fsdevel,
> > not the driver staging tree.
> 
> So far everything's been cc'ed to linux-fsdevel, which has been spending
> more time discussing unlikely() usage in a different filesystem.

That's just noise - you'll get whitespace and other trivial
review on any list you post a patch series for review. Go back and
look at what other people have raised w.r.t. to that filesystem -
on-disk format validation, re-implementation of largely generic
code, lack of namespacing of functions leading to conflicts with
generic/VFS functionality, etc.

Review bandwidth for things like on-disk format definition and
manipulation, consistency with other filesystems, efficient
integration into the generic infrastructure, etc is limited.
Everyone has to juggle that around the load they have for their own
filesystem maintenance, and there's usually only bandwidth for a
single filesystem at a time.

Just be patient - trying to force the merging of code before there's
even been consensus on fundamental architecture choices doesn't make
things better for anyone.  Merging incomplete filesystem code early
in the development cycle has -always- been something we've regretted
in the long run.  We've learn this lesson many times before, yet we
seem doomed to repeat it yet again...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] staging: exfat: add exfat filesystem code to

[Park Ju Hyung]

Hi.

I just noticed that this exfat-staging drivers are based on the old 
Samsung's 1.x exFAT drivers.

I've been working to get the newer Samsung's driver(now named "sdFAT") 
to fit better for general Linux users, and I believe it can provide a 
better base for the community to work on(and hopefully complies better 
to the mainline coding standard).

GitHub link
https://github.com/arter97/exfat-linux

I also included some rudimentary benchmark results.

I encourage mainline developers to explore this driver base and see if 
it's worth to switch, since it's the early days of exfat-staging.

To others watching this thread:
It's more than likely that you can start using exFAT reliably right 
away by following the link above. It's tested on all major LTS kernels 
ranging from 3.4 to 4.19 and the ones Canonical uses for Ubuntu: 3.4, 
3.10, 3.18, 4.1, 4.4, 4.9, 4.14, 4.19 and 4.15, 5.0, 5.2, and 5.3-rc.

Thanks.

Re: [PATCH] staging: exfat: add exfat filesystem code to

[Greg KH]

On Sat, Sep 14, 2019 at 10:39:51PM +0900, Park Ju Hyung wrote:
> Hi.
> 
> I just noticed that this exfat-staging drivers are based on the old 
> Samsung's 1.x exFAT drivers.
> 
> I've been working to get the newer Samsung's driver(now named "sdFAT") 
> to fit better for general Linux users, and I believe it can provide a 
> better base for the community to work on(and hopefully complies better 
> to the mainline coding standard).
> 
> GitHub link
> https://github.com/arter97/exfat-linux
> 
> I also included some rudimentary benchmark results.
> 
> I encourage mainline developers to explore this driver base and see if 
> it's worth to switch, since it's the early days of exfat-staging.

Note, this just showed up publically on August 12, where were you with
all of this new code before then?  :)

> To others watching this thread:
> It's more than likely that you can start using exFAT reliably right 
> away by following the link above. It's tested on all major LTS kernels 
> ranging from 3.4 to 4.19 and the ones Canonical uses for Ubuntu: 3.4, 
> 3.10, 3.18, 4.1, 4.4, 4.9, 4.14, 4.19 and 4.15, 5.0, 5.2, and 5.3-rc.

For the in-kernel code, we would have to rip out all of the work you did
for all older kernels, so that's a non-starter right there.

As for what codebase to work off of, I don't want to say it is too late,
but really, this shows up from nowhere and we had to pick something so
we found the best we could at that point in time.

Is there anything specific in the codebase you have now, that is lacking
in the in-kernel code?  Old-kernel-support doesn't count here, as we
don't care about that as it is not applicable.  But functionality does
matter, what has been added here that we can make use of?

And do you have any "real" development history to look at instead of the
"one giant commit" of the initial code drop?  That is where we could
actually learn what has changed over time.  Your repo as-is shows none
of the interesting bits :(

thanks,

greg kh

Re: [PATCH] staging: exfat: add exfat filesystem code to

[Ju Hyung Park]

Hi Greg,

On Sun, Sep 15, 2019 at 10:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> Note, this just showed up publically on August 12, where were you with
> all of this new code before then?  :)

My sdFAT port, exfat-nofuse and the one on the staging tree, were all
made by Samsung.
And unless you guys had a chance to talk to Samsung developers
directly, those all share the same faith of lacking proper development
history.

The source I used was from http://opensource.samsung.com, which
provides kernel sources as tar.gz files.
There is no code history available.

> For the in-kernel code, we would have to rip out all of the work you did
> for all older kernels, so that's a non-starter right there.

I'm aware.
I'm just letting mainline know that there is potentially another (much
better) base that could be upstreamed.

If you want me to rip out older kernel support for upstreaming, I'm
more than happy to do so.

> As for what codebase to work off of, I don't want to say it is too late,
> but really, this shows up from nowhere and we had to pick something so
> we found the best we could at that point in time.

To be honest, whole public exFAT sources are all from nowhere unless
you had internal access to Samsung's development archive.
The one in the current staging tree isn't any better.

I'm not even sure where the staging driver is from, actually.

Samsung used the 1.2.x versioning until they switched to a new code
base - sdFAT.
The one in the staging tree is marked version 1.3.0(exfat_super.c).
I failed to find anything 1.3.x from Samsung's public kernel sources.

The last time exFAT 1.2.x was used was in Galaxy S7(released in 2016).
Mine was originally based on sdFAT 2.1.10, used in Galaxy S10(released
in March 2019) and it just got updated to 2.2.0, used in Galaxy
Note10(released in August 2019).

> Is there anything specific in the codebase you have now, that is lacking
> in the in-kernel code?  Old-kernel-support doesn't count here, as we
> don't care about that as it is not applicable.  But functionality does
> matter, what has been added here that we can make use of?

This is more of a suggestion of
"Let's base on a *much more recent* snapshot for the community to work on",
since the current one on the staging tree also lacks development history.

The diff is way too big to even start understanding the difference.


With that said though, I do have some vague but real reason as to why
sdFAT base is better.

With some major Android vendors showing interests in supporting exFAT,
Motorola notably published their work on public Git repository with
full development history(the only vendor to do this that I'm aware
of).
Commits like this:
https://github.com/MotorolaMobilityLLC/kernel-msm/commit/7ab1657 is
not merged to exFAT(including the current staging tree one) while it
did for sdFAT.


The only thing I regret is not working on porting sdFAT sooner.
I definitely didn't anticipate Microsoft to suddenly lift legal issues
on upstreaming exFAT just around when I happen to gain interest in
porting sdFAT.

If my port happened sooner, it would have been a no-brainer for it to
be considered as a top candidate for upstreaming.

> And do you have any "real" development history to look at instead of the
> "one giant commit" of the initial code drop?  That is where we could
> actually learn what has changed over time.  Your repo as-is shows none
> of the interesting bits :(

As I mentioned, development history is unobtainable, even for the
current staging tree or exfat-nofuse.
(If you guys took exfat-nofuse, you can also see that there's barely
any real exFAT-related development done in that tree. Everything is
basically fixes for newer kernel versions.)

The best I could do, if someone's interested, is to diff all versions
of exFAT/sdFAT throughout the Samsung's kernel versions, but that
still won't give us reasons as to why the changes were made.

TL;DR
My suggestion - Let's base on a much newer driver that's matured more,
contains more fixes, gives (slightly?) better performance and
hopefully has better code quality.

Both drivers are horrible.
You said it yourself(for the current staging one), and even for my new
sdFAT-base proposal, I'm definitely not comfortable seeing this kind
of crap in mainline:
https://github.com/arter97/exfat-linux/commit/0f1ddde

However, it's clear to me that the sdFAT base is less-horrible.

Please let me know what you think.

> thanks,
>
> greg kh

Thanks.

Re: [PATCH] staging: exfat: rebase to sdFAT v2.2.0

[Greg KH]

On Thu, Sep 19, 2019 at 04:59:20AM +0900, Park Ju Hyung wrote:
> The new sdFAT driver base brings many improvements to the table.
> Quoting Namjae Jeon:
> 
> ======================================================================
> 1. sdfat has been refactored to improve compatibility, readability and
> to be linux friendly.(included support mass storages larger than 2TB.)
> 
> 2. sdfat has been optimized for the performance of SD-cards.
>   - Support SD-card friendly block allocation and delayed allocation
>     for vfat-fs only.
>   - Support aligned_mpage_write for both vfat-fs and exfat-fs
> 
> 3. sdfat has been optimized for the performance of general operations
>     like create,lookup and readdir.
> 
> 4. Fix many critical and minor bugs
>  - Handle many kinds of error conditions gracefully to prevent panic.
>  - Fix critical bugs related to rmdir, truncate behavior and so on...
> 
> 5. Fix NLS functions
> ======================================================================
> 
> sdFAT v2.2.0 (from N970FXXU1ASHE) was forked and then cleaned up to better
> suit mainline's standards:
> 
>  * Remove fat12/16/32 handlings and rename to exFAT.
>  * Remove older kernel compatibilities.
>  * Remove Samsung's userspace-specific code (e.g. defrag).
>  * Fix compilation warnings on modern compiler.
>  * Remove unused functions.
>  * Rename non-static functions for avoiding global symbol table pollutions.
>  * Declare functions as static whenever possible.
>  * Fix checkpatch.pl errors.
>  * Remove aligned mpage write for portability.
> 
> Full rebase history can be found on:
> https://github.com/arter97/exfat-linux/commits/for-next
> 
> Signed-off-by: Park Ju Hyung <qkrwngud825@gmail.com>
> ---
>  drivers/staging/exfat/Kconfig        |   88 +-
>  drivers/staging/exfat/Makefile       |   11 +-
>  drivers/staging/exfat/TODO           |   15 +-
>  drivers/staging/exfat/api.h          |  265 ++
>  drivers/staging/exfat/blkdev.c       |  330 +++
>  drivers/staging/exfat/cache.c        |  785 +++++
>  drivers/staging/exfat/config.h       |   32 +
>  drivers/staging/exfat/core.c         | 3169 ++++++++++++++++++++
>  drivers/staging/exfat/core.h         |  149 +
>  drivers/staging/exfat/core_exfat.c   | 1485 ++++++++++
>  drivers/staging/exfat/exfat.h        | 1261 +++-----
>  drivers/staging/exfat/exfat_blkdev.c |  136 -
>  drivers/staging/exfat/exfat_cache.c  |  724 -----
>  drivers/staging/exfat/exfat_core.c   | 3701 -----------------------
>  drivers/staging/exfat/exfat_fs.h     |  398 +++
>  drivers/staging/exfat/exfat_nls.c    |  404 ---
>  drivers/staging/exfat/exfat_super.c  | 4049 --------------------------
>  drivers/staging/exfat/exfat_upcase.c |  740 -----
>  drivers/staging/exfat/extent.c       |  329 +++
>  drivers/staging/exfat/fatent.c       |  113 +
>  drivers/staging/exfat/misc.c         |  356 +++
>  drivers/staging/exfat/nls.c          |  323 ++
>  drivers/staging/exfat/super.c        | 3168 ++++++++++++++++++++
>  drivers/staging/exfat/upcase.h       |  394 +++
>  drivers/staging/exfat/xattr.c        |   76 +
>  25 files changed, 11777 insertions(+), 10724 deletions(-)

That's a lot of rewriting :(

How about at least keeping the file names the same to make it easier to
see what happened here?

Then send a follow-on patch that just does the rename?

And by taking something like this, are you agreeing that Samsung will
help out with the development of this code to clean it up and get it
into "real" mergable shape?

Also, I can't take this patch for this simple reason alone:

> --- a/drivers/staging/exfat/Makefile
> +++ b/drivers/staging/exfat/Makefile
> @@ -1,10 +1,5 @@
> -# SPDX-License-Identifier: GPL-2.0
> -

Don't delete SPDX lines :)

thanks,

greg k-h

Re: [PATCH] staging: exfat: rebase to sdFAT v2.2.0

[Greg KH]

On Thu, Sep 19, 2019 at 04:59:20AM +0900, Park Ju Hyung wrote:
> --- a/drivers/staging/exfat/exfat.h
> +++ b/drivers/staging/exfat/exfat.h
> @@ -1,4 +1,4 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> +// SPDX-License-Identifier: GPL-2.0-or-later

You just changed the license of this file.  Are you SURE about that?

I would like to get a signed-off-by from a samsung developer/lawyer to
have on the record for this, otherwise I can't take such a thing.

thanks,

greg k-h

Re: [PATCH] staging: exfat: rebase to sdFAT v2.2.0

[Ju Hyung Park]

Hi Greg,

On Thu, Sep 19, 2019 at 5:12 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> That's a lot of rewriting :(
>
> How about at least keeping the file names the same to make it easier to
> see what happened here?
>
> Then send a follow-on patch that just does the rename?

That's still not quite useful tbh, lemme bring the diff stat I wrote
in previous email just in case you missed it:
<Full diff stat>
 Kconfig      |   79 +-
 Makefile     |   46 +-
 api.c        |  423 ----
 api.h        |  310 ---
 blkdev.c     |  409 +---
 cache.c      | 1142 ++++-----
 config.h     |   49 -
 core.c       | 5583 ++++++++++++++++++++++++--------------------
 core.h       |  196 --
 core_exfat.c | 1553 ------------
 exfat.h      | 1309 +++++++----
 exfat_fs.h   |  417 ----
 extent.c     |  351 ---
 fatent.c     |  182 --
 misc.c       |  401 ----
 nls.c        |  490 ++--
 super.c      | 5103 +++++++++++++++++++++-------------------
 upcase.c     |  740 ++++++
 upcase.h     |  407 ----
 version.h    |   29 -
 xattr.c      |  136 --
 21 files changed, 8186 insertions(+), 11169 deletions(-)

<diff-filter=M>
 Kconfig  |   79 +-
 Makefile |   46 +-
 blkdev.c |  409 +---
 cache.c  | 1142 +++++-----
 core.c   | 5583 ++++++++++++++++++++++++++----------------------
 exfat.h  | 1309 ++++++++----
 nls.c    |  490 ++---
 super.c  | 5103 ++++++++++++++++++++++---------------------
 8 files changed, 7446 insertions(+), 6715 deletions(-)

These diff stats were taken by removing "exfat_" prefix from the
current staging drivers.

But if that's still what you want, I'll do it.
btw, removing "exfat_" prefix from the current one makes more sense imo.

If we add "exfat_" prefix to the new one, we get weird file names like
"exfat_core_exfat.c".

> And by taking something like this, are you agreeing that Samsung will
> help out with the development of this code to clean it up and get it
> into "real" mergable shape?

Well, I think you got me confused with Namjae.
(Yeah Korean names are confusing I know :) )

Namjae (or anyone else from Samsung) should answer that, not me.

I just prepared a patch as we were getting nowhere like you mentioned :)

> Also, I can't take this patch for this simple reason alone:
> Don't delete SPDX lines :)

Sorry.
I'll add that back for v2.

On Thu, Sep 19, 2019 at 5:13 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Sep 19, 2019 at 04:59:20AM +0900, Park Ju Hyung wrote:
> > --- a/drivers/staging/exfat/exfat.h
> > +++ b/drivers/staging/exfat/exfat.h
> > @@ -1,4 +1,4 @@
> > -/* SPDX-License-Identifier: GPL-2.0 */
> > +// SPDX-License-Identifier: GPL-2.0-or-later
>
> You just changed the license of this file.  Are you SURE about that?

The sdFAT code release explicitly states "either version 2 of the
License, or (at your option) any later version", so I thought that
makes sense:
https://github.com/arter97/exfat-linux/commit/d5393c4cbe0e5b50231aacd33d9b5b0ddf46a005

Please correct me if I'm wrong.

Thanks.

Re: [PATCH] staging: exfat: rebase to sdFAT v2.2.0

[Greg KH]

On Thu, Sep 19, 2019 at 05:22:03AM +0900, Ju Hyung Park wrote:
> Hi Greg,
> 
> On Thu, Sep 19, 2019 at 5:12 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > That's a lot of rewriting :(
> >
> > How about at least keeping the file names the same to make it easier to
> > see what happened here?
> >
> > Then send a follow-on patch that just does the rename?
> 
> That's still not quite useful tbh, lemme bring the diff stat I wrote
> in previous email just in case you missed it:
> <Full diff stat>
>  Kconfig      |   79 +-
>  Makefile     |   46 +-
>  api.c        |  423 ----
>  api.h        |  310 ---
>  blkdev.c     |  409 +---
>  cache.c      | 1142 ++++-----
>  config.h     |   49 -
>  core.c       | 5583 ++++++++++++++++++++++++--------------------
>  core.h       |  196 --
>  core_exfat.c | 1553 ------------
>  exfat.h      | 1309 +++++++----
>  exfat_fs.h   |  417 ----
>  extent.c     |  351 ---
>  fatent.c     |  182 --
>  misc.c       |  401 ----
>  nls.c        |  490 ++--
>  super.c      | 5103 +++++++++++++++++++++-------------------
>  upcase.c     |  740 ++++++
>  upcase.h     |  407 ----
>  version.h    |   29 -
>  xattr.c      |  136 --
>  21 files changed, 8186 insertions(+), 11169 deletions(-)
> 
> <diff-filter=M>
>  Kconfig  |   79 +-
>  Makefile |   46 +-
>  blkdev.c |  409 +---
>  cache.c  | 1142 +++++-----
>  core.c   | 5583 ++++++++++++++++++++++++++----------------------
>  exfat.h  | 1309 ++++++++----
>  nls.c    |  490 ++---
>  super.c  | 5103 ++++++++++++++++++++++---------------------
>  8 files changed, 7446 insertions(+), 6715 deletions(-)
> 
> These diff stats were taken by removing "exfat_" prefix from the
> current staging drivers.
> 
> But if that's still what you want, I'll do it.

It will show easier when you do funny things like take off the
formatting of the huge tables for no good reason :)

> btw, removing "exfat_" prefix from the current one makes more sense imo.

I agree.

> If we add "exfat_" prefix to the new one, we get weird file names like
> "exfat_core_exfat.c".

Agreed.

> > And by taking something like this, are you agreeing that Samsung will
> > help out with the development of this code to clean it up and get it
> > into "real" mergable shape?
> 
> Well, I think you got me confused with Namjae.
> (Yeah Korean names are confusing I know :) )

It was a general ask, that's all :)

> Namjae (or anyone else from Samsung) should answer that, not me.
> 
> I just prepared a patch as we were getting nowhere like you mentioned :)
> 
> > Also, I can't take this patch for this simple reason alone:
> > Don't delete SPDX lines :)
> 
> Sorry.
> I'll add that back for v2.
> 
> On Thu, Sep 19, 2019 at 5:13 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Sep 19, 2019 at 04:59:20AM +0900, Park Ju Hyung wrote:
> > > --- a/drivers/staging/exfat/exfat.h
> > > +++ b/drivers/staging/exfat/exfat.h
> > > @@ -1,4 +1,4 @@
> > > -/* SPDX-License-Identifier: GPL-2.0 */
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> >
> > You just changed the license of this file.  Are you SURE about that?
> 
> The sdFAT code release explicitly states "either version 2 of the
> License, or (at your option) any later version", so I thought that
> makes sense:
> https://github.com/arter97/exfat-linux/commit/d5393c4cbe0e5b50231aacd33d9b5b0ddf46a005
> 
> Please correct me if I'm wrong.

That differs from the original exfat code, so something is odd here.  I
need some sort of clarification from Samsung as to when they changed the
license in order to be able to relicense these files.

thanks,

greg k-h

Re: [PATCH] staging: exfat: rebase to sdFAT v2.2.0

[Ju Hyung Park]

On Thu, Sep 19, 2019 at 5:26 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> That differs from the original exfat code, so something is odd here.  I
> need some sort of clarification from Samsung as to when they changed the
> license in order to be able to relicense these files.

We should probably ask Valdis on what happened there.

Even the old exFAT v1.2.24 from Galaxy S7 is using "either version 2
of the License, or (at your option) any later version".
You can go check it yourself by searching "G930F" from
http://opensource.samsung.com.

I'm guessing he probably used "GPL-2.0" during his clean-up.

Re: [PATCH] staging: exfat: rebase to sdFAT v2.2.0

[Valdis Klētnieks]

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

On Thu, 19 Sep 2019 05:31:21 +0900, Ju Hyung Park said:

> We should probably ask Valdis on what happened there.
>
> Even the old exFAT v1.2.24 from Galaxy S7 is using "either version 2
> of the License, or (at your option) any later version".
> You can go check it yourself by searching "G930F" from
> http://opensource.samsung.com.
>
> I'm guessing he probably used "GPL-2.0" during his clean-up.

My  screw-up.

Original had:

/*
 *  Copyright (C) 2012-2013 Samsung Electronics Co., Ltd.
 *
 *  This program is free software; you can redistribute it and/or
 *  modify it under the terms of the GNU General Public License
 *  as published by the Free Software Foundation; either version 2
 *  of the License, or (at your option) any later version.

So yes, I dorked up the SPDX tags, as I didn't realize GPL-2.0-or-later
was an actual thing for those...

[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] staging: exfat: rebase to sdFAT v2.2.0

[kbuild test robot]

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

Hi Park,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190917]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Park-Ju-Hyung/staging-exfat-rebase-to-sdFAT-v2-2-0/20190919-040526
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging/exfat/misc.c: In function 'exfat_uevent_ro_remount':
>> drivers/staging/exfat/misc.c:57:2: error: implicit declaration of function 'ST_LOG'; did you mean 'PT_LOAD'? [-Werror=implicit-function-declaration]
     ST_LOG("[EXFAT](%s[%d:%d]): Uevent triggered\n",
     ^~~~~~
     PT_LOAD
   cc1: some warnings being treated as errors

vim +57 drivers/staging/exfat/misc.c

    43	
    44	void exfat_uevent_ro_remount(struct super_block *sb)
    45	{
    46		struct block_device *bdev = sb->s_bdev;
    47		dev_t bd_dev = bdev ? bdev->bd_dev : 0;
    48	
    49		char major[16], minor[16];
    50		char *envp[] = { major, minor, NULL };
    51	
    52		snprintf(major, sizeof(major), "MAJOR=%d", MAJOR(bd_dev));
    53		snprintf(minor, sizeof(minor), "MINOR=%d", MINOR(bd_dev));
    54	
    55		kobject_uevent_env(&exfat_uevent_kobj, KOBJ_CHANGE, envp);
    56	
  > 57		ST_LOG("[EXFAT](%s[%d:%d]): Uevent triggered\n",
    58				sb->s_id, MAJOR(bd_dev), MINOR(bd_dev));
    59	}
    60	#endif
    61	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69834 bytes --]

Re: [PATCH] staging: exfat: rebase to sdFAT v2.2.0

[kbuild test robot]

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

Hi Park,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190917]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Park-Ju-Hyung/staging-exfat-rebase-to-sdFAT-v2-2-0/20190919-040526
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging/exfat/super.c: In function 'exfat_debug_kill_sb':
>> drivers/staging/exfat/super.c:3079:4: error: implicit declaration of function 'exfat_cache_release'; did you mean 'exfat_dcache_release'? [-Werror=implicit-function-declaration]
       exfat_cache_release(sb);
       ^~~~~~~~~~~~~~~~~~~
       exfat_dcache_release
   cc1: some warnings being treated as errors
--
   drivers/staging/exfat/misc.c: In function 'exfat_uevent_ro_remount':
>> drivers/staging/exfat/misc.c:57:2: error: implicit declaration of function 'ST_LOG'; did you mean 'DT_REG'? [-Werror=implicit-function-declaration]
     ST_LOG("[EXFAT](%s[%d:%d]): Uevent triggered\n",
     ^~~~~~
     DT_REG
   cc1: some warnings being treated as errors

vim +3079 drivers/staging/exfat/super.c

  3063	
  3064	#ifdef CONFIG_EXFAT_DBG_IOCTL
  3065	static void exfat_debug_kill_sb(struct super_block *sb)
  3066	{
  3067		struct exfat_sb_info *sbi = EXFAT_SB(sb);
  3068		struct block_device *bdev = sb->s_bdev;
  3069	
  3070		long flags;
  3071	
  3072		if (sbi) {
  3073			flags = sbi->debug_flags;
  3074	
  3075			if (flags & EXFAT_DEBUGFLAGS_INVALID_UMOUNT) {
  3076				/* invalidate_bdev drops all device cache include dirty.
  3077				 * we use this to simulate device removal
  3078				 */
> 3079				exfat_cache_release(sb);
  3080				invalidate_bdev(bdev);
  3081			}
  3082		}
  3083	
  3084		kill_block_super(sb);
  3085	}
  3086	#endif /* CONFIG_EXFAT_DBG_IOCTL */
  3087	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69879 bytes --]

Re: [PATCH] staging: exfat: rebase to sdFAT v2.2.0

[Ju Hyung Park]

Welp, looks like I didn't test debug configs properly.

Allow me 1-2 days to work on fixing it for v2.

Thanks in advance.

On Thu, Sep 19, 2019 at 6:31 AM kbuild test robot <lkp@intel.com> wrote:
>
> Hi Park,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [cannot apply to v5.3 next-20190917]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Park-Ju-Hyung/staging-exfat-rebase-to-sdFAT-v2-2-0/20190919-040526
> config: x86_64-allyesconfig (attached as .config)
> compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
>    drivers/staging/exfat/super.c: In function 'exfat_debug_kill_sb':
> >> drivers/staging/exfat/super.c:3079:4: error: implicit declaration of function 'exfat_cache_release'; did you mean 'exfat_dcache_release'? [-Werror=implicit-function-declaration]
>        exfat_cache_release(sb);
>        ^~~~~~~~~~~~~~~~~~~
>        exfat_dcache_release
>    cc1: some warnings being treated as errors
> --
>    drivers/staging/exfat/misc.c: In function 'exfat_uevent_ro_remount':
> >> drivers/staging/exfat/misc.c:57:2: error: implicit declaration of function 'ST_LOG'; did you mean 'DT_REG'? [-Werror=implicit-function-declaration]
>      ST_LOG("[EXFAT](%s[%d:%d]): Uevent triggered\n",
>      ^~~~~~
>      DT_REG
>    cc1: some warnings being treated as errors
>
> vim +3079 drivers/staging/exfat/super.c
>
>   3063
>   3064  #ifdef CONFIG_EXFAT_DBG_IOCTL
>   3065  static void exfat_debug_kill_sb(struct super_block *sb)
>   3066  {
>   3067          struct exfat_sb_info *sbi = EXFAT_SB(sb);
>   3068          struct block_device *bdev = sb->s_bdev;
>   3069
>   3070          long flags;
>   3071
>   3072          if (sbi) {
>   3073                  flags = sbi->debug_flags;
>   3074
>   3075                  if (flags & EXFAT_DEBUGFLAGS_INVALID_UMOUNT) {
>   3076                          /* invalidate_bdev drops all device cache include dirty.
>   3077                           * we use this to simulate device removal
>   3078                           */
> > 3079                          exfat_cache_release(sb);
>   3080                          invalidate_bdev(bdev);
>   3081                  }
>   3082          }
>   3083
>   3084          kill_block_super(sb);
>   3085  }
>   3086  #endif /* CONFIG_EXFAT_DBG_IOCTL */
>   3087
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

On Friday 30 August 2019 09:56:47 Pali Rohár wrote:
> On Thursday 29 August 2019 19:35:06 Sasha Levin wrote:
> > With regards to missing specs/docs/whatever - our main concern with this
> > release was that we want full interoperability, which is why the spec
> > was made public as-is without modifications from what was used
> > internally. There's no "secret sauce" that Microsoft is hiding here.
> 
> Ok, if it was just drop of "current version" of documentation then it
> makes sense.
> 
> > How about we give this spec/code time to get soaked and reviewed for a
> > bit, and if folks still feel (in a month or so?) that there are missing
> > bits of information related to exfat, I'll be happy to go back and try
> > to get them out as well.

Hello Sasha!

Now one month passed, so do you have some information when missing parts
of documentation like TexFAT would be released to public?

> Basically external references in that released exFAT specification are
> unknown / not released yet. Like TexFAT. So if you have an input in MS
> could you forward request about these missing bits?

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

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Sasha Levin]

On Wed, Oct 16, 2019 at 04:03:53PM +0200, Pali Rohár wrote:
>On Friday 30 August 2019 09:56:47 Pali Rohár wrote:
>> On Thursday 29 August 2019 19:35:06 Sasha Levin wrote:
>> > With regards to missing specs/docs/whatever - our main concern with this
>> > release was that we want full interoperability, which is why the spec
>> > was made public as-is without modifications from what was used
>> > internally. There's no "secret sauce" that Microsoft is hiding here.
>>
>> Ok, if it was just drop of "current version" of documentation then it
>> makes sense.
>>
>> > How about we give this spec/code time to get soaked and reviewed for a
>> > bit, and if folks still feel (in a month or so?) that there are missing
>> > bits of information related to exfat, I'll be happy to go back and try
>> > to get them out as well.
>
>Hello Sasha!
>
>Now one month passed, so do you have some information when missing parts
>of documentation like TexFAT would be released to public?

Sure, I'll see if I can get an approval to open it up.

Can I assume you will be implementing TexFAT support once the spec is
available?

-- 
Thanks,
Sasha

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

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

On Wednesday 16 October 2019 10:31:13 Sasha Levin wrote:
> On Wed, Oct 16, 2019 at 04:03:53PM +0200, Pali Rohár wrote:
> > On Friday 30 August 2019 09:56:47 Pali Rohár wrote:
> > > On Thursday 29 August 2019 19:35:06 Sasha Levin wrote:
> > > > With regards to missing specs/docs/whatever - our main concern with this
> > > > release was that we want full interoperability, which is why the spec
> > > > was made public as-is without modifications from what was used
> > > > internally. There's no "secret sauce" that Microsoft is hiding here.
> > > 
> > > Ok, if it was just drop of "current version" of documentation then it
> > > makes sense.
> > > 
> > > > How about we give this spec/code time to get soaked and reviewed for a
> > > > bit, and if folks still feel (in a month or so?) that there are missing
> > > > bits of information related to exfat, I'll be happy to go back and try
> > > > to get them out as well.
> > 
> > Hello Sasha!
> > 
> > Now one month passed, so do you have some information when missing parts
> > of documentation like TexFAT would be released to public?
> 
> Sure, I'll see if I can get an approval to open it up.

Ok!

> Can I assume you will be implementing TexFAT support once the spec is
> available?

I cannot promise that I would implement something which I do not know
how is working... It depends on how complicated TexFAT is and also how
future exfat support in kernel would look like.

But I'm interesting in implementing it.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Valdis Klētnieks]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 676 bytes --]

On Wed, 16 Oct 2019 10:31:13 -0400, Sasha Levin said:
> On Wed, Oct 16, 2019 at 04:03:53PM +0200, Pali Rohár wrote:

> >Now one month passed, so do you have some information when missing parts
> >of documentation like TexFAT would be released to public?
>
> Sure, I'll see if I can get an approval to open it up.
>
> Can I assume you will be implementing TexFAT support once the spec is
> available?

It's certainly something that *should* be supported. The exact timeframe, and
who the "you" that actually writes the patch is of course up in the air (and
will likely end up being a collaborative effort between the first author and
corrections from others).


[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Sasha Levin]

On Wed, Oct 16, 2019 at 06:03:49PM +0200, Pali Rohár wrote:
>On Wednesday 16 October 2019 10:31:13 Sasha Levin wrote:
>> On Wed, Oct 16, 2019 at 04:03:53PM +0200, Pali Rohár wrote:
>> > On Friday 30 August 2019 09:56:47 Pali Rohár wrote:
>> > > On Thursday 29 August 2019 19:35:06 Sasha Levin wrote:
>> > > > With regards to missing specs/docs/whatever - our main concern with this
>> > > > release was that we want full interoperability, which is why the spec
>> > > > was made public as-is without modifications from what was used
>> > > > internally. There's no "secret sauce" that Microsoft is hiding here.
>> > >
>> > > Ok, if it was just drop of "current version" of documentation then it
>> > > makes sense.
>> > >
>> > > > How about we give this spec/code time to get soaked and reviewed for a
>> > > > bit, and if folks still feel (in a month or so?) that there are missing
>> > > > bits of information related to exfat, I'll be happy to go back and try
>> > > > to get them out as well.
>> >
>> > Hello Sasha!
>> >
>> > Now one month passed, so do you have some information when missing parts
>> > of documentation like TexFAT would be released to public?
>>
>> Sure, I'll see if I can get an approval to open it up.
>
>Ok!
>
>> Can I assume you will be implementing TexFAT support once the spec is
>> available?
>
>I cannot promise that I would implement something which I do not know
>how is working... It depends on how complicated TexFAT is and also how
>future exfat support in kernel would look like.
>
>But I'm interesting in implementing it.

Sure, thank you. This is more to let me tell my management that "there
is someone who wants to work on it" rather than committing you to do
that work :)

-- 
Thanks,
Sasha

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Greg Kroah-Hartman]

On Wed, Oct 16, 2019 at 06:03:49PM +0200, Pali Rohár wrote:
> > Can I assume you will be implementing TexFAT support once the spec is
> > available?
> 
> I cannot promise that I would implement something which I do not know
> how is working... It depends on how complicated TexFAT is and also how
> future exfat support in kernel would look like.
> 
> But I'm interesting in implementing it.

What devices need TexFAT?  I thought it the old devices that used it are
long obsolete and gone.  How is this feature going to be tested/used?

thanks,

greg k-h

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

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

On Wednesday 16 October 2019 09:22:11 Greg Kroah-Hartman wrote:
> On Wed, Oct 16, 2019 at 06:03:49PM +0200, Pali Rohár wrote:
> > > Can I assume you will be implementing TexFAT support once the spec is
> > > available?
> > 
> > I cannot promise that I would implement something which I do not know
> > how is working... It depends on how complicated TexFAT is and also how
> > future exfat support in kernel would look like.
> > 
> > But I'm interesting in implementing it.
> 
> What devices need TexFAT?  I thought it the old devices that used it are
> long obsolete and gone.  How is this feature going to be tested/used?

Hi Greg! Per 3.1.16 of exFAT specification [1], TexFAT extension is the
only way how to use more FAT tables, like in FAT32 (where you normally
have two FATs). Secondary FAT table can be used e.g. for redundancy or
data recovery. For FAT32 volumes, e.g. fsck.fat uses secondary FAT table
when first one is corrupted.

Usage of just one FAT table in exFAT is just step backward from FAT32 as
secondary FAT table is sometimes the only way how to recover broken FAT
fs. So I do not think that exFAT is for old devices, but rather
non-exFAT is for old devices. Modern filesystems have journal or other
technique to do (fast|some) recovery, exFAT has nothing.

And how is this feature going to be used? That depends on specification.

[1] - https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification#3116-numberoffats-field

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Greg Kroah-Hartman]

On Wed, Oct 16, 2019 at 06:32:31PM +0200, Pali Rohár wrote:
> On Wednesday 16 October 2019 09:22:11 Greg Kroah-Hartman wrote:
> > On Wed, Oct 16, 2019 at 06:03:49PM +0200, Pali Rohár wrote:
> > > > Can I assume you will be implementing TexFAT support once the spec is
> > > > available?
> > > 
> > > I cannot promise that I would implement something which I do not know
> > > how is working... It depends on how complicated TexFAT is and also how
> > > future exfat support in kernel would look like.
> > > 
> > > But I'm interesting in implementing it.
> > 
> > What devices need TexFAT?  I thought it the old devices that used it are
> > long obsolete and gone.  How is this feature going to be tested/used?
> 
> Hi Greg! Per 3.1.16 of exFAT specification [1], TexFAT extension is the
> only way how to use more FAT tables, like in FAT32 (where you normally
> have two FATs). Secondary FAT table can be used e.g. for redundancy or
> data recovery. For FAT32 volumes, e.g. fsck.fat uses secondary FAT table
> when first one is corrupted.
> 
> Usage of just one FAT table in exFAT is just step backward from FAT32 as
> secondary FAT table is sometimes the only way how to recover broken FAT
> fs. So I do not think that exFAT is for old devices, but rather
> non-exFAT is for old devices. Modern filesystems have journal or other
> technique to do (fast|some) recovery, exFAT has nothing.
> 
> And how is this feature going to be used? That depends on specification.
> 
> [1] - https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification#3116-numberoffats-field

Ok, but given that the "only" os that can read/write the TexFAT
extension is WinCE, and that os is obsolete these days, it might be hard
to find images to test/validate against :)

But hey, I'll take the patch if you write it, no objection!

thanks,

greg k-h

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Sasha Levin]

On Wed, Oct 16, 2019 at 06:03:49PM +0200, Pali Rohár wrote:
>On Wednesday 16 October 2019 10:31:13 Sasha Levin wrote:
>> On Wed, Oct 16, 2019 at 04:03:53PM +0200, Pali Rohár wrote:
>> > On Friday 30 August 2019 09:56:47 Pali Rohár wrote:
>> > > On Thursday 29 August 2019 19:35:06 Sasha Levin wrote:
>> > > > With regards to missing specs/docs/whatever - our main concern with this
>> > > > release was that we want full interoperability, which is why the spec
>> > > > was made public as-is without modifications from what was used
>> > > > internally. There's no "secret sauce" that Microsoft is hiding here.
>> > >
>> > > Ok, if it was just drop of "current version" of documentation then it
>> > > makes sense.
>> > >
>> > > > How about we give this spec/code time to get soaked and reviewed for a
>> > > > bit, and if folks still feel (in a month or so?) that there are missing
>> > > > bits of information related to exfat, I'll be happy to go back and try
>> > > > to get them out as well.
>> >
>> > Hello Sasha!
>> >
>> > Now one month passed, so do you have some information when missing parts
>> > of documentation like TexFAT would be released to public?
>>
>> Sure, I'll see if I can get an approval to open it up.
>
>Ok!
>
>> Can I assume you will be implementing TexFAT support once the spec is
>> available?
>
>I cannot promise that I would implement something which I do not know
>how is working... It depends on how complicated TexFAT is and also how
>future exfat support in kernel would look like.
>
>But I'm interesting in implementing it.

It looks like the reason this wasn't made public along with the exFAT
spec is that TexFAT is pretty much dead - it's old, there are no users
we are aware of, and digging it out of it's grave to make it public is
actually quite the headache.

Is this something you actually have a need for? an entity that has a
requirement for TexFAT? I'd would rather spend my time elsewhere than
digging TexFAT out of it's grave.

Is there anything else in the exFAT spec that is missing (and someone
actually wants/uses)?

-- 
Thanks,
Sasha

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Valdis Klētnieks]

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

On Wed, 16 Oct 2019 16:33:17 -0400, Sasha Levin said:

> It looks like the reason this wasn't made public along with the exFAT
> spec is that TexFAT is pretty much dead - it's old, there are no users
> we are aware of, and digging it out of it's grave to make it public is
> actually quite the headache.

Ahh.. For some reason I had convinced myself that base exfat implementations
used at least the 'keep a backup FAT' part of TexFAT, because on a terabyte
external USB drive, losing the FAT would be painful.  But if Windows 10 doesn't
do that either, then it's no great sin for Linux to not do it (and may cause
problems if Linux says "currently using FAT 2", and the disk is next used on a
Windows 10 box that only looks at FAT 1....)


[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

On Wednesday 16 October 2019 16:33:17 Sasha Levin wrote:
> On Wed, Oct 16, 2019 at 06:03:49PM +0200, Pali Rohár wrote:
> > On Wednesday 16 October 2019 10:31:13 Sasha Levin wrote:
> > > On Wed, Oct 16, 2019 at 04:03:53PM +0200, Pali Rohár wrote:
> > > > On Friday 30 August 2019 09:56:47 Pali Rohár wrote:
> > > > > On Thursday 29 August 2019 19:35:06 Sasha Levin wrote:
> > > > > > With regards to missing specs/docs/whatever - our main concern with this
> > > > > > release was that we want full interoperability, which is why the spec
> > > > > > was made public as-is without modifications from what was used
> > > > > > internally. There's no "secret sauce" that Microsoft is hiding here.
> > > > >
> > > > > Ok, if it was just drop of "current version" of documentation then it
> > > > > makes sense.
> > > > >
> > > > > > How about we give this spec/code time to get soaked and reviewed for a
> > > > > > bit, and if folks still feel (in a month or so?) that there are missing
> > > > > > bits of information related to exfat, I'll be happy to go back and try
> > > > > > to get them out as well.
> > > >
> > > > Hello Sasha!
> > > >
> > > > Now one month passed, so do you have some information when missing parts
> > > > of documentation like TexFAT would be released to public?
> > > 
> > > Sure, I'll see if I can get an approval to open it up.
> > 
> > Ok!
> > 
> > > Can I assume you will be implementing TexFAT support once the spec is
> > > available?
> > 
> > I cannot promise that I would implement something which I do not know
> > how is working... It depends on how complicated TexFAT is and also how
> > future exfat support in kernel would look like.
> > 
> > But I'm interesting in implementing it.
> 
> It looks like the reason this wasn't made public along with the exFAT
> spec is that TexFAT is pretty much dead - it's old, there are no users
> we are aware of, and digging it out of it's grave to make it public is
> actually quite the headache.
> 
> Is this something you actually have a need for? an entity that has a
> requirement for TexFAT?

Hi!

I do not have device with requirements for TexFAT. The first reason why
I wanted to use TexFAT was that it it the only way how to use more FAT
tables (e.g. secondary for backup) and information for that is missing
in released exFAT specification. This could bring better data recovery.

> I'd would rather spend my time elsewhere than digging TexFAT out of it's grave.

Ok.

I was hoping that it would be possible to easily use secondary FAT table
(from TexFAT extension) for redundancy without need to implement full
TexFAT, which could be also backward compatible with systems which do
not implement TexFAT extension at all. Similarly like using FAT32 disk
with two FAT tables is possible also on system which use first FAT
table.

> Is there anything else in the exFAT spec that is missing (and someone
> actually wants/uses)?

Currently I do not know.

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

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

On Wednesday 16 October 2019 17:53:43 Valdis Klētnieks wrote:
> and may cause problems if Linux says "currently using FAT 2", and the
> disk is next used on a Windows 10 box that only looks at FAT 1....

You should use same algorithm which is used for FAT32. Primary FAT is
first. And all operations are done on Secondary FAT and then is
Secondary FAT copied to Primary. This is backward compatible with
systems which operates only with first primary FAT. And other systems
which see both FATs can benefit from redundancy/recovery.

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

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

On Wednesday 28 August 2019 18:08:17 Greg Kroah-Hartman wrote:
> The full specification of the filesystem can be found at:
>   https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification

FYI, it looks like that this released specification is just copy+paste
from exFAT patent https://patents.google.com/patent/US20150370821 which
was there publicly available for a while. And similar/same? copy was
available on the following site https://www.ntfs.com/exfat-overview.htm

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

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

Hello!

On Thursday 17 October 2019 09:50:08 Pali Rohár wrote:
> On Wednesday 16 October 2019 16:33:17 Sasha Levin wrote:
> > On Wed, Oct 16, 2019 at 06:03:49PM +0200, Pali Rohár wrote:
> > > On Wednesday 16 October 2019 10:31:13 Sasha Levin wrote:
> > > > On Wed, Oct 16, 2019 at 04:03:53PM +0200, Pali Rohár wrote:
> > > > > On Friday 30 August 2019 09:56:47 Pali Rohár wrote:
> > > > > > On Thursday 29 August 2019 19:35:06 Sasha Levin wrote:
> > > > > > > With regards to missing specs/docs/whatever - our main concern with this
> > > > > > > release was that we want full interoperability, which is why the spec
> > > > > > > was made public as-is without modifications from what was used
> > > > > > > internally. There's no "secret sauce" that Microsoft is hiding here.
> > > > > >
> > > > > > Ok, if it was just drop of "current version" of documentation then it
> > > > > > makes sense.
> > > > > >
> > > > > > > How about we give this spec/code time to get soaked and reviewed for a
> > > > > > > bit, and if folks still feel (in a month or so?) that there are missing
> > > > > > > bits of information related to exfat, I'll be happy to go back and try
> > > > > > > to get them out as well.
> > > > >
> > > > > Hello Sasha!
> > > > >
> > > > > Now one month passed, so do you have some information when missing parts
> > > > > of documentation like TexFAT would be released to public?
> > > > 
> > > > Sure, I'll see if I can get an approval to open it up.
> > > 
> > > Ok!
> > > 
> > > > Can I assume you will be implementing TexFAT support once the spec is
> > > > available?
> > > 
> > > I cannot promise that I would implement something which I do not know
> > > how is working... It depends on how complicated TexFAT is and also how
> > > future exfat support in kernel would look like.
> > > 
> > > But I'm interesting in implementing it.
> > 
> > It looks like the reason this wasn't made public along with the exFAT
> > spec is that TexFAT is pretty much dead - it's old, there are no users
> > we are aware of, and digging it out of it's grave to make it public is
> > actually quite the headache.
> > 
> > Is this something you actually have a need for? an entity that has a
> > requirement for TexFAT?
> 
> Hi!
> 
> I do not have device with requirements for TexFAT. The first reason why
> I wanted to use TexFAT was that it it the only way how to use more FAT
> tables (e.g. secondary for backup) and information for that is missing
> in released exFAT specification. This could bring better data recovery.
> 
> > I'd would rather spend my time elsewhere than digging TexFAT out of it's grave.
> 
> Ok.
> 
> I was hoping that it would be possible to easily use secondary FAT table
> (from TexFAT extension) for redundancy without need to implement full
> TexFAT, which could be also backward compatible with systems which do
> not implement TexFAT extension at all. Similarly like using FAT32 disk
> with two FAT tables is possible also on system which use first FAT
> table.

By the chance, is there any possibility to release TexFAT specification?
Usage of more FAT tables (even for Linux) could help with data recovery.

> > Is there anything else in the exFAT spec that is missing (and someone
> > actually wants/uses)?
> 
> Currently I do not know.

I found one missing thing:

In released exFAT specification is not written how are Unicode code
points above U+FFFF represented in exFAT upcase table. Normally in
UTF-16 are Unicode code points above U+FFFF represented by surrogate
pairs but compression format of exFAT upcase table is not clear how to
do it there.

Are you able to send question about this problem to relevant MS people?

New Linux implementation of exfat which is waiting on mailing list just
do not support Unicode code points above U+FFFF in exFAT upcase table.

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

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Sasha Levin]

On Thu, Feb 13, 2020 at 01:06:56AM +0100, Pali Rohár wrote:
>Hello!
>
>On Thursday 17 October 2019 09:50:08 Pali Rohár wrote:
>> On Wednesday 16 October 2019 16:33:17 Sasha Levin wrote:
>> > On Wed, Oct 16, 2019 at 06:03:49PM +0200, Pali Rohár wrote:
>> > > On Wednesday 16 October 2019 10:31:13 Sasha Levin wrote:
>> > > > On Wed, Oct 16, 2019 at 04:03:53PM +0200, Pali Rohár wrote:
>> > > > > On Friday 30 August 2019 09:56:47 Pali Rohár wrote:
>> > > > > > On Thursday 29 August 2019 19:35:06 Sasha Levin wrote:
>> > > > > > > With regards to missing specs/docs/whatever - our main concern with this
>> > > > > > > release was that we want full interoperability, which is why the spec
>> > > > > > > was made public as-is without modifications from what was used
>> > > > > > > internally. There's no "secret sauce" that Microsoft is hiding here.
>> > > > > >
>> > > > > > Ok, if it was just drop of "current version" of documentation then it
>> > > > > > makes sense.
>> > > > > >
>> > > > > > > How about we give this spec/code time to get soaked and reviewed for a
>> > > > > > > bit, and if folks still feel (in a month or so?) that there are missing
>> > > > > > > bits of information related to exfat, I'll be happy to go back and try
>> > > > > > > to get them out as well.
>> > > > >
>> > > > > Hello Sasha!
>> > > > >
>> > > > > Now one month passed, so do you have some information when missing parts
>> > > > > of documentation like TexFAT would be released to public?
>> > > >
>> > > > Sure, I'll see if I can get an approval to open it up.
>> > >
>> > > Ok!
>> > >
>> > > > Can I assume you will be implementing TexFAT support once the spec is
>> > > > available?
>> > >
>> > > I cannot promise that I would implement something which I do not know
>> > > how is working... It depends on how complicated TexFAT is and also how
>> > > future exfat support in kernel would look like.
>> > >
>> > > But I'm interesting in implementing it.
>> >
>> > It looks like the reason this wasn't made public along with the exFAT
>> > spec is that TexFAT is pretty much dead - it's old, there are no users
>> > we are aware of, and digging it out of it's grave to make it public is
>> > actually quite the headache.
>> >
>> > Is this something you actually have a need for? an entity that has a
>> > requirement for TexFAT?
>>
>> Hi!
>>
>> I do not have device with requirements for TexFAT. The first reason why
>> I wanted to use TexFAT was that it it the only way how to use more FAT
>> tables (e.g. secondary for backup) and information for that is missing
>> in released exFAT specification. This could bring better data recovery.
>>
>> > I'd would rather spend my time elsewhere than digging TexFAT out of it's grave.
>>
>> Ok.
>>
>> I was hoping that it would be possible to easily use secondary FAT table
>> (from TexFAT extension) for redundancy without need to implement full
>> TexFAT, which could be also backward compatible with systems which do
>> not implement TexFAT extension at all. Similarly like using FAT32 disk
>> with two FAT tables is possible also on system which use first FAT
>> table.
>
>By the chance, is there any possibility to release TexFAT specification?
>Usage of more FAT tables (even for Linux) could help with data recovery.

This would be a major pain in the arse to pull off (even more that
releasing exFAT itself) because TexFAT is effectively dead and no one
here cares about it. It's not even the case that there are devices which
are now left unsupported, the whole TexFAT scheme is just dead and gone.

Could I point you to the TexFAT patent instead
(https://patents.google.com/patent/US7613738B2/en)? It describes well
how TexFAT used to work.

>> > Is there anything else in the exFAT spec that is missing (and someone
>> > actually wants/uses)?
>>
>> Currently I do not know.
>
>I found one missing thing:
>
>In released exFAT specification is not written how are Unicode code
>points above U+FFFF represented in exFAT upcase table. Normally in
>UTF-16 are Unicode code points above U+FFFF represented by surrogate
>pairs but compression format of exFAT upcase table is not clear how to
>do it there.
>
>Are you able to send question about this problem to relevant MS people?
>
>New Linux implementation of exfat which is waiting on mailing list just
>do not support Unicode code points above U+FFFF in exFAT upcase table.

Sure, I'll forward this question on. I'll see if I can get someone from
their team who could be available to answer questions such as these in
the future - Microsoft is interested in maintaining compatiblity between
Linux and Windows exFAT implementations.

-- 
Thanks,
Sasha

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Valdis Klētnieks]

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

On Thu, 13 Feb 2020 16:18:47 -0500, Sasha Levin said:

> >> I was hoping that it would be possible to easily use secondary FAT table
> >> (from TexFAT extension) for redundancy without need to implement full
> >> TexFAT, which could be also backward compatible with systems which do
> >> not implement TexFAT extension at all. Similarly like using FAT32 disk
> >> with two FAT tables is possible also on system which use first FAT
> >> table.

OK.. maybe I'm not sufficiently caffeinated, but how do you use 2 FAT tables on
a physical device and expect it to work properly on a system that uses just the
first FAT table, if the device is set to "use second table" when you mount it?
That sounds just too much like the failure modes of running fsck on a mounted
filesystem....

> >By the chance, is there any possibility to release TexFAT specification?
> >Usage of more FAT tables (even for Linux) could help with data recovery.
>
> This would be a major pain in the arse to pull off (even more that
> releasing exFAT itself) because TexFAT is effectively dead and no one
> here cares about it. It's not even the case that there are devices which
> are now left unsupported, the whole TexFAT scheme is just dead and gone.
>
> Could I point you to the TexFAT patent instead
> (https://patents.google.com/patent/US7613738B2/en)? It describes well
> how TexFAT used to work.

I don't think anybody wants the full TexFAT support - but having a backup copy
of the FAT would be nice in some circumstances.

Actually, that raises an question....

What the published spec says:

The File Allocation Table (FAT) region may contain up to two FATs, one in the
First FAT sub-region and another in the Second FAT sub-region. The NumberOfFats
field describes how many FATs this region contains. The valid values for the
NumberOfFats field are 1 and 2. Therefore, the First FAT sub-region always
contains a FAT. If the NumberOfFats field is two, then the Second FAT
sub-region also contains a FAT.

The ActiveFat field of the VolumeFlags field describes which FAT is active.
Only the VolumeFlags field in the Main Boot Sector is current. Implementations
shall treat the FAT which is not active as stale. Use of the inactive FAT and
switching between FATs is implementation specific.

Sasha:  can you find out what the Windows implementation does regarding that
last sentence?  Does it actively use both FAT sub-regions and switch between
them (probably not), or does it read the ActiveFAT value and use that one, or
does Windows just use NumberOfFats == 1?

I'm assuming that the fact the doc also says "NumberOfFats == 2 is only valid
for TexFAT volumes" possibly means "Microsoft thinks that's hardcoded at 1"
given the death of TexFAT.  That would make adding alternate FAT support a
major challenge.

On the other hand, if Windows actually looks at the NumberOfFats value, finds
a 2, and ActiveFAT ==1 (meaning use second table) and says "OK, whatever" and
just uses the second table from there on out, it becomes a lot easier.



[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Pali Rohár]

On Friday 14 February 2020 17:16:18 Valdis Klētnieks wrote:
> On Thu, 13 Feb 2020 16:18:47 -0500, Sasha Levin said:
> 
> > >> I was hoping that it would be possible to easily use secondary FAT table
> > >> (from TexFAT extension) for redundancy without need to implement full
> > >> TexFAT, which could be also backward compatible with systems which do
> > >> not implement TexFAT extension at all. Similarly like using FAT32 disk
> > >> with two FAT tables is possible also on system which use first FAT
> > >> table.
> 
> OK.. maybe I'm not sufficiently caffeinated, but how do you use 2 FAT tables on
> a physical device and expect it to work properly on a system that uses just the
> first FAT table, if the device is set to "use second table" when you mount it?
> That sounds just too much like the failure modes of running fsck on a mounted
> filesystem....

Idea is simple. Expects that we have a clean filesystem in correct
state. We load primary/active/main FAT table (just call it FAT1) and all
changes to filesystem would be done via second non-active FAT table
(FAT2). At unmount or sync or flush buffer times, FAT2 would be copied
back to the FAT1 and filesystem would be back in clean state.

So this should not break support for implementations which use just
FAT1. And if above implementation which use both FAT1 and FAT2 "crash"
during write operations to FAT2 it may be possible to reconstruct and
repair some parts of filesystem from FAT1 (as it would contain previous
state of some filesystem parts).

Via dirty bit can be detected if proper unmount occurred or not, and
fsck implementation could do use this fact and try to do repairing
(possible by asking user what should do).

Of course if implementation use only FAT1 we cannot use FAT2 for
repairing and therefore fsck should really ask user if it should use
FAT2 for repair or not.

If implementation use only FAT1, does not crash and let filesystem in
clean/correct state then there should not be any problem for
implementation which can use both FATs as it reads main state from FAT1.
Therefore these two implementations should be compatible and problem can
happen only if they let filesystem in inconsistent state. (But if they
let it in inconsistent state, then any implementation may have troubles
and fsck is needed).

I hope that it is more clear now...

> > >By the chance, is there any possibility to release TexFAT specification?
> > >Usage of more FAT tables (even for Linux) could help with data recovery.
> >
> > This would be a major pain in the arse to pull off (even more that
> > releasing exFAT itself) because TexFAT is effectively dead and no one
> > here cares about it. It's not even the case that there are devices which
> > are now left unsupported, the whole TexFAT scheme is just dead and gone.
> >
> > Could I point you to the TexFAT patent instead
> > (https://patents.google.com/patent/US7613738B2/en)? It describes well
> > how TexFAT used to work.
> 
> I don't think anybody wants the full TexFAT support - but having a backup copy
> of the FAT would be nice in some circumstances.

Main problem is that we do not know what "full TexFAT support" means as
currently it is secret.

My original question for TexFAT was also because of NumberOfFats set to
2 is according to released exFAT specification possible only for TexFAT
volumes.

And from reading whole exFAT specification I see that better data
recovery can be achieved only by having backup copy of FAT table (and
allocation bitmap), which is limited to (currently undocumented) TexFAT
extension.

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

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Valdis Klētnieks]

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


> Idea is simple. Expects that we have a clean filesystem in correct
> state. We load primary/active/main FAT table (just call it FAT1) and all
> changes to filesystem would be done via second non-active FAT table
> (FAT2). At unmount or sync or flush buffer times, FAT2 would be copied
> back to the FAT1 and filesystem would be back in clean state.

Somehow, scribbling on the non-active table for actual changes sounds like a
bad idea waiting to happen (partly because if you do that and crash, after the
reboot you remount, and it starts up with the now-stale FAT1 because you never
flagged that FAT as stale.

That means that if we started using the secondary FAT, we'd change the
ActiveFAT variable to indicate that.  And if we do that, we need to also set
num_fat to 2 because num_fat 1 and ActiveFAT pointing at the second FAT is
*definitely* bogus.

And that could result in us crashing and leaving the device with a header that
says 'num_fat == 2', ActiveFAT == second, and the dirty bit set - and the user
next uses the  filesystem on a system/device that hard-codes that there's only 1 FAT,
so it blindly goes on its merry way using a FAT that's stale and never realizing it.

And that's actually the same failure mode as in the first paragraph - you start
off using FAT1 because you don't see an indication that it's stale.

[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

exfat upcase table for code points above U+FFFF (Was: Re: [PATCH] staging: exfat: add exfat filesystem code to staging)

[Pali Rohár]

On Thursday 13 February 2020 16:18:47 Sasha Levin wrote:
> On Thu, Feb 13, 2020 at 01:06:56AM +0100, Pali Rohár wrote:
> > In released exFAT specification is not written how are Unicode code
> > points above U+FFFF represented in exFAT upcase table. Normally in
> > UTF-16 are Unicode code points above U+FFFF represented by surrogate
> > pairs but compression format of exFAT upcase table is not clear how to
> > do it there.
> > 
> > Are you able to send question about this problem to relevant MS people?
> > 
> > New Linux implementation of exfat which is waiting on mailing list just
> > do not support Unicode code points above U+FFFF in exFAT upcase table.
> 
> Sure, I'll forward this question on. I'll see if I can get someone from
> their team who could be available to answer questions such as these in
> the future - Microsoft is interested in maintaining compatiblity between
> Linux and Windows exFAT implementations.

Hello Sasha! Have you got any answer from exfat MS team about upcase
table for Unicode code points above U+FFFF?

Re: exfat upcase table for code points above U+FFFF (Was: Re: [PATCH] staging: exfat: add exfat filesystem code to staging)

[Sasha Levin]

On Tue, Apr 21, 2020 at 11:30:45PM +0200, Pali Rohár wrote:
>On Thursday 13 February 2020 16:18:47 Sasha Levin wrote:
>> On Thu, Feb 13, 2020 at 01:06:56AM +0100, Pali Rohár wrote:
>> > In released exFAT specification is not written how are Unicode code
>> > points above U+FFFF represented in exFAT upcase table. Normally in
>> > UTF-16 are Unicode code points above U+FFFF represented by surrogate
>> > pairs but compression format of exFAT upcase table is not clear how to
>> > do it there.
>> >
>> > Are you able to send question about this problem to relevant MS people?
>> >
>> > New Linux implementation of exfat which is waiting on mailing list just
>> > do not support Unicode code points above U+FFFF in exFAT upcase table.
>>
>> Sure, I'll forward this question on. I'll see if I can get someone from
>> their team who could be available to answer questions such as these in
>> the future - Microsoft is interested in maintaining compatiblity between
>> Linux and Windows exFAT implementations.
>
>Hello Sasha! Have you got any answer from exfat MS team about upcase
>table for Unicode code points above U+FFFF?

Sorry for taking so long. This is my understanding from the Windows
folks: Windows filesystems just don't support variable encoding length,
and expect UCS-2 strings.

-- 
Thanks,
Sasha

Re: exfat upcase table for code points above U+FFFF (Was: Re: [PATCH] staging: exfat: add exfat filesystem code to staging)

[Pali Rohár]

On Monday 27 April 2020 11:49:13 Sasha Levin wrote:
> On Tue, Apr 21, 2020 at 11:30:45PM +0200, Pali Rohár wrote:
> > On Thursday 13 February 2020 16:18:47 Sasha Levin wrote:
> > > On Thu, Feb 13, 2020 at 01:06:56AM +0100, Pali Rohár wrote:
> > > > In released exFAT specification is not written how are Unicode code
> > > > points above U+FFFF represented in exFAT upcase table. Normally in
> > > > UTF-16 are Unicode code points above U+FFFF represented by surrogate
> > > > pairs but compression format of exFAT upcase table is not clear how to
> > > > do it there.
> > > >
> > > > Are you able to send question about this problem to relevant MS people?
> > > >
> > > > New Linux implementation of exfat which is waiting on mailing list just
> > > > do not support Unicode code points above U+FFFF in exFAT upcase table.
> > > 
> > > Sure, I'll forward this question on. I'll see if I can get someone from
> > > their team who could be available to answer questions such as these in
> > > the future - Microsoft is interested in maintaining compatiblity between
> > > Linux and Windows exFAT implementations.
> > 
> > Hello Sasha! Have you got any answer from exfat MS team about upcase
> > table for Unicode code points above U+FFFF?
> 
> Sorry for taking so long. This is my understanding from the Windows
> folks: Windows filesystems just don't support variable encoding length,
> and expect UCS-2 strings.

Ok, so should I understand your answer as exFAT upcase table does not
support representing Unicode code points above U+FFFF and therefore
exFAT implementation should expect that toupper(u) = u and tolower(u) = u
for any Unicode code point u in range [U+10000, U+10FFFF]? This is how
current exfat linux driver behave.