Re: [PATCH] hfsplus: release bnode pages after use, not before

[Sergei Antonov <saproj@gmail.com>]

On 19 June 2015 at 00:16, Hin-Tak Leung <hintak.leung@gmail.com> wrote:
> On 18 June 2015 at 21:51, Sergei Antonov <saproj@gmail.com> wrote:
>> On 18 June 2015 at 19:16, Hin-Tak Leung <hintak.leung@gmail.com> wrote:
>>> On 18 June 2015 at 17:19, Sergei Antonov <saproj@gmail.com> wrote:
>>>> On 18 June 2015 at 17:37, Hin-Tak Leung <hintak.leung@gmail.com> wrote:
>>>>> On 18 June 2015 at 13:58, Sergei Antonov <saproj@gmail.com> wrote:
>>>>>> On 18 June 2015 at 04:51, Hin-Tak Leung <hintak.leung@gmail.com> wrote:
>>>>>>> Hi Andrew and everybody else,
>>>>>>>
>>>>>>> I looked through the pre-git history and seem to have found the reason of how
>>>>>>> the current code had come to be, and why Sergei's fix seems to involve
>>>>>>> re-enable "#if 0"'ed out code. It is this - I think the code was wrong on entry,
>>>>>>> in that the first !REF_PAGES should be REF_PAGES. Then the "remove old
>>>>>>> debug code"
>>>>>>> commit below did not remove the right stuff.
>>>>>>>
>>>>>>> Looking at the old code, I think REF_PAGES may have started out being
>>>>>>> 1 (i.e. pages are released right after create, then get/put when needed, no
>>>>>>> need to release on bnode_free),
>>>>>>> then the code was modified for efficiency so that pages are not released
>>>>>>> on bnode_create and not put/get at bnode_put/get but release at bnode_free.
>>>>>>> But it was still largely working in the REF_PAGE=1 mode (and with the commented
>>>>>>> out release at bnode_free, which is supposed to be spanned by a !REF_PAGE).
>>>>>>> Then it was flipped over, and seems to work, and things were forgotten.
>>>>>>>
>>>>>>> I think the release at bnode_free was commented out because the person who
>>>>>>> wrote it wasn't sure - I am not sure either, since it seems like there might be
>>>>>>> other/more places that one might need to release the pages than just
>>>>>>> at bnode_free().
>>>>>>>
>>>>>>> Does this train of thought make sense?
>>>>>>
>>>>>> No. The occurrence of !REF_PAGES was not a typo, it seems to me. Well,
>>>>>> at first it was my thought too. But later I considered what the logic
>>>>>> of the author could have been with REF_PAGES=0 and REF_PAGES=1 and
>>>>>> came to a conclusion that REF_PAGES=1 was a safe approach and
>>>>>> REF_PAGES=0 was an experimental unsafe release-early approach, so
>>>>>> !REF_PAGES was what he really meant.
>>>>>>
>>>>>
>>>>> You are wrong, it seems. I found even earlier versions of the code,
>>>>> with history, at
>>>>> http://sourceforge.net/projects/linux-hfsplus/ .
>>>>> The earlier version of the code
>>>>> (http://sourceforge.net/projects/linux-hfsplus/files/Linux%202.4.x%20patch/hfsplus%200.1/)
>>>>> does get_page and put_page at bnode_get and bnode_put, while the later version
>>>>> (http://sourceforge.net/projects/linux-hfsplus/files/Linux%202.4.x%20patch/hfsplus%200.2/
>>>>> )
>>>>> have those removed, dated 2001 december and 2002 june.
>>>>
>>>> Interesting. I only looked at historical linux git.
>>>>
>>>>> The change log says so:
>>>>>
>>>>> Date:   Thu Jun 6 09:45:14 2002 +0000
>>>>>
>>>>>     Use buffer cache instead of page cache in bnode.c. Cache inode extents.
>>>>>
>>>>> The change also removed any page_cache_release() . It would seem that
>>>>> the additional REF_PAGES
>>>>> conditional was a fork/rewrite/clean-up which re-uses the earlier
>>>>> development logic, and eventually took over.
>>>>>
>>>>> The release-early code came first, not after. That makes sense, as
>>>>> releasing early
>>>>
>>>> We may be talking about different things. I did not write what code
>>>> came earlier and what after. Anyway, this becomes to difficult to
>>>> follow. My point was that in the variant without refs, calling
>>>> page_cache_release() right after to read_cache_page() was exactly what
>>>> the author wanted (although it is a bug).
>>>>
>>>> Again, we are probably arguing about different things. And I'm a
>>>> little less zealous than you to do this code archeology.
>>>>
>>>
>>> I would really prefer that you don't use personal descriptions such as "zealous"
>>> as well as making the kind of comments such as
>>> "The result of the test (which I'm sure will show the bug is gone) will
>>> be by far more valuable contribution than your other messages in the
>>> thread.". Those are not helpful.
>>>
>>> In my first reply I have already stated my reasoning: your fix consists
>>> mainly of removing an "if 0" and re-enabling seemingly dead code. So the
>>> dead code was written for a purpose which is now lost and forgotten.
>>
>> Of course. And it is not hard to guess that purpose quite reliably :).
>> The code used to release pages correctly, i. e. when freeing the bnode
>> structure.
>>
>>> And here is that purpose - early development of the code was done
>>> with getting and putting pages within each routine, for safeness, and migrating
>>> towards caching pages for a longer life cycle in 2001-2002. When the
>>> code was cleaned
>>> up and re-worked before putting into the kernel in 2005, this
>>> development process
>>> was repeated but with a REF_PAGES conditional and some additional code
>>> which was // commented out.
>>>
>>> The migration to caching pages
>>> would have been more or less correct, if REF_PAGES was flipped
>>> and the // commented out code was enabled.  Except there was a mistake
>>> in one of the conditionals
>>
>> By mistake you mean !REF_PAGES in __hfs_bnode_create()? But it is not
>> a mistake under the conditions you provided above. With REF_PAGES
>> flipped to 1 and the // piece uncommented you don't want to release in
>> __hfs_bnode_create() because releasing is done in hfs_bnode_free().
>>
>
> Yes, that's what I am saying. I believe the development of the code happens
> as follows:
>
> - bnode_get() does get_page(), bnode_put() does put_page(), and the rest
> of the routines - i.e. bnode_create() look up the pages but
> immediately release them. bnode_free()
> does nothing about pages.
>
> - then it was desired to keep the pages longer without the repeated
> get_pages and put_pages.
> so those are "#if REF_PAGES" out. And we no longer get and put pages on demand
> and should also stop releasing them after looking up at bnode_create() also.
> (the bnode_create() is a poor name for what it does, but never mind). New code
> is added to release at bnode_free(). The new code should have been spanned
> by the opposite, i.e. "#if !REF_PAGES", but was inserted as place
> holder with //.
>
> The rest of the story I have already tried to explain a couple of
> times. The switch
> from REF_PAGES=1 to =0 (i.e. from get_pages/put_pages per
> bnode_get/bnode_put routines,
> to keeping pages around longer between bnode_create/bnode_free) did
> not quite work correctly.
>
> For the others who want to see the version of the code with those REF_PAGES
> and want to weigh in on this,
> I cannot find a web interface for the repo, but it is the commit below
> in git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

A web link: http://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/log/fs/hfsplus/bnode.c

> commit 91556682e0bf004d98a529bf829d339abb98bbbd
> Author: Andrew Morton <akpm@osdl.org>
> Date:   Wed Feb 25 16:17:48 2004 -0800
>
>     [PATCH] HFS+ support
>
>     From: Roman Zippel <zippel@linux-m68k.org>
>
>     This driver adds full read/write support for HFS+ and is based on the
>     readonly driver by Brad Broyer.
>
>     Thanks to Ethan Benson <erbenson@alaska.net> for a number of patches to
>     make the driver more compliant with the spec.
>
> I would also repeat that earlier code and change history was at
> http://sourceforge.net/projects/linux-hfsplus/, and it showed the
> changes between
> 2001 dec to 2002 june
>
> from get_page/put_page per bnode_get/bnode_put to removing those and
> switching to a longer page life-cycle with the log message:
>
>  "Use buffer cache instead of page cache in bnode.c. Cache inode extents."

Alright. I did not correctly understand your original idea and write
some irrelevant stuff. Maybe you were right. However, it is only a
question of historical interest.