On Fri, Dec 12, 2008 at 12:52:53PM -0500, Theodore Ts'o wrote: > From: Toshiyuki Okajima <toshi.okajima@xxxxxxxxxxxxxx> > > Implement blkdev_releasepage() to release the buffer_heads and page > after we release private data which belongs to a client of the block > device, such as a filesystem. > > blkdev_releasepage() call the client's releasepage() which is > registered by blkdev_register_client_releasepage() to release its > private data. > > Signed-off-by: Toshiyuki Okajima <toshi.okajima@xxxxxxxxxxxxxx> > Signed-off-by: "Theodore Ts'o" <tytso@xxxxxxx> > Cc: linux-fsdevel@xxxxxxxxxxxxxxx Entire-pile-NAKed-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx> a) Use of fs_type to hide a callback is wrong. Pass it explicitly to whatever helper you want, don't do that crapin get_sb_bdev() b) the same goes from unregistration c) you are unregistering your callback before doing generic_shutdown_super(). Odd, seeing that a _lot_ of fs operations can happen at that point. d) we *already* have exclusion mechanism. It's called open_bdev_exclusive() and it is used by get_sb_bdev() and friends already. Don't reinvent the wheel, please. e) what's going on with the locking there? Comments about mount/umount races are absolutely bogus - we *have* serialization for fs shutdown/startup. And if we hadn't, we would have far worse problems with races than that. The other kind of race is possible, but... this interface is asking for trouble. It sounds like a way to attach some data structures of your own to page and rely on that callback for freeing them. But as soon as somebody tries that we'll have a problem; page can outlive the unregistration of callback and we'll get a leak (in the best case). Sure, ext3 and ext4 won't step into it (journal shutdown will deal with that), but it's a trap for unaware. At the very least it needs to be commented. Said that, I still don't like the use of rwlock here ;-/ If nothing else, that calls for rcu - fs shutdown is extremely rare compared to releasepage... -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
