On 19 June 2015 at 00:16, Hin-Tak Leung <hintak.leung@xxxxxxxxx> wrote: > On 18 June 2015 at 21:51, Sergei Antonov <saproj@xxxxxxxxx> wrote: >> On 18 June 2015 at 19:16, Hin-Tak Leung <hintak.leung@xxxxxxxxx> wrote: >>> On 18 June 2015 at 17:19, Sergei Antonov <saproj@xxxxxxxxx> wrote: >>>> On 18 June 2015 at 17:37, Hin-Tak Leung <hintak.leung@xxxxxxxxx> wrote: >>>>> On 18 June 2015 at 13:58, Sergei Antonov <saproj@xxxxxxxxx> wrote: >>>>>> On 18 June 2015 at 04:51, Hin-Tak Leung <hintak.leung@xxxxxxxxx> 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@xxxxxxxx> > Date: Wed Feb 25 16:17:48 2004 -0800 > > [PATCH] HFS+ support > > From: Roman Zippel <zippel@xxxxxxxxxxxxxx> > > This driver adds full read/write support for HFS+ and is based on the > readonly driver by Brad Broyer. > > Thanks to Ethan Benson <erbenson@xxxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
