Re: A few thoughts on hfsplus dev (Re: [PATCH v2] hfsplus: add journal replay)

Re: A few thoughts on hfsplus dev (Re: [PATCH v2] hfsplus: add journal replay)

[Hin-Tak Leung]

Hi Sergei,

------------------------------
On Tue, Mar 25, 2014 1:37 AM GMT Sergei Antonov wrote:

>Let me inject a remark here.
>A reader of my discussion with Vyacheslav here may get an impression that my code cares less about data safety. This impression is false. To make a long story short: replaying+removing transactions one by one is as safe as replaying them all at once and then removing them alltogether. Maybe it is not so for other journal formats, for HFS+ journal it is.
>

I think a reader of your commit message might get the impression that you are more
concerned about performance/elegance/cleverness than correctness.

"Doing A is as safe as doing B" - you still have to pick one way or another.
Or supply a patch to do both, to demonstrate switching between the two.

>
>Sigh. You have been misled. That's probably because claims like "you do not follow this excerpt from the spec" are more impressive and easier to remember than my following explanations why I am not.
>

You are probably assuming that you will always be around to explain to future persons
who might want to understand this code; and you are assuming that you are the 
only person to be working on it? How do you feel about a future person
making a commit to revert/correct your work, for example?

>> - I have a more re-produce'able recipe of making disk images with strange 
>> location of 2nd volume header using hdiutil (Apple's loopback mounting, etc disk image
>> manipulation tool). At some point we should re-visit this issue.
>
>Well, present your recepie. But! There is only one proper placement for this header. All other placements are improper, and the spec even explicitly gives one example of an improper placement. I imagine no ways to get it wrong. I see no need for any elaborate code for finding the 2nd header.
>

Apple owns the spec, and owns the rights of interpretation and mis-interpretation of it,
and they are not bound by it, and they can freely deviate and change their minds, etc. it
is not an iso spec, it was not submitted to external bodies for standardization - it 
is just provided as is.

You can say Appple's tool is wrong - but a more pragmatic way of seeing it, is that
they don't have to keep the spec up to date or correct; keeping the spec up to date
is going to cost somebody's time to do so; there is no business incentive to
spend the engineering hours to keep the spec up to date. Their tools/implementations
in software is more authoritative than the words in the spec.

hdiutil is Apple proprietary software. source code is not available.

Hin-Tak

Re: A few thoughts on hfsplus dev (Re: [PATCH v2] hfsplus: add journal replay)

[Sergei Antonov]

On 5 April 2014 03:02, Hin-Tak Leung <htl10@users.sourceforge.net> wrote:
>>> - I have a more re-produce'able recipe of making disk images with strange
>>> location of 2nd volume header using hdiutil (Apple's loopback mounting, etc disk image
>>> manipulation tool). At some point we should re-visit this issue.
>>
>>Well, present your recepie. But! There is only one proper placement for this header. All other placements are improper, and the spec even explicitly gives one example of an improper placement. I imagine no ways to get it wrong. I see no need for any elaborate code for finding the 2nd header.
>>
>
> Apple owns the spec, and owns the rights of interpretation and mis-interpretation of it,
> and they are not bound by it, and they can freely deviate and change their minds, etc. it
> is not an iso spec, it was not submitted to external bodies for standardization - it
> is just provided as is.
>
> You can say Appple's tool is wrong - but a more pragmatic way of seeing it, is that
> they don't have to keep the spec up to date or correct; keeping the spec up to date
> is going to cost somebody's time to do so; there is no business incentive to
> spend the engineering hours to keep the spec up to date. Their tools/implementations
> in software is more authoritative than the words in the spec.
>
> hdiutil is Apple proprietary software. source code is not available.

So what is the recipe? I'll make a patch if there really is a bug in the driver.

Re: A few thoughts on hfsplus dev (Re: [PATCH v2] hfsplus: add journal replay)

[Sergei Antonov]

