Re: hfsplus BUG(), kmap and journalling.

[Vyacheslav Dubeyko <slava@dubeyko.com>]

Hi Hin-Tak,

On Tue, 2012-10-30 at 08:24 +0000, Hin-Tak Leung wrote:
> 

[snip]
> >
> > Usually, hfs_bnode_read() is called after searching of some object in b-tree.
> > It needs to initialize struct hfsplus_find_data by means of hfs_find_init()
> > before any search and operations inside b-tree node. And then, it needs to
> > call hfs_find_exit(). The hfsplus_find_data structure contains mutex that it
> > locks of b-tree during hfs_find_init() call and unlock during
> > hfs_find_exit(). And, usually, hfs_bnode_read() is placed
> > between hfs_find_init()/hfs_find_exit() calls. So, as I can understand, your
> > mutex inside hfs_bnode_read() is useless. But, maybe, not all hfs_bnode_read()
> > calls are placed inside hfs_find_init()/hfs_find_exit() calls. It needs to check.
> 
> > I can't clear understand about what simultaneous kmap()'s you are talking.
> > As
> > I can see, usually, (especially, in the case of hfs_bnode_read()) pairs of
> > kmap()/kunmap() localize in small parts of code and I expect that it executes
> > fast. Do I misunderstand something?
> 
> Perhaps it is easier to show some code for the discussion. The attached 
> serializes kmap in hfs_bnode_read() . This makes the driver works more reliably, 
> somehow. In real usage, multiple instances of hfs_bnode_read() do get invoked in 
> parallel. I assume that is because of both SMP as well as file system access 
> itself are parallelized at various level - e.g. recursion like running du 
> probably invokes a few instances of readdir/getdents?
> 

As I understand, readdir/getdents syscalls call hfsplus_readdir()
method, as a result. It is called hfs_find_init() in the begin of
hfsplus_readdir() (as in hfsplus_lookup() also). The hfs_find_init()
method locks the mutex on the whole catalog tree. Then, it can be called
hfs_bnode_read() and other methods that operates by b-tree content in
the environment of locked b-tree. And, finally, it is called
hfs_find_exit() method at the end of hfsplus_readdir(). The
hfs_find_exit() method unlock the mutex. So, even if you have several
threads with readdir/getdents calls then only one thread will operate
with the b-tree inside hfsplus_readdir() operation.

What serialized kmap in hfs_bnode_read() do you mean? How is it possible
to have such situation for locked b-tree?


> >> I tried swapping those by kmap_atomic()/kunmap_atomic() (beware the arguments are different for unmap) - but the kernel immediately warned that *_atom() code is used where code can sleep.
> >>
> >
> > Could you describe in more details about warning?
> 
> In hfs_bnode_read(), I tried changing kmap/kunmap to 
> kmap_atomic()/kunmap_atomic() . (Sorry I deleted the change since it does not 
> work - but it shouldn't be too difficult to redo since it is just changing two 
> lines in hfs_bnode_read()) - then I get many:
> 
> BUG: scheduling while atomic: ...
> 

It is strange. I need to check it.

[snip]
> BTW, I think I see another bug - unrelated to journalling - in the current hfs+ 
> code: folder counts are not updated correctly. It seems that hfs+ folders have 
> recursive folder counts (i.e. a parent dir knows how many sub-dir's it has, 
> without needing to recurse down), and this is currently not updated correctly. 
> One way of demo'ing this is to:
> 
>     (make sure fs is fsck-clean ; mount)
> 
>     cp -ipr somedir_with_some_files/ some_hfs+_place/someplace_inside/
> 
>     (umount; run fsck_hfs again, now it complains about folder counts)
> 
> The actual message looks a it like this:
> 
> ** Checking catalog hierarchy.
>     HasFolderCount flag needs to be set (id = 393055)
>     (It should be 0x10 instead of 0)
>     Incorrect folder count in a directory (id = 205)
>     (It should be 18 instead of 17)

Sorry, I can't reproduce this issue. I tried to reproduce as you
described. Maybe, do you miss something?

With the best regards,
Vyacheslav Dubeyko.