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+ represented in exFAT upcase table. Normally in
> > > > UTF-16 are Unicode code points above U+ 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+ 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+?
> 
> 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+ and therefore
exFAT implementation should expect that toupper(u) = u and tolower(u) = u
for any Unicode code point u in range [U+1, U+10]? This is how
current exfat linux driver behave.

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.ro...@gmail.com

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.ro...@gmail.com

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

[Valdis Klētnieks]

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)



pgp36Nhqfju4G.pgp
Description: PGP signature

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

[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

[Pali Rohár]

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.ro...@gmail.com


signature.asc
Description: PGP signature

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

[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

[Valdis Klētnieks]

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



pgpuzy44yR0z4.pgp
Description: PGP signature

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

[Pali Rohár]

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.ro...@gmail.com


signature.asc
Description: PGP signature

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]

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.ro...@gmail.com

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
da...@fromorbit.com

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

[Valdis Klētnieks]

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.




pgpEuWl9W6Eg_.pgp
Description: PGP signature

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

[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
da...@fromorbit.com

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

[Pali Rohár]

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.ro...@gmail.com


signature.asc
Description: PGP signature

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

[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

[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

[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: [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

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

[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.ro...@gmail.com

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.ro...@gmail.com

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

[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

[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

[Valdis Klētnieks]

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


pgp7mhV7YWTlo.pgp
Description: PGP signature

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:
> From: Valdis Klētnieks 
> 
> 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 
> Signed-off-by: Sasha Levin 
> Signed-off-by: Greg Kroah-Hartman 

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.ro...@gmail.com


signature.asc
Description: PGP signature

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

[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-gaoxian...@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 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 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-gaoxian...@huawei.com/

Does it look good to you?

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

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

[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

[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

[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]

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

[Markus Elfring]

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

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

Regards,
Markus

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.ro...@gmail.com

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

[Valdis Klētnieks]

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


pgpzRQ8Fc3Jm7.pgp
Description: PGP signature

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.ro...@gmail.com

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

[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

[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

[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

[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

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

[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.gk2...@zeniv.linux.org.uk/
[2] https://lore.kernel.org/r/20190720224955.gd17...@zeniv.linux.org.uk/

Thanks,
Gao Xiang

> 
> Thanks,
> Gao Xiang
> 

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

[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

[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 06:08:17PM +0200, Greg Kroah-Hartman wrote:
> From: Valdis Klētnieks 
> 
> 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 
> Signed-off-by: Sasha Levin 
> Signed-off-by: Greg Kroah-Hartman 
> ---
>  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 
> +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