On 5 April 2014 01:35, Hin-Tak Leung <htl10@users.sourceforge.net> wrote:
> Hi Sergei,
>
> ------------------------------
> On Fri, Mar 28, 2014 1:42 PM GMT Sergei Antonov wrote:
>
>>> I am ready for further convincing efforts.
>
> As I said, I am happy to review your work, and I hope you and Vyacheslav can work together.
> However, I wish to point out that Vyacheslav has a track record going back for a few years
> of contributing to the hfs+ driver, and I believe there was a patch submitted a few months
> ago to make him the maintainer. In the hierarchy of linux kernel development, informal
> as it is, he is still probably one of the people that you need to convince. The way you are
> going about the matter, it is not contributing to getting your ideas accepted.

This saddens me. I thought I just asked questions.

>>> Though now that Vyacheslav has stopped replying in the thread there is no progress. Two compromises have been suggested by me in the thread, I'll recap them here for you or anyone who wasn't following.
>>> 1. Since collecting all transactions requires more memory (up to 12 MB in my tests, but theoretically more), I suggested a threshold. Let's say it will be 1 MB. Once collected data reaches one meg, flush it, free the buffers and go on collecting.
>>> 2. A very simple suggestions that makes my logic closer to Vyacheslav's one-by-one transaction processing approach. In case we encounter a corrupted transaction, replay (good) transactions collected so far. Corrupted transaction is a rare thing (caused by cosmic rays!), but partial replay looks sane and not a big change to the code.
>>>
>
> ...
>
>>Could you clarify "transaction based"? Seriously. Because since there
>>are transactions in HFS+ journal, all replay implementations are
>>transaction-based in a way.
>
> Transactions are what they are. The journal is an ordered list of transactions.
> Each of them has a checksum for integrity, and is treated as one unit of
> change. Theorectically, you should apply one after the other in the correct order,
> until the list is exhausted, or until you encounter the first of one such unit
> that does not pass sanity checks (the checksum, and other out-of-bound
> conditions, etc).
>
> How you go about it, or buffering a number of transactions for performance
> reasons, is up to you (or the implementor), *up to a certain point*. I
> suggest you supply a patch on top of Vyacheslav's patch set to
> demonstrate how you want to go about it. It is all rather vague and
> not constructive going around in circle about words and meaning of
> words. So I suggest that you show us the code to do it,
> how you would like to modify Vyacheslav's to work in your way.
>
> Be really careful about saying things like "all replay implementations". There is
> no such thing as "all" replay implementations: there is an official specification
> - TN1150, and there is an official implementation, that inside xnu and diskdev_cmds
> from Apple. Anything else is about you making a Sergei's file system that's
> not quite HFS+.
>
> If you read/write the journal qualitatively different from how Apple does it,
> you are by definition wrong, even if your implemention of journalling
> does it faster and "cleverer" than Apple's.

Apple also first collects data from all transactions and only after
that writes data to disk.
See http://www.opensource.apple.com/source/xnu/xnu-2422.1.72/bsd/vfs/vfs_journal.c
(search for "coalesce").

Re: A few thoughts on hfsplus dev (Re: [PATCH v2] hfsplus: add journal replay)

[Hin-Tak Leung]

Hi Sergei,

------------------------------
On Fri, Mar 28, 2014 1:42 PM GMT Sergei Antonov wrote:

>> I am ready for further convincing efforts.

As I said, I am happy to review your work, and I hope you and Vyacheslav can work together.
However, I wish to point out that Vyacheslav has a track record going back for a few years
of contributing to the hfs+ driver, and I believe there was a patch submitted a few months
ago to make him the maintainer. In the hierarchy of linux kernel development, informal
as it is, he is still probably one of the people that you need to convince. The way you are
going about the matter, it is not contributing to getting your ideas accepted.

>> Though now that Vyacheslav has stopped replying in the thread there is no progress. Two compromises have been suggested by me in the thread, I'll recap them here for you or anyone who wasn't following.
>> 1. Since collecting all transactions requires more memory (up to 12 MB in my tests, but theoretically more), I suggested a threshold. Let's say it will be 1 MB. Once collected data reaches one meg, flush it, free the buffers and go on collecting.
>> 2. A very simple suggestions that makes my logic closer to Vyacheslav's one-by-one transaction processing approach. In case we encounter a corrupted transaction, replay (good) transactions collected so far. Corrupted transaction is a rare thing (caused by cosmic rays!), but partial replay looks sane and not a big change to the code.
>>

...

>Could you clarify "transaction based"? Seriously. Because since there
>are transactions in HFS+ journal, all replay implementations are
>transaction-based in a way.

Transactions are what they are. The journal is an ordered list of transactions.
Each of them has a checksum for integrity, and is treated as one unit of
change. Theorectically, you should apply one after the other in the correct order,
until the list is exhausted, or until you encounter the first of one such unit
that does not pass sanity checks (the checksum, and other out-of-bound
conditions, etc).

How you go about it, or buffering a number of transactions for performance
reasons, is up to you (or the implementor), *up to a certain point*. I
suggest you supply a patch on top of Vyacheslav's patch set to
demonstrate how you want to go about it. It is all rather vague and
not constructive going around in circle about words and meaning of
words. So I suggest that you show us the code to do it,
how you would like to modify Vyacheslav's to work in your way.

Be really careful about saying things like "all replay implementations". There is
no such thing as "all" replay implementations: there is an official specification
- TN1150, and there is an official implementation, that inside xnu and diskdev_cmds
from Apple. Anything else is about you making a Sergei's file system that's
not quite HFS+.

If you read/write the journal qualitatively different from how Apple does it,
you are by definition wrong, even if your implemention of journalling
does it faster and "cleverer" than Apple's.

Hin-Tak

 

Re: A few thoughts on hfsplus dev (Re: [PATCH v2] hfsplus: add journal replay)

[Sergei Antonov]

On 28 March 2014 16:09, Vyacheslav Dubeyko <slava@dubeyko.com> wrote:
> On Fri, 2014-03-28 at 14:42 +0100, Sergei Antonov wrote:
>
>> >
>> > I have such principal points:
>> > (1) On-disk layout declarations should live in peace with Technical Note
>> > TN1150;
>> > (2) Journal replay should be transaction based.
>>
>> Could you clarify "transaction based"? Seriously. Because since there
>> are transactions in HFS+ journal, all replay implementations are
>> transaction-based in a way.
>
> I suppose that I described my vision very clearly in previous e-mails.
> Sorry, I don't see any necessity to repeat the same things.
>
> If you want to ask something, please, ask concrete question. Because,
> from my point of view, we had deep discussion earlier. And our
> discussion hasn't goal to elaborate theoretical principles of
> transaction approach. So, your current question is not concrete for me.
>
> If you want to suggest something, please, do it. But, please, suggest
> something new and don't suggest the same things.
>
> Thanks,
> Vyacheslav Dubeyko.

My concrete question is what is your definition of the term? A
definition, you know, an unambiguous piece of text describing the
term.

Of course you wrote about it before. Let's state (if not reconcile)
our disagreements on the issue in this thread. Of the two principal
points you mentioned, one is clear to me, the other is a little too
vaguely formulated.

Re: A few thoughts on hfsplus dev (Re: [PATCH v2] hfsplus: add journal replay)

[Vyacheslav Dubeyko]

On Fri, 2014-03-28 at 14:42 +0100, Sergei Antonov wrote:

> >
> > I have such principal points:
> > (1) On-disk layout declarations should live in peace with Technical Note
> > TN1150;
> > (2) Journal replay should be transaction based.
> 
> Could you clarify "transaction based"? Seriously. Because since there
> are transactions in HFS+ journal, all replay implementations are
> transaction-based in a way.

I suppose that I described my vision very clearly in previous e-mails.
Sorry, I don't see any necessity to repeat the same things.

If you want to ask something, please, ask concrete question. Because,
from my point of view, we had deep discussion earlier. And our
discussion hasn't goal to elaborate theoretical principles of
transaction approach. So, your current question is not concrete for me.

If you want to suggest something, please, do it. But, please, suggest
something new and don't suggest the same things.

Thanks,
Vyacheslav Dubeyko.

Re: A few thoughts on hfsplus dev (Re: [PATCH v2] hfsplus: add journal replay)

[Sergei Antonov]

On 25 March 2014 07:30, Vyacheslav Dubeyko <slava@dubeyko.com> wrote:
> On Tue, 2014-03-25 at 02:37 +0100, Sergei Antonov wrote:
>
>> > I would also have
>> > preferred that you point out exactly where Vyacheslav's work went wrong, and supply
>> > a patch on top, instead of starting your own. If you want credit for it, I hope you can
>> > arrange with Vyacheslav to swap the order of signed-off, for one or more of the merged patches,
>> > for example. In any case, while I am happy to see your work, and am willing to review it,
>> > you have not convinced me to try your work out yet. So.
>>
>> I am ready for further convincing efforts.
>> Though now that Vyacheslav has stopped replying in the thread there is no progress. Two compromises have been suggested by me in the thread, I'll recap them here for you or anyone who wasn't following.
>> 1. Since collecting all transactions requires more memory (up to 12 MB in my tests, but theoretically more), I suggested a threshold. Let's say it will be 1 MB. Once collected data reaches one meg, flush it, free the buffers and go on collecting.
>> 2. A very simple suggestions that makes my logic closer to Vyacheslav's one-by-one transaction processing approach. In case we encounter a corrupted transaction, replay (good) transactions collected so far. Corrupted transaction is a rare thing (caused by cosmic rays!), but partial replay looks sane and not a big change to the code.
>>
>
> It doesn't make sense for me to repeat the same statements again and
> again.
>
> I have such principal points:
> (1) On-disk layout declarations should live in peace with Technical Note
> TN1150;
> (2) Journal replay should be transaction based.

Could you clarify "transaction based"? Seriously. Because since there
are transactions in HFS+ journal, all replay implementations are
transaction-based in a way.

Re: A few thoughts on hfsplus dev (Re: [PATCH v2] hfsplus: add journal replay)

[Vyacheslav Dubeyko]

On Tue, 2014-03-25 at 02:37 +0100, Sergei Antonov wrote:

> > I would also have
> > preferred that you point out exactly where Vyacheslav's work went wrong, and supply
> > a patch on top, instead of starting your own. If you want credit for it, I hope you can
> > arrange with Vyacheslav to swap the order of signed-off, for one or more of the merged patches,
> > for example. In any case, while I am happy to see your work, and am willing to review it,
> > you have not convinced me to try your work out yet. So.
> 
> I am ready for further convincing efforts.
> Though now that Vyacheslav has stopped replying in the thread there is no progress. Two compromises have been suggested by me in the thread, I'll recap them here for you or anyone who wasn't following.
> 1. Since collecting all transactions requires more memory (up to 12 MB in my tests, but theoretically more), I suggested a threshold. Let's say it will be 1 MB. Once collected data reaches one meg, flush it, free the buffers and go on collecting.
> 2. A very simple suggestions that makes my logic closer to Vyacheslav's one-by-one transaction processing approach. In case we encounter a corrupted transaction, replay (good) transactions collected so far. Corrupted transaction is a rare thing (caused by cosmic rays!), but partial replay looks sane and not a big change to the code.
> 

It doesn't make sense for me to repeat the same statements again and
again.

I have such principal points:
(1) On-disk layout declarations should live in peace with Technical Note
TN1150;
(2) Journal replay should be transaction based.

I'll never change my opinion about it.

Thanks,
Vyacheslav Dubeyko.

Re: A few thoughts on hfsplus dev (Re: [PATCH v2] hfsplus: add journal replay)

[Sergei Antonov]

> Am 24.03.2014 um 00:15 schrieb Hin-Tak Leung <htl10@users.sourceforge.net>:
> 
> Hi Vyacheslav and Sergei,
> 
> I was hoping not to feed into the issues. I believe the bottom line is that, there are
> a few interesting and challenging issues with HFS+ support, and there are simply not
> enough people who are both knowledgeable and motivated to improve on it. So we
> should try to work together. Does it sound reasonable?

Yes, it does.

> 
> Sergei: - I appreciate that you are the first to point out the endian issue with the journal,
> and been working on other aspects, such as the foldercount issue. I would be happy
> to see more work from you, and continue to be willing to have a look and review, where
> time permits. However, I do see a few problems with the manner in which you approach
> this work. From my personal point of view (and I cannot speak for others), file system
> work is data-safety first.

Let me inject a remark here.
A reader of my discussion with Vyacheslav here may get an impression that my code cares less about data safety. This impression is false. To make a long story short: replaying+removing transactions one by one is as safe as replaying them all at once and then removing them alltogether. Maybe it is not so for other journal formats, for HFS+ journal it is.

> So while Vyacheslav's work might be on the verbose side,
> it is re-assuring to read, maybe repeatedly, about the details, than skipping on boring
> details. I don't think length of code should be a criteria, and certainly not "elegance",
> or cleverness. Here is a widely quoted saying from Brian Kernighan:
> "Debugging is twice as hard as writing the code in the first place. Therefore, if you write the
> code as cleverly as possible, you are, by definition, not smart enough to debug it."
> 
> - Also, if you deviate from how HFS+ works either at the specification level, the behavior level
> or the code level, you are just implementing your own journalling work.

Sigh. You have been misled. That's probably because claims like "you do not follow this excerpt from the spec" are more impressive and easier to remember than my following explanations why I am not.

> I would also have
> preferred that you point out exactly where Vyacheslav's work went wrong, and supply
> a patch on top, instead of starting your own. If you want credit for it, I hope you can
> arrange with Vyacheslav to swap the order of signed-off, for one or more of the merged patches,
> for example. In any case, while I am happy to see your work, and am willing to review it,
> you have not convinced me to try your work out yet. So.

I am ready for further convincing efforts.
Though now that Vyacheslav has stopped replying in the thread there is no progress. Two compromises have been suggested by me in the thread, I'll recap them here for you or anyone who wasn't following.
1. Since collecting all transactions requires more memory (up to 12 MB in my tests, but theoretically more), I suggested a threshold. Let's say it will be 1 MB. Once collected data reaches one meg, flush it, free the buffers and go on collecting.
2. A very simple suggestions that makes my logic closer to Vyacheslav's one-by-one transaction processing approach. In case we encounter a corrupted transaction, replay (good) transactions collected so far. Corrupted transaction is a rare thing (caused by cosmic rays!), but partial replay looks sane and not a big change to the code.

> Vyacheslav: Thanks for the continuous good work. While your adherence to TN1150 is
> good, I have noticed a few times that you have not paid attention to Apple's
> implementation of it - xnu's kernel code and diskdev_cmds (apple's mkfs/fsck). It is apparent
> in some of the discussion between you and Sergei that he has read some of those but
> you haven't. This is shown in the discussion about meaning and naming of variables,
> and interpretation of reserved fields. About TN1150, I think there is one instance about
> journalling support (a block count?) where it is ambiguous and contradicctory about a +1 offset.
> Anyway, I think reference to the actual implementations are welcomed in resolving
> these ambiguities and clarifying issues.
> 
> To the both of you, looking forward, and a few technical issues:
> 
> - I think fsck.hfsplus must manipulate the journal - fs with a non-empty journal is by definition
> unclean; but fsck may or may not be doing journal replay. It probably does something else,
> like just zero'ing the journal and fixes inconsistency in the rest. At some point we should find
> out what fsck does when it fixes an unclean fs with non-empty journal.

Easy. There is no non-empty journal by the time there is something to fix. It is replayed or discarded before the check.

> - I have a more re-produce'able recipe of making disk images with strange 
> location of 2nd volume header using hdiutil (Apple's loopback mounting, etc disk image
> manipulation tool). At some point we should re-visit this issue.

Well, present your recepie. But! There is only one proper placement for this header. All other placements are improper, and the spec even explicitly gives one example of an improper placement. I imagine no ways to get it wrong. I see no need for any elaborate code for finding the 2nd header.

> - on the many-year-old issue of the driver getting confused just running "du" recursing
> down large directories, I have managed to make it happen under Ftrace the Linux
> kernel internal tracer, and even with code mod's, but the resulting log is too
> large and events are being dropped. So I need to do some filtering to get
> events down to a fully-recordable level. I.e. it would help to confirm issues
> if I know where to look...  
> 
> Hin-Tak
> 

Re: A few thoughts on hfsplus dev (Re: [PATCH v2] hfsplus: add journal replay)

[Vyacheslav Dubeyko]

Hi Hin-Tak

On Sun, 2014-03-23 at 23:15 +0000, Hin-Tak Leung wrote:
> Hi Vyacheslav and Sergei,
> 
> I was hoping not to feed into the issues. I believe the bottom line is that, there are
> a few interesting and challenging issues with HFS+ support, and there are simply not
> enough people who are both knowledgeable and motivated to improve on it. So we
> should try to work together. Does it sound reasonable?
> 

Thank you for suggestions and sharing your opinion.

Sorry, currently, I've decided to finish my implementation activity for
HFS+. I will work for another file system. It is not emotions. I really
haven't time for useless activity without results. I have many working
duties and it is really hard to find time for spending it in useless
way.

I am open for discussion but I don't see discussion for this topic. I
see only blaming and stupid declaration about superiority of one patch
with regard of another. If I suggest really bad patches then I should
stop to do it. Because I really haven't time for useless activity and
meaningless discussions. But I will ever disagree with Sergei's patch in
the current state because I think that my remarks are reasonable.

So, I've decided to spend my time on implementation functionality for
another file systems because there are many interesting directions for
it. If I encounter likewise situation in the future then I will simply
stop my open-source activity, completely. Because there are many
interesting things in real life.

Thanks,
Vyacheslav Dubeyko.

A few thoughts on hfsplus dev (Re: [PATCH v2] hfsplus: add journal replay)

[Hin-Tak Leung]

Hi Vyacheslav and Sergei,

I was hoping not to feed into the issues. I believe the bottom line is that, there are
a few interesting and challenging issues with HFS+ support, and there are simply not
enough people who are both knowledgeable and motivated to improve on it. So we
should try to work together. Does it sound reasonable?

Sergei: - I appreciate that you are the first to point out the endian issue with the journal,
and been working on other aspects, such as the foldercount issue. I would be happy
to see more work from you, and continue to be willing to have a look and review, where
time permits. However, I do see a few problems with the manner in which you approach
this work. From my personal point of view (and I cannot speak for others), file system
work is data-safety first. So while Vyacheslav's work might be on the verbose side,
it is re-assuring to read, maybe repeatedly, about the details, than skipping on boring
details. I don't think length of code should be a criteria, and certainly not "elegance",
or cleverness. Here is a widely quoted saying from Brian Kernighan:
"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the
code as cleverly as possible, you are, by definition, not smart enough to debug it."

- Also, if you deviate from how HFS+ works either at the specification level, the behavior level
or the code level, you are just implementing your own journalling work. I would also have
preferred that you point out exactly where Vyacheslav's work went wrong, and supply
a patch on top, instead of starting your own. If you want credit for it, I hope you can
arrange with Vyacheslav to swap the order of signed-off, for one or more of the merged patches,
for example. In any case, while I am happy to see your work, and am willing to review it,
you have not convinced me to try your work out yet. So.

Vyacheslav: Thanks for the continuous good work. While your adherence to TN1150 is
good, I have noticed a few times that you have not paid attention to Apple's
implementation of it - xnu's kernel code and diskdev_cmds (apple's mkfs/fsck). It is apparent
in some of the discussion between you and Sergei that he has read some of those but
you haven't. This is shown in the discussion about meaning and naming of variables,
and interpretation of reserved fields. About TN1150, I think there is one instance about
journalling support (a block count?) where it is ambiguous and contradicctory about a +1 offset.
Anyway, I think reference to the actual implementations are welcomed in resolving
these ambiguities and clarifying issues.

To the both of you, looking forward, and a few technical issues:

- I think fsck.hfsplus must manipulate the journal - fs with a non-empty journal is by definition
unclean; but fsck may or may not be doing journal replay. It probably does something else,
like just zero'ing the journal and fixes inconsistency in the rest. At some point we should find
out what fsck does when it fixes an unclean fs with non-empty journal.

- I have a more re-produce'able recipe of making disk images with strange 
location of 2nd volume header using hdiutil (Apple's loopback mounting, etc disk image
manipulation tool). At some point we should re-visit this issue.

- on the many-year-old issue of the driver getting confused just running "du" recursing
down large directories, I have managed to make it happen under Ftrace the Linux
kernel internal tracer, and even with code mod's, but the resulting log is too
large and events are being dropped. So I need to do some filtering to get
events down to a fully-recordable level. I.e. it would help to confirm issues
if I know where to look...  

Hin-Tak